Bug 28623 – svn-[un]apply should change directories to the repository root before [un]applying

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

* SUMMARY

Hi.

r45939 broke my workflow. Here's the related bugzilla bug: https://bugs.webkit.org/show_bug.cgi?id=26999.

Old "Roll out a patch" workflow:

cd JavaScriptCore
svn-create-patch > patch.txt
svn-unapply patch.txt

Old "Roll in a patch" workflow:

cd JavaScriptCore
svn-apply patch.txt

These old ways of doing things no longer work because svn-apply and svn-unapply don't match svn-create-patch's new behavior of changing to the WebKit root directory if you're currently working in a WebKit subdirectory.

Here's an example command-line interaction:

> ~/webkit/WebKitTools/Scripts$ svn-create-patch > ro.txt
> ~/webkit/WebKitTools/Scripts$ svn-unapply !$
> svn-unapply ro.txt
> 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: WebKitTools/Scripts/webkitdirs.pm
> |===================================================================
> |--- WebKitTools/Scripts/webkitdirs.pm    (revision 47638)
> |+++ WebKitTools/Scripts/webkitdirs.pm    (working copy)
> --------------------------
> File to patch: ^C'WebKitTools/Scripts' is not a directory at ./svn-unapply line 315, <> line 9.

I tried to ignore this for a while, but it's really causing problems for me and at least one other WebKit developer.

Should we revert r45939?

Is there an easy fix to svn-apply and svn-unapply that you can make?

Thanks,
Geoff

[David Kilzer (:ddkilzer)]

http://trac.webkit.org/changeset/45939

[Joseph Pecoraro]

Created attachment 38392 [details]
Give This a Shot

Geoff, sorry about the problems you've been experiencing.  I've since switched to git and no longer have an svn checkout to test on, but I'll give this a look.  Things have progressed since my original patch, and a few convenience functions have been made that are used in svn-create-patch and should work well here.  Could you try applying this patch to see if it fixes your problem for svn-unapply.  If it works things will likely be as easy for svn-apply.

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Created an attachment (id=38392) [details]
> Give This a Shot

Isn't a corresponding change needed for svn-apply?

[Joseph Pecoraro]

(In reply to comment #2)
> Created an attachment (id=38392) [details]

Now that I've thought about it some more, since the patches to un-apply are provided on the command line relative to where the script is originally run, then changing the directory as early as I had done in this patch will likely not find the file (otherwise you'd be applying from the svnRoot anyways).

I can think of a few solutions:
  - store the result of chdirReturningRelativePath() and use it when grabbing the command line arguments.
  - or, starting by storing the absolute paths of all the command line arguments, then proceeding to change directory and apply the patches per usual.

Either way, I don't think I could write this and provide a reliable patch without testing.  pkasting, are you still using svn?

[Joseph Pecoraro]

> Isn't a corresponding change needed for svn-apply?

For a final patch yes, I was just looking for feedback to see if the general idea would work.  A fix for svn-unapply would be enough to test the "example command-line scenario" originally provided.

[Peter Kasting]

(In reply to comment #4)
> Either way, I don't think I could write this and provide a reliable patch
> without testing.  pkasting, are you still using svn?

Yes... I was hoping to avoid diving back into the WebKitTools directory :/

It seems like the right fix is to modify the code that applies individual diffs to change to the right directory, apply the diff, and then change back.  Then no extra global state or preprocessing is required.

A simple version of "the right directory" is to just move to the repository root, while a more complex one would be to try each directory on the way up the chain from the current directory.  This second method would make subtree-relative patches work too.

[Eric Seidel (no email)]

WebKitTools/Scripts/modules/scm_unittest.py has a unit testing setup which could be used to test changes like this one.

[Joseph Pecoraro]

Created attachment 38399 [details]
[PATCH] Quick Fix

Okay, it turns out that svn-apply and svn-unapply work with git too (with the exception of an error I ran into with that I attempted to fix svn-unapply).  Note that this always jumps back to the repository root to apply to patch.  This is compatible with svn-create-patch.  The optimal solution would be to do something like what pkasting suggested, to walk up the tree and try each directory to see if the patch will apply from there.  This would work not only with patches made from the root, but also manual svn patches that are created in the middle of the repository.

This was working in my test cases!

[Geoffrey Garen]

Hi Joseph.

Thanks for looking into this!

It looks like the use of VCSUtils is incompatible with my system:

~/webkit/WebKitTools/mangleme$ svn-create-patch > ro.txt
?       ro.txt
~/webkit/WebKitTools/mangleme$ svn-unapply !$
svn-unapply ro.txt
Can't locate VCSUtils.pm in @INC (@INC contains: /Library/Perl/Updates/5.10.0 /System/Library/Perl/5.10.0/darwin-thread-multi-2level /System/Library/Perl/5.10.0 /Library/Perl/5.10.0/darwin-thread-multi-2level /Library/Perl/5.10.0 /Network/Library/Perl/5.10.0/darwin-thread-multi-2level /Network/Library/Perl/5.10.0 /Network/Library/Perl /System/Library/Perl/Extras/5.10.0/darwin-thread-multi-2level /System/Library/Perl/Extras/5.10.0 .) at /Users/ggaren/webkit/WebKitTools/Scripts/svn-unapply line 67.
BEGIN failed--compilation aborted at /Users/ggaren/webkit/WebKitTools/Scripts/svn-unapply line 67.

[Geoffrey Garen]

Comment on attachment 38399 [details]
[PATCH] Quick Fix

I guess I'll r- this for now to remove it from the queue.

[Eric Seidel (no email)]

This patch is missing the Find::Bin trick:

use FindBin;
use lib $FindBin::Bin;

Will make it possible to include perl modules form the same directory.

[Joseph Pecoraro]

(In reply to comment #11)
> This patch is missing the Find::Bin trick:
> 
> use FindBin;
> use lib $FindBin::Bin;
> 
> Will make it possible to include perl modules form the same directory.

Geoffrey Garen have you tried this with the patch?  Odd that this trick wasn't necessary for my computer.

[David Kilzer (:ddkilzer)]

Created attachment 38763 [details]
Patch v3

[Eric Seidel (no email)]

Comment on attachment 38763 [details]
Patch v3

LGTM.

[Eric Seidel (no email)]

Comment on attachment 38763 [details]
Patch v3

Clearing flags on attachment: 38763

Committed r47923: <http://trac.webkit.org/changeset/47923>

[Eric Seidel (no email)]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #14)
> (From update of attachment 38763 [details])
> LGTM.

Geoff Garen was supposed to test this (see Comment #12) before landing it.

[Geoffrey Garen]

Works for me. Thanks for fixing this!