Bug 137541 - Errors using git-add-reviewer on a branch whose name includes parentheses
Summary: Errors using git-add-reviewer on a branch whose name includes parentheses
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-10-08 16:43 PDT by Dana Burkart
Modified: 2014-10-09 14:10 PDT (History)
1 user (show)

See Also:


Attachments
Quote arguments to git. (1.64 KB, patch)
2014-10-08 16:51 PDT, Dana Burkart
darin: review+
Details | Formatted Diff | Diff
Fix which doesn't use backticks at all (6.24 KB, patch)
2014-10-09 10:44 PDT, Dana Burkart
no flags Details | Formatted Diff | Diff
Forgot to update ChangeLog (6.54 KB, patch)
2014-10-09 10:48 PDT, Dana Burkart
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dana Burkart 2014-10-08 16:43:23 PDT
"I have a branch named "parsec-caching-improvements(18462642)". When trying to use git-add-reviewer to add a reviewer to a patch on this branch, I see:

 % git add-reviewer HEAD "Brian Weinstein"
sh: -c: line 0: syntax error near unexpected token `('
sh: -c: line 0: `git rev-parse parsec-caching-improvements(18462642)'
sh: -c: line 0: syntax error near unexpected token `('
sh: -c: line 0: `git merge-base 88d471e44f9cd5b4e5cca657747d1fcc3d82fe5f parsec-caching-improvements(18462642)'
Brian Weinstein is not an ancestor of HEAD. at /Users/anshu/code/Safari/OpenSource/Tools/Scripts/git-add-reviewer line 171."
Comment 1 Dana Burkart 2014-10-08 16:43:37 PDT
<rdar://problem/18588042>
Comment 2 Radar WebKit Bug Importer 2014-10-08 16:44:15 PDT
<rdar://problem/18590158>
Comment 3 Dana Burkart 2014-10-08 16:49:47 PDT
The fix here is to quote arguments passed to git. Additionally, let's fix that incorrect error message isAncestor() returns false.
Comment 4 Dana Burkart 2014-10-08 16:51:30 PDT
Created attachment 239500 [details]
Quote arguments to git.
Comment 5 Darin Adler 2014-10-09 09:28:09 PDT
Comment on attachment 239500 [details]
Quote arguments to git.

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

> Tools/Scripts/git-add-reviewer:311
> +    chomp(my $mergeBase = `git merge-base '$ancestor' '$descendant'`);

This relies on the names not having single quote in them. Is that guaranteed?

> Tools/Scripts/git-add-reviewer:319
> +    chomp(my $commit = `git rev-parse '$arg'`);

This relies on the argument not having a single quote in it. Is that guaranteed?
Comment 6 Dana Burkart 2014-10-09 09:36:56 PDT
(In reply to comment #5)
> (From update of attachment 239500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=239500&action=review
> 
> > Tools/Scripts/git-add-reviewer:311
> > +    chomp(my $mergeBase = `git merge-base '$ancestor' '$descendant'`);
> 
> This relies on the names not having single quote in them. Is that guaranteed?

For $ancestor, it should be guaranteed, however, for $descendant it isn't... apparently, git allows quotes in branch names

> 
> > Tools/Scripts/git-add-reviewer:319
> > +    chomp(my $commit = `git rev-parse '$arg'`);
> 
> This relies on the argument not having a single quote in it. Is that guaranteed?

Not guaranteed for $arg either. 

An alternative solution could be to replace the backticks with a call to perl's system(), which won't invoke sh and should side-step the parenthesis problem.
Comment 7 Dana Burkart 2014-10-09 09:45:41 PDT
Well, I guess system() wouldn't be a valid replacement as we need to capture the output of the command, so we would need to do something a bit more involved (or just escape any single-quotes).
Comment 8 Dana Burkart 2014-10-09 10:16:09 PDT
Also, upon second look at this script, it turns out we are *not* correctly using the return code from any of the system() calls. For example, the following system() call:

>    my $result = system "git cherry-pick -n $item->{commit} > /dev/null";
>    !$result or return fail("Failed to cherry-pick $item->{commit}");

The perl documentation for the system command states that system returns the result from the wait() system call (http://perldoc.perl.org/functions/system.html), which is the _pid_ of the subprocess, not the return value.

To get the return value, we must shift by 8, i.e.:

    my $result = system "git cherry-pick -n $item->{commit} > /dev/null";
    !($result >> 8) or return fail("Failed to cherry-pick $item->{commit}");

Maybe this is out of the scope of this bug, but it needs fixing.
Comment 9 Dana Burkart 2014-10-09 10:44:01 PDT
Created attachment 239548 [details]
Fix which doesn't use backticks at all

This patch adds a function `runCommandWithOutput` which does the same thing as backticks, but does not use sh, so we don't have any interpolation problems.

It also fixes a bunch of issues discovered with the use of the system() command in the script (see above comment).
Comment 10 Dana Burkart 2014-10-09 10:48:47 PDT
Created attachment 239549 [details]
Forgot to update ChangeLog
Comment 11 Dana Burkart 2014-10-09 14:10:57 PDT
Committed in r174526.