(the subject is my preliminary speculation)

     Thank you for filling out a Git bug report!
     Please answer the following questions to help us understand your
     issue.

     What did you do before the bug happened? (Steps to reproduce your
     issue)

``` git shortlog -n --not-an-option master ```

Note that you need a real option like `-n` before the `--not-an-option`. Or else it will work correctly.

    What did you expect to happen? (Expected behavior)

This error:

``` error: unknown option `--not-an-option' [usage printout] ```

    What happened instead? (Actual behavior)

This error:

``` error: unknown option `(null)' [usage printout] ```

    What's different between what you expected and what actually
    happened?

I expected it to print the option in quotes. Instead it printed `(null)` which I think is the placeholder for when the `%s` arg is `NULL`.

    Anything else you want to add:

I have tested and reproduced on:

• master: 0f8e75ab (Revert "Merge branch
  'en/no-amend-during-conflicts'", 2026-09-23)
• seen: e844e042 (Merge branch 'je/doc-merge-conflicts' into seen,
  2026-09-24)
• next: d58861e6 (Revert "Merge branch 'gg/http-ssl-verify-status' into
  next", 2026-09-23)

I have bisected this to cd439487 (revision: manage memory ownership of argv in setup_revisions(), 2025-09-19).

Note that the recent topic jk/rev-info-argv-to-free fixes issues caused by commit cd439487, but that topic does not change how this option handling behaves; the topic is part of `master` now which I tested. I also tested on top of the topic and got the same `(null)' result.

    Please review the rest of the bug report below.
    You can delete any lines you don't wish to share.

[System Info]
git version:
git version 2.55.0.793.gc667de3f2c5
cpu: x86_64
built from commit: c667de3f2c5e43830a8dfaa79a27d5d1f106f0ab
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
rust: enabled
feature: fsmonitor--daemon
gettext: enabled
libcurl: 7.81.0
OpenSSL: OpenSSL 3.0.2 15 Mar 2022
zlib: 1.2.11
SHA-1: SHA1_DC
SHA-256: SHA256_BLK
default-ref-format: files
default-hash: sha1
uname: Linux 6.8.0-138-generic #138~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Fri Aug  7 13:43:15 UTC  x86_64
compiler info: gnuc: 11.4
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks] commit-msg post-applypatch post-checkout post-commit sendemail-validate

On Fri, Sep 25, 2026 at 02:28:57AM +0200, Kristoffer Haugsbakk wrote:

Yep, definitely my fault. I don't have time to do a full write-up now, but the most direct solution is:

On Fri, Sep 25, 2026 at 04:26:36AM -0400, Jeff King wrote:

I ended up reversing my decision there, for reasons that are explained in the second commit. But the good news is that doing so also revealed a related bug that predates even cd439487.

So here are the fixes. I apologize in advance for the length of the second commit message. At least I feel good about my decision to go to bed before writing it. ;)

  [1/2]: revision: avoid reporting known options as unknown on error
  [2/2]: revision: handle argv movement in parse_revision_opt()

 revision.c          |  7 +++++--
 t/t4201-shortlog.sh | 11 +++++++++++
 2 files changed, 16 insertions(+), 2 deletions(-)

-Peff

In parse_revision_opt(), we report "unknown option" when handle_revision_opt() returns a negative value or zero. But these are two different conditions: a negative value indicates a malformed option (like a missing argument) and zero indicates an unknown option.

So malformed options produce nonsense like this:

  $ git shortlog --default
  error: bad --default argument
  error: unknown option `--default'
  usage: [...]

We should treat a negative return as a logic error which has already been reported by handle_revision_opt(), and just show the regular usage message.

Signed-off-by: Jeff King <peff@peff.net>
---
There's obviously a way to write this that makes the diff a little
shorter and doesn't repeat the usage_with_options() line. I think the
if/else cascade makes the mental model more clear, though.

 revision.c          | 5 ++++-
 t/t4201-shortlog.sh | 6 ++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

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

This works fine for setup_revisions() itself, but the underlying handle_revision_opt() has another entry point: parse_revision_opt(). This lets a parse-options user parse a single revision option, but the movement introduced by cd43948798 confuses its error code path. If we see an unknown option, then handle_revision_opt() will move it out of the way (to the "unknown options" section) and return an error. But parse_revision_opt() then tries to access the original argv location, which has now been set to NULL, and you get:

  $ git shortlog -n --no-such-option
  error: unknown option `(null)'

Whereas prior to cd43948798, it would have been a leftover copy of the pointer (that may or may not eventually get written over, but was valid for this immediate message). And you get what you'd expect:

  $ git shortlog -n --no-such-option
  error: unknown option `--no-such-option'

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

  $ git shortlog --no-such-option

we "consolidate" to the exact same spot, and no movement occurs at all.

Note that we use shortlog in these examples because it is one of only two commands that use the parse_revision_opt() interface (the other is blame).

There are a few options for fixing this. One is that we can observe that the "move" semantics introduced by cd43948798 only matter when the argv strings are allocated on the heap, in which case the caller passes in the free_removed_argv_elements flag to tell us. But we never use that flag with parse_revision_opt(). So we could do something like this:

  diff --git a/revision.c b/revision.c
  index ee1df92d1d..7b858d54c1 100644
  --- a/revision.c
  +++ b/revision.c
  @@ -2340,7 +2340,8 @@ static void overwrite_argv(int *argc, const char **argv,
   	if (*value != argv[*argc]) {
   		mark_argv_for_free(opt, revs, argv[*argc]);
   		argv[*argc] = *value;
  -		*value = NULL;
  +		if (opt && opt->free_removed_argv_elements)
  +			*value = NULL;
   	}
   	(*argc)++;
   }

to restore the pre-cd43948798 semantics when heap-allocated strings are not in use. We'd just keep the extra pointer in the original location, but nobody cares because they're not going to free anything anyway. That's enough to fix this case, and could fix any other theoretical cases we haven't noticed. The downside is that it's an accident waiting to happen if we ever do teach parse_revision_opt() to handle allocated argv strings.

But are there other theoretical cases? I don't think so. The code paths touched by cd43948798 are either in setup_revisions() itself (which also learned how to handle this movement) or in handle_revision_opt(), the low-level static helper. It has only two callers: setup_revisions() itself, and parse_revision_opt() in which we see the current breakage. So fixing parse_revision_opt() should cover all of our bases, and keep the code ready for a potential future change to handle allocated strings.

The fix is just to tell parse_revision_opt() to look for the unknown option in the consolidated destination rather than the original location. We might write to that consolidated location for other reasons (like moving pseudo-revision options like "--all"), but there is only one code path that returns the 0 for an unknown option, and it always moves the option before doing so. So the "end" of that consolidated area will always have our unknown option.

This patch implements that solution and demonstrates the breakage and fix using shortlog.

Reported-by: Kristoffer Haugsbakk <kristofferhaugsbakk@fastmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Obviously another possible fix is for parse_revision_opt() to record the
string before passing it along, and use that for its error message. That
seemed clunkier to me.

 revision.c          | 2 +-
 t/t4201-shortlog.sh | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

Jeff King <peff@peff.net> writes:

Both patches make sense.

: git; rungit v2.52.0 shortlog -n --no-such-option 2>&1 | head -n1 error: unknown option `(null)' : git; rungit v2.51.0 shortlog -n --no-such-option 2>&1 | head -n1 error: unknown option `--no-such-option' : git; ./git shortlog -n --no-such-option 2>&1 | head -n1 error: unknown option `--no-such-option'

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

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

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

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.

Thanks for fixing. :)

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

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

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

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.

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.

Thanks!

-Peff

 On Mon, Sep 28, 2026, at 05:25, Jeff King wrote:

Yes, that’s the reason. There’s no dynamic remapping after the fact. But with a mailmap entry you can ask git-check-mailmap(1) if you have the correct entry while writing or normalizing the commit.

I had use for that once when I was collecting `Reported-by`. One report was from three years prior. But in the meantime, this person had changed their address. But they had recorded it in the `.mailmap`.

But for such a normalization to be workable at all I would have to try to add something like `--evaluate-all-cmds` to git-interpret- trailers(1). Because `trailer.<key-alias>.cmd` will not evaluate trailer values if it is just reading in a file without any `--trailer` options (as well).

***

Which is an itch that no one else has to care about.