Re: [PATCH 3/3] shortlog: do not accept revisions when run outside repo

[Jonathan Nieder <jrnieder@gmail.com>]

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