Bug 29065 - svn-unapply doesn't revert directories correctly
Summary: svn-unapply doesn't revert directories correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.5
: P2 Minor
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-08 18:45 PDT by Cameron McCormack (:heycam)
Modified: 2009-09-09 22:17 PDT (History)
1 user (show)

See Also:


Attachments
Patch v1 (2.55 KB, patch)
2009-09-08 18:51 PDT, Cameron McCormack (:heycam)
eric: review-
Details | Formatted Diff | Diff
Patch v2 (4.16 KB, patch)
2009-09-09 16:34 PDT, Cameron McCormack (:heycam)
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 (:heycam) 2009-09-08 18:45:47 PDT
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
Comment 1 Cameron McCormack (:heycam) 2009-09-08 18:51:47 PDT
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.
Comment 2 Eric Seidel (no email) 2009-09-09 08:01:34 PDT
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.
Comment 3 Eric Seidel (no email) 2009-09-09 08:01:48 PDT
Comment on attachment 39242 [details]
Patch v1

r- for my personal taste nits.
Comment 4 Cameron McCormack (:heycam) 2009-09-09 16:34:30 PDT
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. :-)
Comment 5 Eric Seidel (no email) 2009-09-09 17:08:20 PDT
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.
Comment 6 WebKit Commit Bot 2009-09-09 17:59:57 PDT
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
Comment 7 Eric Seidel (no email) 2009-09-09 18:45:16 PDT
Comment on attachment 39309 [details]
Patch v2

The media madness never ends!
compositing/color-matching/image-color-matching.html -> timed out
bug 28624.
Comment 8 WebKit Commit Bot 2009-09-09 22:17:53 PDT
Comment on attachment 39309 [details]
Patch v2

Clearing flags on attachment: 39309

Committed r48242: <http://trac.webkit.org/changeset/48242>
Comment 9 WebKit Commit Bot 2009-09-09 22:17:57 PDT
All reviewed patches have been landed.  Closing bug.