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?
[...]
> --- 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/
[...]
> --- 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
