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

[Jeff King <peff@peff.net>]

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