[webkit-reviews] review granted: [Bug 14590] svn-create-patch fails when svn mv is used on directory trees : [Attachment 348961] Patch v6

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has granted David Kilzer (:ddkilzer)
<ddkilzer at webkit.org>'s request for review:
Bug 14590: svn-create-patch fails when svn mv is used on directory trees
https://bugs.webkit.org/show_bug.cgi?id=14590

Attachment 348961: Patch v6

https://bugs.webkit.org/attachment.cgi?id=348961&action=review




--- Comment #20 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 348961
  --> https://bugs.webkit.org/attachment.cgi?id=348961
Patch v6

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

> Tools/ChangeLog:12
> +	   when parsing git patches using `git diff --no-prefix` non-native

git  => Git

This sentence does not read well. I think it needs a “that have non-native...”

(“git” is the name of a command line tool. The proper noun “Git” refers to the
version control system. This sentence seems to refer to the latter.)

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl:39
> +my @testCases = (

For your consideration, I suggest that we order the keys in these test cases
such that the keys testName and input (in that order) come before expected. The
bandit of this is that for and English speaker they first read testName and
gain an understanding of what the test is testing then they read the value of
key input and see the diff line that is being tested and finally they see the
expected result. This makes it straightforward to read and understand each
test.

Another benefit is that it would make the order of the keys consistent with the
ordering used in other Perl tests.

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl:43
> +    isGit => 1,

When I first read the name of this key I got the impression that this was part
of the expected result as opposed to forcing Git parsing of the input. Can we
come up with a more descriptive name for this? Maybe parseAsGitDiffStartLine?
If we do decide to rename this then we should similarly rename isSvn.

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl:45
> +    testName => "Git",

Can we come up with a more descriptive name for this test case? Maybe “Git
simple”? Or “Git prefixed modified file”?