Re: [PATCH 6/6] revision: retain argv NULL invariant in setup_revisions()

From

Jeff King <peff@peff.net>

Date

Sep 19, 2025, 23:07 UTC

Message-ID

<20250919230721.GA936044@coredump.intra.peff.net>

In-Reply-To

<20250919225146.GF594545@coredump.intra.peff.net>

On Fri, Sep 19, 2025 at 06:51:46PM -0400, Jeff King wrote:

> It is tempting to do likewise for all of the entries afterwards, too, as
> some of them may also need to be freed (e.g., if coming from a strvec).
> But doing so isn't entirely trivial, as we munge argc in the function
> (e.g., when we find "--" and move all of the entries after it into the
> prune_data list). It would be possible with some light refactoring, but
> it's probably not worth it. Nobody should ever look at them (they are
> beyond the revised argc and past the NULL argv entry) outside of strvec
> cleanup, and setup_revisions_from_strvec() already handles this case.

diff --git a/revision.c b/revision.c
index 806a1c4c24..96188ab4ad 100644
--- a/revision.c
+++ b/revision.c
@@ -3003,14 +3003,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		seen_dashdash = 0;
 		for (i = 1; i < argc; i++) {
 			const char *arg = argv[i];
+			int j;
 			if (strcmp(arg, "--"))
 				continue;
-			if (opt && opt->free_removed_argv_elements)
-				free((char *)argv[i]);
-			argv[i] = NULL;
+			for (j = i; j < argc; j++) {
+				if (i != j)
+					strvec_push(&prune_data, argv[j]);
+				if (opt && opt->free_removed_argv_elements)
+					free((char *)argv[j]);
+				argv[j] = NULL;
+			}
 			argc = i;
-			if (argv[i + 1])
-				strvec_pushv(&prune_data, argv + i + 1);
 			seen_dashdash = 1;
 			break;
 		}
@@ -3192,10 +3195,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 		revs->show_notes_given = 1;
 	}
 
-	if (argv) {
+	while (left < argc--) {
 		if (opt && opt->free_removed_argv_elements)
-			free((char *)argv[left]);
-		argv[left] = NULL;
+			free((char *)argv[argc]);
+		argv[argc] = NULL;
 	}
 
 	return left;
@@ -3205,19 +3208,14 @@ void setup_revisions_from_strvec(struct strvec *argv, struct rev_info *revs,
 				 struct setup_revision_opt *opt)
 {
 	struct setup_revision_opt fallback_opt;
-	int ret;
 
 	if (!opt) {
 		memset(&fallback_opt, 0, sizeof(fallback_opt));
 		opt = &fallback_opt;
 	}
 	opt->free_removed_argv_elements = 1;
 
-	ret = setup_revisions(argv->nr, argv->v, revs, opt);
-
-	for (size_t i = ret; i < argv->nr; i++)
-		free((char *)argv->v[i]);
-	argv->nr = ret;
+	argv->nr = setup_revisions(argv->nr, argv->v, revs, opt);
 }
 
 static void release_revisions_cmdline(struct rev_cmdline_info *cmdline)