Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo
From: Jonathan Nieder <hidden>
Date: 2018-03-13 19:57:14
Hi, Martin Ågren wrote:
If we are outside a repo and have any arguments left after option-parsing, `setup_revisions()` will try to do its job and something like this will happen: $ git shortlog v2.16.0.. BUG: environment.c:183: git environment hasn't been setup Aborted (core dumped)
Yikes. Thanks for fixing it. [...]
(So yes, after this patch, we will still silently ignore stdin for confused usage such as `git log v2.15.0.. | git shortlog v2.16.0..`. But at least that does not crash.)
I don't follow here. Are you saying this command should notice that there is input in stdin? How would it notice? [...]
quoted hunk ↗ jump to hunk
--- a/t/t4201-shortlog.sh +++ b/t/t4201-shortlog.sh@@ -127,6 +127,11 @@ test_expect_success !MINGW 'shortlog can read --format=raw output' ' test_cmp expect out ' +test_expect_success 'shortlog from non-git directory refuses revisions' ' + test_must_fail env GIT_DIR=non-existing git shortlog HEAD 2>out && + test_i18ngrep "no revisions can be given" out +'
\o/ [...]
quoted hunk ↗ jump to hunk
--- a/builtin/shortlog.c +++ b/builtin/shortlog.c@@ -293,6 +293,12 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix) parse_done: argc = parse_options_end(&ctx); + if (nongit && argc != 1) {
Just curious: would argc ever be 0 here? 'argc <= 1' might be clearer.
+ error(_("no revisions can be given when running "
+ "from outside a repository"));
+ usage_with_options(shortlog_usage, options);
+ }
+
The error message is
error: no revisions can be given when running from outside a repository
usage: ...
Do we need to dump usage here? I wonder if a simple die() call would
be easier for the user to act on.
Not about this patch: I was a little surprised to see 'error:' instead
of 'usage:' or 'fatal:'. It turns out git is pretty inconsistent
about that: e.g. there is
error(_("no remote specified"));
usage_with_options(builtin_remote_setbranches_usage, options);
Some other callers just use usage_with_options without describing the
error. check-attr has a private error_with_usage() helper to implement
the error() followed by usage_with_options() idiom. Most callers just
use die(), like
die(_("'%s' cannot be used with %s"), "--merge", "--patch");
Documentation/technical/api-error-handling.txt says
- `usage` is for errors in command line usage. After printing its
message, it exits with status 129. (See also `usage_with_options`
in the link:api-parse-options.html[parse-options API].)
which is not prescriptive enough to help.
Separate from that, I wonder if the error message can be made a little
shorter and clearer. E.g.
fatal: shortlog <revs> can only be used inside a git repository
Thanks and hope that helps,
Jonathan