[BUG] revision: premature free-and-null causes “unknown option `(null)`”

[BUG] revision: premature free-and-null causes “unknown option `(null)`”

[Kristoffer Haugsbakk]

(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

-- 
  Kristoffer Haugsbakk
  kristofferhaugsbakk@fastmail.com

Re: [BUG] revision: premature free-and-null causes “unknown option `(null)`”

[Jeff King]

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

> git shortlog -n --not-an-option master
> [...]
>
> 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`.
> [...]
> 
> I have bisected this to cd439487 (revision: manage memory ownership of
> argv in setup_revisions(), 2025-09-19).

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

diff --git a/revision.c b/revision.c
index ee1df92d1d..501a4ba36e 100644
--- a/revision.c
+++ b/revision.c
@@ -2768,13 +2768,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 			const struct option *options,
 			const char * const usagestr[])
 {
 	int n = handle_revision_opt(revs, ctx->argc, ctx->argv,
 				    &ctx->cpidx, ctx->out, NULL);
 	if (n <= 0) {
-		error("unknown option `%s'", ctx->argv[0]);
+		error("unknown option `%s'", ctx->out[ctx->cpidx - 1]);
 		usage_with_options(usagestr, options);
 	}
 	ctx->argv += n;
 	ctx->argc -= n;
 }

But I think instead doing 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)++;
 }

will restore some of the hidden assumptions made by pre-cd439487 code.
So it would fix this case, along with any other lurkers.

I know that's probably quite opaque. ;) I'll fully explain what's going
on in a follow-up tomorrow, but I wanted to post the solution quickly so
nobody else wasted time digging.

Thanks for a clear report.

-Peff

[PATCH 0/2] some parse_revision_opt() bugfixes

[Jeff King]

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

> > I have bisected this to cd439487 (revision: manage memory ownership of
> > argv in setup_revisions(), 2025-09-19).
> 
> Yep, definitely my fault. I don't have time to do a full write-up now,
> but the most direct solution is:
> [...]
> But I think instead doing this:

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

[PATCH 1/2] revision: avoid reporting known options as unknown on error

[Jeff King]

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

diff --git a/revision.c b/revision.c
index ee1df92d1d..f958d8c301 100644
--- a/revision.c
+++ b/revision.c
@@ -2771,7 +2771,10 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 {
 	int n = handle_revision_opt(revs, ctx->argc, ctx->argv,
 				    &ctx->cpidx, ctx->out, NULL);
-	if (n <= 0) {
+	if (n < 0) {
+		/* handle_revision_opt() has already reported the error. */
+		usage_with_options(usagestr, options);
+	} else if (!n) {
 		error("unknown option `%s'", ctx->argv[0]);
 		usage_with_options(usagestr, options);
 	}
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 023fbff546..4ba7f5aec6 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -436,4 +436,10 @@ test_expect_success 'stdin with multiple groups reports error' '
 	test_must_fail git shortlog --group=author --group=committer <log
 '
 
+test_expect_success 'invalid revision options are not reported as unknown' '
+	test_must_fail git shortlog --default 2>err &&
+	test_grep "bad --default argument" err &&
+	test_grep ! "unknown option" err
+'
+
 test_done
-- 
2.56.0.rc2.289.g137cf50cac

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

[Jeff King]

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

diff --git a/revision.c b/revision.c
index f958d8c301..a83e499047 100644
--- a/revision.c
+++ b/revision.c
@@ -2775,7 +2775,7 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
 		/* handle_revision_opt() has already reported the error. */
 		usage_with_options(usagestr, options);
 	} else if (!n) {
-		error("unknown option `%s'", ctx->argv[0]);
+		error("unknown option `%s'", ctx->out[ctx->cpidx - 1]);
 		usage_with_options(usagestr, options);
 	}
 	ctx->argv += n;
diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 4ba7f5aec6..10c43e6e75 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -442,4 +442,9 @@ test_expect_success 'invalid revision options are not reported as unknown' '
 	test_grep ! "unknown option" err
 '
 
+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
+'
+
 test_done
-- 
2.56.0.rc2.289.g137cf50cac

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

[Kristoffer Haugsbakk]

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

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

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

[Jeff King]

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

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

[Kristoffer Haugsbakk]

 On Mon, Sep 28, 2026, at 05:25, Jeff King wrote:
> On Sat, Sep 26, 2026 at 11:00:05AM +0200, Kristoffer Haugsbakk wrote:
>> I ought to send in a `.mailmap` change with my canonical email address.
>
> We don't mailmap trailers, though. [...]

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.

Re: [PATCH 0/2] some parse_revision_opt() bugfixes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Sep 25, 2026 at 04:26:36AM -0400, Jeff King wrote:
>
>> > I have bisected this to cd439487 (revision: manage memory ownership of
>> > argv in setup_revisions(), 2025-09-19).
>> 
>> Yep, definitely my fault. I don't have time to do a full write-up now,
>> but the most direct solution is:
>> [...]
>> But I think instead doing this:
>
> 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(-)

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'