[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: [PATCH] v5 #3460

From: Daniel Näslund <daniel_at_longitudo.com>
Date: Thu, 28 Jan 2010 16:03:01 +0100

Hi again Stefan and Julian :-)!

On Thu, Jan 28, 2010 at 12:53:08PM +0000, Julian Foad wrote:
> > +/* Indicate in *MATCHED whether the original text of HUNK matches the patch
> > + * TARGET at its current line. Lines at FUZZ offset from start or end of the
>
> "Lines within FUZZ lines of the start or end ..."

Fixed! For all linguistic issues I trust everyone but me.

> > + * hunk will always match. When this function returns, neither
> > + * TARGET->CURRENT_LINE nor the file offset in the target file will have
> > + * changed. HUNK->ORIGINAL_TEXT will be reset. Do temporary allocations in
> > + * POOL. */
> > static svn_error_t *
> > match_hunk(svn_boolean_t *matched, patch_target_t *target,
> > - const svn_hunk_t *hunk, apr_pool_t *pool)
> > + const svn_hunk_t *hunk, int fuzz, apr_pool_t *pool)
>
>
> > /* Scan lines of TARGET for a match of the original text of HUNK,
> > * up to but not including the specified UPPER_LINE.
> > + * Use fuzz factor FUZZ everywhere if possible.
>
> I would drop the "if possible". It implies that this function decides
> not to use the fuzz factor in some cases.

Fixed!

> > /* Copy HUNK_TEXT into TARGET, line by line, such that the line filter
> > * and transformation callbacks set on HUNK_TEXT by the diff parsing
> > * code in libsvn_diff will trigger. ABSPATH is the absolute path to the
> > - * file underlying TARGET. */
> > + * file underlying TARGET. Do not copy the lines that is within FUZZ offset
> > + * from the beginning or end of hunk unless NR_OF_LINES is set to 0. If
> > + * NR_OF_LINES is non-zero, it represents the number of lines in HUNK_TEXT. */
> > static svn_error_t *
> > copy_hunk_text(svn_stream_t *hunk_text, svn_stream_t *target,
> > - const char *abspath, apr_pool_t *scratch_pool)
> > + const char *abspath, int fuzz, int nr_of_lines,
> > + apr_pool_t *scratch_pool)
>
> This looks odd. Before now, the function worked without needing to be
> told the number of lines in the hunk, and now it needs to be told, but
> that means it now has two ways of knowing. Looking at the
> implementation, I can see why you want this, but from an interface point
> of view it would be better if it could still work without being told. (I
> can envisage a way of doing that - by keeping the last FUZZ lines that
> have been read in a circular buffer before writing them out, so that at
> the end of the input you can discard the FUZZ lines that are in the
> buffer.)

That's a nice solution. Maybe I'll do it later but for now I'll do it as
simple as possible.
 
> But let's say you want to keep the simple implementation that needs to
> know NR_OF_LINES in advance. Why have two ways to request non-fuzzy
> operation: the obvious way (fuzz=0) or the special way (nr_of_lines=0)?
> I don't think you need that special exception "unless NR_OF_LINES is 0".
> Can you keep the rules simple and delete that special case?
 
So obvious... If no fuzz, then we can copy all lines. Fixed!
 
> > Index: subversion/libsvn_diff/parse-diff.c
> > ===================================================================
> > --- subversion/libsvn_diff/parse-diff.c (revision 903978)
> > +++ subversion/libsvn_diff/parse-diff.c (arbetskopia)
> > @@ -278,20 +281,45 @@
> > }
> >
> > c = line->data[0];
> > - if (original_lines > 0 && (c == ' ' || c == '-'))
> > + if (original_lines > 0 && c == ' ' )
> > {
> > hunk_seen = TRUE;
> > original_lines--;
> > + if (changed_line_seen)
> > + trailing_context++;
> > + else
> > + leading_context++;
> > }
> > + else if (original_lines > 0 && c == '-')
> > + {
> > + hunk_seen = TRUE;
> > + original_lines--;
> > + changed_line_seen = TRUE;
> > +
> > + /* A hunk may have context in the middle. We only want the
> > + last lines of context. */
> > + if (trailing_context > 0)
> > + trailing_context = 0;
> > + }
> > else if (c == '+')
> > {
> > hunk_seen = TRUE;
> > + changed_line_seen = TRUE;
> > +
> > + /* A hunk may have context in the middle. We only want the
> > + last lines of context. */
> > + if (trailing_context > 0)
> > + trailing_context = 0;
> > }
> > /* Tolerate chopped leading spaces on empty lines. */
> > else if (original_lines > 0 && ! eof && line->len == 0)
> > {
> > hunk_seen = TRUE;
> > original_lines--;
> > + if (changed_line_seen)
> > + trailing_context++;
> > + else
> > + leading_context++;
> > }
>
> This fourth block would neatly combine with the first block:
>
> if (original_lines > 0 && (c == ' ' || (! eof && line->len == 0))
> { ... }

Yeah, that piece of code had some duplicate code parts. I grouped the
'-' and '+' cases together too. That increased readability a bit.

And now Stefans suggestions.

On Thu, Jan 28, 2010 at 01:46:56PM +0100, Stefan Sperling wrote:
> On Thu, Jan 28, 2010 at 01:55:08PM +0100, Daniel Näslund wrote:
> > Index: subversion/libsvn_diff/parse-diff.c
> > svn_boolean_t lines_matched;
> > @@ -630,9 +635,21 @@
> > eol_str, FALSE,
> > target->keywords, FALSE,
> > iterpool));
> > + lines_read++;
> > +
> > +
> You're adding 2 empty lines here on purpose?

Oups, fixed.
 
> > @@ -833,13 +853,17 @@
> > /* Copy HUNK_TEXT into TARGET, line by line, such that the line filter
> > * and transformation callbacks set on HUNK_TEXT by the diff parsing
> > * code in libsvn_diff will trigger. ABSPATH is the absolute path to the
> > - * file underlying TARGET. */
> > + * file underlying TARGET. Do not copy the lines that is within FUZZ offset
> > + * from the beginning or end of hunk unless NR_OF_LINES is set to 0. If
> > + * NR_OF_LINES is non-zero, it represents the number of lines in HUNK_TEXT. */
>
> Maybe rename the function to copy_hunk_lines and call 'nr_of_lines'
> just 'n'?

Fixed! We're copying lines so saying that increases readability, I
think and N for number of is well established.
 
> > @@ -895,15 +936,20 @@
> > +apply_one_hunk(patch_target_t *target, hunk_info_t *hi,
> > + apr_pool_t *pool)
> > {
> > + svn_linenum_t end_line_nr;
>
> Call it last_line ?

Fixed!

Thanks for your feedback!

A new log msg:

[[[
Fix #3460 - svn patch is not fuzzy when applying unidiffs. Refactor some
parts of the hunk parsing while at it.

* subversion/include/private/svn_diff_private.h
  (svn_hunk_t): Add fields leading_context and trailing_context. They are
    used for determining if there is enough context to apply a patch
    with fuzz.

* subversion/libsvn_diff/parse-diff.c
  (parse_next_hunk): Count number of lines of context at start and end
    of hunk and save the information in hunk. Refactored some if
    statements for increased readability.

* subversion/tests/cmdline/patch_tests.py
  (patch_with_fuzz): New.
  (test_list): Add new test.

* subversion/libsvn_client/patch.c
  (hunk_info_t): Add field fuzz.
  (match_hunk): Add new parameter fuzz. Record nr of lines read and
    ignore the ones at the beginning and end of the hunk that is fuzzy.
    Use leading_context and trailing_context to ignore the cases when there
    isn't enough context to do fuzzy patching.
  (scan_for_match): Add new parameter fuzz. Call match_hunk() with fuzz.
  (get_hunk_info): Add new parameter fuzz. Call scan_for_match() with
    fuzz. Save the used fuzz in hi to be used when the hunk should be
    copied to the target.
  (copy_hunk_text): Rename this to ..
  (copy_hunk_lines): .. And add n and fuzz parameter. Record
    read_lines and only copy those who are not within the fuzz limit. They
    have already been or will be copied from the target. If fuzz is zero
    we ignore n and copy all lines.
  (apply_one_hunk): Adjust lines copied to the target to include the
    context lines at the beginning of the hunk who are fuzzy. Adjust the
    current_line of the target to point to the last line of the hunk
    minus fuzz.
  (apply_one_patch): Try to call get_hunk_info() with no fuzz. If we get
    no matching line try with fuzz 1 and if that fails try with fuzz 2.

Patch by: Daniel Näslund <daniel{_AT_}longitudo.com>
Review by: julianfoad
           stsp
]]]

Received on 2010-01-28 16:03:43 CET

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.