[PATCH] revision: replace "struct cmdline_pathspec" with argv_array
- From
Jeff King <peff@peff.net>
- Date
- Sep 20, 2017, 20:36 UTC
- Message-ID
- <20170920203659.xqy76bg5nfabvbfx@sigill.intra.peff.net>
- In-Reply-To
- <20170920202552.kkwhigmv7lq6cj3y@sigill.intra.peff.net>
On Wed, Sep 20, 2017 at 04:25:52PM -0400, Jeff King wrote:
Show 7 quoted lines
> Isn't this whole thing just an argv_array, and this is argv_array_pushv? > We even NULL-terminate it manually later on! > > So rather than increasing the line count by adding > free_cmdline_pathspec, I think we could actually _reduce_ it by > converting to an argv array, as below. And then adding in your free > would be one extra line.
Here it is with a commit message, and that final free added.
Sorry for stealing your patch, but I didn't want to suggest "couldn't you replace this with argv_array" without actually seeing if it was possible. At which point the patch was pretty much done.
-- >8 -- Subject: [PATCH] revision: replace "struct cmdline_pathspec" with argv_array
We assemble an array of strings in a custom struct, NULL-terminate the result, and then pass it to parse_pathspec().
But then we never free the array or the individual strings (nor can we do the latter, as they are heap-allocated when they come from stdin but not when they come from the passed-in argv).
Let's swap this out for an argv_array. It does the same thing with fewer lines of code, and it's safe to call argv_array_clear() at the end to avoid a memory leak.
Reported-by: Martin Ågren <martin.agren@gmail.com> Signed-off-by: Jeff King <peff@peff.net> --- revision.c | 39 +++++++++++---------------------------- 1 file changed, 11 insertions(+), 28 deletions(-)
diff --git a/revision.c b/revision.c index f9a90d71d2..1520f69d93 100644 --- a/revision.c +++ b/revision.c @@ -21,6 +21,7 @@ #include "bisect.h" #include "packfile.h" #include "worktree.h" +#include "argv-array.h" volatile show_early_output_fn_t show_early_output; @@ -1672,31 +1673,15 @@ int handle_revision_arg(const char *arg_, struct rev_info *revs, int flags, unsi return 0; } -struct cmdline_pathspec { - int alloc; - int nr; - const char **path; -}; - -static void append_prune_data(struct cmdline_pathspec *prune, const char **av) -{ - while (*av) { - ALLOC_GROW(prune->path, prune->nr + 1, prune->alloc); - prune->path[prune->nr++] = *(av++); - } -} - static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb, - struct cmdline_pathspec *prune) + struct argv_array *prune) { - while (strbuf_getline(sb, stdin) != EOF) { - ALLOC_GROW(prune->path, prune->nr + 1, prune->alloc); - prune->path[prune->nr++] = xstrdup(sb->buf); - } + while (strbuf_getline(sb, stdin) != EOF) + argv_array_push(prune, sb->buf); } static void read_revisions_from_stdin(struct rev_info *revs, - struct cmdline_pathspec *prune) + struct argv_array *prune) { struct strbuf sb; int seen_dashdash = 0; @@ -2286,10 +2271,9 @@ static void NORETURN diagnose_missing_default(const char *def) int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct setup_revision_opt *opt) { int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0, revarg_opt; - struct cmdline_pathspec prune_data; + struct argv_array prune_data = ARGV_ARRAY_INIT; const char *submodule = NULL; - memset(&prune_data, 0, sizeof(prune_data)); if (opt) submodule = opt->submodule; @@ -2305,7 +2289,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s argv[i] = NULL; argc = i; if (argv[i + 1]) - append_prune_data(&prune_data, argv + i + 1); + argv_array_pushv(&prune_data, argv + i + 1); seen_dashdash = 1; break; } @@ -2366,14 +2350,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s for (j = i; j < argc; j++) verify_filename(revs->prefix, argv[j], j == i); - append_prune_data(&prune_data, argv + i); + argv_array_pushv(&prune_data, argv + i); break; } else got_rev_arg = 1; } - if (prune_data.nr) { + if (prune_data.argc) { /* * If we need to introduce the magic "a lone ':' means no * pathspec whatsoever", here is the place to do so. @@ -2388,11 +2372,10 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s * call init_pathspec() to set revs->prune_data here. * } */ - ALLOC_GROW(prune_data.path, prune_data.nr + 1, prune_data.alloc); - prune_data.path[prune_data.nr++] = NULL; parse_pathspec(&revs->prune_data, 0, 0, - revs->prefix, prune_data.path); + revs->prefix, prune_data.argv); } + argv_array_clear(&prune_data); if (revs->def == NULL) revs->def = opt ? opt->def : NULL;
-- 2.14.1.1040.gcaf8795f39