Bug 169003 - Use git's -C flag when possible in VCSUtils.pm
Summary: Use git's -C flag when possible in VCSUtils.pm
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P3 Normal
Assignee: Kocsen Chung
URL:
Keywords:
Depends on:
Blocks: 170859
  Show dependency treegraph
 
Reported: 2017-02-28 16:17 PST by Kocsen Chung
Modified: 2017-04-14 16:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (1.95 KB, patch)
2017-02-28 16:22 PST, Kocsen Chung
no flags Details | Formatted Diff | Diff
Patch (2.01 KB, patch)
2017-03-01 11:44 PST, Kocsen Chung
no flags Details | Formatted Diff | Diff
Patch (3.75 KB, patch)
2017-03-13 10:27 PDT, Kocsen Chung
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kocsen Chung 2017-02-28 16:17:18 PST
Use git's -C flag when possible in VCSUtils.pm
Comment 1 Kocsen Chung 2017-02-28 16:22:40 PST
Created attachment 303006 [details]
Patch
Comment 2 Jason Marcell 2017-02-28 17:17:25 PST
This looks good!
Comment 3 Daniel Bates 2017-02-28 18:46:38 PST
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.
Comment 4 Daniel Bates 2017-02-28 18:50:02 PST
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().
Comment 5 Kocsen Chung 2017-03-01 11:43:30 PST
Sending a Take 2
Comment 6 Kocsen Chung 2017-03-01 11:44:41 PST
Created attachment 303091 [details]
Patch
Comment 7 Daniel Bates 2017-03-01 13:29:44 PST
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.
Comment 8 Kocsen Chung 2017-03-01 13:31:55 PST
Missed it! Will send out a Take 3
Comment 9 Kocsen Chung 2017-03-13 10:27:45 PDT
Created attachment 304271 [details]
Patch
Comment 10 WebKit Commit Bot 2017-03-13 19:47:33 PDT
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
Comment 11 Kocsen Chung 2017-03-14 14:27:37 PDT
I think commit-queue had trouble given the DNS problem yesterday. Setting 'commit-queue ?' once more.
Comment 12 WebKit Commit Bot 2017-03-15 10:21:04 PDT
Comment on attachment 304271 [details]
Patch

Clearing flags on attachment: 304271

Committed r213986: <http://trac.webkit.org/changeset/213986>
Comment 13 WebKit Commit Bot 2017-03-15 10:21:08 PDT
All reviewed patches have been landed.  Closing bug.