Bug 29059 - svn-apply doesn't handle changes to files copied to new directories properly
Summary: svn-apply doesn't handle changes to files copied to new directories properly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 29001 29058
  Show dependency treegraph
 
Reported: 2009-09-08 17:45 PDT by Cameron McCormack
Modified: 2009-09-09 17:01 PDT (History)
2 users (show)

See Also:


Attachments
Patch v1 (1.24 KB, patch)
2009-09-08 17:48 PDT, Cameron McCormack
eric: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch v2 (1.31 KB, patch)
2009-09-09 16:11 PDT, Cameron McCormack
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack 2009-09-08 17:45:50 PDT
The output of svn-create-patch trips up svn-apply when a file has been copied to a newly added directory and changes have been made to the copy.  Compare these two diffs:

$ cd WebKitTools
$ svn cp Makefile Scripts   # Copy file into an existing directory.
A         Scripts/Makefile
$ echo >>Makefile           # Make a change to the copy.
$ svn-create-patch | egrep '^(--|\+\+)'
--- Scripts/Makefile    (revision 48142)        (from Makefile:48142)
+++ Scripts/Makefile    (working copy)
--- Scripts/Makefile    (revision 48142)
+++ Scripts/Makefile    (working copy)


and:


$ cd WebKitTools
$ mkdir NewDirectory
$ svn add NewDirectory
A         NewDirectory
$ svn cp Makefile NewDirectory   # Copy file into a new directory.
A         NewDirectory/Makefile
$ echo >>Makefile                # Make a change to the copy.
$ svn-create-patch | egrep '^(--|\+\+)'
--- NewDirectory/Makefile       (revision 48142)        (from Makefile:48142)
+++ NewDirectory/Makefile       (working copy)
--- NewDirectory/Makefile       (revision 0)
+++ NewDirectory/Makefile       (working copy)


So when the diff of the copied file in the new directory is created it says "revision 0", making svn-apply think that it is a new file, whereas it has already copied it in the first part of the patch.  svn-apply moves the copied file out of the way to Makefile.orig and then fails when attempting to apply the second part of the patch (since NewDirectory/Makefile doesn't exist now).

I guess that the "revision 0" output comes from "svn diff" itself; you can see that "svn info" gives different revision info for the two copies:

$ svn info Scripts/Makefile | grep ^Revision
Revision: 48142

$ svn info NewDirectory/Makefile | grep ^Revision
Revision: 0

So one way to fix this would be to change svn-apply to recognise this case and not treat the second part of the patch there as a new file.
Comment 1 Cameron McCormack 2009-09-08 17:48:53 PDT
Created attachment 39236 [details]
Patch v1

This makes the suggested change.
Comment 2 Eric Seidel (no email) 2009-09-09 08:10:09 PDT
Comment on attachment 39236 [details]
Patch v1

Looks sane.  Ideally we'd have some unit testing, which is possible (although slightly awkward) using WebKitTools/Scripts/modules/scm_unittest.py.
Comment 3 WebKit Commit Bot 2009-09-09 16:03:07 PDT
Comment on attachment 39236 [details]
Patch v1

Rejecting patch 39236 from commit-queue.

This patch will require manual commit. Patch https://bugs.webkit.org/attachment.cgi?id=39236 from bug 29059 failed to download and apply.
Comment 4 Eric Seidel (no email) 2009-09-09 16:03:39 PDT
Hunk #1 succeeded at 1 with fuzz 3.
can't find file to patch at input line 5
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: Scripts/svn-apply
|===================================================================
|--- Scripts/svn-apply	(revision 48142)
|+++ Scripts/svn-apply	(working copy)
--------------------------
No file to patch.  Skipping patch.
1 out of 1 hunk ignored
Comment 5 Eric Seidel (no email) 2009-09-09 16:04:32 PDT
Looks like you didn't use svn-create-patch to make this.  It's been made from the WebKitTools directory instead of the root.  svn-create-patch should automatically create patches with full paths for you.
Comment 6 Cameron McCormack 2009-09-09 16:11:38 PDT
Created attachment 39308 [details]
Patch v2

Huh.  I'm pretty sure I did use svn-create-patch, but I probably did run it from WebKitTools/ (just to save it churning through the rest of the checkout for minutes only to find no changes).  Anyway, here's one with full paths.
Comment 7 Eric Seidel (no email) 2009-09-09 16:15:59 PDT
svn-create-patch will make patches with full paths regardless of where you run it from.  Unless you're running an old version somehow?
Comment 8 Eric Seidel (no email) 2009-09-09 16:16:42 PDT
Comment on attachment 39308 [details]
Patch v2

OK.  Still would love to see more unit tests for these scripts now that we have scm_unittest.py. :)
Comment 9 WebKit Commit Bot 2009-09-09 16:41:51 PDT
Comment on attachment 39308 [details]
Patch v2

Clearing flags on attachment: 39308

Committed r48236: <http://trac.webkit.org/changeset/48236>
Comment 10 WebKit Commit Bot 2009-09-09 16:41:55 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Cameron McCormack 2009-09-09 16:58:53 PDT
(In reply to comment #7)
> svn-create-patch will make patches with full paths regardless of where you run
> it from.  Unless you're running an old version somehow?

Seems not (at least not on my machine):

$ pwd
/Users/cam/svn/WebKit-2/WebKitTools
$ echo >>Scripts/svn-create-patch
$ ./Scripts/svn-create-patch 
Index: Scripts/svn-create-patch
===================================================================
--- Scripts/svn-create-patch    (revision 48236)
+++ Scripts/svn-create-patch    (working copy)
@@ -457,3 +457,4 @@ sub testfilecmp($$)
     return $fileDataA->{isTestFile} <=> $fileDataB->{isTestFile};
 }
 
+
$ svn diff -rBASE:HEAD Scripts/svn-create-patch
$
Comment 12 Eric Seidel (no email) 2009-09-09 17:00:47 PDT
Such was supposed to be fixed by bug 26999.
Comment 13 Eric Seidel (no email) 2009-09-09 17:01:15 PDT
If you find it's regressed, please file a bug.