[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 #15 from David Kilzer (ddkilzer) <ddkilzer at webkit.org>  2010-07-11 10:23:07 PST ---
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 57494 [details] [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.

No need for a separate bug.  Adding an explanation in the ChangeLog is sufficient.

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

Okay, this is fine as is.

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

Thanks for the explanation--it didn't fully register the first time through.

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