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

[Jeff King <peff@peff.net>]

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

> > Reported-by: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
> 
> I personally prefer the email that I use for commits:
> 
> <code@khaugsbakk.name>
> 
> (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).

> > +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