Summary: | svn-[un]apply should change directories to the repository root before [un]applying | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Kilzer (:ddkilzer) <ddkilzer> | ||||||||
Component: | Tools / Tests | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | eric, ggaren, joepeck, pkasting | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
URL: | https://lists.webkit.org/pipermail/webkit-dev/2009-August/009546.html | ||||||||||
Attachments: |
|
Description
David Kilzer (:ddkilzer)
2009-08-21 13:29:54 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.
(In reply to comment #2) > Created an attachment (id=38392) [details] > Give This a Shot Isn't a corresponding change needed for svn-apply? (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? > 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.
(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. WebKitTools/Scripts/modules/scm_unittest.py has a unit testing setup which could be used to test changes like this one. 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!
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 on attachment 38399 [details]
[PATCH] Quick Fix
I guess I'll r- this for now to remove it from the queue.
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. (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. Created attachment 38763 [details]
Patch v3
Comment on attachment 38763 [details]
Patch v3
LGTM.
Comment on attachment 38763 [details] Patch v3 Clearing flags on attachment: 38763 Committed r47923: <http://trac.webkit.org/changeset/47923> All reviewed patches have been landed. Closing bug. (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. Works for me. Thanks for fixing this! |