{"thread":{"id":"59473","subject":"range-diff: slight usability problem with mistyped ref","startedAt":"2023-03-25T10:04:41Z","lastAt":"2023-04-04T20:20:25Z","messageCount":4,"participants":["Kristoffer Haugsbakk","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"474165","messageId":"b93934a2-91e7-4645-9a24-4f2354172f31@app.fastmail.com","threadId":"59473","inReplyTo":null,"subject":"range-diff: slight usability problem with mistyped ref","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-03-25T10:04:12Z","receivedAt":"2023-03-25T10:04:41Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"I was using `git range-diff`:[1]\n\n    $ git range-diff main feature-v2 HEAD\n\nAnd got an error message:\n\n    fatal: need two commit ranges\n\nI intended to give three arguments to the command. I started to wonder\nif I maybe had just typed `main feature-v2` (forgot the third\nargument). No, it was three arguments.\n\nThe problem was that I had mistyped one argument; it was `feature-v3`,\nnot `feature-v2`.\n\nI thought that this error message was a little misleading or too\ngeneric. I looked at the history of the file (`builtin/range-diff.c`)\nand saw this commit: b75747829f (range-diff: optionally accept\npathspecs, 2022-08-26).\n\nMaybe it works better with a `pathspec` marker (`--`)?\n\n    $ git range-diff HEAD main feature-v2 --\n    fatal: not a revision: 'feature-v2'\n\n    usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>\n       or: git range-diff [<options>] <old-tip>...<new-tip>\n       or: git range-diff [<options>] <base> <old-tip> <new-tip>\n\nYes it does: `not a revision: 'feature-v2'`.\n\nSo I tried to contrast the behavior on the current release with the\nbehavior on the release before the aforementioned commit.\n\n    $ git checkout v2.40.0\n    $ make clean\n    $ NO_CURL=true make -j 4\n    $ # Misspelled ref `seen` as `seent`\n    $ ./bin-wrappers/git range-diff master next seent\n    fatal: need two commit ranges\n\n    usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>\n       or: git range-diff [<options>] <old-tip>...<new-tip>\n       or: git range-diff [<options>] <base> <old-tip> <new-tip>\n    […]\n\nExpected behavior: tell me that `seent` is not a revision.\n\nActual behavior: generic error message.\n\nBut I get a nice error message if I append `--`:\n\n    $ ./bin-wrappers/git range-diff master next seent --\n    fatal: not a revision: 'seent'\n\n    usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>\n       or: git range-diff [<options>] <old-tip>...<new-tip>\n       or: git range-diff [<options>] <base> <old-tip> <new-tip>\n\nContrast the behavior on `v2.37.2`, which is one of the tags before\nrev. b75747829f (range-diff: optionally accept pathspecs,\n2022-08-26).\n\n    $ git checkout v2.37.2\n    $ make clean\n    $ NO_CURL=true make -j 4\n    $ ./bin-wrappers/git range-diff master next seent\n    fatal: not a revision: 'seent'\n\n    usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>\n       or: git range-diff [<options>] <old-tip>...<new-tip>\n       or: git range-diff [<options>] <base> <old-tip> <new-tip>\n    […]\n\n    $ # Tag before rev. b75747829f (range-diff: optionally\n    $ # accept pathspecs, 2022-08-26)\n    $ git checkout v2.37.2\n    $ ./bin-wrappers/git range-diff HEAD master seent\n    fatal: ambiguous argument 'HEAD..seent': unknown revision or path not in the working tree.\n    Use '--' to separate paths from revisions, like this:\n    'git <command> [<revision>...] -- [<file>...]'\n    error: could not parse log for 'HEAD..seent'\n\nThis error message is exactly what I expect to see when I mistype a ref\n(my git(1) conditioning).\n\nBut the `v2.40.0` error message with `--` is better: `fatal: not a\nrevision: 'seent'`.\n\nSo what if I give a `--`:\n\n    $ # Still on v2.37.2\n    $ ./bin-wrappers/git range-diff master next seent --\n    fatal: ambiguous argument 'master..seent': unknown revision or path not in the working tree.\n    Use '--' to separate paths from revisions, like this:\n    'git <command> [<revision>...] -- [<file>...]'\n    error: could not parse log for 'master..seent'\n\nIt’s the same. Oh, right—I guess rev. b75747829f taught `range-diff` \nabout `--`.\n\nIn conclusion: IMO and assuming that my cross-version testing is\ncorrect, `range-diff` has a slight usability regression for when you\nmistype the ref. It would be nice if the error message without a\npathspec separator (`--`/`dash_dash`) was as nice as the one without it.\n\n[1]: Original thread: https://groups.google.com/g/git-users/c/O5uB-E68S_0\n\n-- \nKristoffer Haugsbakk\n"},{"id":"474273","messageId":"818b3ad0-df43-3484-8c19-d95026f6b2b1@gmx.de","threadId":"59473","inReplyTo":"b93934a2-91e7-4645-9a24-4f2354172f31@app.fastmail.com","subject":"Re: range-diff: slight usability problem with mistyped ref","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-03-28T11:37:57Z","receivedAt":"2023-03-28T11:38:07Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kristoffer,\n\nOn Sat, 25 Mar 2023, Kristoffer Haugsbakk wrote:\n\n>     $ # Misspelled ref `seen` as `seent`\n>     $ ./bin-wrappers/git range-diff master next seent\n>     fatal: need two commit ranges\n>\n>     usage: git range-diff [<options>] <old-base>..<old-tip> <new-base>..<new-tip>\n>        or: git range-diff [<options>] <old-tip>...<new-tip>\n>        or: git range-diff [<options>] <base> <old-tip> <new-tip>\n>     […]\n>\n> Expected behavior: tell me that `seent` is not a revision.\n>\n> Actual behavior: generic error message.\n>\n> [...]\n>\n> In conclusion: IMO and assuming that my cross-version testing is\n> correct, `range-diff` has a slight usability regression for when you\n> mistype the ref. It would be nice if the error message without a\n> pathspec separator (`--`/`dash_dash`) was as nice as the one without it.\n\nI can see how the error message is confusing. At the same time, the usage\nbelow the error message should provide an indicator what forms are\napplicable (even if all of the synopses are missing the `[--] [<path>...]`\npart).\n\nNow, it seems to be very, very tricky to address your concern properly.\nThe reason is that:\n\n\tgit range-diff a.x b c\n\ncould have a typo where the user actually meant to say `a...x`, i.e. the\nsymmetric range form. Or the user might have meant `a..x` and the full\nhistory of `b` with a file `c`. Or `a.x` was a mistyped ref name and the\nthree-commit form was intended.\n\nSo even making the exact error message depend on the number of arguments\ncould result in misleading error message.\n\nWe _could_ extend the `else` arm in\nhttps://github.com/git/git/blob/v2.40.0/builtin/range-diff.c#L142-L144 to\ntry to parse up to the first three arguments as commit-ishs and report for\nwhich argument that fails, of course, but even that is subject to\nambiguities: what if the third argument happens to match both a ref and a\nfile?\n\nDo you have any splendid idea how to phrase the error message (or adapt\nit to the concrete invocation)?\n\nCiao,\nJohannes\n"},{"id":"474710","messageId":"de43cd37-bfa5-4dbc-84d5-9362e9af6a9a@app.fastmail.com","threadId":"59473","inReplyTo":"818b3ad0-df43-3484-8c19-d95026f6b2b1@gmx.de","subject":"Re: range-diff: slight usability problem with mistyped ref","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-04-03T19:29:17Z","receivedAt":"2023-04-03T19:29:56Z","isPatch":false,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"Hi Johannes and thanks for your reply\n\nOn Tue, Mar 28, 2023, at 13:37, Johannes Schindelin wrote:\n> So even making the exact error message depend on the number of\n> arguments could result in misleading error message.\n\nYeah, I see. It seems that the variadic nature of the command makes it\ndifficult to guess what the user might have meant in all cases.\n\n> Do you have any splendid idea how to phrase the error message (or\n> adapt it to the concrete invocation)?\n\nNo. I was going to look more closely at that if-else chain, but given\nthe fact that I don’t know C and all the cases that would need to be\ncovered (“what if the third argument happens to match both a ref and a\nfile?”) I wouldn’t get anywhere.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"474806","messageId":"b8e9b054-4cd5-9c65-a397-088143d2fdc4@gmx.de","threadId":"59473","inReplyTo":"de43cd37-bfa5-4dbc-84d5-9362e9af6a9a@app.fastmail.com","subject":"Re: range-diff: slight usability problem with mistyped ref","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-04-04T20:20:15Z","receivedAt":"2023-04-04T20:20:25Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Kristoffer,\n\nOn Mon, 3 Apr 2023, Kristoffer Haugsbakk wrote:\n\n> On Tue, Mar 28, 2023, at 13:37, Johannes Schindelin wrote:\n> > So even making the exact error message depend on the number of\n> > arguments could result in misleading error message.\n>\n> Yeah, I see. It seems that the variadic nature of the command makes it\n> difficult to guess what the user might have meant in all cases.\n>\n> > Do you have any splendid idea how to phrase the error message (or\n> > adapt it to the concrete invocation)?\n>\n> No. I was going to look more closely at that if-else chain, but given\n> the fact that I don’t know C and all the cases that would need to be\n> covered (“what if the third argument happens to match both a ref and a\n> file?”) I wouldn’t get anywhere.\n\nKnowledge of C is not required to come up with an error message ;-)\n\nThe forms in which `git range-diff` can be called are:\n\n\tgit range-diff A...B [[--] <path>...]\n\tgit range-diff A..B C..D [[--] <path>...]\n\tgit range-diff A B C [[--] <path>...]\n\nIn general, I find it hard to guess which form was meant when a typo\ncauses an error message. For example, `git range-diff A..B C` might have\nbeen intended to be the three-rev form `A B C`, but it could also have\nbeen intended to pass two commit ranges `A..B HEAD..C`.\n\nAs such, I wonder whether we can just cleverly _avoid_ saying _which_ form\nwe suspect the user to have intended.\n\nIn other words, instead of\n\n\tneed two commit ranges\n\nwe could say:\n\n\t`range-diff` needs three revs, two ranges or a symmetric range\n\nSince this does not require any C expertise, it would probably be a good\nidea to let C experts work on other patches, patches that do require their\nexpertise: The patch to improve the error message does not require any\nknowledge of the C language at all.\n\nCiao,\nJohannes\n"}]}