Bug 166914 – 'webkit-patch post' no longer works with moved/copied files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Sam Weinig]

If I move rename a file (using svn rename foo.txt bar.txt) and then upload a patch with that move using webkit-patch post, the bots are not able to apply the change. (See bug https://bugs.webkit.org/show_bug.cgi?id=166913 with result https://webkit-queues.webkit.org/results/2866798).

The bot gives the following output:

Failed to run "['/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-misc-style', 'apply-attachment', '--no-update', '--non-interactive', 298531]" exit_code: 2 cwd: /Volumes/Data/StyleQueue/Webkit

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

Did not find end of header block corresponding to index path "Source/WebCore/bindings/js/JSSQLStatementErrorCallbackCustom.cpp". at /Volumes/Data/StyleQueue/Webkit/Tools/Scripts/VCSUtils.pm line 980, <ARGV> line 249.

Failed to run "[u'/Volumes/Data/StyleQueue/Webkit/Tools/Scripts/svn-apply', '--force']" exit_code: 255 cwd: /Volumes/Data/StyleQueue/Webkit

[Sam Weinig]

I'm using:

svn, version 1.9.4 (r1740329)
   compiled Jul 14 2016, 14:02:40 on x86_64-apple-darwin15.0.0

[Sam Weinig]

Manually uploading a patch created with svn-create-patch also does not work.

[Alexey Proskuryakov]

Sounds related to bug 165953.

[Jonathan Bedard]

This is not a regression from 165953, it's that svn 1.9 has different behavior when moving files.  I'll look into it.

[Jonathan Bedard]

svn 1.7.19 actually has a more serious version of this issue (although, at least in part, this was a bug in svn).

[Jonathan Bedard]

This bug is in svn-create-patch.  In the case that a file has multiple indices with the same name, we should ignore the duplicates.  Will have a patch posted shortly.

[Jonathan Bedard]

Created attachment 298625 [details]
Patch

[Jonathan Bedard]

This isn't the only issue with svn 1.9.  There is a related issue in Tools/Scripts/webkitpy/common/checkout/diff_parser.py which also occurs with renamed files.  It is less serious, however, since it is in the style checker and shouldn't block anyone's workflow.  Bug 166948 is tracking this issue.

[Radar WebKit Bug Importer]

<rdar://problem/29979707>

[Daniel Bates]

Comment on attachment 298625 [details]
Patch

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

> Tools/Scripts/svn-create-patch:257
> +    my $validFlag = 1;
>      while (<DIFF>) {
> -        $patch .= $_;
> +
> +        if (/$svnDiffStartRegEx/) {
> +            if (grep { $_ eq $1 } @indecies ) {
> +                $validFlag = 0;
> +            } else {
> +                push (@indecies, $1);
> +                $validFlag = 1;
> +            }
> +        }
> +        if ($validFlag) {
> +            $patch .= $_;
> +        }
> +        if (/$svnDiffStartEndRegEx/) {
> +            $validFlag = 1;
> +        }
>      }

This does not seem like the correct approach. From my understanding `svn diff --diff-cmd diff -x -$diffOptions '$escapedFile'` now prints an "Index: " line for an added file $escapedFile with history in SVN 1.9. In earlier versions of SVN, it would return nothing. On first glance, this seems like a bug in SVN. Or can we opt into the old behavior?

[Jonathan Bedard]

Comment on attachment 298625 [details]
Patch

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

>> Tools/Scripts/svn-create-patch:257
>>      }
> 
> This does not seem like the correct approach. From my understanding `svn diff --diff-cmd diff -x -$diffOptions '$escapedFile'` now prints an "Index: " line for an added file $escapedFile with history in SVN 1.9. In earlier versions of SVN, it would return nothing. On first glance, this seems like a bug in SVN. Or can we opt into the old behavior?

That's correct.  Even if this is a bug in SVN (which it might be since I don't see anything noting it in the release logs) I'm not sure what we're doing here is correct.  After sleeping on this, I think this is the incorrect change, and I will update the patch with one I believe more accurately encapsulates the problem.  It doesn't make sense to print out an index for a file and then, immediately after, print an index for the very same file, whether it's empty or not.  If the duplicate index is not empty, it seems that it's data should just be included with initial index.

[Jonathan Bedard]

Created attachment 298684 [details]
Patch

[Daniel Bates]

Comment on attachment 298684 [details]
Patch

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

> Tools/ChangeLog:7
> +

We should explain the purpose of this change to workaround an SVN 1.9 bug for moved/copied files without post change. We should also file a SVN bug for this issue, if one does not already exist, and reference its URL in our description and in a comment in generateDiff() where we apply the workaround.

> Tools/Scripts/svn-create-patch:222
> +    my $currentIndex = "";

It seems sufficient to track the number of diff chunks emitted by "svn diff" (or track that there is at least one text chunk is emitted) to fix this bug as opposed to tracking the file we are generating a diff for and quasi-parsing the diff. See my remark below.

> Tools/Scripts/svn-create-patch:231
> +        $currentIndex = $fileData->{path};

Ditto.

> Tools/Scripts/svn-create-patch:259
> +
> +    my $svnDiffStartRegEx = qr#^Index: ([^\r\n]+)#;
> +    my $svnDiffStartEndRegEx = qr#^=+\n#;
> +    my $validFlag = 1;
> +    
>      while (<DIFF>) {
> -        $patch .= $_;
> +
> +        if (/$svnDiffStartRegEx/) {
> +            if ($1 eq  $currentIndex) {
> +                $validFlag = 0;
> +            } else {
> +                $currentIndex = $1;
> +                $validFlag = 1;
> +            }
> +        }
> +        if ($validFlag) {
> +            $patch .= $_;
> +        }
> +        if (/$svnDiffStartEndRegEx/) {
> +            $validFlag = 1;
> +        }
>      }
>      close DIFF;

I would write this as:

my $numTextChunks = 0;
while (<DIFF>) {
    $numTextChunks += 1 if parseChunkRange($_);
    $patch .= $_;
}
close DIFF;
if (!$numTextChunks) {
    # For moved/copied files without post changes SVN 1.9 or greater emits a diff with an empty
    # body as opposed to emitting nothing as in earlier versions of SVN. For example, copy file
    # A.txt to B.txt then the diff of B.txt in SVN 1.9 or greater is:
    #     Index: B.txt
    #     ===================================================================
    # Therefore we ignore emitting such a diff.
    $patch = "";
}

[Daniel Bates]

To clarify, I like the idea in the patch to fix up $patch when we detect that it is a diff without an en empty body (as emitted by SVN 1.9 or greater for a move/copy without post changes). The reason I r-'ed the patch is because it duplicated some of the parsing logic that is in VCSUtils.pm when it seems sufficient to make use of VCSUtils::parseChunkRanges() to detect chunks (we are interested in a diff with at least one chunk) and avoid such duplication. I was also unhappy with the ChangeLog as it did not explain the motivation for this change. I would like to see another iteration of this patch.

As mentioned in comment 10 and comment 13, I feel that the behavior change to svn diff in SVN 1.9 for moved/copied files without post changes is a bug. We should file this bug with the Apache Subversion project.

[Daniel Bates]

(In reply to comment #14)
> To clarify, I like the idea in the patch to fix up $patch when we detect
> that it is a diff without an en empty body (as emitted by SVN 1.9 or greater
> for a move/copy without post changes).

* diff with an empty body

[Jonathan Bedard]

Created attachment 298710 [details]
Patch

[Daniel Bates]

Comment on attachment 298710 [details]
Patch

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

There are issues with this patch and is not representative of the code change I suggested in comment #13. Did this code not work out? If so, what were the issues.

> Tools/ChangeLog:8
> +        SVN 1.9 has a bug where when a file has been moved, the svn diff of the new

Reiterating what I said in comment 13, please file a SVN bug for this issue and reference it in this ChangeLog. Please explain that this bug affects both copy and move operations.

> Tools/ChangeLog:9
> +        file outputs only the index of the file with no changes.  This work-around

work-around => workaround

"the index with no changes" => the "Index" line with an empty body.

> Tools/Scripts/svn-create-patch:234
> +    my $numTextChuncks = 0;

numTextChuncks => numTextChunks

I suggest that we move the initialization of this variable to before the while loop and after open() so that it is closer to where it is used to improve readability. This is good practice to follow in general, especially in other languages (e.g. C++) where there are higher costs and possible side-effects to construction and destruction of objects.

> Tools/Scripts/svn-create-patch:237
> +        $numTextChuncks += 1;

The word "chunks" has a specific meaning in the domain of diffs and patches. It represents a section of a diff that begins with a chunk range line of the form "@@ -1,3 +1,14 @@". That is, a chunk is not an arbitrary line of text in a diff. Unconditionality incrementing the number of text chunks makes this code brittle to future changes to the output of "svn diff". I mean, this bug was caused by us assuming (incorrectly) that "svn diff" would only emit a diff with at least one chunk. As it turns out, "svn diff" in SVN 1.9 emits a diff with zero chunks for a copied/moved file without post changes. We should explicitly check that "svn diff" emitted a diff with at least one chunk as the prerequisite that we captured a valid diff/patch. This approach will fix this bug and is less brittle in the face of future of SVN changes/bugs.

> Tools/Scripts/svn-create-patch:254
> +    if ($numTextChuncks > 2) {
> +        # For moved/copied files without post changes SVN 1.9 or greater emits a diff with an empty
> +        # body as opposed to emitting nothing as in earlier versions of SVN. For example, move file
> +        # A.txt to B.txt then the diff of B.txt in SVN 1.9 or greater is:
> +        # Index: B.txt
> +        # ===================================================================
> +        # Therefore we ignore emitting such a diff.
> +        print $patch;
> +    }

Will the placement of this work when the copied/moved file is a ChangeLog? The code I wrote in comment 13, handles this case and avoid the need to conditionally call print $patch (if patch is the empty string print will emit nothing). I also prefer formatting I used in analogous comment in comment #13 as it demarcates the comment text from the example text. I prefer the example using a copy as opposed to move, though I do not have a strong opinion on this.

[Jonathan Bedard]

Created attachment 298743 [details]
Manual patch

[Daniel Bates]

Comment on attachment 298743 [details]
Manual patch

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

Did you have a chance to read through all of my comments in comment 17? You seem to have selectively addressed some of my comments and not others without providing a reason for the ones not addressed. Please read though my comments in comment 17 and address them.

> Tools/ChangeLog:8
> +        SVN 1.9 has a bug where when a file has been moved, the svn diff of the new

The bug also affects copies.

> Tools/Scripts/svn-create-patch:250
> +        # Index: B.txt

I would appreciate if you could preserve the original indentation of the example as seen in comment 13. The indentation of the example improves the readability of this comment by demarcating the example from the first and last paragraphs.

[Jonathan Bedard]

(In reply to comment #19)
> Comment on attachment 298743 [details]
> Manual patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298743&action=review
> 
> Did you have a chance to read through all of my comments in comment 17? You
> seem to have selectively addressed some of my comments and not others
> without providing a reason for the ones not addressed. Please read though my
> comments in comment 17 and address them.
> 

I had drafted but apparently not posted my reason for not including the subversion bug information.

Unfortunately, subversion's issue tracker was offline yesterday.  It's back up today, but they request that bugs first be sent to [e-mail] (which has been done) but these issue don't immediately receive tracking information.  I will look through tracked issues this morning to see if this has already been filed.

[Jonathan Bedard]

(In reply to comment #20)
> ...

A small update on currently tracked Subversion issues.

Issue 4662 (https://issues.apache.org/jira/browse/SVN-4662) has an example of the kind of malformed patch we're concerned with, but has to with applying this type of patch, not generating it.

Reading through this issue, it seems that an empty index IS legal.  Perhaps this change really belongs in svn-apply-patch.

I still think that we should keep these changes, because the empty index is confusing, but I also think that we should consider the empty index when applying patches as well.

[Daniel Bates]

<http://mail-archives.apache.org/mod_mbox/subversion-users/201701.mbox/[e-mail]%3e>

[Daniel Bates]

(In reply to comment #22)
> <http://mail-archives.apache.org/mod_mbox/subversion-users/201701.mbox/
> [e-mail]%3e>

err, the email thread begins at <http://mail-archives.apache.org/mod_mbox/subversion-users/201701.mbox/[e-mail]%3e>

[Jonathan Bedard]

Created attachment 298764 [details]
Patch

[Daniel Bates]

Comment on attachment 298764 [details]
Patch

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

> Tools/ChangeLog:9
> +        file outputs only the index line with an empty body.  This workaround

Nit: index => "Index:"

From my understanding Subversion does not have a concept of an "index" (like Git does), refer to modified/added/deleted files in a working copy as files in an index, or refer to the working copy revision as a revision in an index. The svn diff tool emits a file where each diff is preceded by an "Index:" line.

> Tools/ChangeLog:10
> +        ignores such an output from svn diff.  A bug has been filed with SVN, a link to the

Nit: "an output" => output

Minor (no need to fix): Given that Apache Subversion has an issue tracker, <https://issues.apache.org/jira/browse/SVN>, it seems a bit imprecise to say "a bug has been filed" when we do not have URL for an issue in the issue tracker as of the time of writing. I would be more precise to say that we have raised this issue to the Subversion [e-mail] mailing lists in <http://mail-archives.apache.org/mod_mbox/subversion-users/201701.mbox/[e-mail]%3e>. According to <https://subversion.apache.org/reporting-issues.html> this is the first step towards creating a formal issue in the issue tracker.

> Tools/ChangeLog:16
> +        (generateDiff): Ignore an index with an empty body.

Nit: index => "Index:"

See my remark on line 9 for my reasoning.

> Tools/Scripts/svn-create-patch:254
> +
> +    if (!$numTextChunks) {
> +        # For moved/copied files without post changes SVN 1.9 or greater emits a diff with an empty
> +        # body as opposed to emitting nothing as in earlier versions of SVN. For example, move file
> +        # A.txt to B.txt then the diff of B.txt in SVN 1.9 or greater is:
> +        #     Index: B.txt
> +        #     ===================================================================
> +        # Therefore we ignore emitting such a diff.
> +        $patch = "";
> +    }

Repeating my question from comment 17, "will the placement of this [code] work when the copied/moved file is a ChangeLog?" The placement of this code disagrees with the placement of the analogous code in my original code snippet in comment 13 and, as I raised in comment 17, I suspect that this proposed placement will not handle moved/copied ChangeLog files. Regardless of whether fixChangeLogPatch() is resilient to a $patch of the form:

[[
Index: ChangeLog
===================================================================
]]

it seems bad programming practice to pass it such a patch. Therefore, I suggest that we respect the placement of the "if (!numTextChunks)"-block in my original code snippet in comment 13 that followed the "close DIFF;" (line 240) statement.

[Jonathan Bedard]

Created attachment 298775 [details]
Patch

[Daniel Bates]

Comment on attachment 298775 [details]
Patch

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

> Tools/Scripts/svn-create-patch:250
> +

Did you intend to put this empty line?

> Tools/Scripts/svn-create-patch:255
> +    

Did you intend to put this empty line?

[Jonathan Bedard]

Created attachment 298782 [details]
Patch

[Daniel Bates]

Comment on attachment 298782 [details]
Patch

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

> Tools/ChangeLog:5
> +

Please put the radar bug URL under the WebKit URL.

[Jonathan Bedard]

Created attachment 298809 [details]
Patch

[Daniel Bates]

(In reply to comment #30)
> Created attachment 298809 [details]
> Patch

Thanks for updating the patch with the radar URL. Looks good.

[WebKit Commit Bot]

Comment on attachment 298809 [details]
Patch

Clearing flags on attachment: 298809

Committed r210820: <http://trac.webkit.org/changeset/210820>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.