Bug 28623 - svn-[un]apply should change directories to the repository root before [un]applying
Summary: svn-[un]apply should change directories to the repository root before [un]app...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL: https://lists.webkit.org/pipermail/we...
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-21 13:29 PDT by David Kilzer (:ddkilzer)
Modified: 2009-09-01 11:55 PDT (History)
4 users (show)

See Also:


Attachments
Give This a Shot (563 bytes, patch)
2009-08-21 13:57 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Quick Fix (2.91 KB, patch)
2009-08-21 14:59 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
Patch v3 (2.92 KB, patch)
2009-08-28 17:10 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2009-08-21 13:29:54 PDT
* 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
Comment 1 David Kilzer (:ddkilzer) 2009-08-21 13:31:13 PDT
http://trac.webkit.org/changeset/45939
Comment 2 Joseph Pecoraro 2009-08-21 13:57:05 PDT
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.
Comment 3 David Kilzer (:ddkilzer) 2009-08-21 14:08:23 PDT
(In reply to comment #2)
> Created an attachment (id=38392) [details]
> Give This a Shot

Isn't a corresponding change needed for svn-apply?
Comment 4 Joseph Pecoraro 2009-08-21 14:09:21 PDT
(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?
Comment 5 Joseph Pecoraro 2009-08-21 14:11:31 PDT
> 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.
Comment 6 Peter Kasting 2009-08-21 14:16:41 PDT
(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.
Comment 7 Eric Seidel (no email) 2009-08-21 14:26:44 PDT
WebKitTools/Scripts/modules/scm_unittest.py has a unit testing setup which could be used to test changes like this one.
Comment 8 Joseph Pecoraro 2009-08-21 14:59:40 PDT
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!
Comment 9 Geoffrey Garen 2009-08-24 11:12:36 PDT
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.
Comment 10 Geoffrey Garen 2009-08-24 11:15:52 PDT
Comment on attachment 38399 [details]
[PATCH] Quick Fix

I guess I'll r- this for now to remove it from the queue.
Comment 11 Eric Seidel (no email) 2009-08-24 11:39:23 PDT
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.
Comment 12 Joseph Pecoraro 2009-08-28 15:05:51 PDT
(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.
Comment 13 David Kilzer (:ddkilzer) 2009-08-28 17:10:53 PDT
Created attachment 38763 [details]
Patch v3
Comment 14 Eric Seidel (no email) 2009-09-01 02:02:35 PDT
Comment on attachment 38763 [details]
Patch v3

LGTM.
Comment 15 Eric Seidel (no email) 2009-09-01 03:24:53 PDT
Comment on attachment 38763 [details]
Patch v3

Clearing flags on attachment: 38763

Committed r47923: <http://trac.webkit.org/changeset/47923>
Comment 16 Eric Seidel (no email) 2009-09-01 03:24:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 David Kilzer (:ddkilzer) 2009-09-01 06:55:39 PDT
(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.
Comment 18 Geoffrey Garen 2009-09-01 11:55:06 PDT
Works for me. Thanks for fixing this!