| Message ID | 0e889c96-6004-96e4-9505-19ce1e7f9900@web.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | run-command: remove run_command_v_*() | expand |
On Thu, Oct 27 2022, René Scharfe wrote: > @@ -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)) > /* > * Errors in `run_command()` itself, signaled by res < 0, > * and errors in the child process, signaled by res > 0 Perhaps I went overboard with it in my version, but it's probably worth mentioning when converting some of these that the reason for the pre-image of some is really not like the others. Now that we're on C99 it perhaps make s no difference, but the pre-image here is explicitly trying to avoid dynamic initializer elements, per 442c27dde78 (CodingGuidelines: mention dynamic C99 initializer elements, 2022-10-10). Well, partially, some of it appears to just be based on a misunderstanding of how our own APIs work, i.e. the use of oid_to_hex_r() over oid_to_hex(). > diff --git a/builtin/am.c b/builtin/am.c > index 39fea24833..20aea0d248 100644 > --- a/builtin/am.c > +++ b/builtin/am.c > @@ -2187,14 +2187,12 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode) > int len; > > if (!is_null_oid(&state->orig_commit)) { > - const char *av[4] = { "show", NULL, "--", NULL }; > - char *new_oid_str; > - int ret; > + struct child_process cmd = CHILD_PROCESS_INIT; > > - av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit)); > - ret = run_command_v_opt(av, RUN_GIT_CMD); > - free(new_oid_str); > - return ret; > + strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit), > + "--", NULL); > + cmd.git_cmd = 1; > + return run_command(&cmd); > } The same goes for this, FWIW I split this one out into its own commit (I left the earlier one alone): https://lore.kernel.org/git/patch-v2-04.10-5cfd6a94ce3-20221017T170316Z-avarab@gmail.com/; It uses the same pattern > diff --git a/builtin/difftool.c b/builtin/difftool.c > index 4b10ad1a36..22bcc3444b 100644 > --- a/builtin/difftool.c > +++ b/builtin/difftool.c > @@ -360,8 +360,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, > struct pair_entry *entry; > struct index_state wtindex; > struct checkout lstate, rstate; > - int flags = RUN_GIT_CMD, err = 0; > - const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL }; > + int err = 0; > + struct child_process cmd = CHILD_PROCESS_INIT; In general, I like the disection of this series, but with this... > struct hashmap wt_modified, tmp_modified; > int indices_loaded = 0; > > @@ -563,16 +563,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, > } > > strbuf_setlen(&ldir, ldir_len); > - helper_argv[1] = ldir.buf; > strbuf_setlen(&rdir, rdir_len); > - helper_argv[2] = rdir.buf; > > if (extcmd) { > - helper_argv[0] = extcmd; > - flags = 0; > - } else > + strvec_push(&cmd.args, extcmd); > + } else { > + strvec_push(&cmd.args, "difftool--helper"); > + cmd.git_cmd = 1; ...and the frequent occurance of just e.g. "cmd.git_cmd = 1" and nothing else I'm wondering if we're not throwing the baby out with the bath water in having no convenience wrappers or macros at all. A lot of your 3-lines would be 1 lines if we just had e.g. (untested, and could be a function not a macro, but you get the idea): #define run_command_git_simple(__VA_ARGS__) \ struct child_process cmd = CHILD_PROCESS_INIT; \ cmd.git_cmd = 1; \ strvec_pushl(&cmd.args, __VA_ARGS__); \ run_command(&cmd); But maybe nobody except me thinks that's worthwhile... > static void read_empty(const struct object_id *oid) > { > - int i = 0; > - const char *args[7]; > - > - args[i++] = "read-tree"; > - args[i++] = "-m"; > - args[i++] = "-u"; > - args[i++] = empty_tree_oid_hex(); > - args[i++] = oid_to_hex(oid); > - args[i] = NULL; > + struct child_process cmd = CHILD_PROCESS_INIT; > + > + strvec_pushl(&cmd.args, "read-tree", "-m", "-u", empty_tree_oid_hex(), > + oid_to_hex(oid), NULL); > + cmd.git_cmd = 1; > > - if (run_command_v_opt(args, RUN_GIT_CMD)) > + if (run_command(&cmd)) > die(_("read-tree failed")); > } > > static void reset_hard(const struct object_id *oid) > { > - int i = 0; > - const char *args[6]; > - > - args[i++] = "read-tree"; > - args[i++] = "-v"; > - args[i++] = "--reset"; > - args[i++] = "-u"; > - args[i++] = oid_to_hex(oid); > - args[i] = NULL; > + struct child_process cmd = CHILD_PROCESS_INIT; > + > + strvec_pushl(&cmd.args, "read-tree", "-v", "--reset", "-u", > + oid_to_hex(oid), NULL); > + cmd.git_cmd = 1; > > - if (run_command_v_opt(args, RUN_GIT_CMD)) > + if (run_command(&cmd)) > die(_("read-tree failed")); > } Two perfect examples, e.g. the former would just be: if (run_command_git_simple("read-tree", "-m", "-u", empty_tree_oid_hex(), oid_to_hex(oid), NULL)) die(...);
Am 27.10.22 um 23:09 schrieb Ævar Arnfjörð Bjarmason: > > On Thu, Oct 27 2022, René Scharfe wrote: > >> @@ -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)) >> /* >> * Errors in `run_command()` itself, signaled by res < 0, >> * and errors in the child process, signaled by res > 0 > > Perhaps I went overboard with it in my version, but it's probably worth > mentioning when converting some of these that the reason for the > pre-image of some is really not like the others. > > Now that we're on C99 it perhaps make s no difference, but the pre-image > here is explicitly trying to avoid dynamic initializer elements, per > 442c27dde78 (CodingGuidelines: mention dynamic C99 initializer elements, > 2022-10-10). True, some cases could be converted to string array initializations, which also would get rid of magic numbers. This would make the final patch to convert them to run_command() longer. > Well, partially, some of it appears to just be based on a > misunderstanding of how our own APIs work, i.e. the use of > oid_to_hex_r() over oid_to_hex(). > >> diff --git a/builtin/am.c b/builtin/am.c >> index 39fea24833..20aea0d248 100644 >> --- a/builtin/am.c >> +++ b/builtin/am.c >> @@ -2187,14 +2187,12 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode) >> int len; >> >> if (!is_null_oid(&state->orig_commit)) { >> - const char *av[4] = { "show", NULL, "--", NULL }; >> - char *new_oid_str; >> - int ret; >> + struct child_process cmd = CHILD_PROCESS_INIT; >> >> - av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit)); >> - ret = run_command_v_opt(av, RUN_GIT_CMD); >> - free(new_oid_str); >> - return ret; >> + strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit), >> + "--", NULL); >> + cmd.git_cmd = 1; >> + return run_command(&cmd); >> } > > The same goes for this, FWIW I split this one out into its own commit (I > left the earlier one alone): > https://lore.kernel.org/git/patch-v2-04.10-5cfd6a94ce3-20221017T170316Z-avarab@gmail.com/; > It uses the same pattern OK, I just chalked that up as "slightly odd" and bulldozed over them without a second thought. Hmm. > >> diff --git a/builtin/difftool.c b/builtin/difftool.c >> index 4b10ad1a36..22bcc3444b 100644 >> --- a/builtin/difftool.c >> +++ b/builtin/difftool.c >> @@ -360,8 +360,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, >> struct pair_entry *entry; >> struct index_state wtindex; >> struct checkout lstate, rstate; >> - int flags = RUN_GIT_CMD, err = 0; >> - const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL }; >> + int err = 0; >> + struct child_process cmd = CHILD_PROCESS_INIT; > > In general, I like the disection of this series, but with this... > >> struct hashmap wt_modified, tmp_modified; >> int indices_loaded = 0; >> >> @@ -563,16 +563,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, >> } >> >> strbuf_setlen(&ldir, ldir_len); >> - helper_argv[1] = ldir.buf; >> strbuf_setlen(&rdir, rdir_len); >> - helper_argv[2] = rdir.buf; >> >> if (extcmd) { >> - helper_argv[0] = extcmd; >> - flags = 0; >> - } else >> + strvec_push(&cmd.args, extcmd); >> + } else { >> + strvec_push(&cmd.args, "difftool--helper"); >> + cmd.git_cmd = 1; > > ...and the frequent occurance of just e.g. "cmd.git_cmd = 1" and nothing > else I'm wondering if we're not throwing the baby out with the bath > water in having no convenience wrappers or macros at all. > > A lot of your 3-lines would be 1 lines if we just had e.g. (untested, > and could be a function not a macro, but you get the idea): > > #define run_command_git_simple(__VA_ARGS__) \ > struct child_process cmd = CHILD_PROCESS_INIT; \ > cmd.git_cmd = 1; \ > strvec_pushl(&cmd.args, __VA_ARGS__); \ > run_command(&cmd); > > But maybe nobody except me thinks that's worthwhile... I have similar temptations; you could see that in my scratch patch https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/ which added run_git_or_die() in builtin/gc.c. Why, oh why? Perhaps because taking a blank form (CHILD_PROCESS_INIT), ticking boxes (.git_cmd = 1), filling out text fields (strvec_push(...)) and submitting it (run_command()) feels tedious and bureaucratic, Java-esque even. And some patterns appear again and again. How bad is that? Is it bad at all? I think overall we should try to reduce the number of external calls and make those we have to do self-documenting and leak-free. A bit of tedium is OK; this API should be used rarely and sparingly. Still I get the urge to search for patterns and define shortcuts when I see all those similar calls.. run_command_git_simple as defined above wouldn't compile, but I get it. Reducing the number of lines feels good, but it also makes the code less flexible -- adding a conditional parameter requires converting back to run_command(). > >> static void read_empty(const struct object_id *oid) >> { >> - int i = 0; >> - const char *args[7]; >> - >> - args[i++] = "read-tree"; >> - args[i++] = "-m"; >> - args[i++] = "-u"; >> - args[i++] = empty_tree_oid_hex(); >> - args[i++] = oid_to_hex(oid); >> - args[i] = NULL; >> + struct child_process cmd = CHILD_PROCESS_INIT; >> + >> + strvec_pushl(&cmd.args, "read-tree", "-m", "-u", empty_tree_oid_hex(), >> + oid_to_hex(oid), NULL); >> + cmd.git_cmd = 1; >> >> - if (run_command_v_opt(args, RUN_GIT_CMD)) >> + if (run_command(&cmd)) >> die(_("read-tree failed")); >> } >> >> static void reset_hard(const struct object_id *oid) >> { >> - int i = 0; >> - const char *args[6]; >> - >> - args[i++] = "read-tree"; >> - args[i++] = "-v"; >> - args[i++] = "--reset"; >> - args[i++] = "-u"; >> - args[i++] = oid_to_hex(oid); >> - args[i] = NULL; >> + struct child_process cmd = CHILD_PROCESS_INIT; >> + >> + strvec_pushl(&cmd.args, "read-tree", "-v", "--reset", "-u", >> + oid_to_hex(oid), NULL); >> + cmd.git_cmd = 1; >> >> - if (run_command_v_opt(args, RUN_GIT_CMD)) >> + if (run_command(&cmd)) >> die(_("read-tree failed")); >> } > > Two perfect examples, e.g. the former would just be: > > if (run_command_git_simple("read-tree", "-m", "-u", empty_tree_oid_hex(), > oid_to_hex(oid), NULL)) > die(...);
On Thu, Oct 27, 2022 at 06:37:52PM +0200, René Scharfe wrote: > @@ -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); I was wondering if this part of the conversion was right, since oid_to_hex() uses a ring of output buffers (see hash_to_hex_algop()). But we do call xstrdup() on the argument from strvec_push(), so we are OK here. Thanks, Taylor
On Fri, Oct 28, 2022 at 04:23:31PM +0200, René Scharfe wrote: > > A lot of your 3-lines would be 1 lines if we just had e.g. (untested, > > and could be a function not a macro, but you get the idea): > > > > #define run_command_git_simple(__VA_ARGS__) \ > > struct child_process cmd = CHILD_PROCESS_INIT; \ > > cmd.git_cmd = 1; \ > > strvec_pushl(&cmd.args, __VA_ARGS__); \ > > run_command(&cmd); > > > > But maybe nobody except me thinks that's worthwhile... > > I have similar temptations; you could see that in my scratch patch > https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/ > which added run_git_or_die() in builtin/gc.c. Why, oh why? Perhaps > because taking a blank form (CHILD_PROCESS_INIT), ticking boxes > (.git_cmd = 1), filling out text fields (strvec_push(...)) and > submitting it (run_command()) feels tedious and bureaucratic, Java-esque > even. And some patterns appear again and again. > > How bad is that? Is it bad at all? I think overall we should try to > reduce the number of external calls and make those we have to do > self-documenting and leak-free. A bit of tedium is OK; this API should > be used rarely and sparingly. Still I get the urge to search for > patterns and define shortcuts when I see all those similar calls.. > > run_command_git_simple as defined above wouldn't compile, but I get it. > Reducing the number of lines feels good, but it also makes the code less > flexible -- adding a conditional parameter requires converting back to > run_command(). For what it's worth, I agree. I don't think there are or should be any hard and fast rules about when extracting a pattern like this into a function or macro is right. But here it feels wrong and counterproductive to the goal of this series. My main gripe with it is that it seems to be overfit to these small handful of calls, and that changing it in the future would require us to go and update existing callers to accommodate new functionality in other callers. So I'd prefer to see it left alone. Thanks, Taylor
diff --git a/bisect.c b/bisect.c index fd581b85a7..ec7487e683 100644 --- a/bisect.c +++ b/bisect.c @@ -22,8 +22,6 @@ static struct oid_array skipped_revs; static struct object_id *current_bad_oid; -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)) /* * 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 39fea24833..20aea0d248 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2187,14 +2187,12 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode) int len; if (!is_null_oid(&state->orig_commit)) { - const char *av[4] = { "show", NULL, "--", NULL }; - char *new_oid_str; - int ret; + struct child_process cmd = CHILD_PROCESS_INIT; - av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit)); - ret = run_command_v_opt(av, RUN_GIT_CMD); - free(new_oid_str); - return ret; + strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit), + "--", NULL); + cmd.git_cmd = 1; + return run_command(&cmd); } switch (sub_mode) { diff --git a/builtin/difftool.c b/builtin/difftool.c index 4b10ad1a36..22bcc3444b 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -360,8 +360,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, struct pair_entry *entry; struct index_state wtindex; struct checkout lstate, rstate; - int flags = RUN_GIT_CMD, err = 0; - const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL }; + int err = 0; + struct child_process cmd = CHILD_PROCESS_INIT; struct hashmap wt_modified, tmp_modified; int indices_loaded = 0; @@ -563,16 +563,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, } strbuf_setlen(&ldir, ldir_len); - helper_argv[1] = ldir.buf; strbuf_setlen(&rdir, rdir_len); - helper_argv[2] = rdir.buf; if (extcmd) { - helper_argv[0] = extcmd; - flags = 0; - } else + strvec_push(&cmd.args, extcmd); + } else { + strvec_push(&cmd.args, "difftool--helper"); + cmd.git_cmd = 1; setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1); - ret = run_command_v_opt(helper_argv, flags); + } + strvec_pushl(&cmd.args, ldir.buf, rdir.buf, NULL); + ret = run_command(&cmd); /* TODO: audit for interaction with sparse-index. */ ensure_full_index(&wtindex); diff --git a/builtin/merge.c b/builtin/merge.c index 864808f51a..b3f75f55c8 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -347,33 +347,25 @@ static int save_state(struct object_id *stash) static void read_empty(const struct object_id *oid) { - int i = 0; - const char *args[7]; - - args[i++] = "read-tree"; - args[i++] = "-m"; - args[i++] = "-u"; - args[i++] = empty_tree_oid_hex(); - args[i++] = oid_to_hex(oid); - args[i] = NULL; + struct child_process cmd = CHILD_PROCESS_INIT; + + strvec_pushl(&cmd.args, "read-tree", "-m", "-u", empty_tree_oid_hex(), + oid_to_hex(oid), NULL); + cmd.git_cmd = 1; - if (run_command_v_opt(args, RUN_GIT_CMD)) + if (run_command(&cmd)) die(_("read-tree failed")); } static void reset_hard(const struct object_id *oid) { - int i = 0; - const char *args[6]; - - args[i++] = "read-tree"; - args[i++] = "-v"; - args[i++] = "--reset"; - args[i++] = "-u"; - args[i++] = oid_to_hex(oid); - args[i] = NULL; + struct child_process cmd = CHILD_PROCESS_INIT; + + strvec_pushl(&cmd.args, "read-tree", "-v", "--reset", "-u", + oid_to_hex(oid), NULL); + cmd.git_cmd = 1; - if (run_command_v_opt(args, RUN_GIT_CMD)) + if (run_command(&cmd)) die(_("read-tree failed")); } diff --git a/builtin/remote.c b/builtin/remote.c index 11304c096a..12632676cd 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -92,13 +92,15 @@ static int verbose; static int fetch_remote(const char *name) { - const char *argv[] = { "fetch", name, NULL, NULL }; - if (verbose) { - argv[1] = "-v"; - argv[2] = name; - } + struct child_process cmd = CHILD_PROCESS_INIT; + + strvec_push(&cmd.args, "fetch"); + if (verbose) + strvec_push(&cmd.args, "-v"); + strvec_push(&cmd.args, name); + cmd.git_cmd = 1; printf_ln(_("Updating %s"), name); - if (run_command_v_opt(argv, RUN_GIT_CMD)) + if (run_command(&cmd)) return error(_("Could not fetch %s"), name); return 0; } diff --git a/compat/mingw.c b/compat/mingw.c index 901375d584..d614f156df 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -196,16 +196,19 @@ static int read_yes_no_answer(void) static int ask_yes_no_if_possible(const char *format, ...) { char question[4096]; - const char *retry_hook[] = { NULL, NULL, NULL }; + const char *retry_hook; va_list args; va_start(args, format); vsnprintf(question, sizeof(question), format, args); va_end(args); - if ((retry_hook[0] = mingw_getenv("GIT_ASK_YESNO"))) { - retry_hook[1] = question; - return !run_command_v_opt(retry_hook, 0); + retry_hook = mingw_getenv("GIT_ASK_YESNO"); + if (retry_hook) { + struct child_process cmd = CHILD_PROCESS_INIT; + + strvec_pushl(&cmd.args, retry_hook, question, NULL); + return !run_command(&cmd); } if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr))) diff --git a/ll-merge.c b/ll-merge.c index 8955d7e1f6..d5f0c62bd8 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -193,7 +193,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, struct strbuf cmd = STRBUF_INIT; struct strbuf_expand_dict_entry dict[6]; struct strbuf path_sq = STRBUF_INIT; - const char *args[] = { NULL, NULL }; + struct child_process child = CHILD_PROCESS_INIT; int status, fd, i; struct stat st; enum ll_merge_result ret; @@ -219,8 +219,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn, strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict); - args[0] = cmd.buf; - status = run_command_v_opt(args, RUN_USING_SHELL); + child.use_shell = 1; + strvec_push(&child.args, cmd.buf); + status = run_command(&child); fd = open(temp[1], O_RDONLY); if (fd < 0) goto bad; diff --git a/sequencer.c b/sequencer.c index 8ab0225f0f..643744fb9b 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3555,12 +3555,13 @@ static int error_failed_squash(struct repository *r, static int do_exec(struct repository *r, const char *command_line) { - const char *child_argv[] = { NULL, NULL }; + struct child_process cmd = CHILD_PROCESS_INIT; int dirty, status; fprintf(stderr, _("Executing: %s\n"), command_line); - child_argv[0] = command_line; - status = run_command_v_opt(child_argv, RUN_USING_SHELL); + cmd.use_shell = 1; + strvec_push(&cmd.args, command_line); + status = run_command(&cmd); /* force re-reading of the cache */ if (discard_index(r->index) < 0 || repo_read_index(r) < 0) diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c index 12beee99ad..2e576bcc11 100644 --- a/t/helper/test-fake-ssh.c +++ b/t/helper/test-fake-ssh.c @@ -8,7 +8,7 @@ int cmd_main(int argc, const char **argv) struct strbuf buf = STRBUF_INIT; FILE *f; int i; - const char *child_argv[] = { NULL, NULL }; + struct child_process cmd = CHILD_PROCESS_INIT; /* First, print all parameters into $TRASH_DIRECTORY/ssh-output */ if (!trash_directory) @@ -25,6 +25,7 @@ int cmd_main(int argc, const char **argv) /* Now, evaluate the *last* parameter */ if (argc < 2) return 0; - child_argv[0] = argv[argc - 1]; - return run_command_v_opt(child_argv, RUN_USING_SHELL); + cmd.use_shell = 1; + strvec_push(&cmd.args, argv[argc - 1]); + return run_command(&cmd); }
Use run_command() with a struct child_process variable and populate its "args" member directly instead of building a string array and passing it to run_command_v_opt(). This avoids the use of magic index numbers. Signed-off-by: René Scharfe <l.s.r@web.de> --- bisect.c | 12 ++++++------ builtin/am.c | 12 +++++------- builtin/difftool.c | 17 +++++++++-------- builtin/merge.c | 32 ++++++++++++-------------------- builtin/remote.c | 14 ++++++++------ compat/mingw.c | 11 +++++++---- ll-merge.c | 7 ++++--- sequencer.c | 7 ++++--- t/helper/test-fake-ssh.c | 7 ++++--- 9 files changed, 59 insertions(+), 60 deletions(-) -- 2.38.1