Re: [PATCH 6/6] revision: retain argv NULL invariant in setup_revisions()
From: Jeff King <hidden>
Date: 2025-09-19 23:07:22
Subsystem:
the rest · Maintainer:
Linus Torvalds
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.
I _think_ that would probably look like this on top (with obvious inspiration from your earlier patch), but I don't know if it is worthwhile or not:
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)