Bug 48527

Summary: commit-log-editor uses full paths for section headers when using msysgit's Perl and multiple ChangeLogs have been edited
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ddkilzer, simon.fraser, webkit.review.bot
Priority: P2 Keywords: PlatformOnly
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows XP   
Attachments:
Description Flags
data dumped when running commit-log-editor from bash prompt
none
data dumped when running commit-log-editor from inside git commit
none
Convert paths in environment variables back to msys-style inside commit-log-editor ddkilzer: review+, ddkilzer: commit-queue-

Description Adam Roben (:aroben) 2010-10-28 09:14:59 PDT
If you use commit-log-editor with msysgit's Perl, and you've edited more than one ChangeLog, you'll get section headers like this:

h:/cyghome/dev/WebKit/OpenSource/LayoutTests:
h:/cyghome/dev/WebKit/OpenSource/WebKit2:

instead of like this:

WebKit2:
LayoutTests:

The badness seems to occur when the ChangeLog path is run through File::Spec->rel2abs.
Comment 1 Adam Roben (:aroben) 2010-10-28 09:16:16 PDT
I cannot reproduce the abs2rel badness when doing this:

perl -e 'use File::Spec; print File::Spec->abs2rel("WebKit2/ChangeLog"), "\n";'

Very strange!
Comment 2 David Kilzer (:ddkilzer) 2010-10-28 12:18:11 PDT
(In reply to comment #0)
> The badness seems to occur when the ChangeLog path is run through File::Spec->rel2abs.

(In reply to comment #1)
> I cannot reproduce the abs2rel badness when doing this:
> 
> perl -e 'use File::Spec; print File::Spec->abs2rel("WebKit2/ChangeLog"), "\n";'

Did you mean abs2rel() or rel2abs()?
Comment 3 Adam Roben (:aroben) 2010-10-28 13:32:17 PDT
(In reply to comment #2)
> (In reply to comment #0)
> > The badness seems to occur when the ChangeLog path is run through File::Spec->rel2abs.
> 
> (In reply to comment #1)
> > I cannot reproduce the abs2rel badness when doing this:
> > 
> > perl -e 'use File::Spec; print File::Spec->abs2rel("WebKit2/ChangeLog"), "\n";'
> 
> Did you mean abs2rel() or rel2abs()?

Whoops, I meant rel2abs.

When I run this script:

perl -e 'use File::Spec; print File::Spec->rel2abs("WebKit2/ChangeLog"), "\n";'

I get:

/h/cyghome/dev/WebKit/OpenSource/WebKit2/ChangeLog

which is an msys-style path, not a Windows-style-with-forward-slashes path like is appearing in the commit log. The same thing happens if I tell perl to print that to a file instead of to stdout.
Comment 4 David Kilzer (:ddkilzer) 2010-10-28 15:04:19 PDT
(In reply to comment #3)
> When I run this script:
> 
> perl -e 'use File::Spec; print File::Spec->rel2abs("WebKit2/ChangeLog"), "\n";'
> 
> I get:
> 
> /h/cyghome/dev/WebKit/OpenSource/WebKit2/ChangeLog
> 
> which is an msys-style path, not a Windows-style-with-forward-slashes path like is appearing in the commit log. The same thing happens if I tell perl to print that to a file instead of to stdout.

What are $changeLog and $topLevel set to when this line of code is run?

    $changeLog = File::Spec->abs2rel(File::Spec->rel2abs($changeLog), $topLevel);
Comment 5 Adam Roben (:aroben) 2010-10-29 07:32:46 PDT
(In reply to comment #4)
> What are $changeLog and $topLevel set to when this line of code is run?
> 
>     $changeLog = File::Spec->abs2rel(File::Spec->rel2abs($changeLog), $topLevel);

$changeLog is "WebKit2/ChangeLog"
$topLevel is "."
File::Spec->rel2abs($changeLog) is "h:/cyghome/dev/WebKit/OpenSource/WebKit2/ChangeLog"
File::Spec->abs2rel(File::Spec->rel2abs($changeLog), $topLevel) is "h:/cyghome/dev/WebKit/OpenSource/WebKit2/ChangeLog"

It's rel2abs that's going screwy. Like I said, I can't reproduce this result with the simple script from comment 3.
Comment 6 Adam Roben (:aroben) 2011-01-19 06:56:02 PST
(In reply to comment #3)
> When I run this script:
> 
> perl -e 'use File::Spec; print File::Spec->rel2abs("WebKit2/ChangeLog"), "\n";'
> 
> I get:
> 
> /h/cyghome/dev/WebKit/OpenSource/WebKit2/ChangeLog
> 
> which is an msys-style path, not a Windows-style-with-forward-slashes path like is appearing in the commit log. The same thing happens if I tell perl to print that to a file instead of to stdout.

I also tried running this script from a normal command prompt instead of from msys's bash prompt. I got the same result: an msys-style path. I can't figure out what's causing the Windows-style path to be printed!
Comment 7 Adam Roben (:aroben) 2011-01-19 07:02:50 PST
I did another experiment. I modified commit-log-editor to print the output of rel2abs and then immediately exit. Then I did:

git status > log.txt
commit-log-editor log.txt

And I got an msys-style path! So something different is happening when git is invoking commit-log-editor than when I invoke it directly.
Comment 8 Adam Roben (:aroben) 2011-01-19 07:07:21 PST
I modified commit-log-editor to also dump %ENV. When run from the bash prompt, paths in environment variables are all msys-style. When run from git commit, many paths (but not all) are Windows-style, including $PWD. I'll bet this is the cause of the difference.
Comment 9 Adam Roben (:aroben) 2011-01-19 07:09:58 PST
Created attachment 79420 [details]
data dumped when running commit-log-editor from bash prompt
Comment 10 Adam Roben (:aroben) 2011-01-19 07:10:17 PST
Created attachment 79421 [details]
data dumped when running commit-log-editor from inside git commit
Comment 11 Adam Roben (:aroben) 2011-01-19 07:26:41 PST
Created attachment 79423 [details]
Convert paths in environment variables back to msys-style inside commit-log-editor
Comment 12 WebKit Review Bot 2011-01-19 07:29:15 PST
Attachment 79423 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2

perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
Updating OpenSource
perl: warning: Setting locale failed.
perl: warning: Please check that your locale settings:
	LANGUAGE = (unset),
	LC_ALL = (unset),
	LANG = "en_US.US-ASCII"
    are supported and installed on your system.
perl: warning: Falling back to the standard locale ("C").
RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295

Died at Tools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Adam Roben (:aroben) 2011-01-19 07:54:55 PST
(In reply to comment #12)
> Attachment 79423 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/update-webkit']" exit_code: 2
> 
> perl: warning: Setting locale failed.
> perl: warning: Please check that your locale settings:
>     LANGUAGE = (unset),
>     LC_ALL = (unset),
>     LANG = "en_US.US-ASCII"
>     are supported and installed on your system.
> perl: warning: Falling back to the standard locale ("C").
> Updating OpenSource
> perl: warning: Setting locale failed.
> perl: warning: Please check that your locale settings:
>     LANGUAGE = (unset),
>     LC_ALL = (unset),
>     LANG = "en_US.US-ASCII"
>     are supported and installed on your system.
> perl: warning: Falling back to the standard locale ("C").
> RA layer request failed: OPTIONS of 'http://svn.webkit.org/repository/webkit': could not connect to server (http://svn.webkit.org) at /usr/lib/git-core/git-svn line 2295
> 
> Died at Tools/Scripts/update-webkit line 132.

Maybe this was caused by the server upgrade that happened this morning?
Comment 14 David Kilzer (:ddkilzer) 2011-01-19 14:54:41 PST
Comment on attachment 79423 [details]
Convert paths in environment variables back to msys-style inside commit-log-editor

View in context: https://bugs.webkit.org/attachment.cgi?id=79423&action=review

r=me, but please fix the regex to only match one alphabetic character.

> Tools/Scripts/commit-log-editor:299
> +        $ENV{$key} =~ s/^(\w):\//\/$1\//g;

Consider using a character other than "/" for the s/// operator here (like s###) so that you don't have to escape the '/' characters in the path:

    $ENV{$key} =~ s#^(\w):/#/$1/#g;

I don't think you want "\w" here since that will match any *word*.  If it's only one character and only a-z (or A-Z), I would use:

    $ENV{$key} =~ s#^([[:alpha:]]):/#/$1/#g;

Or this:

    $ENV{$key} =~ s#^([a-zA-Z]):/#/$1/#g;
Comment 15 Adam Roben (:aroben) 2011-01-19 15:03:55 PST
Comment on attachment 79423 [details]
Convert paths in environment variables back to msys-style inside commit-log-editor

View in context: https://bugs.webkit.org/attachment.cgi?id=79423&action=review

>> Tools/Scripts/commit-log-editor:299
>> +        $ENV{$key} =~ s/^(\w):\//\/$1\//g;
> 
> Consider using a character other than "/" for the s/// operator here (like s###) so that you don't have to escape the '/' characters in the path:
> 
>     $ENV{$key} =~ s#^(\w):/#/$1/#g;
> 
> I don't think you want "\w" here since that will match any *word*.  If it's only one character and only a-z (or A-Z), I would use:
> 
>     $ENV{$key} =~ s#^([[:alpha:]]):/#/$1/#g;
> 
> Or this:
> 
>     $ENV{$key} =~ s#^([a-zA-Z]):/#/$1/#g;

\w only matches a single character. It matches a "word" character (alphanumeric or underscore). But you made me realize we shouldn't match numbers or underscore, so I'll go with [[:alpha:]].

I'll also use #.
Comment 16 Adam Roben (:aroben) 2011-01-20 09:14:23 PST
Committed r76255: <http://trac.webkit.org/changeset/76255>