Martin Ågren wrote:
> On 13 March 2018 at 20:56, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
>> Martin Ågren wrote:
>>> (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:
[...]
>>> 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.
[...]
>>> + 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.
>> 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
