Bug 165953 - svn-apply failed to apply a patch (deleting file with svn property)
Summary: svn-apply failed to apply a patch (deleting file with svn property)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-12-16 08:16 PST by Jonathan Bedard
Modified: 2017-01-11 16:19 PST (History)
6 users (show)

See Also:


Attachments
Patch (1.26 KB, patch)
2017-01-05 15:15 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Reduced test case (1.11 KB, application/octet-stream)
2017-01-06 10:43 PST, Jonathan Bedard
no flags Details
Patch (3.74 KB, patch)
2017-01-09 09:26 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (2.84 KB, patch)
2017-01-09 15:08 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (2.93 KB, patch)
2017-01-10 08:44 PST, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2016-12-16 08:16:14 PST
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.
Comment 1 Daniel Bates 2016-12-20 20:19:50 PST
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 buildbot@hotmail.com...
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>
Comment 2 Jonathan Bedard 2017-01-05 11:38:20 PST
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.
Comment 3 Jonathan Bedard 2017-01-05 15:15:21 PST
Created attachment 298143 [details]
Patch
Comment 4 Daniel Bates 2017-01-05 18:36:18 PST
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.
Comment 5 Daniel Bates 2017-01-05 18:37:50 PST
(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?
Comment 6 Jonathan Bedard 2017-01-06 08:28:06 PST
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.
Comment 7 Jonathan Bedard 2017-01-06 10:42:13 PST
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.
Comment 8 Jonathan Bedard 2017-01-06 10:43:21 PST
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.
Comment 9 Jonathan Bedard 2017-01-09 09:26:58 PST
Created attachment 298365 [details]
Patch
Comment 10 Jonathan Bedard 2017-01-09 09:29:23 PST
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.
Comment 11 Daniel Bates 2017-01-09 14:20:23 PST
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>
Comment 12 Daniel Bates 2017-01-09 14:28:39 PST
The patch looks reasonable. I would like to see one more iteration.
Comment 13 Jonathan Bedard 2017-01-09 15:08:21 PST
Created attachment 298396 [details]
Patch
Comment 14 Jonathan Bedard 2017-01-09 15:11:40 PST
The unit tests have addition and removal.  Not sure what a property change looks like in 1.7, so I didn't add that.
Comment 15 Daniel Bates 2017-01-09 17:16:46 PST
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?
Comment 16 Jonathan Bedard 2017-01-10 08:44:17 PST
Created attachment 298467 [details]
Patch
Comment 17 WebKit Commit Bot 2017-01-10 10:06:09 PST
Comment on attachment 298467 [details]
Patch

Clearing flags on attachment: 298467

Committed r210551: <http://trac.webkit.org/changeset/210551>
Comment 18 WebKit Commit Bot 2017-01-10 10:06:13 PST
All reviewed patches have been landed.  Closing bug.