Re: [PATCH v2] bisect--helper: plug strvec leak
- From
Jeff King <peff@peff.net>
- Date
- Oct 15, 2022, 18:21 UTC
- Message-ID
- <Y0r6LaxDLjpiRNqS@coredump.intra.peff.net>
- In-Reply-To
- <9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de>
On Sat, Oct 15, 2022 at 08:51:34AM +0200, René Scharfe wrote:
Show 8 quoted lines
> Am 14.10.22 um 21:44 schrieb Jeff King: > > And I'd be happy to see all of the run_command_v() variants go away > > entirely. It does save a few lines, but with modern niceties, it's > > not very many, and it's much less flexible. > That would look something like below. > > [...] > 27 files changed, 330 insertions(+), 374 deletions(-)
I was a little surprised we ended up with fewer lines, but I guess most of that is not in the callers themselves, but deleting the function and flag definitions.
Most call sites seem to be about as long. IMHO the result is quite readable, though of course any big change like this carries risk of regression.
Show 25 quoted lines
> diff --git a/add-interactive.c b/add-interactive.c
> index f071b2a1b4..ecc5ae1b24 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -997,18 +997,17 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
> count = list_and_choose(s, files, opts);
> opts->flags = 0;
> if (count > 0) {
> - struct strvec args = STRVEC_INIT;
> + struct child_process cmd = CHILD_PROCESS_INIT;
>
> - strvec_pushl(&args, "git", "diff", "-p", "--cached",
> + strvec_pushl(&cmd.args, "git", "diff", "-p", "--cached",
> oid_to_hex(!is_initial ? &oid :
> s->r->hash_algo->empty_tree),
> "--", NULL);
> for (i = 0; i < files->items.nr; i++)
> if (files->selected[i])
> - strvec_push(&args,
> + strvec_push(&cmd.args,
> files->items.items[i].string);
> - res = run_command_v_opt(args.v, 0);
> - strvec_clear(&args);
> + res = run_command(&cmd);
> }So ones like this are just swapping "args" for "cmd.args". It's nice that we don't have to remember to free (and in some cases, that lets us return directly rather than stashing the result of run_command(), cleaning up, and then returning it).
Show 29 quoted lines
> -static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
> -
> static const char *term_bad;
> static const char *term_good;
>
> @@ -729,20 +727,22 @@ static int is_expected_rev(const struct object_id *oid)
> enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
> int no_checkout)
> {
> - char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
> struct commit *commit;
> struct pretty_print_context pp = {0};
> struct strbuf commit_msg = STRBUF_INIT;
>
> - oid_to_hex_r(bisect_rev_hex, bisect_rev);
> update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
>
> - argv_checkout[2] = bisect_rev_hex;
> if (no_checkout) {
> update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
> UPDATE_REFS_DIE_ON_ERR);
> } else {
> - if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
> + struct child_process cmd = CHILD_PROCESS_INIT;
> +
> + cmd.git_cmd = 1;
> + strvec_pushl(&cmd.args, "checkout", "-q",
> + oid_to_hex(bisect_rev), "--", NULL);
> + if (run_command(&cmd))This one is a little longer, but IMHO worth it regardless to avoid the magic "2" that must match the NULL stuffed into argv_checkout.
That is mostly coming from using a strvec at all, though. It _could_ use one and then run_command_v_opt() or whatever, though I like the full conversion.
Show 30 quoted lines
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 5900b81729..5e14decc22 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -347,58 +347,52 @@ static int save_state(struct object_id *stash)
>
> static void read_empty(const struct object_id *oid, int verbose)
> {
> - int i = 0;
> - const char *args[7];
> + struct child_process cmd = CHILD_PROCESS_INIT;
>
> - args[i++] = "read-tree";
> + strvec_push(&cmd.args, "read-tree");
> if (verbose)
> - args[i++] = "-v";
> - args[i++] = "-m";
> - args[i++] = "-u";
> - args[i++] = empty_tree_oid_hex();
> - args[i++] = oid_to_hex(oid);
> - args[i] = NULL;
> + strvec_push(&cmd.args, "-v");
> + strvec_pushl(&cmd.args, "-m", "-u", empty_tree_oid_hex(),
> + oid_to_hex(oid), NULL);
>
> - if (run_command_v_opt(args, RUN_GIT_CMD))
> + cmd.git_cmd = 1;
> + if (run_command(&cmd))
> die(_("read-tree failed"));
> }Likewise this one, which is even worse, as that "7" must match the number of "i++" lines. So regardless of any run_command_v() conversion, I am happy to see this move to a strvec (and again, IMHO you might as well go straight to using cmd.args rather than a separate strvec).
-Peff