[webkit-reviews] review granted: [Bug 33447] Create a function for svn-apply and svn-unapply to parse diff headers : [Attachment 46237] Proposed patch 2

Sun Jan 10 16:51:17 PST 2010

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Chris Jerdonek
<chris.jerdonek at gmail.com>'s request for review:
Bug 33447: Create a function for svn-apply and svn-unapply to parse diff
headers
https://bugs.webkit.org/show_bug.cgi?id=33447

Attachment 46237: Proposed patch 2
https://bugs.webkit.org/attachment.cgi?id=46237&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> Index: WebKitTools/Scripts/VCSUtils.pm
> +# Parse the next diff header from the given file handle, and advance
> +# the file handle so the last line read is the first line after the
> +# parsed header block.
> +#
> +# This subroutine dies if given leading junk.
> +#
> +# Args:
> +#   $fileHandle: advanced so the last line read is the first line of the
> +#		    next diff header. For SVN-formatted diffs, this is the
> +#		    "Index: " line.
> +#   $lastReadLine: the last line read from $fileHandle

Nit:  This is named $line, not $lastReadLine in the method.

> +# Returns ($headerHashRef, $foundHeaderEnding, $lastReadLine):
> +#   $headerHashRef: a hash reference representing a diff header
> +#	 copiedFromPath:
> +#	 copiedFromVersion:
> +#	 indexPath:
> +#	 svnConvertedText: header text converted to SVN format. Unrecognized
> +#			   Git lines are left alone.
> +#	 version:
> +#   $foundHeaderEnding: whether the last header block line was found.
> +#			   This is a line beginning with "+++".
> +#   $lastReadLine:

I think $lastReadLine's description is on the previous line?  Or is there just
no description?  Capitalization and indenting make this hard to read.  I think
all the various should have some kind of a description.

> Index: WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiffHeader.pl
> +{
> +    # New test
> +    diffName => "Git: unrecognized lines",
> +    inputText => <<'END',
> +diff --git
a/LayoutTests/http/tests/security/listener/xss-inactive-closure.html
b/LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> +new file mode 100644
> +index 0000000..3c9f114
> +--- /dev/null
> ++++ b/LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> +@@ -0,0 +1,34 @@
> ++<html>
> +END
> +    # Header keys to check
> +    svnConvertedText => <<'END',
> +Index: LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> +new file mode 100644
> +index 0000000..3c9f114
> +--- LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> ++++ LayoutTests/http/tests/security/listener/xss-inactive-closure.html
> +END
> +    copiedFromPath => undef,
> +    copiedFromVersion => undef,
> +    indexPath =>
"LayoutTests/http/tests/security/listener/xss-inactive-closure.html",
> +    version => undef,
> +    # Other values to check
> +    foundHeaderEnding => 1,
> +    lastReadLine => "@@ -0,0 +1,34 @@\n",
> +    nextLine => "+<html>\n",
> +},
> +);

Heh, this test output will fail to apply.  Is that why you're writing these
tests (to fix that)?  :)

> +plan tests => @diffTests * 8;

I've never really been a fan of missing parenthesis in Perl.  I think this
would read better as:

plan(tests => @diffTests * 8);

It's up to you, though.  It also took me a while to get used to warn/die
statements without parenthesis.

r=me, but it would be nice if the method documentation issues were addressed.