On Fri, Sep 19, 2025 at 09:58:25AM -0700, Junio C Hamano wrote:

For completeness, here is how the other approach may look like, but
I have made my share of off-by-one mistakes all over the place over
the ears, so somebody else needs to lend an eyeball and check it for
sanity.

I _thought_ there was an off-by-one at first, but I think what you have
is correct:

+	/*
+	 * NULL out the leftover args we did not understand, which has
+	 * shallow copies in earlier slots in the array.
+	 */
+	while (left < argc--)
+		argv[argc] = NULL;
 	return left;

We definitely want argv[left] to be NULL, which I thought at first did
not happen because of the "<". But because it is post-increment that
happens in the loop condition, it works.

I probably would have written:

  while (left <= argc)
	argv[argc--] = NULL;

which I think is the same (but I didn't test it, so it probably does
have an off-by-one!).

But really, I do not know that we need to NULL the whole thing. We have
given the caller the reduced argc. The only argv invariant we are
violating is that argv[argc] should be NULL (or in this case,
argv[left]). Anything after argv+left should be considered
uninitialized. So just:

  argv[left] = NULL;

would be enough, I'd think.

This passes now, but fails with SANITIZE=leak. Along with a bunch of
other tests, as we are now overwriting entries in strvecs with NULL, so
it has no opportunity to free them. We need to respect the
setup_revision_opt to free.

I'm working up a few alternative patches.

-Peff