Bug 165953 – svn-apply failed to apply a patch (deleting file with svn property)

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

In https://bugs.webkit.org/show_bug.cgi?id=165927, EWS says the patch in question cannot be applied to trunk even though the patch came from trunk on my local machine.  This may have something to do with attempting to delete an entire directory.

[Daniel Bates]

The patch referenced in comment 0 is attachment #297250 [details]. Here is the EWS failure output:

[[
Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=ews104', 'apply-attachment', '--no-update', '--non-interactive', 297250, '--port=mac-wk2']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=297250&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=165927&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Processing patch 297250 from bug 165927.
Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 21 cwd: /Volumes/Data/EWS/WebKit

The final property value type found "+" does not correspond to the property change type found "Deleted". at /Volumes/Data/EWS/WebKit/Tools/Scripts/VCSUtils.pm line 1526.

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 21 cwd: /Volumes/Data/EWS/WebKit
]]
<https://webkit-queues.webkit.org/results/2732316>

For completeness, the EWS run for attachment #297250 [details] is at <https://webkit-queues.webkit.org/patch/297250>

[Jonathan Bedard]

This seems to be related to deleting a file whose changes are being ignored.  In the case of the referenced patch, that would be the Xcode project.

[Jonathan Bedard]

Created attachment 298143 [details]
Patch

[Daniel Bates]

Comment on attachment 298143 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298143&action=review

Pleas add a unit test for this change. The unit tests are in Tools/Scripts/webkitperl/VCSUtils_unittest. You can run the tests by running the script test-webkitperl

> Tools/Scripts/VCSUtils.pm:1238
> +        if ($line =~ $svnPropertiesStartRegEx && !($svnText =~ m/\+\+\+(.*)(nonexistent)/)) {

This seems weird. Can you elaborate on tyke need to look at the compulsive text, $svnText.

[Daniel Bates]

(In reply to comment #4)
> Comment on attachment 298143 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298143&action=review
> 
> Pleas add a unit test for this change. The unit tests are in
> Tools/Scripts/webkitperl/VCSUtils_unittest. You can run the tests by running
> the script test-webkitperl
> 
> > Tools/Scripts/VCSUtils.pm:1238
> > +        if ($line =~ $svnPropertiesStartRegEx && !($svnText =~ m/\+\+\+(.*)(nonexistent)/)) {
> 
> This seems weird. Can you elaborate on tyke need to look at the compulsive
> text, $svnText.

Autocomplete fail :( The last sentence should read:

Can you please elaborate on the need to look at the cumulative text, $svnText?

[Jonathan Bedard]

Comment on attachment 298143 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298143&action=review

>>> Tools/Scripts/VCSUtils.pm:1238
>>> +        if ($line =~ $svnPropertiesStartRegEx && !($svnText =~ m/\+\+\+(.*)(nonexistent)/)) {
>> 
>> This seems weird. Can you elaborate on tyke need to look at the compulsive text, $svnText.
> 
> Autocomplete fail :( The last sentence should read:
> 
> Can you please elaborate on the need to look at the cumulative text, $svnText?

This error only happens in the rare case where a file is being deleted which also has properties.  What is going on here is checking to see if the file with changed properties has been deleted, $svnText has this information.  If a file has changed it's properties but also been deleted, we should ignore the changed properties.

[Jonathan Bedard]

I've attached a reduced example of this bug.  It's not just deleting of a file whose changes are being ignored, it's doing so and then adding something to a file later.

[Jonathan Bedard]

Created attachment 298214 [details]
Reduced test case

This is also definitely in the parser, as if you attempt to run this patch as a unit test, it will throw the same error.

[Jonathan Bedard]

Created attachment 298365 [details]
Patch

[Jonathan Bedard]

A bit of an update about the root cause of this bug: later versions of svn do not include new-lines between diffs.  The code which parses properties does not take this into account.  It is possible that this change in behavior may cause other issues in svn-apply, although I have not seen examples of this.

[Daniel Bates]

Comment on attachment 298365 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298365&action=review

> Tools/Scripts/VCSUtils.pm:1571
> +        if (/$svnDiffStartRegEx/) {
> +            return ($propertyValue, $_);
> +        }

I do not see the need to extract this early return condition from the other early return disjuncts on line 1568. We should add the condition "/$svnDiffStartRegEx/" (without quotes) to the list of disjuncts.

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiff.pl:1011
> +{
> +    # New test
> +    diffName => "Back-to-back svn diffs with no newline",
> +    inputText => <<'END',
> +Index: Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj
> +===================================================================
> +--- Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj (revision 210365)
> ++++ Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj (nonexistent)
> +
> +Property changes on: Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj
> +___________________________________________________________________
> +Deleted: svn:ignore
> +## -1,2 +0,0 ##
> +-project.xcworkspace
> +-xcuserdata
> +Index: Tools/Scripts/webkitpy/port/base.py
> +===================================================================
> +--- Tools/Scripts/webkitpy/port/base.py (revision 210365)
> ++++ Tools/Scripts/webkitpy/port/base.py (working copy)
> +@@ -131,6 +131,7 @@
> +         self._web_platform_test_server = None
> +         self._image_differ = None
> +         self._server_process_constructor = server_process.ServerProcess  # overridable for testing
> ++        self._test_runner_process_constructor = server_process.ServerProcess
> + 
> +         if not hasattr(options, 'configuration') or not options.configuration:
> +             self.set_option_default('configuration', self.default_configuration())
> +END
> +    expectedReturn => [
> +[{
> +    svnConvertedText =>  <<'END', # Same as input text
> +Index: Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj
> +===================================================================
> +--- Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj
> ++++ Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj
> +
> +END
> +    indexPath => "Tools/LayoutTestRelay/LayoutTestRelay.xcodeproj",
> +    isSvn => 1,
> +    numTextChunks => 0,
> +}],
> +"Index: Tools/Scripts/webkitpy/port/base.py\n"],
> +    expectedNextLine => "===================================================================\n",
> +},

This test case is excessive given that the change to parseSvnPropertyValue() affects only the parsing of lines 979 thru 983. We should come up with a smaller test case (unit to test). I suggest that we add a unit test to file  file Tools/Scripts/webkitperl/VCSUtils_unittest/parseSvnProperty.pl similar to the test "add svn:executable, followed by empty line and start of next diff, <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/parseSvnProperty.pl?rev=210520#L243>. I suggest Maybe call the test "remove svn:ignore using SVN 1.7 syntax, followed by start of next diff" and I would put this test under its own section "Property value using SVN 1.7 syntax followed by start of next diff" so as to demarcate it from the tests "Using SVN 1.7 syntax" and tests "Property value followed by empty line and start of next diff" test using the same embellished comment as seen in [1].

Although not necessary for this bug, it would be good to add tests for property modification and property removal diffs using the SVN 1.7 syntax that are followed by the start of a new diff.

[1] <https://trac.webkit.org/browser/trunk/Tools/Scripts/webkitperl/VCSUtils_unittest/parseSvnProperty.pl?rev=210520#L178>

[Daniel Bates]

The patch looks reasonable. I would like to see one more iteration.

[Jonathan Bedard]

Created attachment 298396 [details]
Patch

[Jonathan Bedard]

The unit tests have addition and removal.  Not sure what a property change looks like in 1.7, so I didn't add that.

[Daniel Bates]

Comment on attachment 298396 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298396&action=review

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseSvnProperty.pl:239
> +{

As mentioned in my remark in comment 11, I suggest that we demarcate these tests by adding an embellished comment of the form:
###
# Property value using SVN 1.7 syntax followed by start of next diff
##

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseSvnProperty.pl:263
> +    -*

Is the indent intentional?

[Jonathan Bedard]

Created attachment 298467 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 298467 [details]
Patch

Clearing flags on attachment: 298467

Committed r210551: <http://trac.webkit.org/changeset/210551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.