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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 13, 2018, 19:56 UTC
Message-ID: <20180313195640.GA147135@aiede.svl.corp.google.com>
In-Reply-To: <78669e644b64fc10c34adb59717d2039f81cb092.1520680894.git.martin.agren@gmail.com>

Hi,

Martin Ågren wrote:

Show 7 quoted lines

> 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?

[...]

Show 10 quoted lines

> --- 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/

[...]

Show 7 quoted lines

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

Show 5 quoted lines

> +		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