[Webkit-unassigned] [Bug 39409] Enable executable support for svn-apply and svn-unapply

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=39409


David Kilzer (ddkilzer) <ddkilzer at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #57494|review?                     |review-
               Flag|                            |




--- Comment #11 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2010-07-09 16:42:20 PST ---
(From update of attachment 57494)
> 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+.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.