Bug 18599 - resolve-ChangeLogs doesn't work with relative paths
Summary: resolve-ChangeLogs doesn't work with relative paths
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: David Kilzer (:ddkilzer)
URL:
Keywords:
: 25456 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-04-18 14:47 PDT by Eric Seidel (no email)
Modified: 2009-08-10 11:19 PDT (History)
4 users (show)

See Also:


Attachments
Potential fix (1.40 KB, patch)
2009-04-28 20:42 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Add error checking to git ls-files command on close() (6.24 KB, patch)
2009-07-31 17:36 PDT, David Kilzer (:ddkilzer)
eric: review+
Details | Formatted Diff | Diff
resolve-ChangeLogs doesn't work with relative paths (2.94 KB, patch)
2009-07-31 17:36 PDT, David Kilzer (:ddkilzer)
eric: review+
Details | Formatted Diff | Diff
Implement VCSUtils::chdirReturningRelativePath() (3.32 KB, patch)
2009-07-31 17:36 PDT, David Kilzer (:ddkilzer)
eric: review+
Details | Formatted Diff | Diff
Implement VCSUtils::determineVCSRoot() (3.92 KB, patch)
2009-07-31 17:36 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 Eric Seidel (no email) 2008-04-18 14:47:32 PDT
SilverTop [4479:WebCore]% resolve-ChangeLogs ../WebKit/mac/ChangeLog [~/Projects/WebKit/WebCore]
fatal: git-ls-files: cannot generate relative filenames containing '..'
WARNING: ../WebKit/mac/ChangeLog does not need merging.
SilverTop [4480:WebCore]% cd ..                                                                              [~/Projects/WebKit/WebCore]
SilverTop [4481:WebKit]% resolve-ChangeLogs WebKit/mac/ChangeLog        [~/Projects/WebKit]
M       WebKit/mac/ChangeLog

Two bugs here.
1.  we should work around the git-ls-files limitation
2. when git-ls-files fails, we should fail...
Comment 1 David Kilzer (:ddkilzer) 2008-09-27 16:54:38 PDT
Could probably use some combination of chdirWebKit() and/or determineSourceDir() from webkitdirs.pm, and fix up any relative paths passed in on the command line.

However, this assumes resolve-ChangeLogs resides in the same WebKitTools directory that would be (in Eric's example) at ../WebKitTools.
Comment 2 David Kilzer (:ddkilzer) 2009-04-28 20:34:05 PDT
*** Bug 25456 has been marked as a duplicate of this bug. ***
Comment 3 David Kilzer (:ddkilzer) 2009-04-28 20:42:12 PDT
Created attachment 29878 [details]
Potential fix

This needs testing.  If you try it out, please report back on your results.

The basic idea is that the resolve-ChangeLogs script will always chdir to the root of the source directory for you (if you don't run it from there), so that this assumption (that it's run from the root source dir) is no longer violated.

What I don't like about this patch is that it makes it impossible to use resolve-ChangeLogs from any project other than WebKit due to the way chdirWebKit() works.  It would be better to use git prefix (and something equivalent for svn--anyone?  Bueller?) to chdir() instead.
Comment 4 David Kilzer (:ddkilzer) 2009-07-31 17:36:46 PDT
Created attachment 33910 [details]
Add error checking to git ls-files command on close()

Reviewed by NOBODY (OOPS!).

Item 2 of <https://bugs.webkit.org/show_bug.cgi?id=18599#c0>.

* Scripts/resolve-ChangeLogs: Added error checking to close()
after running git ls-files.  Added error checking to all
system() calls by checking for a non-zero WEXITSTATUS($?).
Changed "|| die;" expressions to "or die $!;".
---
 2 files changed, 34 insertions(+), 15 deletions(-)
Comment 5 David Kilzer (:ddkilzer) 2009-07-31 17:36:50 PDT
Created attachment 33911 [details]
resolve-ChangeLogs doesn't work with relative paths

Reviewed by NOBODY (OOPS!).

* Scripts/resolve-ChangeLogs: Used chdirReturningRelativePath()
and determineVCSRoot() to change directories to the root of the
project before running the command and to provide a path for
removeChangeLogArguments() to make sure any ChangeLog arguments
on the command line are still found.
(canonicalRelativePath): Added.  Returns a canonical path (e.g.,
stripping 'dir/../' from the path) relative to the current
directory.
(removeChangeLogArguments): Added argument which contains a
relative path that must be prepended to any ChangeLog arguments.
Used canonicalRelativePath() and File::Spec->catfile() to
construct a normalized, relative path to each file.
---
 2 files changed, 34 insertions(+), 4 deletions(-)
Comment 6 David Kilzer (:ddkilzer) 2009-07-31 17:36:53 PDT
Created attachment 33912 [details]
Implement VCSUtils::chdirReturningRelativePath()

Reviewed by NOBODY (OOPS!).

Step 2 to fix:
<http://webkit.org/b/18599> resolve-ChangeLogs doesn't work with relative paths

* Scripts/VCSUtils.pm:
(VCSUtils::chdirReturningRelativePath): Moved here from
chdirAndGetDifference() in svn-create-patch.
* Scripts/svn-create-patch: Switched to use
chdirReturningRelativePath() instead of chdirAndGetDifference().
(chdirAndGetDifference): Removed.
---
 3 files changed, 29 insertions(+), 13 deletions(-)
Comment 7 David Kilzer (:ddkilzer) 2009-07-31 17:36:56 PDT
Created attachment 33913 [details]
Implement VCSUtils::determineVCSRoot()

Reviewed by NOBODY (OOPS!).

Step 1 to fix:
<http://webkit.org/b/18599> resolve-ChangeLogs doesn't work with relative paths

* Scripts/VCSUtils.pm: Removed reference to webkitdirs module.
(VCSUtils::EXPORT): Added &determineVCSRoot.  Realphabetized.
(VCSUtils::determineGitRoot): Added.  Based on code in
commit-log-editor.
(VCSUtils::determineVCSRoot): Implemented using
determineGitRoot() and determineSVNRoot().
* Scripts/commit-log-editor: Replaced use of
topLevelSourceDirectory() with determineVCSRoot().  Resorted
use statements.
(topLevelSourceDirectory): Removed.
---
 3 files changed, 44 insertions(+), 16 deletions(-)
Comment 8 David Kilzer (:ddkilzer) 2009-07-31 17:42:03 PDT
Odd...I expected the patches to be posted in the reverse order from bugzilla-tool!
Comment 9 Eric Seidel (no email) 2009-07-31 18:25:39 PDT
Comment on attachment 33910 [details]
Add error checking to git ls-files command on close()

Perl is so disgusting.
Comment 10 Eric Seidel (no email) 2009-07-31 18:26:48 PDT
Comment on attachment 33911 [details]
resolve-ChangeLogs doesn't work with relative paths

I don't fully understand, but so long as it works, rs=me. :)
Comment 11 Eric Seidel (no email) 2009-07-31 18:29:19 PDT
Comment on attachment 33912 [details]
Implement VCSUtils::chdirReturningRelativePath()

you don't need qw() just (), iirc qw() == (), or?

I've never seen this line actually do anything:
    @EXPORT      = qw(&chdirReturningRelativePath &determineSVNRoot &determineVCSRoot &isGit &isGitDirectory &isSVN &isSVNDirectory &makeFilePathRelative
I think we just have it to comply with perl hear-say. ;)

LGTM.
Comment 12 Eric Seidel (no email) 2009-07-31 18:30:20 PDT
Comment on attachment 33913 [details]
Implement VCSUtils::determineVCSRoot()

LGTM.  Shame we have the same logic in both our python libraries and our perl libraries (yes, I know the perl ones came first). :)
Comment 13 David Kilzer (:ddkilzer) 2009-08-01 06:30:32 PDT
(In reply to comment #11)
> (From update of attachment 33912 [details])
> you don't need qw() just (), iirc qw() == (), or?

Nope.  The qw() or quote word operator is different from just adding parenthesis and making a list.  To change qw(alpha beta gamma) to drop the qw operator, you would have to write ("alpha", "beta", "gamma").  See "man perlop" and search for "qw" for more details.  See also the q, qq, qr and qx operators.

> I've never seen this line actually do anything:
>     @EXPORT      = qw(&chdirReturningRelativePath &determineSVNRoot
> &determineVCSRoot &isGit &isGitDirectory &isSVN &isSVNDirectory
> &makeFilePathRelative
> I think we just have it to comply with perl hear-say. ;)

Then you've never tried removing a method from the list and then running a script that uses the method.  This line tells the perl module which methods are implicitly exported by default when the module is 'use'd in another module or script.  This is the "opposite" of explicitly importing a list of variables or methods or :groups in a use statement, e.g.:

use Getopt::Long qw(:config pass_through);

See "man perlmod" for more details about how to write Perl modules.  See "man perlfunc" for more information about "use".
Comment 14 David Kilzer (:ddkilzer) 2009-08-01 07:26:13 PDT
Comment on attachment 33913 [details]
Implement VCSUtils::determineVCSRoot()

Clearing review+ flag.

Landed in r46669.  <http://trac.webkit.org/changeset/46669>
Comment 15 David Kilzer (:ddkilzer) 2009-08-01 07:26:27 PDT
Comment on attachment 33913 [details]
Implement VCSUtils::determineVCSRoot()

Clearing review+ flag.

Landed in r46669.  <http://trac.webkit.org/changeset/46669>
Comment 16 David Kilzer (:ddkilzer) 2009-08-01 07:29:11 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/VCSUtils.pm
	M	WebKitTools/Scripts/svn-create-patch
Committed r46670
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/VCSUtils.pm
	M	WebKitTools/Scripts/svn-create-patch
r46670 = f28f8d26c5ba542a9e276de8ecc1ba1eb85ec487 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46670
Comment 17 David Kilzer (:ddkilzer) 2009-08-01 07:29:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 18 David Kilzer (:ddkilzer) 2009-08-01 07:29:24 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/resolve-ChangeLogs
Committed r46671
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/resolve-ChangeLogs
r46671 = 368a45ec528e8feb5b11df9c57b625a8d371e678 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46671
Comment 19 David Kilzer (:ddkilzer) 2009-08-01 07:29:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 David Kilzer (:ddkilzer) 2009-08-01 07:29:37 PDT
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/resolve-ChangeLogs
Committed r46672
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/resolve-ChangeLogs
r46672 = e8e8619072e33a2a51ac3dc86e988e2b41ea1570 (trunk)
No changes between current HEAD and refs/remotes/trunk
Resetting to the latest refs/remotes/trunk
http://trac.webkit.org/changeset/46672
Comment 21 David Kilzer (:ddkilzer) 2009-08-01 07:29:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 David Kilzer (:ddkilzer) 2009-08-01 07:31:12 PDT
Hmm...my local 'land-commits' patch to bugzilla-tool needs a lot more work when landing a patch series on a single bug.
Comment 23 Peter Kasting 2009-08-10 11:19:29 PDT
Additional fix (work around abs2rel() bug with symlinks) committed as r46988.