range-diff: slight usability problem with mistyped ref

range-diff: slight usability problem with mistyped ref

[Kristoffer Haugsbakk]

I was using `git range-diff`:[1]

    $ git range-diff main feature-v2 HEAD

And got an error message:

    fatal: need two commit ranges

I intended to give three arguments to the command. I started to wonder
if I maybe had just typed `main feature-v2` (forgot the third
argument). No, it was three arguments.

The problem was that I had mistyped one argument; it was `feature-v3`,
not `feature-v2`.

I thought that this error message was a little misleading or too
generic. I looked at the history of the file (`builtin/range-diff.c`)
and saw this commit: b75747829f (range-diff: optionally accept
pathspecs, 2022-08-26).

Maybe it works better with a `pathspec` marker (`--`)?

    $ git range-diff HEAD main feature-v2 --
    fatal: not a revision: 'feature-v2'

    usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>
       or: git range-diff [<options>] <old-tip>...<new-tip>
       or: git range-diff [<options>] <base> <old-tip> <new-tip>

Yes it does: `not a revision: 'feature-v2'`.

So I tried to contrast the behavior on the current release with the
behavior on the release before the aforementioned commit.

    $ git checkout v2.40.0
    $ make clean
    $ NO_CURL=true make -j 4
    $ # Misspelled ref `seen` as `seent`
    $ ./bin-wrappers/git range-diff master next seent
    fatal: need two commit ranges

    usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>
       or: git range-diff [<options>] <old-tip>...<new-tip>
       or: git range-diff [<options>] <base> <old-tip> <new-tip>
    […]

Expected behavior: tell me that `seent` is not a revision.

Actual behavior: generic error message.

But I get a nice error message if I append `--`:

    $ ./bin-wrappers/git range-diff master next seent --
    fatal: not a revision: 'seent'

    usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>
       or: git range-diff [<options>] <old-tip>...<new-tip>
       or: git range-diff [<options>] <base> <old-tip> <new-tip>

Contrast the behavior on `v2.37.2`, which is one of the tags before
rev. b75747829f (range-diff: optionally accept pathspecs,
2022-08-26).

    $ git checkout v2.37.2
    $ make clean
    $ NO_CURL=true make -j 4
    $ ./bin-wrappers/git range-diff master next seent
    fatal: not a revision: 'seent'

    usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>
       or: git range-diff [<options>] <old-tip>...<new-tip>
       or: git range-diff [<options>] <base> <old-tip> <new-tip>
    […]

    $ # Tag before rev. b75747829f (range-diff: optionally
    $ # accept pathspecs, 2022-08-26)
    $ git checkout v2.37.2
    $ ./bin-wrappers/git range-diff HEAD master seent
    fatal: ambiguous argument 'HEAD..seent': unknown revision or path not in the working tree.
    Use '--' to separate paths from revisions, like this:
    'git <command> [<revision>...] -- [<file>...]'
    error: could not parse log for 'HEAD..seent'

This error message is exactly what I expect to see when I mistype a ref
(my git(1) conditioning).

But the `v2.40.0` error message with `--` is better: `fatal: not a
revision: 'seent'`.

So what if I give a `--`:

    $ # Still on v2.37.2
    $ ./bin-wrappers/git range-diff master next seent --
    fatal: ambiguous argument 'master..seent': unknown revision or path not in the working tree.
    Use '--' to separate paths from revisions, like this:
    'git <command> [<revision>...] -- [<file>...]'
    error: could not parse log for 'master..seent'

It’s the same. Oh, right—I guess rev. b75747829f taught `range-diff` 
about `--`.

In conclusion: IMO and assuming that my cross-version testing is
correct, `range-diff` has a slight usability regression for when you
mistype the ref. It would be nice if the error message without a
pathspec separator (`--`/`dash_dash`) was as nice as the one without it.

[1]: Original thread: https://groups.google.com/g/git-users/c/O5uB-E68S_0

-- 
Kristoffer Haugsbakk

Re: range-diff: slight usability problem with mistyped ref

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 2042 bytes --]

Hi Kristoffer,

On Sat, 25 Mar 2023, Kristoffer Haugsbakk wrote:

>     $ # Misspelled ref `seen` as `seent`
>     $ ./bin-wrappers/git range-diff master next seent
>     fatal: need two commit ranges
>
>     usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>
>        or: git range-diff [<options>] <old-tip>...<new-tip>
>        or: git range-diff [<options>] <base> <old-tip> <new-tip>
>     […]
>
> Expected behavior: tell me that `seent` is not a revision.
>
> Actual behavior: generic error message.
>
> [...]
>
> In conclusion: IMO and assuming that my cross-version testing is
> correct, `range-diff` has a slight usability regression for when you
> mistype the ref. It would be nice if the error message without a
> pathspec separator (`--`/`dash_dash`) was as nice as the one without it.

I can see how the error message is confusing. At the same time, the usage
below the error message should provide an indicator what forms are
applicable (even if all of the synopses are missing the `[--] [<path>...]`
part).

Now, it seems to be very, very tricky to address your concern properly.
The reason is that:

	git range-diff a.x b c

could have a typo where the user actually meant to say `a...x`, i.e. the
symmetric range form. Or the user might have meant `a..x` and the full
history of `b` with a file `c`. Or `a.x` was a mistyped ref name and the
three-commit form was intended.

So even making the exact error message depend on the number of arguments
could result in misleading error message.

We _could_ extend the `else` arm in
https://github.com/git/git/blob/v2.40.0/builtin/range-diff.c#L142-L144 to
try to parse up to the first three arguments as commit-ishs and report for
which argument that fails, of course, but even that is subject to
ambiguities: what if the third argument happens to match both a ref and a
file?

Do you have any splendid idea how to phrase the error message (or adapt
it to the concrete invocation)?

Ciao,
Johannes

Re: range-diff: slight usability problem with mistyped ref

[Kristoffer Haugsbakk]

Hi Johannes and thanks for your reply

On Tue, Mar 28, 2023, at 13:37, Johannes Schindelin wrote:
> So even making the exact error message depend on the number of
> arguments could result in misleading error message.

Yeah, I see. It seems that the variadic nature of the command makes it
difficult to guess what the user might have meant in all cases.

> Do you have any splendid idea how to phrase the error message (or
> adapt it to the concrete invocation)?

No. I was going to look more closely at that if-else chain, but given
the fact that I don’t know C and all the cases that would need to be
covered (“what if the third argument happens to match both a ref and a
file?”) I wouldn’t get anywhere.

-- 
Kristoffer Haugsbakk

Re: range-diff: slight usability problem with mistyped ref

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1851 bytes --]

Hi Kristoffer,

On Mon, 3 Apr 2023, Kristoffer Haugsbakk wrote:

> On Tue, Mar 28, 2023, at 13:37, Johannes Schindelin wrote:
> > So even making the exact error message depend on the number of
> > arguments could result in misleading error message.
>
> Yeah, I see. It seems that the variadic nature of the command makes it
> difficult to guess what the user might have meant in all cases.
>
> > Do you have any splendid idea how to phrase the error message (or
> > adapt it to the concrete invocation)?
>
> No. I was going to look more closely at that if-else chain, but given
> the fact that I don’t know C and all the cases that would need to be
> covered (“what if the third argument happens to match both a ref and a
> file?”) I wouldn’t get anywhere.

Knowledge of C is not required to come up with an error message ;-)

The forms in which `git range-diff` can be called are:

	git range-diff A...B [[--] <path>...]
	git range-diff A..B C..D [[--] <path>...]
	git range-diff A B C [[--] <path>...]

In general, I find it hard to guess which form was meant when a typo
causes an error message. For example, `git range-diff A..B C` might have
been intended to be the three-rev form `A B C`, but it could also have
been intended to pass two commit ranges `A..B HEAD..C`.

As such, I wonder whether we can just cleverly _avoid_ saying _which_ form
we suspect the user to have intended.

In other words, instead of

	need two commit ranges

we could say:

	`range-diff` needs three revs, two ranges or a symmetric range

Since this does not require any C expertise, it would probably be a good
idea to let C experts work on other patches, patches that do require their
expertise: The patch to improve the error message does not require any
knowledge of the C language at all.

Ciao,
Johannes