Bug 39409

Created attachment 56846 [details] Patch with unit tests

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.

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

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

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.

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

Created attachment 57494 [details] Patch with unit tests Updated patch with Chris's suggestions.

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.

Is this a dupe of Bug 27204?

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

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

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

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.

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.

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

Comment on attachment 61128 [details] Patch with unit tests r=me

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