[Webkit-unassigned] [Bug 61162] REGRESSION (r86515): svn-apply ignores diffs that omit line count in chunk range

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61162





--- Comment #13 from Daniel Bates <dbates at webkit.org>  2011-05-29 12:36:29 PST ---
(In reply to comment #12)
> (From update of attachment 94252 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94252&action=review

Thanks for the review David.

> 
> > Tools/ChangeLog:10
> > +        not match a chunk range line that omits a line count. GNU patch(1) will omit the
> 
> Change "patch(1)" to "diff(1)" here as discussed in IRC.

Will fix before landing.

> 
> > Tools/Scripts/VCSUtils.pm:504
> > +    my $chunkRangeRegEx = qr#^\@\@ -(\d+)(,(\d+))? \+(\d+)(,(\d+))? \@\@#; # e.g. "@@ -2,6 +2,18 @@" or "@@ -2,6 +2,18 @@ foo()"
> 
> Let's move the "e.g." example text into the header for the method, and document the GNU diff(1) variables (for both the starting and ending ranges).

I've added an explanation to the documentation of parseChunkRange() so that it reads:

[[
# Parses a chunk range line into its components.
#
# A chunk range line has the form: @@ -L_1,N_1 +L_2,N_2 @@, where the pairs (L_1, N_1),
# (L_2, N_2) are ranges that represent the starting line number and line count in the
# original file and new file, respectively.
#
# Note, some versions of GNU diff may omit the comma and trailing line count (e.g. N_1),
# in which case the omitted line count defaults to 1. For example, GNU diff may output
# @@ -1 +1 @@, which is equivalent to @@ -1,1 +1,1 @@.
# [...]
]][

> 
> > Tools/Scripts/VCSUtils.pm:937
> > +            $numTextChunks += $isChunkRange;
> 
> I would prefer it if you made the boolean-to-integer conversion here more explicit:
> 
>     $numTextChunks += 1 if $isChunkRange;

Will change before landing.

> 
> > Tools/Scripts/webkitperl/VCSUtils_unittest/parseChunkRange.pl:44
> > +###
> > +# Invalid and malformed chunk range
> > +##
> > +# FIXME: We should make this set of tests more comprehensive.
> 
> Is the following a valid range?
> 
> @@ -1 +1 @@

Yes, it's valid. For completeness, one such case where you will see this chunk range is when the diff describes an edit to a single line in a file that contains exactly one line. 

> 
> We should add a test for this (whether or not it's invalid).

I will add a test case for this before landing.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.