My favorites | Sign in
Project Home Downloads Wiki Issues Source
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 800: "gerrit review" command line does not parse args as advertised
11 people starred this issue and may be notified of changes. Back to list
Status:  Released
Owner:  ----
Closed:  Apr 2011


Sign in to add a comment
 
Reported by di...@google.com, Dec 14, 2010
Affected Version: 2.1.5

What steps will reproduce the problem?
1. Run "gerrit review --help" server command.
2. Look at the help output:
$ gerrit review --help
gerrit review {COMMIT | CHANGE,PATCHSET} ... [--] [--help (-h)] [--message (-m) MESSAGE] [--project (-p) PROJECT] [--submit (-s)] [--code-review N]
...
3. Try to follow the documented syntax:
$ gerrit review <change-id1> <change-id2> -- -s

What is the expected output? What do you see instead?
Should try to submit the specified changes. Instead it errors:
fatal: "-s" is not a valid patch set

Please provide any additional information below.
It seems that the documented syntax is not matching what the code implements. It seems that the code actually implements this:
gerrit review [--help (-h)] [--message (-m) MESSAGE] [--project (-p)
PROJECT] [--submit (-s)] [--code-review N] [--] {COMMIT | CHANGE,PATCHSET}

Mar 30, 2011
#1 Maciej.k...@gmail.com
Moreover, this works:
$ ssh -p 29418 login@host gerrit review --message "Test" 91e15ac4f6732ff95456b79139861b8de49903e1 

while this do not:
$ ssh -p 29418 login@host gerrit review --message "Test asd"  91e15ac4f6732ff95456b79139861b8de49903e1 
fatal: "asd" is not a valid patch set

on Gerrit 2.1.6.1
Mar 30, 2011
#2 di...@google.com
That's an shell/SSH escaping issue, try --message '"Test asd"'.
Mar 31, 2011
#3 Maciej.k...@gmail.com
confirmed: Now it works ok. I think that the example of the use of double quotes should be included in the doc, because it is not so obvious.
Apr 4, 2011
#4 sop@google.com
Documentation updated in commit 9c0cfc22f626a7e9a70c121ff0cf389edfd23894,
will be in 2.1.7.
Status: Submitted
Labels: FixedIn-2.1.7
May 31, 2011
#5 sop@google.com
(No comment was entered for this change.)
Status: Released
Jun 22, 2011
#6 di...@google.com
This issue is not resolved. The mentioned fix commit (9c0cfc22f626a7e9a70c121ff0cf389edfd23894) has nothing to do with the reported issue here. It just adds some "gerrit review" example to cmd-review.txt (and it seems like it is a broken example, it uses "gerrit review" without specifying a commit/change-id to review). 

The issue here is about the "gerrit review --help" output that does not match what Gerrit expects (the change SHA1s/IDs are expected at the end of the command not in the beginning). I'll make a patch and send it upstream.
Jun 22, 2011
#7 di...@google.com
OK, so I found the Gerrit commit that I think broke all this (if I revert some of it I get command parsing behavior as advertised, where the commits are expected in the beginning, before an optional "--"). Commit 9ead06f5256cff45f879d99babeeb09ed17f14f9 has changed the commands syntax if "--" is used. By disabling option parsing once a "--" is encountered args4j will take anything after "--" to be elements of the variable length arguments defined before it. But that's different from the advertised command line interface that still says it expects the arguments to be first:
gerrit review {COMMIT | CHANGE,PATCHSET} ... [--] [--help (-h)] [--message (-m) MESSAGE] [--project (-p) PROJECT] [--submit (-s)] [--code-review N]

Notice that this has broken "--help" for other commands too such as "gerrit query" that now expects the variable number predicates to be at the end of the command if "--" is being used.

Things seem to be working as advertised if "--" is not being used.

The help message needs fixing but I have no idea how to do that as it seems to be generated automatically by "args4j".
Jun 22, 2011
#8 di...@google.com
To make things more clear, the actual expected command syntax is:
<command> <arguments>... [--option1] [--option2] ... [-- <arguments>...]

As it can still take arguments before any options but after (if) "--" has been found it will expect only arguments.
Sign in to add a comment

Powered by Google Project Hosting