Re: [PATCH 2/2] revision: handle argv movement in parse_revision_opt()

On Sat, Sep 26, 2026 at 11:00:05AM +0200, Kristoffer Haugsbakk wrote:

quoted

Reported-by: Kristoffer Haugsbakk <redacted>

I personally prefer the email that I use for commits:

[off-list ref]

(Which has always been the case. But I didn’t want to disrupt the
process previously.)

OK. I pulled it from your From header, of course. :)

I ought to send in a `.mailmap` change with my canonical email address.

We don't mailmap trailers, though. I have a patch to let you do so with
%(trailers:mailmap), but you'd still see the original most of the time
(since git-log, etc, just dump the raw contents and expect the trailers
to be readable).

quoted

+test_expect_success 'unknown revision options are reported correctly' '
+	test_must_fail git shortlog -n --no-such-option 2>err &&
+	test_grep "unknown option .*--no-such-option" err
+'

Just thinking this through. This is a regression test indirectly related
to git-shortlog(1). So the test does not name `shortlog`, so that’s good.
The subtlety of the previously discussed:

    Making things even more confusing, it only happens if there's
    another option before the unknown one!

is not obvious from the test description, but one can surmise that it is
needed since it’s there in the first place.

Yeah. I sort of assume that anybody wondering about the details of a
line of code in this project will be able to dig around with blame or
pickaxe. Perhaps a comment could help, but I think anything beyond "it
is important that there are two options here" would end up re-hashing
the whole explanation in the commit message.

For the next readers of this test suite that come along, it might seem
strange that this specific sequence is tested for, and on a shortlog
test suite. But it seems normal in this project to add tests that, in
the context of the file alone, might not be obvious why they are there
(because they are regression tests for very specific bugs). I could
imagine some system where regression tests are marked with some
identifier that however indirectly links back to whatever triggered the
fix. But for one, this would be a new system/convention and wouldn’t
make sense to use on just one test. And second, this would just make it
more directly accessible; it is still directly accessible for people who
know how to query git(1). Well, maybe more indirectly as time goes on if
the test is changed and you use the “pickaxe” technique.

Yeah, exactly. Both patches are really bugs in the revision /
parse-options integration function that just happens to be triggerable
by shortlog. Possibly something like t0040 would make sense, but it
feels weird to be sticking a shortlog invocation there. I dunno. Again,
I sort of rely on people to find the relevant commits.

This is all to say that this test makes sense as it is written now.

Thanks!

-Peff