Bug 137541 – Errors using git-add-reviewer on a branch whose name includes parentheses

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dana Burkart]

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

[Dana Burkart]

<rdar://problem/18588042>

[Radar WebKit Bug Importer]

<rdar://problem/18590158>

[Dana Burkart]

The fix here is to quote arguments passed to git. Additionally, let's fix that incorrect error message isAncestor() returns false.

[Dana Burkart]

Created attachment 239500 [details]
Quote arguments to git.

[Darin Adler]

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?

[Dana Burkart]

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

[Dana Burkart]

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

[Dana Burkart]

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.

[Dana Burkart]

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

[Dana Burkart]

Created attachment 239549 [details]
Forgot to update ChangeLog

[Dana Burkart]

Committed in r174526.