Am 27.10.22 um 23:46 schrieb Ævar Arnfjörð Bjarmason: > > On Thu, Oct 27 2022, René Scharfe wrote: > >> Replace the convenience functions run_command_v_opt() et. al. and use >> struct child_process and run_command() directly instead, for an overall >> code reduction and a simpler and more flexible API that allows creating >> argument lists without magic numbers and reduced risk of memory leaks. >> >> This is a broken-out and polished version of the original scratch at >> https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@xxxxxx/ >> >> Ævar Arnfjörð Bjarmason (1): >> merge: remove always-the-same "verbose" arguments >> >> René Scharfe (7): >> bisect--helper: factor out do_bisect_run() >> use child_process members "args" and "env" directly >> use child_process member "args" instead of string array variable >> replace and remove run_command_v_opt_cd_env() >> replace and remove run_command_v_opt_tr2() >> replace and remove run_command_v_opt_cd_env_tr2() >> replace and remove run_command_v_opt() > > Even though I had a an earlier alternate series series[1] for this I'd > be happy to see this version go in. I left some comments here and there, > but with/without a re-roll am happy to give this: > > Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> > > I think I would have just gone for this in the first place, but thought > that people loved the convenience functions too much. It can be hard to > judge sentiments in advance :) > > One reason I hadn't re-submitted something is that there were > outstanding new conflicts with "seen", and I see from applying this > topic & merging it that it conflicts somewhat heavily. Junio seems to be > on-board with this though, so maybe he won't mind. > > I didn't see any glaring instances where this made things worse, so > maybe we didn't need these convenience wrappers in the first place. Right, and I think this is important. > > But from the earlier discussion it does seema bit like we tossed the > very notion out because some didn't like the duplicating of struct > members with the flags (which I also doen't like). > > So I came up with the below experiment on top, it's not an attempt to > convert all callers, just a demo. > > Maybe you think some ideas here are worth using, I probably won't pursue > it (but maybe as ideas for some other future API). > > It's a combination of stuff, some of which you might like, some not, > namely: > > - Have the API work the same way, but just have a run_commandl(&opt, > ...) in addition to the run_command(). It's just a trivial wrapper to > push stuff into &cmd.args for you. > > - I saw a few callers that could have perhaps used a similarly trivial > run_commandv(), but that doesn't benefit from LAST_ARG_MUST_BE_NULL, > so I didn't bother. I thought about that as well, but at least for me is probably a just a case of loss aversion, which I have aplenty. run_command_v_opt() alone isn't bad and patch 8 on its own probably wouldn't fly, so I mentally cling to it. But without it the API is untangled and simpler. Only a single way to specify flags and arguments, no shortcuts for special combinations. Overall easier to use once we forget the old ways. > > - I wish C had a nicer syntax for not just declaring but squashing > together compile_time bracketed lists (think set operations). But the > below "CHILD_PROCESS_INIT_LIST" is a pretty good poor man's version. > > I see gcc/clang nicely give us safety rails for that with > "-Woverride-init", for this sort of "opts struct with internal stuff, > but also user options" I think it works out very nicely. > That's a nice and simple macro. I played with a gross variant à la #define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ } which would allow e.g. struct child_process cmd = CHILD_PROCESS_INIT_EX(.git_cmd = 1); Yours is better, but they share the downside of not actually saving any lines of code.. > - We have quite a few callers that want "on error, die", so maybe we > should have something like that "on_error" sooner than later. We could add a die_on_error bit for that, or start_command_or_die() and run_command_or_die() variants (there I go again, multiplying APIs..). They could report the failed command, which a caller can't do because the internal strvec is already cleared once it learns of the failure. > > On clever (but perhaps overly clever) thing I didn't use, but played > with recently in another context, is that now with C99 you can also do: > > int run_commandl(struct child_process *cmd, ...); > #define run_command(...) run_command_1(__VA_ARGS__, NULL) > > I.e. make the API itself support all of: > > run_command(&cmd); > run_command(&cmd, "reboot"); > run_command(&cmd, "reboot", NULL); > > I haven't made up my mind on whether that's just overly clever, or > outright insane :) Neither, I'd say. By combining two operations it's less flexible than a pure run method plus direct access to a strvec member. It's a shortcut that may save a line but requires more effort to extend its callers, e.g. to conditionally add a new argument. > > diff --git a/bisect.c b/bisect.c > index ec7487e6836..ef4f80650f7 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -740,9 +740,8 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev, > 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)) > + if (run_commandl(&cmd, "checkout", "-q", > + oid_to_hex(bisect_rev), "--", NULL)) > /* > * Errors in `run_command()` itself, signaled by res < 0, > * and errors in the child process, signaled by res > 0 > diff --git a/builtin/am.c b/builtin/am.c > index 20aea0d2487..3b7df32ce22 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -2189,10 +2189,9 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode) > if (!is_null_oid(&state->orig_commit)) { > struct child_process cmd = CHILD_PROCESS_INIT; > > - strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit), > - "--", NULL); > cmd.git_cmd = 1; > - return run_command(&cmd); > + return run_commandl(&cmd, "show", oid_to_hex(&state->orig_commit), > + "--", NULL); > } > > switch (sub_mode) { > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 1d2ce8a0e12..087d21c614a 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -764,12 +764,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a > strbuf_read_file(&start_head, git_path_bisect_start(), 0); > strbuf_trim(&start_head); > if (!no_checkout) { > - struct child_process cmd = CHILD_PROCESS_INIT; > - > - cmd.git_cmd = 1; > - strvec_pushl(&cmd.args, "checkout", start_head.buf, > - "--", NULL); > - if (run_command(&cmd)) { > + struct child_process cmd = { > + CHILD_PROCESS_INIT_LIST, > + .git_cmd = 1, > + }; > + if (run_commandl(&cmd, "checkout", start_head.buf, > + "--", NULL)) { > res = error(_("checking out '%s' failed." > " Try 'git bisect start " > "<valid-branch>'."), > @@ -1147,8 +1147,7 @@ static int do_bisect_run(const char *command) > > printf(_("running %s\n"), command); > cmd.use_shell = 1; > - strvec_push(&cmd.args, command); > - return run_command(&cmd); > + return run_commandl(&cmd, command, NULL); > } > > static int verify_good(const struct bisect_terms *terms, const char *command) > diff --git a/builtin/clone.c b/builtin/clone.c > index 0e4348686b6..80d09e0fbf1 100644 > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -655,7 +655,6 @@ static int git_sparse_checkout_init(const char *repo) > { > struct child_process cmd = CHILD_PROCESS_INIT; > int result = 0; > - strvec_pushl(&cmd.args, "-C", repo, "sparse-checkout", "set", NULL); > > /* > * We must apply the setting in the current process > @@ -664,7 +663,7 @@ static int git_sparse_checkout_init(const char *repo) > core_apply_sparse_checkout = 1; > > cmd.git_cmd = 1; > - if (run_command(&cmd)) { > + if (run_commandl(&cmd, "-C", repo, "sparse-checkout", "set", NULL)) { > error(_("failed to initialize sparse-checkout")); > result = 1; > } > @@ -868,13 +867,14 @@ static void dissociate_from_references(void) > char *alternates = git_pathdup("objects/info/alternates"); > > if (!access(alternates, F_OK)) { > - struct child_process cmd = CHILD_PROCESS_INIT; > - > - cmd.git_cmd = 1; > - cmd.no_stdin = 1; > - strvec_pushl(&cmd.args, "repack", "-a", "-d", NULL); > - if (run_command(&cmd)) > - die(_("cannot repack to clean up")); > + struct child_process cmd = { > + CHILD_PROCESS_INIT_LIST, > + .git_cmd = 1, > + .no_stdin = 1, > + .on_error = CHILD_PROCESS_ON_ERROR_DIE, > + }; > + > + run_commandl(&cmd, "repack", "-a", "-d", NULL); > if (unlink(alternates) && errno != ENOENT) > die_errno(_("cannot unlink temporary alternates file")); > } > diff --git a/builtin/difftool.c b/builtin/difftool.c > index d7f08c8a7fa..b4165b5a8ae 100644 > --- a/builtin/difftool.c > +++ b/builtin/difftool.c > @@ -44,11 +44,12 @@ static int difftool_config(const char *var, const char *value, void *cb) > > static int print_tool_help(void) > { > - struct child_process cmd = CHILD_PROCESS_INIT; > - > - cmd.git_cmd = 1; > - strvec_pushl(&cmd.args, "mergetool", "--tool-help=diff", NULL); > - return run_command(&cmd); > + struct child_process cmd = { > + CHILD_PROCESS_INIT_LIST, > + .git_cmd = 1, > + }; > + > + return run_commandl(&cmd, "mergetool", "--tool-help=diff", NULL); > } > > static int parse_index_info(char *p, int *mode1, int *mode2, > diff --git a/git.c b/git.c > index 6662548986f..93179f44f78 100644 > --- a/git.c > +++ b/git.c > @@ -724,7 +724,13 @@ static void handle_builtin(int argc, const char **argv) > > static void execv_dashed_external(const char **argv) > { > - struct child_process cmd = CHILD_PROCESS_INIT; > + struct child_process cmd = { > + CHILD_PROCESS_INIT_LIST, > + .clean_on_exit = 1, > + .wait_after_clean = 1, > + .silent_exec_failure = 1, > + .trace2_child_class = "dashed", > + }; > int status; > > if (get_super_prefix()) > @@ -736,10 +742,6 @@ static void execv_dashed_external(const char **argv) > > strvec_pushf(&cmd.args, "git-%s", argv[0]); > strvec_pushv(&cmd.args, argv + 1); > - cmd.clean_on_exit = 1; > - cmd.wait_after_clean = 1; > - cmd.silent_exec_failure = 1; > - cmd.trace2_child_class = "dashed"; > > trace2_cmd_name("_run_dashed_"); > > diff --git a/run-command.c b/run-command.c > index 23e100dffc4..4b20aa1b577 100644 > --- a/run-command.c > +++ b/run-command.c > @@ -993,15 +993,45 @@ int finish_command_in_signal(struct child_process *cmd) > > int run_command(struct child_process *cmd) > { > - int code; > + int starting = 1; > + int code = 0; > > if (cmd->out < 0 || cmd->err < 0) > BUG("run_command with a pipe can cause deadlock"); > > code = start_command(cmd); > if (code) > + goto error; > + starting = 0; > + code = finish_command(cmd); > + if (!code) > + return 0; > +error: > + switch (cmd->on_error) { > + case CHILD_PROCESS_ON_ERROR_DIE: > + die(starting ? > + _("start_command() for '%s' failed (error code %d)") : > + _("'%s': got non-zero exit code '%d'"), > + cmd->args.v[0], code); > + break; > + case CHILD_PROCESS_ON_ERROR_RETURN: > return code; > - return finish_command(cmd); > + default: > + BUG("unreachable"); > + } > +} > + > +int run_commandl(struct child_process *cmd, ...) > +{ > + va_list ap; > + const char *arg; > + > + va_start(ap, cmd); > + while ((arg = va_arg(ap, const char *))) > + strvec_push(&cmd->args, arg); > + va_end(ap); > + > + return run_command(cmd); > } > > #ifndef NO_PTHREADS > diff --git a/run-command.h b/run-command.h > index fe2717ad11e..71e390350ed 100644 > --- a/run-command.h > +++ b/run-command.h > @@ -15,7 +15,22 @@ > * produces in the caller in order to process it. > */ > > +enum child_process_on_error { > + /** > + * Return a status code from run_command(). Set to 0 so that > + * it'll be { 0 } init'd. If it's specified in a > + * CHILD_PROCESS_INIT_LIST initialization we don't want a > + * "-Woverride-init" warning. > + */ > + CHILD_PROCESS_ON_ERROR_RETURN = 0, > > + /** > + * die() with some sensible message if run_command() would > + * have returned a non-zero exit code. > + */ > + CHILD_PROCESS_ON_ERROR_DIE, > +}; > + > /** > * This describes the arguments, redirections, and environment of a > * command to run in a sub-process. > @@ -42,6 +57,10 @@ > * redirected. > */ > struct child_process { > + /** > + * Error behavior, see "enum child_process_on_error" above. > + */ > + enum child_process_on_error on_error; > > /** > * The .args is a `struct strvec', use that API to manipulate > @@ -144,10 +163,11 @@ struct child_process { > void (*clean_on_exit_handler)(struct child_process *process); > }; > > -#define CHILD_PROCESS_INIT { \ > +#define CHILD_PROCESS_INIT_LIST \ > + /* .on_error = CHILD_PROCESS_ON_ERROR_RETURN */ \ > .args = STRVEC_INIT, \ > - .env = STRVEC_INIT, \ > -} > + .env = STRVEC_INIT > +#define CHILD_PROCESS_INIT { CHILD_PROCESS_INIT_LIST } > > /** > * The functions: child_process_init, start_command, finish_command, > @@ -218,6 +238,14 @@ int finish_command_in_signal(struct child_process *); > */ > int run_command(struct child_process *); > > +/** > + * Like run_command() but takes a variable number of arguments, which > + * will be appended with the equivalent of strvec_pushl(&cmd.args, > + * ...) before invoking run_command(). > + */ > +LAST_ARG_MUST_BE_NULL > +int run_commandl(struct child_process *cmd, ...); > + > /* > * Trigger an auto-gc > */
