Bug 14590 – svn-create-patch fails when svn mv is used on directory trees

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Holger Freyther]

The most simple example is the following:
svn mv Bakefiles Foofiles
WebKitTools/Scripts/svn-create-patch

The resulting patch will remove all of Bakefiles but will not create Foofiles or any other commands. So it is highly likely svn-create-patch can't track or wrongly tracks moving.

WebKit is from ToT and version of svn is Version 1.4.3 (r23084)

[Holger Freyther]

I had a look at prepare-Changelog as it reicgnized the copy/move and it invokes svn info on candidates:

svn st
A  +   Blafiles
D      Bakefiles
D      Bakefiles/update-file-lists.py
D      Bakefiles/ChangeLog
D      Bakefiles/Bakefiles.bkgen
D      Bakefiles/Readme.txt
D      Bakefiles/presets.bkl

holger-freythers-computer:/space/hacking/webkit/WebKit-svn ich$ LC_ALL=C svn info Blafiles/
Path: Blafiles
URL: http://svn.webkit.org/repository/webkit/trunk/Blafiles
Repository Root: http://svn.webkit.org/repository/webkit
Repository UUID: 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Revision: 24235
Node Kind: directory
Schedule: add
Copied From URL: http://svn.webkit.org/repository/webkit/trunk/Bakefiles
Copied From Rev: 24235
Last Changed Author: ddkilzer
Last Changed Rev: 17735
Last Changed Date: 2006-11-11 22:28:20 +0100 (Sat, 11 Nov 2006)

[Holger Freyther]

And reading the header of svn-apply reveals the following:

# Missing features:
#
#   Handle property changes.
#   Handle copied and moved directories (would require patches made by svn-create-patch).
#   When doing a removal, check that old file matches what's being removed.
#   Notice a patch that's being applied at the "wrong level" and make it work anyway.
#   Do a dry run on the whole patch and don't do anything if part of the patch is
#       going to fail (probably too strict unless we exclude ChangeLog).

So it is known that svn-create-patch doesn't track moved directories.

[David Kilzer (:ddkilzer)]

How do you propose handling moved (or copied) directories in svn-create-patch (and svn-apply and svn-unapply)?

Handling copied/moved files was fixed in r18513 (Bug 12023) by doing the following:

- For files created by 'svn cp', extra information was added to the patch (in a way that plain old patch(1) would ignore) that svn-apply and svn-unapply read and "do the right thing" with.  This generates a single 'add file' patch.

- For files created by 'svn mv', the extra information is added in the same way as 'svn cp'.  Two patches get created:  one to add the moved file (with the extra info), and one to delete the old file. 

- In the special case where the moved or copied file also had local modifications, a *third* patch is created (after the initial add) which captures the local changes.

http://trac.webkit.org/projects/webkit/changeset/18513

For moved or copied directories, I see two approaches:

1. For each file in the moved/copied directory hierarchy, do the same thing for an individual moved/copied file.  Then svn-appy and svn-unapply would have to "recognize" that an entire directory tree had changed.

2. Come up with a way to describe the directory tree move/copy so that patch(1) would ignore it, but svn-appy and svn-unapply would handle it properly.

Any ideas or suggestions?

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> - In the special case where the moved or copied file also had local
> modifications, a *third* patch is created (after the initial add) which
> captures the local changes.

For 'svn cp', a *second* file is added, while for 'svn mv' it's a *third* file.

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> 1. For each file in the moved/copied directory hierarchy, do the same thing for
> an individual moved/copied file.  Then svn-appy and svn-unapply would have to
> "recognize" that an entire directory tree had changed.

Obviously this doesn't scale well at all, so it should not be done.

> 2. Come up with a way to describe the directory tree move/copy so that patch(1)
> would ignore it, but svn-appy and svn-unapply would handle it properly.

This is the better approach.  Although patch(1) would ignore this, it's probably okay because a directory move/rename is really outside the scope of what patch(1) was intended to be used for.  The only question is:  What should this look like?

[Peter Kasting]

svn-create-patch has changed a lot in the past few years.  Is this still an issue?

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> svn-create-patch has changed a lot in the past few years.  Is this still an
> issue?

Yes.

$ svn mv WebKitTools/mangleme WebKitTools/mangleyou
A         WebKitTools/mangleyou
D         WebKitTools/mangleme/LICENSE
D         WebKitTools/mangleme/tags.h
D         WebKitTools/mangleme/mangle.cgi.c
D         WebKitTools/mangleme/remangle.cgi.c
D         WebKitTools/mangleme/Makefile
D         WebKitTools/mangleme/README
D         WebKitTools/mangleme

$ ./WebKitTools/Scripts/svn-create-patch WebKitTools > diff

$ svn revert --recursive WebKitTools
Reverted 'WebKitTools/mangleme'
Reverted 'WebKitTools/mangleme/LICENSE'
Reverted 'WebKitTools/mangleme/tags.h'
Reverted 'WebKitTools/mangleme/mangle.cgi.c'
Reverted 'WebKitTools/mangleme/remangle.cgi.c'
Reverted 'WebKitTools/mangleme/Makefile'
Reverted 'WebKitTools/mangleme/README'
Reverted 'WebKitTools/mangleyou'

$ rm -rf WebKitTools/mangleyou

$ ./WebKitTools/Scripts/svn-apply diff
patching file WebKitTools/mangleme/LICENSE
D         WebKitTools/mangleme/LICENSE
patching file WebKitTools/mangleme/Makefile
D         WebKitTools/mangleme/Makefile
patching file WebKitTools/mangleme/README
D         WebKitTools/mangleme/README
patching file WebKitTools/mangleme/mangle.cgi.c
D         WebKitTools/mangleme/mangle.cgi.c
patching file WebKitTools/mangleme/remangle.cgi.c
D         WebKitTools/mangleme/remangle.cgi.c
patching file WebKitTools/mangleme/tags.h
D         WebKitTools/mangleme/tags.h
D         WebKitTools/mangleme

$ svn stat WebKitTools
D       WebKitTools/mangleme
D       WebKitTools/mangleme/LICENSE
D       WebKitTools/mangleme/tags.h
D       WebKitTools/mangleme/mangle.cgi.c
D       WebKitTools/mangleme/remangle.cgi.c
D       WebKitTools/mangleme/Makefile
D       WebKitTools/mangleme/README

As you can see, the patch only contains the removed files.  It doesn't contain the "added" files in the renamed directory, and it doesn't even "understand" that the directory was renamed (with no other changes).

Because git only cares about files (and not directories), doing the same thing in git results in just a "rename" patch for each file moved:

$ git mv WebKitTools/mangleme WebKitTools/mangleyou

$ git status
# On branch master
# Your branch is ahead of 'origin/master' by 1 commit.
#
# Changes to be committed:
#   (use "git reset HEAD <file>..." to unstage)
#
#	renamed:    WebKitTools/mangleme/LICENSE -> WebKitTools/mangleyou/LICENSE
#	renamed:    WebKitTools/mangleme/Makefile -> WebKitTools/mangleyou/Makefile
#	renamed:    WebKitTools/mangleme/README -> WebKitTools/mangleyou/README
#	renamed:    WebKitTools/mangleme/mangle.cgi.c -> WebKitTools/mangleyou/mangle.cgi.c
#	renamed:    WebKitTools/mangleme/remangle.cgi.c -> WebKitTools/mangleyou/remangle.cgi.c
#	renamed:    WebKitTools/mangleme/tags.h -> WebKitTools/mangleyou/tags.h

If we make svn-create-patch mimic what git does, then we'd have a series of rename patches for each file (although svn-create-patch does those differently--it puts a full delete and a full add patch in for each file, plus a third patch if the source was changed after the move).  However, you still have to have some logic in svn-apply (and svn-unapply) that can figure out that we renamed an entire directory before it applies the individual patches.

One approach would be to add a special patch entry (similar to a property change "patch", except it would denote directory moves/renames) that does nothing when used with patch(1), but provides a hint to svn-apply and svn-unapply that whole directories were renamed or moved.  Then we could "ignore" the whole-file-delete and whole-file-add patches and only apply the "changes-after-move" patch for any given file under that directory.

Does that make any sense?  :)

[David Kilzer (:ddkilzer)]

(In reply to comment #7)
> If we make svn-create-patch mimic what git does, then we'd have a series of
> rename patches for each file (although svn-create-patch does those
> differently--it puts a full delete and a full add patch in for each file, plus
> a third patch if the source was changed after the move).  However, you still
> have to have some logic in svn-apply (and svn-unapply) that can figure out that
> we renamed an entire directory before it applies the individual patches.
> 
> One approach would be to add a special patch entry (similar to a property
> change "patch", except it would denote directory moves/renames) that does
> nothing when used with patch(1), but provides a hint to svn-apply and
> svn-unapply that whole directories were renamed or moved.  Then we could
> "ignore" the whole-file-delete and whole-file-add patches and only apply the
> "changes-after-move" patch for any given file under that directory.

BTW, the reason the "whole-file-delete" and "whole-file-add" patches (followed optionally by a "changes-after-move" patch) are included in the output of svn-create-patch was so that if you don't have svn-apply available, you have some chance of still using the patch file with the patch(1) command.

In practice, patch(1) sometimes chokes when it has a patch that creates a new file, followed immediately by a second patch that changes that file.  (I just tried it on Snow Leopard and my test patch applied without any issues, so maybe this issue was fixed or I misremembered the problem.)

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #5)
> (In reply to comment #3)
> > 1. For each file in the moved/copied directory hierarchy, do the same thing for
> > an individual moved/copied file.  Then svn-appy and svn-unapply would have to
> > "recognize" that an entire directory tree had changed.
> 
> Obviously this doesn't scale well at all, so it should not be done.
> 
> > 2. Come up with a way to describe the directory tree move/copy so that patch(1)
> > would ignore it, but svn-appy and svn-unapply would handle it properly.
> 
> This is the better approach.  Although patch(1) would ignore this, it's
> probably okay because a directory move/rename is really outside the scope of
> what patch(1) was intended to be used for.  The only question is:  What
> should this look like?

I've been thinking about this again since I started cleaning up projects for tidy-Xcode-project-file (see Bug 188754).

The Tools/TestWebKitAPI/Tests/WTF/BlockPtr.mm source file really should be in Tools/TestWebKitAPI/Tests/WTF/ns/, and that directory should be named /cocoa/ instead of /ns/, so I made these changes in an svn repo (including a directory rename) that looked like this:

$ svn mv Tools/TestWebKitAPI/Tests/WTF/ns Tools/TestWebKitAPI/Tests/WTF/cocoa
$ svn mv Tools/TestWebKitAPI/Tests/WTF/BlockPtr.mm Tools/TestWebKitAPI/Tests/WTF/cocoa/BlockPtr.mm
$ vi Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
$ svn stat Tools
M       Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
D       Tools/TestWebKitAPI/Tests/WTF/BlockPtr.mm
        > moved to Tools/TestWebKitAPI/Tests/WTF/cocoa/BlockPtr.mm
A  +    Tools/TestWebKitAPI/Tests/WTF/cocoa
        > moved from Tools/TestWebKitAPI/Tests/WTF/ns
A  +    Tools/TestWebKitAPI/Tests/WTF/cocoa/BlockPtr.mm
        > moved from Tools/TestWebKitAPI/Tests/WTF/BlockPtr.mm
D       Tools/TestWebKitAPI/Tests/WTF/ns
        > moved to Tools/TestWebKitAPI/Tests/WTF/cocoa

However, svn-create-patch could not handle this because it currently doesn't "see" the file (RetainPtr.mm) inside the moved ns -> cocoa directory, and plain old patch(1) doesn't have a way to describe moved directories anyway.

I think the only way to support moved directories is actually #1 above.

The reason is that if you do #2, these are the challenges:

- It will never work with plain old patch(1).
- If you try to apply the patch and just rename the directory without checking which files are in it, then you run the risk of performing the rename after new files have been added or existing files have been changed or deleted.
- If you look at creating a patch from the "git" approach, it's all about changes to individual files, so enumerating all the moved files under the renamed/moved directory is the approach that is required anyway if you want to be able to make the equivalent changes in a git repo.

Thus the challenges with implementing #1 are:

- When creating a patch from svn, make sure the patch includes all the files inside the moved directory structure (for when applying to a git repo or to a clean svn repo).
- Changes to moved files also need to be captured (although we should get that "for free" after enumerating the list of files and creating a patch).
- Each patch for an individual file needs to capture it's original moved-from location (although we should have that information from the history of the moved directory).

Note that when applying a patch created from a git-svn repo to an svn repo, it's not strictly necessary to recreate any directory moves because you just can move all the files individually (with their history in tact).  It may be strictly less efficient, but you also don't have to worry about modifications to the directory structure (like files being added, changed or deleted) since the patch was created, since those will "fall out" and be obvious after the patch is applied (resulting in conflicts or files left in the "moved" directory structure).

[David Kilzer (:ddkilzer)]

Created attachment 348766 [details]
Patch v1 - WIP

This needs more testing.  It works for the simple example in Comment #9, but I haven't tried moving multiple directories into one another, or making modifications to moved files (that were inside or outside the moved directory structure).

[David Kilzer (:ddkilzer)]

Comment on attachment 348766 [details]
Patch v1 - WIP

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

> Tools/Scripts/svn-create-patch:355
> +        print STDERR "Performing \"svn diff --diff-cmd diff -x -$diffOptions '$escapedDirectory'\"\n" if $verbose;
> +        open DIFF, "svn diff --diff-cmd diff -x -$diffOptions '$escapedDirectory' |" or die;

The funny part about this command is that it simply lists the new files in the moved directory using patch headers with empty bodies:

$ svn diff --diff-cmd diff -x -uap 'Tools/TestWebKitAPI/Tests/WTF/cocoa'
Index: Tools/TestWebKitAPI/Tests/WTF/cocoa/BlockPtr.mm
===================================================================
Index: Tools/TestWebKitAPI/Tests/WTF/cocoa/RetainPtr.mm
===================================================================

I'm not sure if this is documented or expected behavior, but it was the best way I found to enumerate the files in the moved directory (both new and moved-from-elsewhere).

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Created attachment 348782 [details]
Patch v2

[David Kilzer (:ddkilzer)]

Created attachment 348784 [details]
Patch v3

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #14)
> Created attachment 348784 [details]
> Patch v3

Just updated the ChangeLog and the license header.

[Daniel Bates]

Comment on attachment 348784 [details]
Patch v3

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

r=me

> Tools/Scripts/svn-create-patch:356
> +            if (m/^Index: (.*)$/) {

This regular expression does not account for Windows CRLF line endings and will capture the CR. We should make use of VCSUtils::parseDiffStartLine() (it parses an SVN diff start and a Git diff start line in that order). If you do not like the idea of it falling back to trying to parse a Git diff start line then we can extract out the SVN parsing into an exported helper, say parseSvnDiffStartLine(), and implement parseDiffStartLine() in terms of it and then make use of parseSvnDiffStartLine() here.

[David Kilzer (:ddkilzer)]

Created attachment 348853 [details]
Patch v4

[David Kilzer (:ddkilzer)]

Created attachment 348862 [details]
Patch v5

Fixed comment in parseDiffStartLine.pl.

[David Kilzer (:ddkilzer)]

Created attachment 348961 [details]
Patch v6

Updated text in ChangeLog.

[Daniel Bates]

Comment on attachment 348961 [details]
Patch v6

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

> Tools/ChangeLog:12
> +        when parsing git patches using `git diff --no-prefix` non-native

git  => Git

This sentence does not read well. I think it needs a “that have non-native...”

(“git” is the name of a command line tool. The proper noun “Git” refers to the version control system. This sentence seems to refer to the latter.)

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl:39
> +my @testCases = (

For your consideration, I suggest that we order the keys in these test cases such that the keys testName and input (in that order) come before expected. The bandit of this is that for and English speaker they first read testName and gain an understanding of what the test is testing then they read the value of key input and see the diff line that is being tested and finally they see the expected result. This makes it straightforward to read and understand each test.

Another benefit is that it would make the order of the keys consistent with the ordering used in other Perl tests.

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl:43
> +    isGit => 1,

When I first read the name of this key I got the impression that this was part of the expected result as opposed to forcing Git parsing of the input. Can we come up with a more descriptive name for this? Maybe parseAsGitDiffStartLine? If we do decide to rename this then we should similarly rename isSvn.

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl:45
> +    testName => "Git",

Can we come up with a more descriptive name for this test case? Maybe “Git simple”? Or “Git prefixed modified file”?

[David Kilzer (:ddkilzer)]

Comment on attachment 348961 [details]
Patch v6

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

>> Tools/ChangeLog:12
>> +        when parsing git patches using `git diff --no-prefix` non-native
> 
> git  => Git
> 
> This sentence does not read well. I think it needs a “that have non-native...”
> 
> (“git” is the name of a command line tool. The proper noun “Git” refers to the version control system. This sentence seems to refer to the latter.)

Fixed.  Thanks!

>> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl:39
>> +my @testCases = (
> 
> For your consideration, I suggest that we order the keys in these test cases such that the keys testName and input (in that order) come before expected. The bandit of this is that for and English speaker they first read testName and gain an understanding of what the test is testing then they read the value of key input and see the diff line that is being tested and finally they see the expected result. This makes it straightforward to read and understand each test.
> 
> Another benefit is that it would make the order of the keys consistent with the ordering used in other Perl tests.

Changed order to:
    testName
    input
    expected
    isGit/isSvn [renamed below]
    isValid

>> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl:43
>> +    isGit => 1,
> 
> When I first read the name of this key I got the impression that this was part of the expected result as opposed to forcing Git parsing of the input. Can we come up with a more descriptive name for this? Maybe parseAsGitDiffStartLine? If we do decide to rename this then we should similarly rename isSvn.

Changed to:
    isGitDiffStartLine/isSvnDiffStartLine

>> Tools/Scripts/webkitperl/VCSUtils_unittest/parseDiffStartLine.pl:45
>> +    testName => "Git",
> 
> Can we come up with a more descriptive name for this test case? Maybe “Git simple”? Or “Git prefixed modified file”?

Changed to "Git diff start line", and changed other test names similarly.

[David Kilzer (:ddkilzer)]

Committed r235733: <https://trac.webkit.org/changeset/235733>

[Radar WebKit Bug Importer]

<rdar://problem/44178718>

[David Kilzer (:ddkilzer)]

Perhaps this goes without saying, but this fix also works for copied directory paths using `svn cp`.