Bug 39409 – Enable executable support for svn-apply and svn-unapply

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

Towards adding svn executable bit support, we need to modify function VCSUtils::parseDiff to recognize an SVN footer and call the appropriate parsing routine.

[Daniel Bates]

Created attachment 56846 [details]
Patch with unit tests

[WebKit Review Bot]

Attachment 56846 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:243:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:244:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:261:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:262:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:284:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:285:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:302:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:303:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:329:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:330:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:351:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:352:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:498:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:499:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:535:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:536:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:559:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:560:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:584:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:585:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:599:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:600:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:611:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:612:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 24 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Daniel Bates]

(In reply to comment #2)
> Attachment 56846 [details] did not pass style-queue:
> 
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:243:  Line contains tab character.  [whitespace/tab] [5]
> WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:244:  Line contains tab character.  [whitespace/tab] [5]
> WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:261:  Line contains tab character.  [whitespace/tab] [5]
> WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:262:  Line contains tab character.  [whitespace/tab] [5]
> WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:284:  Line contains tab character. 
> [...]
> If any of these errors are false positives, please file a bug against check-webkit-style.

These tabs are expected as they occur in the example input text for some of the unit tests.

[Chris Jerdonek]

Comment on attachment 56846 [details]
Patch with unit tests

Some initial comments on the first part of the patch:

> Index: WebKitTools/ChangeLog
> +        Make VCSUtils::parseDiff recognize an SVN footer
> +        https://bugs.webkit.org/show_bug.cgi?id=39409

It seems like a better summary would be to say that you are adding executable-bit
support to svn-apply and svn-unapply, since that's the end result people would
care about more.  SVN footer support is just a means to that end in this patch.
By the way, since you stopped using the word "footer" in the code, you might
want to change that terminology in the ChangeLog as well.

> +        Connect up the Git, and SVN executable bit support in the function parseDiff

"Connect up Git and SVN executable-bit support in parseDiff() so that..."
(no "the" and no comma).

A suggestion: I usually append "()" to function names in my ChangeLog descriptions so
I don't have to type the word "function."

> +          - Modified function parseDiff to call parseSvnDiffProperties when
> +            it find the start of an SVN property change diff.

finds

> Index: WebKitTools/Scripts/VCSUtils.pm
>  my $svnDiffStartRegEx = qr#^Index: ([^\r\n]+)#;
> +my $svnDiffPropertiesStartRegEx = qr#Property changes on: ([^\r\n]+)#; # $1 is normally the same as the index path.
>  my $svnPropertyStartRegEx = qr#^(Modified|Name|Added|Deleted): ([^\r\n]+)#; # $2 is the name of the property.
>  my $svnPropertyValueStartRegEx = qr#^   (\+|-) ([^\r\n]+)#; # $2 is the start of the property's value (which may span multiple lines).

It seems $svnPropertiesStartRegEx would be more consistent with the other
regex variables here (even though the function it's actually used in is called
parseSvnDiffProperties()).  If you think the other regexes should be changed to
begin with $svnDiff..., you might want to add a FIXME.

> +#   executableBitDelta: the value 1 or -1 if the executable bit was added or
> +#                       removed from the target file, respectively.

By the way, are you maintaining in your code the convention that
deleted executable files have delta = -1?  I think I did that for the 
Git code.  That will help for unapplying since adding the file back
should have delta = +1.

>      my $headerHashRef; # Last header found, as returned by parseDiffHeader().
> +    my $propertyHashRef; # Last SVN properties diff found, as returned by parseSvnDiffProperties().

Would $svnPropertiesHashRef be a better name?

> +        if ($line =~ $svnDiffPropertiesStartRegEx) {
> +            if ($propertyHashRef) {
> +                # This is the start of the second consecutive property diff of
> +                # this while loop (i.e. we just processed a property diff and
> +                # just encountered the start of another property diff).
> +                last;
> +            }
> +            if ($headerHashRef && ($1 ne $headerHashRef->{indexPath})) {

Could you perhaps read $1 much closer to the regex (possibly setting a variable)?

> +                # 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.
> +                last;

It seems like it would be better to combine these two if blocks that both call "last".

> +            }
> +            ($propertyHashRef, $line) = parseSvnDiffProperties($fileHandle, $line);
> +            next;
> +        }
>          if ($line !~ $headerStartRegEx) {
>              # Then we are in the body of the diff.
>              $svnText .= $line;
> @@ -789,8 +809,9 @@ sub parseDiff($$)
>              next;
>          } # Otherwise, we found a diff header.
>  
> -        if ($headerHashRef) {
> -            # Then this is the second diff header of this while loop.
> +        if ($propertyHashRef || $headerHashRef) {
> +            # Then either we just processed an SVN property change or this
> +            # is the start of the second diff header of this while loop.
>              last;
>          }

Something tells me the "if" logic in the while loop can be made simpler somehow...
For example, if either property changes or a header can begin the diff, then
why is parseDiffHeader() called at the end of the while loop, whereas
parseSvnDiffProperties() is called towards the beginning of the loop.
There's an asymmetry here.  Can this be made simpler?

>  
> @@ -812,12 +833,30 @@ sub parseDiff($$)
>          $copyHash{copiedFromPath} = $headerHashRef->{copiedFromPath};
>          $copyHash{indexPath} = $headerHashRef->{indexPath};
>          $copyHash{sourceRevision} = $headerHashRef->{sourceRevision} if $headerHashRef->{sourceRevision};
> +        if ($headerHashRef->{isSvn}) {
> +            # SVN represents a copy of a file with an executable bit as two distinct diffs: the actual copy, and setting the executable bit.
> +            $copyHash{executableBitDelta} = $propertyHashRef->{executableBitDelta} if $propertyHashRef->{executableBitDelta};
> +        }

If this is the case, then why would this not cause a problem for parseDiff()?
If svn-create-patch creates two diffs for that case, then wouldn't this result
in two calls to parseDiff() -- which would throw off your logic here?
Is parseDiff() treating the "Property Changes" section as a separate diff
or part of the same diff?  We need to clarify what we are calling a "diff".

It might be worth putting a comment somewhere explaining the relationship/ordering
between the header and property changes section (and possibly also contents and
binary contents).

[Chris Jerdonek]

Comment on attachment 56846 [details]
Patch with unit tests

Marking r- to remove from review queue.  My initial review comments are in the previous comment.

[Daniel Bates]

(In reply to comment #4)
> (From update of attachment 56846 [details])
> Some initial comments on the first part of the patch:
> 
> > Index: WebKitTools/ChangeLog
> > +        Make VCSUtils::parseDiff recognize an SVN footer
> > +        https://bugs.webkit.org/show_bug.cgi?id=39409
> 
> It seems like a better summary would be to say that you are adding executable-bit
> support to svn-apply and svn-unapply, since that's the end result people would
> care about more.  SVN footer support is just a means to that end in this patch.
> By the way, since you stopped using the word "footer" in the code, you might
> want to change that terminology in the ChangeLog as well.
> 
> > +        Connect up the Git, and SVN executable bit support in the function parseDiff
> 
> "Connect up Git and SVN executable-bit support in parseDiff() so that..."
> (no "the" and no comma).
> 
> A suggestion: I usually append "()" to function names in my ChangeLog descriptions so
> I don't have to type the word "function."

Will change.

> 
> > +          - Modified function parseDiff to call parseSvnDiffProperties when
> > +            it find the start of an SVN property change diff.
> 
> finds

Will change.

> 
> > Index: WebKitTools/Scripts/VCSUtils.pm
> >  my $svnDiffStartRegEx = qr#^Index: ([^\r\n]+)#;
> > +my $svnDiffPropertiesStartRegEx = qr#Property changes on: ([^\r\n]+)#; # $1 is normally the same as the index path.
> >  my $svnPropertyStartRegEx = qr#^(Modified|Name|Added|Deleted): ([^\r\n]+)#; # $2 is the name of the property.
> >  my $svnPropertyValueStartRegEx = qr#^   (\+|-) ([^\r\n]+)#; # $2 is the start of the property's value (which may span multiple lines).
> 
> It seems $svnPropertiesStartRegEx would be more consistent with the other
> regex variables here (even though the function it's actually used in is called
> parseSvnDiffProperties()).  If you think the other regexes should be changed to
> begin with $svnDiff..., you might want to add a FIXME.

Will change.

> 
> > +#   executableBitDelta: the value 1 or -1 if the executable bit was added or
> > +#                       removed from the target file, respectively.
> 
> By the way, are you maintaining in your code the convention that
> deleted executable files have delta = -1?  I think I did that for the 
> Git code.  That will help for unapplying since adding the file back
> should have delta = +1.

SVN deleted file diffs don't include a property change entry. When you unapply a deleted file diff then we perform an svn revert, which restores the file mode.

> 
> >      my $headerHashRef; # Last header found, as returned by parseDiffHeader().
> > +    my $propertyHashRef; # Last SVN properties diff found, as returned by parseSvnDiffProperties().
> 
> Would $svnPropertiesHashRef be a better name?

Will change.

> 
> > +        if ($line =~ $svnDiffPropertiesStartRegEx) {
> > +            if ($propertyHashRef) {
> > +                # This is the start of the second consecutive property diff of
> > +                # this while loop (i.e. we just processed a property diff and
> > +                # just encountered the start of another property diff).
> > +                last;
> > +            }
> > +            if ($headerHashRef && ($1 ne $headerHashRef->{indexPath})) {
> 
> Could you perhaps read $1 much closer to the regex (possibly setting a variable)?

Will change.

> 
> > +                # 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.
> > +                last;
> 
> It seems like it would be better to combine these two if blocks that both call "last".

Will change.

> 
> > +            }
> > +            ($propertyHashRef, $line) = parseSvnDiffProperties($fileHandle, $line);
> > +            next;
> > +        }
> >          if ($line !~ $headerStartRegEx) {
> >              # Then we are in the body of the diff.
> >              $svnText .= $line;
> > @@ -789,8 +809,9 @@ sub parseDiff($$)
> >              next;
> >          } # Otherwise, we found a diff header.
> >  
> > -        if ($headerHashRef) {
> > -            # Then this is the second diff header of this while loop.
> > +        if ($propertyHashRef || $headerHashRef) {
> > +            # Then either we just processed an SVN property change or this
> > +            # is the start of the second diff header of this while loop.
> >              last;
> >          }
> 
> Something tells me the "if" logic in the while loop can be made simpler somehow...
> For example, if either property changes or a header can begin the diff, then
> why is parseDiffHeader() called at the end of the while loop, whereas
> parseSvnDiffProperties() is called towards the beginning of the loop.
> There's an asymmetry here.  Can this be made simpler?

We can make this simpler. I would suggest we do this in a separate patch as this would requires changes to parseDiffHeader(). In particular, so that it does not die when it does not find a line that begins with "Index" or "diff --git".

> 
> >  
> > @@ -812,12 +833,30 @@ sub parseDiff($$)
> >          $copyHash{copiedFromPath} = $headerHashRef->{copiedFromPath};
> >          $copyHash{indexPath} = $headerHashRef->{indexPath};
> >          $copyHash{sourceRevision} = $headerHashRef->{sourceRevision} if $headerHashRef->{sourceRevision};
> > +        if ($headerHashRef->{isSvn}) {
> > +            # SVN represents a copy of a file with an executable bit as two distinct diffs: the actual copy, and setting the executable bit.
> > +            $copyHash{executableBitDelta} = $propertyHashRef->{executableBitDelta} if $propertyHashRef->{executableBitDelta};
> > +        }
> 
> If this is the case, then why would this not cause a problem for parseDiff()?
> If svn-create-patch creates two diffs for that case, then wouldn't this result
> in two calls to parseDiff() -- which would throw off your logic here?
> Is parseDiff() treating the "Property Changes" section as a separate diff
> or part of the same diff?  We need to clarify what we are calling a "diff".

This comment was ambiguous, so I removed it.

I was trying to describe the composition of an SVN copy file diff. Our implementation considers a header for file A followed by property change for file A to be part of the same diff (i.e. pass one patch to patch()). Hence, we don't treat these as separate diffs.

> 
> It might be worth putting a comment somewhere explaining the relationship/ordering
> between the header and property changes section (and possibly also contents and
> binary contents).

Will add.

[Daniel Bates]

Created attachment 57494 [details]
Patch with unit tests

Updated patch with Chris's suggestions.

[WebKit Review Bot]

Attachment 57494 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:243:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:244:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:261:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:262:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:284:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:285:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:302:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:303:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:329:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:330:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:351:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:352:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:498:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:499:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:535:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:536:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:559:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:560:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:584:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:585:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:599:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:600:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:611:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:612:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 24 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Is this a dupe of Bug 27204?

[Daniel Bates]

*** Bug 27204 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

Comment on attachment 57494 [details]
Patch with unit tests

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

[Daniel Bates]

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

[Daniel Bates]

Created attachment 61128 [details]
Patch with unit tests

Updated patch based on David Kilzer's comments, including updating the change log to mention the exported function scmToggleExecutableBit() and elaborate on the changes to the existing unit tests.

[WebKit Review Bot]

Attachment 61128 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:243:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:244:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:261:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:262:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:284:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:285:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:302:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:303:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:329:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:330:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:351:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:352:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:498:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:499:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:535:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:536:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:559:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:560:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:584:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:585:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:599:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:600:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:611:  Line contains tab character.  [whitespace/tab] [5]
WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:612:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 24 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Comment on attachment 61128 [details]
Patch with unit tests

r=me

[Daniel Bates]

Committed r63062: <http://trac.webkit.org/changeset/63062>