range-diff: slight usability problem with mistyped ref

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

Hi Kristoffer,

On Sat, 25 Mar 2023, Kristoffer Haugsbakk wrote:

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

Hi Johannes and thanks for your reply

On Tue, Mar 28, 2023, at 13:37, Johannes Schindelin wrote:

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.

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.

Hi Kristoffer,

On Mon, 3 Apr 2023, Kristoffer Haugsbakk wrote:

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