[webkit-reviews] review denied: [Bug 39409] Enable executable support for svn-apply and svn-unapply : [Attachment 57494] Patch with unit tests

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has denied Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 39409: Enable executable support for svn-apply and svn-unapply
https://bugs.webkit.org/show_bug.cgi?id=39409

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

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> Index: WebKitTools/Scripts/VCSUtils.pm
> +	   &scmToggleExecutableBit

Why is this method exported?  It doesn't seem to be used anywhere else in the
patch.

> +	   if ($line =~ $svnPropertiesStartRegEx) {
> +	       my $propertyPath = $1;
> +	       if ($svnPropertiesHashRef || $headerHashRef && ($propertyPath ne
$headerHashRef->{indexPath})) {
> +		   # This is the start of the second diff of this while loop.
In particular, this
> +		   # is the start of a property diff for a file that only has
property changes.
> +		   # Note, if $svnPropertiesHashRef is defined then this is the
start the second
> +		   # consecutive property diff in this while loop.

This comment reads funny (like it was two separate comments put together). 
Please reword.	Maybe:

# This is the start of the second diff in the while loop, which happens to
# be a property diff.  If $svnPropertiesHasRef is defined, then this is the
# second consecutive property diff, otherwise it's the start of a property
# diff for a file that only has property changes.

> +	   if ($headerHashRef->{isSvn}) {
> +	       $copyHash{executableBitDelta} =
$svnPropertiesHashRef->{executableBitDelta} if
$svnPropertiesHashRef->{executableBitDelta};
> +	   }

It's kind of funny to have "if (expr1) { $foo = $bar if expr2; }" instead of
"if (expr1 && expr2) { $foo = $bar; }" or even "$foo = $bar if (expr1 &&
expr2);" in Perl.

Was this done in case an {isGit} check is added later?	I'm okay with with it
either way, I guess.

> Index: WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl
> @@ -91,10 +91,6 @@ Index: test_file.swf
>  Cannot display: file marked as a binary type.
>  svn:mime-type = application/octet-stream
>  
> -Property changes on: test_file.swf
> -___________________________________________________________________
> -Name: svn:mime-type
> -   + application/octet-stream
>  
>  
>  Q1dTBx0AAAB42itg4GlgYJjGwMDDyODMxMDw34GBgQEAJPQDJA==

Why was this removed?  It should just be ignored.  The new code should be able
to parse it but ignore it.  (I guess other tests cover this later, but I still
don't see any reason to remove it.)

> +{
> +    # New test
> +    diffName => "SVN: copied file with property change",
> +    inputText => <<'END',
> +Index: NMakefile
> +===================================================================
> +--- NMakefile	(revision 60021)	(from Makefile:60021)
> ++++ NMakefile	(working copy)
> +@@ -0,0 +1,17 @@
> ++MODULES = JavaScriptCore JavaScriptGlue WebCore WebKit WebKit2 WebKitTools 

> +
> +Property changes on: NMakefile
> +___________________________________________________________________
> +Added: svn:executable
> +   + *
> +END

This is an invalid patch.  There are not 17 new lines added to NMakefile in
this patch.  You should change "-0,0 +1,17" to "-0,0 +1,1" above.

r- for unneeded export and questions about the test cases, but this is very
close to being an r+.