Bug 18599 – resolve-ChangeLogs doesn't work with relative paths

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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...

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

*** Bug 25456 has been marked as a duplicate of this bug. ***

[David Kilzer (:ddkilzer)]

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.

[David Kilzer (:ddkilzer)]

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(-)

[David Kilzer (:ddkilzer)]

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(-)

[David Kilzer (:ddkilzer)]

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(-)

[David Kilzer (:ddkilzer)]

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(-)

[David Kilzer (:ddkilzer)]

Odd...I expected the patches to be posted in the reverse order from bugzilla-tool!

[Eric Seidel (no email)]

Comment on attachment 33910 [details]
Add error checking to git ls-files command on close()

Perl is so disgusting.

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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). :)

[David Kilzer (:ddkilzer)]

(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".

[David Kilzer (:ddkilzer)]

Comment on attachment 33913 [details]
Implement VCSUtils::determineVCSRoot()

Clearing review+ flag.

Landed in r46669.  <http://trac.webkit.org/changeset/46669>

[David Kilzer (:ddkilzer)]

Comment on attachment 33913 [details]
Implement VCSUtils::determineVCSRoot()

Clearing review+ flag.

Landed in r46669.  <http://trac.webkit.org/changeset/46669>

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

Hmm...my local 'land-commits' patch to bugzilla-tool needs a lot more work when landing a patch series on a single bug.

[Peter Kasting]

Additional fix (work around abs2rel() bug with symlinks) committed as r46988.