Bug 12023 – svn-create-patch and friends should handle moved/copied files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

When creating (svn-create-patch), applying (svn-apply) and unapplying (svn-unapply) patches, moved and copied files should be handled properly on working copies (trees) other than the one they were originally created in.

Note that in svn, a "move" is the same thing as "copy" then "delete", so as long as you handle a "copy" (before "delete"), you solve both "move" and "copy".

The other challenge is a "move" or "copy" followed by changes to the file after the change.  The best solution here is to separate the "copy" from the changes, since combining them could cause changes to be lost if only the whole version from the patch is applied.  This means that a moved/copied file with changes will be split up into two patches within a diff, but as long as they appear in the correct order, the end result is the same.

[David Kilzer (:ddkilzer)]

Created attachment 12097 [details]
First pass at changes to svn-create-patch

This is a first pass at changes to svn-create-patch to make it work with moved/copied files.  The only thing left out is property changes for the moved/changed files.

We also need changes to svn-apply and svn-unapply to handle the (subtle) changes in the patches created.

[David Kilzer (:ddkilzer)]

(In reply to comment #1)
> Created an attachment (id=12097) [edit]
> First pass at changes to svn-create-patch
> 
> This is a first pass at changes to svn-create-patch to make it work with
> moved/copied files.  The only thing left out is property changes for the
> moved/changed files.
> 
> We also need changes to svn-apply and svn-unapply to handle the (subtle)
> changes in the patches created.

Hmm...this patch also doesn't include removed files in the diff!

[David Kilzer (:ddkilzer)]

Created attachment 12139 [details]
Patch v2

This patch includes changes for svn-apply and svn-unapply, and fixes the issue of missing removed file patches mentioned in Comment #2.

Theory of operation:

In subversion, a moved (or renamed) file is identical to a copy and a delete, so the modified patch only needs to identify the special case when an added file was copied from an existing file.

The subtle difference introduced to identify copied files is a modified "from" line (the patch line that starts with "--- ") that references the original path and revision number instead of the new path.

Thus an svn copy will generate up to two patches:  one for the copy and one for any changes made after the copy.  An svn move (or svn rename) will generate up to three patches:  one for the delete, one for the copy, and one for any post-copy changes.

Compatibility issues with plain old patch(1):

- In POSIX mode, patch(1) determines the file name to patch by checking for the existence of the first path in this list:  old ("--- " line), new ("+++ " line) and index ("Index: " line).  Because svn-create-patch now modifies the old line to point to the original file, patch(1) wants to patch the wrong file (and ends up doing nothing because that file already exists).  In "normal" mode (-n), patch(1) completely ignores a patch formatted this way.

- If changes are made after a file is copied or moved, svn-create-patch will create two patches:  one to represent the copy and one to represent post-copy changes.  When using patch(1) to reverse the change, these patches must be applied in the reverse order as well (although I don't think patch(1) applies the patches in a file in reverse order).  Assuming the previous issue was resolved, this issue would still remain.

[David Kilzer (:ddkilzer)]

After typing up Comment #3, I've almost convinced myself that I need another way to add a hint to the patch to describe copied files.  This is what svn-create-patch from Patch v2 will generate (from Bug 12036):

Index: JavaScriptCore/pcre/pcre_printint.src
===================================================================
--- JavaScriptCore/pcre/pcre_printint.c (revision 18482)
+++ JavaScriptCore/pcre/pcre_printint.src       (working copy)

Perhaps a better approach would be to add another set of parenthesis on the "old" line describing the original source like this (will probably wrap poorly):

Index: JavaScriptCore/pcre/pcre_printint.src
===================================================================
--- JavaScriptCore/pcre/pcre_printint.src (revision 18482) (JavaScriptCore/pcre/pcre_printint.c@18482)
+++ JavaScriptCore/pcre/pcre_printint.src       (working copy)

I need to do some testing, but that may be more acceptable to patch(1) than changing the "old" line.

Comments?

[David Kilzer (:ddkilzer)]

Another possible format would be to mimic what "svn log -v" does with copied files:

Index: JavaScriptCore/pcre/pcre_printint.src
===================================================================
--- JavaScriptCore/pcre/pcre_printint.src (revision 18482) (from JavaScriptCore/pcre/pcre_printint.c:18482)
+++ JavaScriptCore/pcre/pcre_printint.src       (working copy)

[David Kilzer (:ddkilzer)]

Created attachment 12152 [details]
Patch v3

Implement format from Comment #5.  This addresses the first issue in Comment #3.  (The second issue still exists since we don't want any changes to the copied file to be lost if the originally copied file changes between the time the patch is made and the time it is applied.)

[Darin Adler]

Comment on attachment 12152 [details]
Patch v3

Looks great! r=me

[David Kilzer (:ddkilzer)]

Committed revision 18513.