[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





--- Comment #12 from Daniel Bates <dbates at webkit.org>  2010-07-09 17:29:29 PST ---
(In reply to comment #11)
> (From update of attachment 57494 [details])
> > Index: WebKitTools/Scripts/VCSUtils.pm
> > +        &scmToggleExecutableBit
> 
> Why is this method exported?  It doesn't seem to be used anywhere else in the patch.

I forgot to export this routine when I landed <http://trac.webkit.org/changeset/58637> (but it wasn't being called yet since we didn't have the executableBitDelta hash key) . Now that we are adding the executableBitDelta hash key to the patch hash structure we need to expose this method.

I'll add a note about this to the change log unless you think this is best addressed in a separate bug.

> 
> > +        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.

Will change.

> 
> > +        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.

I wrote it like this to be consistent with the syntactical form used throughout the parseDiff function.

I can move the $svnPropertiesHashRef->{executableBitDelta} into the outer if-statement if you want.

> 
> > 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.)

Chris and I want to deprecate svnConvertedText and instead use the hash structure returned by parsePatch() to provide all the necessary information about a patch. Towards this, we decided against adding support for svnConvertedText to the property parsing routines (since we plan to remove svnConvertedText).

We have an existing FIXME comment in parseDiff() that explains that we plan to remove svnConvertedText. I further extended this comment in the patch with the text "Note, we may not always have SVN converted text since we intend to deprecate it in the future...."

> 
> > +{
> > +    # 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.

Will change.

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