Bug 61162

webkit-patch just uses svn-apply. Does the issue reproduce with just svn-apply? +dbates as our resident svn-apply maven.

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

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.

Created attachment 94250 [details] Patch and unit tests

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.

Created attachment 94252 [details] Patch and unit tests Removed extraneous tab characters from patch.

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

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

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

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

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

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.

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

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