[webkit-reviews] review granted: [Bug 61162] REGRESSION (r86515): svn-apply ignores diffs that omit line count in chunk range : [Attachment 94252] Patch and unit tests

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 61162: REGRESSION (r86515): svn-apply ignores diffs that omit line count in
chunk range
https://bugs.webkit.org/show_bug.cgi?id=61162

Attachment 94252: Patch and unit tests
https://bugs.webkit.org/attachment.cgi?id=94252&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
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.