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