On Sat, Oct 15, 2022 at 08:51:34AM +0200, René Scharfe wrote:
> 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.
> 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).
> -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.
> 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
