Use git's -C flag when possible in VCSUtils.pm

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Kocsen Chung]

Use git's -C flag when possible in VCSUtils.pm

[Kocsen Chung]

Created attachment 303006 [details]
Patch

[Jason Marcell]

This looks good!

[Daniel Bates]

Comment on attachment 303006 [details]
Patch

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

Please fix up the Git cases in VCSUtils::svn{InfoForPath, RevisionForDirectory}() to use "git -C" instead of changing directories before invoking git before landing this change.

> Tools/ChangeLog:3
> +        Use the git -C flag where appropriate to avoid changing directories.

This line should be the bug title. The bug description should be below the "Reviewed by" line.

> Tools/Scripts/VCSUtils.pm:226
> +    return system("git -C $dir rev-parse > " . File::Spec->devnull() . " 2>&1") == 0;

We should put surround the value of $dir in single quotes so as to support paths that contain spaces. I also do not see the need to abbreviate the word directory to dir. I suggest we take this opportunity to rename the local variable to $directory.

> Tools/Scripts/VCSUtils.pm:244
> +    my $output = `git -C $directory config --get svn-remote.svn.fetch 2>& 1`;

This change breaks this code when $directory contains a space character(s) since this form of system() will invoke the specified string using the shell and the shell will interpret a space character as the delimiter between command line arguments. One way to fix this is to surround $directory in single quotes.

[Daniel Bates]

Comment on attachment 303006 [details]
Patch

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

>> Tools/Scripts/VCSUtils.pm:244
>> +    my $output = `git -C $directory config --get svn-remote.svn.fetch 2>& 1`;
> 
> This change breaks this code when $directory contains a space character(s) since this form of system() will invoke the specified string using the shell and the shell will interpret a space character as the delimiter between command line arguments. One way to fix this is to surround $directory in single quotes.

I know you didn't add the "2>& 1" to this code. I would fix this to read "2>&1" (remove the space character before the 1). This makes the formatting of this expression consistent with the formatting we use in other functions in this Perl module, including isGitDirectory().

[Kocsen Chung]

Sending a Take 2

[Kocsen Chung]

Created attachment 303091 [details]
Patch

[Daniel Bates]

Comment on attachment 303091 [details]
Patch

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

Is there a reason that you did not address my first remark in comment #3 about fixing up VCSUtils::svn{InfoForPath, RevisionForDirectory}()?

> Tools/ChangeLog:10
> +        to `cd` in and out of the target directory.

Please add an empty line under this line.

[Kocsen Chung]

Missed it! Will send out a Take 3

[Kocsen Chung]

Created attachment 304271 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 304271 [details]
Patch

Rejecting attachment 304271 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 304271, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
8da0 M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
fatal: unable to connect to git.webkit.org:
git.webkit.org[0: 54.190.50.177]: errno=Operation timed out

Current branch master is up to date.

Full output: http://webkit-queues.webkit.org/results/3313140

[Kocsen Chung]

I think commit-queue had trouble given the DNS problem yesterday. Setting 'commit-queue ?' once more.

[WebKit Commit Bot]

Comment on attachment 304271 [details]
Patch

Clearing flags on attachment: 304271

Committed r213986: <http://trac.webkit.org/changeset/213986>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.