Bug 61162 – REGRESSION (r86515): svn-apply ignores diffs that omit line count in chunk range

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Julien Chaffraix]

See https://bugs.webkit.org/show_bug.cgi?id=39027 where the bots were constantly complaining about new lines that were in the patch but were not in the expected result.

The URL points to a patch that failed locally on a linux machine: if you use 

webkit-patch apply-attachment 94027 [details]

The LayoutTests expected files are not updated as expected.

[Adam Barth]

webkit-patch just uses svn-apply.  Does the issue reproduce with just svn-apply?

+dbates as our resident svn-apply maven.

[Julien Chaffraix]

(In reply to comment #1)
> webkit-patch just uses svn-apply.  Does the issue reproduce with just svn-apply?

I reproduced it locally so this is indeed an svn-apply issue.

[Daniel Bates]

Consider the following diff D from the patch attachment 94027 [details]:

[[
diff --git a/LayoutTests/canvas/philip/tests/2d.composite.uncovered.pattern.source-in-expected.txt b/LayoutTests/canvas/philip/tests/2d.composite.uncovered.pattern.source-in-expected.txt
index 863339f..db418b2 100644
--- a/LayoutTests/canvas/philip/tests/2d.composite.uncovered.pattern.source-in-expected.txt
+++ b/LayoutTests/canvas/philip/tests/2d.composite.uncovered.pattern.source-in-expected.txt
@@ -1 +1,2 @@
 Passed
+
]]

In particular, the chunk range line "@@ -1 +1,2 @@", call this C, omits a line count for the original file.
Notice, the chunk range regular expression we use is: ^\@\@ -(\d+),(\d+) \+\d+,(\d+) \@\@ (http://trac.webkit.org/browser/trunk/Tools/Scripts/VCSUtils.pm?rev=86515#L101). So, we don't match C. 

With the landing of changeset 86515 <http://trac.webkit.org/changeset/86515> we only pass a diff to patch(1) if it contains at least one valid chunk range line (since patch(1) can't handle a diff without one). We don't pass D to patch(1) since it doesn't contain a valid chunk range line; => we ignore D.

[Daniel Bates]

Created attachment 94250 [details]
Patch and unit tests

[WebKit Review Bot]

Attachment 94250 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/VCSUtils..." exit_code: 1

Tools/Scripts/VCSUtils.pm:498:  Line contains tab character.  [whitespace/tab] [5]
Tools/Scripts/VCSUtils.pm:500:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

Created attachment 94252 [details]
Patch and unit tests

Removed extraneous tab characters from patch.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> (In reply to comment #1)
> > webkit-patch just uses svn-apply.  Does the issue reproduce with just svn-apply?
> 
> I reproduced it locally so this is indeed an svn-apply issue.

How was this patch created?  Was it hand-edited?

I'm not convinced we should make our tools accept malformed patches.

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > webkit-patch just uses svn-apply.  Does the issue reproduce with just svn-apply?
> > I reproduced it locally so this is indeed an svn-apply issue.
> How was this patch created?  Was it hand-edited?
> 
> I'm not convinced we should make our tools accept malformed patches.

The patch was created by git, but was this a transient bug in the version of git you were using Julien, or is this a slightly altered patch format that git is now using?

[Julien Chaffraix]

> The patch was created by git, but was this a transient bug in the version of git you were using Julien, or is this a slightly altered patch format that git is now using?

The bug was reproduced both on my machine (old Ubuntu - Lucid IIRC) and the EWS chromium-linux. I guess the versions are different thus it looks like a git patch format issue more than a specific bug.

[Julien Chaffraix]

(In reply to comment #7)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > webkit-patch just uses svn-apply.  Does the issue reproduce with just svn-apply?
> > 
> > I reproduced it locally so this is indeed an svn-apply issue.
> 
> How was this patch created?  Was it hand-edited?

Missed that in my previous reply. This is a vanilla patch created by git and I did not changed it.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> The patch was created by git, but was this a transient bug in the version of git you were using Julien, or is this a slightly altered patch format that git is now using?

Dan says on IRC:

I just wanted to be clear, the patch isn't malformed. Instead, as Wikipedia remarks in the diff article "In many versions of GNU diff, each range can omit the comma and trailing value s, in which case s defaults to 1."

This can also be seen in the function that prints the unified number range print_unidiff_number_range() in <http://git.savannah.gnu.org/cgit/diffutils.git/tree/src/context.c#n275>

I tried to elaborate about this behavior in my commit message but I inadvertently wrote "GNU patch(1)" instead of "GNU diff(1)".

The source for GNU patch(1) has similar logic when generateing the .rej file. For completeness, the Wikipedia article is here: <http://en.wikipedia.org/wiki/Diff#Unified_format>.

[David Kilzer (:ddkilzer)]

Comment on attachment 94252 [details]
Patch and unit tests

View in context: https://bugs.webkit.org/attachment.cgi?id=94252&action=review

r=me  Thanks Dan!

> 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.

> 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).

> 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;

> 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 @@

We should add a test for this (whether or not it's invalid).  Otherwise, the test coverage looks good.

[Daniel Bates]

(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.

[Daniel Bates]

Committed r87641: <http://trac.webkit.org/changeset/87641>