Bug 29065 – svn-unapply doesn't revert directories correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Cameron McCormack (:heycam)]

Trying to unapply a patch that added a directory will not revert and then remove that directory:


fox:~/svn/WebKit-2/WebKitTools $ cat /tmp/c.patch 
Index: NewDirectory/NewFile
===================================================================
--- NewDirectory/NewFile        (revision 0)
+++ NewDirectory/NewFile        (revision 0)
@@ -0,0 +1 @@
+
fox:~/svn/WebKit-2/WebKitTools $ ./Scripts/svn-apply /tmp/c.patch 
A         NewDirectory
patching file NewDirectory/NewFile
A         NewDirectory/NewFile
fox:~/svn/WebKit-2/WebKitTools $ ./Scripts/svn-unapply /tmp/c.patch 
patching file NewDirectory/NewFile
Reverted 'NewDirectory/NewFile'
A      NewDirectoryfox:~/svn/WebKit-2/WebKitTools $


Notice also that the svn status line printed by svn-unapply doesn't have a newline.

Similarly, trying to unapply a patch that removed a directory will not revert its deletion:


fox:~/svn/WebKit-2/WebKitTools $ svn rm record-memory-win
D         record-memory-win/record-memory-win.vcproj
D         record-memory-win/main.cpp
D         record-memory-win
fox:~/svn/WebKit-2/WebKitTools $ Scripts/svn-create-patch >/tmp/d.patch
fox:~/svn/WebKit-2/WebKitTools $ ./Scripts/svn-unapply /tmp/d.patch 
patching file record-memory-win/main.cpp
Reverted 'record-memory-win/main.cpp'
patching file record-memory-win/record-memory-win.vcproj
Reverted 'record-memory-win/record-memory-win.vcproj'
D      record-memory-winfox:~/svn/WebKit-2/WebKitTools $
fox:~/svn/WebKit-2/WebKitTools $ svn status record-memory-win 
D      record-memory-win

[Cameron McCormack (:heycam)]

Created attachment 39242 [details]
Patch v1

The problem is that svnStatus() returns a status line with a newline from one branch, but without a newline from the other branch.  That caused the regexes in revertDirectories() to fail to match, because they were looking for a \n at the end.

[Eric Seidel (no email)]

Comment on attachment 39242 [details]
Patch v1

it seems this repeated regexp really should be in a function, or at least as a constant.

Also, I think perl is slightly more readable when you make use of $_ explicit, so I prefer the old code.

[Eric Seidel (no email)]

Comment on attachment 39242 [details]
Patch v1

r- for my personal taste nits.

[Cameron McCormack (:heycam)]

Created attachment 39309 [details]
Patch v2

(In reply to comment #2)
> it seems this repeated regexp really should be in a function, or at least as a
> constant.

Done.  Note also I fixed a small bug -- the second s/// I added was being applied to $_ when it should have been applied to $svnStatus.

> Also, I think perl is slightly more readable when you make use of $_ explicit,
> so I prefer the old code.

I'd argue that leaving off the "$_ =~" is more idiomatic Perl, but OK, done.  Perhaps if I'd included it I would've picked up the above bug first time around. :-)

[Eric Seidel (no email)]

Comment on attachment 39309 [details]
Patch v2

OK.  I guess these could move to VCSTools.pm at some point...

I think it's about time to get you commit bit.  I'll look into it.

[WebKit Commit Bot]

Comment on attachment 39309 [details]
Patch v2

Rejecting patch 39309 from commit-queue.

This patch will require manual commit. ['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1'] failed with exit code 1

[Eric Seidel (no email)]

Comment on attachment 39309 [details]
Patch v2

The media madness never ends!
compositing/color-matching/image-color-matching.html -> timed out
bug 28624.

[WebKit Commit Bot]

Comment on attachment 39309 [details]
Patch v2

Clearing flags on attachment: 39309

Committed r48242: <http://trac.webkit.org/changeset/48242>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.