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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Mar 13, 2018, 21:36 UTC
Message-ID: <20180313213628.GB147135@aiede.svl.corp.google.com>
In-Reply-To: <CAN0heSoP1oVWH0YAFNcL5LG_K7TsmKMAHUA_TiDacVdPtWjTZw@mail.gmail.com>

Martin Ågren wrote:

> On 13 March 2018 at 20:56, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Martin Ågren wrote:

Show 14 quoted lines

>>>                                                       (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?
>
> I have no idea how it would notice (portably!) and the gain seems
> minimal. I added this to keep the reader from wondering "but wait a
> minute, doesn't that mean that we fail to catch this bad usage if we're
> in a repo?". So my answer would be "yep, but it's not a huge problem".
> Of course, my attempt to pre-emptively answer a question only provoked
> another one. :-) I could phrase this better.

Ah, I think I see what I was missing. Let me look again at the whole paragraph in context:

[...]

Show 7 quoted lines

>>> Disallow left-over arguments when run from outside a repo. Another
>>> approach would be to disallow them when reading from stdin. However, our
>>> logic is currently the other way round: we check the number of revisions
>>> in order to decide whether we should read from stdin. (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.)

How about something like this?

	Disallow left-over arguments when run from outside a repo.  This
	way, such invalid usage is diagnosed correctly:

		$ git shortlog v2.16.0..
		error: [...]
		[...]

	while still permitting the piped form:

		$ git -C ~/src/git log --pretty=short | git shortlog
		A Large Angry SCM (15):
		[...]

	This cannot catch an incorrect usage that combines the piped and
	<revs> forms:

		$ git log --pretty=short | git shortlog v2.16.0..
		Alban Gruin (1)
		[...]

	but (1) the operating system does not provide a straightforward
	way to detect that and (2) at least it doesn't crash.

That detail is mostly irrelevant to what the patch does, though. I wouldn't expect git to be able to diagnose that third variant anyway. If we want to make the command less error-prone, I think a good path forward would be to introduce an explicit --stdin option. So I'd be tempted to go with the short and sweet:

	Disallow left-over arguments when run from outside a repo.

[...]

Show 18 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.
>
> I can see an argument for "dumping the usage makes the error harder than
> necessary to find". I mainly went for consistency. This ties into your
> other observations below: what little consistency do we have and in
> which direction do we want to push it...

[...]

> I think it would be a larger project to make these consistent. The one
> I'm adding here is at least consistent with the other one in this file.

Ah, thanks.  That makes sense.

Show 11 quoted lines

>> 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
>
> Some grepping suggests we do not usually name the command ("shortlog
> ..."), perhaps to play well with aliasing, nor do we use "such <syntax>"
> very often, but it does happen. Quoting and allowing for options might
> make this more correct, but perhaps less readable: "'shortlog [...]
> <revs>' can only ...". Slightly better than what I had, "revisions can
> only be given inside a git repository" would avoid some negating.

Sounds good to me.

Thanks, Jonathan