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

On Fri, Sep 25, 2026, at 22:39, Jeff King wrote:

The argument parser used by setup_revisions() modifies the argv array
that is passed to it, consolidating non-options and unknown options at
the start of the array. This led to problems with memory leaks when argv
pointed to allocated strings. We addressed that in cd43948798 (revision:
manage memory ownership of argv in setup_revisions(), 2025-09-19). Now
instead of copying strings to the earlier part of argv, we actually move
them, setting the original location to NULL (so that we know we have
exactly one pointer to the string).

[snip]

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.)

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

[snip]
+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.

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.

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

+
 test_done
--
2.56.0.rc2.289.g137cf50cac

Thanks for fixing. :)