On Tue, Oct 18 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes: > >> This series provides a more idiomatic set of run-command API helpers >> to match our current use-cases for run_command_v_opt(). See v1[1] for >> a more general overview. > > Hmph... I somehow thought that the concensus is rather try the > complete opposite approach shown by René's patch to lose the use of > run_command_v_opt() by replacing it with run_command(&args), with > args.v populated using strvec_pushl() and other strvec API > functions. > > One of the reasons I would prefer to see us moving in that direction > was because the first iteration of this series was a good > demonstration of the relatively limited usefulness of _l_opt() > variant and also it seemed to be error prone to use it. I'm getting somewhat mixed messages. Jeff seemed to like the end-to-end safety of run_command_l_opt() before I wrote it. I think the run_command_l_opt() still really shines for the simple cases. I don't see how *_l_opt() is particularly error prone, I just had a stupid think-o in v1 of this, but that if/else if bug is something that could have snuck in with run_command() given the same stupidity :) I checked out René's [1] and diff'd it with mine, excluding various parts that inflated the diff for no good explanatory purpose (e.g. the API itself, or where I omitted some conversions). I think the diffstat is a good argument for *some* version of my series, but as e.g. the first hunk shows we could also just convert e.g. run_diff() to run_command() instead of run_command_sv_opt(). I wonder if a run_command() that just had the prototype (struct child_process *cmd, ...) might not be the best of both worlds (or a run_commandl() alternative). I.e. to do away with the whole custom way of specifying the flag(s), and just take the passed-in arguments and append them to "&cmd.args". That would give us run_command_l_opt() behavior, which is handy in some cases, and gives us compile-time checks. We could BUG() out if "cmd.args.nr > 0" if we use it, to ensure we use one or the other (although a combination would be handy in some cases, so maybe not). What do you all think? It's also interesting to consider adding a --noop option to be supported by parse-options.c. The reason we can't use a run_command_l_opt() in some cases is because we're doing e.g.: if (progress) strvec_push(&args, "--progress"); We have a --no-progress, but in those cases the recipient at the end often cares about a default of -1 for a bool variable, or similar. So if we could do: run_command_l_opt(0, command, (progress ? "--progress" : "--noop"), ..., NULL ); We could benefit from compile-time checks, and wouldn't have to allocate a strvec just for building up the arguments in some cases. Just food for thought... 1. https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@xxxxxx/ add-interactive.c | 8 +-- builtin/add.c | 15 +++--- builtin/am.c | 12 +++-- builtin/clone.c | 44 +++++++++------ builtin/difftool.c | 24 +++++---- builtin/fetch.c | 9 ++-- builtin/gc.c | 74 ++++++++++++++++--------- builtin/merge-index.c | 4 +- builtin/pull.c | 138 ++++++++++++++++++++++++----------------------- builtin/remote.c | 37 +++++++------ compat/mingw.c | 11 ++-- diff.c | 5 +- fsmonitor-ipc.c | 4 +- ll-merge.c | 5 +- merge.c | 17 +++--- scalar.c | 9 ++-- sequencer.c | 23 ++++---- t/helper/test-fake-ssh.c | 5 +- t/helper/test-trace2.c | 4 +- tmp-objdir.h | 6 +-- 20 files changed, 265 insertions(+), 189 deletions(-) diff --git a/add-interactive.c b/add-interactive.c index 9c86f3b9532..ecc5ae1b249 100644 --- a/add-interactive.c +++ b/add-interactive.c @@ -997,17 +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_sv_opt(&args, 0); + res = run_command(&cmd); } putchar('\n'); diff --git a/builtin/add.c b/builtin/add.c index 7c783eebc0e..626c71ec6aa 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -241,7 +241,7 @@ int run_add_interactive(const char *revision, const char *patch_mode, const struct pathspec *pathspec) { int i; - struct strvec argv = STRVEC_INIT; + struct child_process cmd = CHILD_PROCESS_INIT; int use_builtin_add_i = git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1); @@ -272,17 +272,18 @@ int run_add_interactive(const char *revision, const char *patch_mode, return !!run_add_p(the_repository, mode, revision, pathspec); } - strvec_push(&argv, "add--interactive"); + strvec_push(&cmd.args, "add--interactive"); if (patch_mode) - strvec_push(&argv, patch_mode); + strvec_push(&cmd.args, patch_mode); if (revision) - strvec_push(&argv, revision); - strvec_push(&argv, "--"); + strvec_push(&cmd.args, revision); + strvec_push(&cmd.args, "--"); for (i = 0; i < pathspec->nr; i++) /* pass original pathspec, to be re-parsed */ - strvec_push(&argv, pathspec->items[i].original); + strvec_push(&cmd.args, pathspec->items[i].original); - return run_command_sv_opt(&argv, RUN_GIT_CMD); + cmd.git_cmd = 1; + return run_command(&cmd); } int interactive_add(const char **argv, const char *prefix, int patch) diff --git a/builtin/am.c b/builtin/am.c index 1d298a91306..20aea0d2487 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2186,10 +2186,14 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode) const char *patch_path; int len; - if (!is_null_oid(&state->orig_commit)) - return run_command_l_opt(RUN_GIT_CMD, "show", - oid_to_hex(&state->orig_commit), - "--", NULL); + 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); + } switch (sub_mode) { case SHOW_PATCH_RAW: diff --git a/builtin/clone.c b/builtin/clone.c index 8e43781e147..219cfbd7355 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -651,19 +651,23 @@ static void update_head(const struct ref *our, const struct ref *remote, 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 * for the later checkout to use the sparse-checkout file. */ core_apply_sparse_checkout = 1; - if (run_command_l_opt(RUN_GIT_CMD, "-C", repo, "sparse-checkout", - "set", NULL)) { + cmd.git_cmd = 1; + if (run_command(&cmd)) { error(_("failed to initialize sparse-checkout")); - return 1; + result = 1; } - return 0; + return result; } static int checkout(int submodule_progress, int filter_submodules) @@ -727,36 +731,38 @@ static int checkout(int submodule_progress, int filter_submodules) oid_to_hex(&oid), "1", NULL); if (!err && (option_recurse_submodules.nr > 0)) { - struct strvec args = STRVEC_INIT; - strvec_pushl(&args, "submodule", "update", "--require-init", "--recursive", NULL); + struct child_process cmd = CHILD_PROCESS_INIT; + strvec_pushl(&cmd.args, "submodule", "update", "--require-init", + "--recursive", NULL); if (option_shallow_submodules == 1) - strvec_push(&args, "--depth=1"); + strvec_push(&cmd.args, "--depth=1"); if (max_jobs != -1) - strvec_pushf(&args, "--jobs=%d", max_jobs); + strvec_pushf(&cmd.args, "--jobs=%d", max_jobs); if (submodule_progress) - strvec_push(&args, "--progress"); + strvec_push(&cmd.args, "--progress"); if (option_verbosity < 0) - strvec_push(&args, "--quiet"); + strvec_push(&cmd.args, "--quiet"); if (option_remote_submodules) { - strvec_push(&args, "--remote"); - strvec_push(&args, "--no-fetch"); + strvec_push(&cmd.args, "--remote"); + strvec_push(&cmd.args, "--no-fetch"); } if (filter_submodules && filter_options.choice) - strvec_pushf(&args, "--filter=%s", + strvec_pushf(&cmd.args, "--filter=%s", expand_list_objects_filter_spec(&filter_options)); if (option_single_branch >= 0) - strvec_push(&args, option_single_branch ? + strvec_push(&cmd.args, option_single_branch ? "--single-branch" : "--no-single-branch"); - return run_command_sv_opt(&args, RUN_GIT_CMD); + cmd.git_cmd = 1; + err = run_command(&cmd); } return err; @@ -860,8 +866,12 @@ static void dissociate_from_references(void) char *alternates = git_pathdup("objects/info/alternates"); if (!access(alternates, F_OK)) { - if (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN, - "repack", "-a", "-d", NULL)) + struct child_process cmd = CHILD_PROCESS_INIT; + + strvec_pushl(&cmd.args, "repack", "-a", "-d", NULL); + cmd.git_cmd = 1; + cmd.no_stdin = 1; + if (run_command(&cmd)) die(_("cannot repack to clean up")); if (unlink(alternates) && errno != ENOENT) die_errno(_("cannot unlink temporary alternates file")); diff --git a/builtin/difftool.c b/builtin/difftool.c index ed211a87322..d6c262e15ff 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -44,8 +44,10 @@ static int difftool_config(const char *var, const char *value, void *cb) static int print_tool_help(void) { - return run_command_l_opt(RUN_GIT_CMD, "mergetool", "--tool-help=diff", - NULL); + struct child_process cmd = CHILD_PROCESS_INIT; + strvec_pushl(&cmd.args, "mergetool", "--tool-help=diff", NULL); + cmd.git_cmd = 1; + return run_command(&cmd); } static int parse_index_info(char *p, int *mode1, int *mode2, @@ -360,8 +362,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_command = "difftool--helper"; + int err = 0; + struct child_process cmd = CHILD_PROCESS_INIT; struct hashmap wt_modified, tmp_modified; int indices_loaded = 0; @@ -564,13 +566,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix, strbuf_setlen(&ldir, ldir_len); strbuf_setlen(&rdir, rdir_len); + if (extcmd) { - helper_command = 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_l_opt(flags, helper_command, ldir.buf, rdir.buf, - NULL); + } + 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/fetch.c b/builtin/fetch.c index a0fca93bb6a..dd060dd65af 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1965,14 +1965,17 @@ static int fetch_multiple(struct string_list *list, int max_children) } else for (i = 0; i < list->nr; i++) { const char *name = list->items[i].string; - strvec_push(&argv, name); + struct child_process cmd = CHILD_PROCESS_INIT; + + strvec_pushv(&cmd.args, argv.v); + strvec_push(&cmd.args, name); if (verbosity >= 0) printf(_("Fetching %s\n"), name); - if (run_command_v_opt(argv.v, RUN_GIT_CMD)) { + cmd.git_cmd = 1; + if (run_command(&cmd)) { error(_("could not fetch %s"), name); result = 1; } - strvec_pop(&argv); } strvec_clear(&argv); diff --git a/builtin/gc.c b/builtin/gc.c index 8393e19b504..b47e53c2101 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -58,6 +58,8 @@ static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE; static struct strvec reflog = STRVEC_INIT; static struct strvec repack = STRVEC_INIT; static struct strvec prune = STRVEC_INIT; +static struct strvec prune_worktrees = STRVEC_INIT; +static struct strvec rerere = STRVEC_INIT; static struct tempfile *pidfile; static struct lock_file log_lock; @@ -165,8 +167,11 @@ static void gc_config(void) struct maintenance_run_opts; static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts) { - return run_command_l_opt(RUN_GIT_CMD, "pack-refs", "--all", "--prune", - NULL); + struct child_process cmd = CHILD_PROCESS_INIT; + + strvec_pushl(&cmd.args, "pack-refs", "--all", "--prune", NULL); + cmd.git_cmd = 1; + return run_command(&cmd); } static int too_many_loose_objects(void) @@ -518,6 +523,16 @@ static int report_last_gc_error(void) return ret; } +static void run_git_or_die(const char **argv) +{ + struct child_process cmd = CHILD_PROCESS_INIT; + + strvec_pushv(&cmd.args, argv); + cmd.git_cmd = 1; + if (run_command(&cmd)) + die(FAILED_RUN, argv[0]); +} + static void gc_before_repack(void) { /* @@ -532,8 +547,8 @@ static void gc_before_repack(void) if (pack_refs && maintenance_task_pack_refs(NULL)) die(FAILED_RUN, "pack-refs"); - if (prune_reflogs && run_command_v_opt(reflog.v, RUN_GIT_CMD)) - die(FAILED_RUN, reflog.v[0]); + if (prune_reflogs) + run_git_or_die(reflog.v); } int cmd_gc(int argc, const char **argv, const char *prefix) @@ -571,6 +586,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix) strvec_pushl(&reflog, "reflog", "expire", "--all", NULL); strvec_pushl(&repack, "repack", "-d", "-l", NULL); strvec_pushl(&prune, "prune", "--expire", NULL); + strvec_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL); + strvec_pushl(&rerere, "rerere", "gc", NULL); /* default expiry time, overwritten in gc_config */ gc_config(); @@ -666,8 +683,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix) gc_before_repack(); if (!repository_format_precious_objects) { - if (run_command_v_opt(repack.v, - RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE)) + struct child_process repack_cmd = CHILD_PROCESS_INIT; + + strvec_pushv(&repack_cmd.args, repack.v); + repack_cmd.git_cmd = 1; + repack_cmd.close_object_store = 1; + if (run_command(&repack_cmd)) die(FAILED_RUN, repack.v[0]); if (prune_expire) { @@ -678,18 +699,16 @@ int cmd_gc(int argc, const char **argv, const char *prefix) if (has_promisor_remote()) strvec_push(&prune, "--exclude-promisor-objects"); - if (run_command_v_opt(prune.v, RUN_GIT_CMD)) - die(FAILED_RUN, prune.v[0]); + run_git_or_die(prune.v); } } - if (prune_worktrees_expire && - run_command_l_opt(RUN_GIT_CMD, "worktree", "prune", "--expire", - prune_worktrees_expire, NULL)) - die(FAILED_RUN, "worktree"); + if (prune_worktrees_expire) { + strvec_push(&prune_worktrees, prune_worktrees_expire); + run_git_or_die(prune_worktrees.v); + } - if (run_command_l_opt(RUN_GIT_CMD, "rerere", "gc", NULL)) - die(FAILED_RUN, "rerere"); + run_git_or_die(rerere.v); report_garbage = report_pack_garbage; reprepare_packed_git(the_repository); @@ -1894,21 +1913,26 @@ static int is_schtasks_available(void) #endif } -#define SCHTASKS_NAME_FMT "Git Maintenance (%s)" +static char *schtasks_task_name(const char *frequency) +{ + struct strbuf label = STRBUF_INIT; + strbuf_addf(&label, "Git Maintenance (%s)", frequency); + return strbuf_detach(&label, NULL); +} static int schtasks_remove_task(enum schedule_priority schedule) { const char *cmd = "schtasks"; - struct strvec args = STRVEC_INIT; + struct child_process child = CHILD_PROCESS_INIT; const char *frequency = get_frequency(schedule); + char *name = schtasks_task_name(frequency); get_schedule_cmd(&cmd, NULL); - strvec_split(&args, cmd); - strvec_pushl(&args, "/delete", "/tn", NULL); - strvec_pushf(&args, SCHTASKS_NAME_FMT, frequency); - strvec_pushl(&args, "/f", NULL); + strvec_split(&child.args, cmd); + strvec_pushl(&child.args, "/delete", "/tn", name, "/f", NULL); + free(name); - return run_command_sv_opt(&args, 0); + return run_command(&child); } static int schtasks_remove_tasks(void) @@ -1926,6 +1950,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority const char *xml; struct tempfile *tfile; const char *frequency = get_frequency(schedule); + char *name = schtasks_task_name(frequency); struct strbuf tfilename = STRBUF_INIT; get_schedule_cmd(&cmd, NULL); @@ -2018,10 +2043,8 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority "</Task>\n"; fprintf(tfile->fp, xml, exec_path, exec_path, frequency); strvec_split(&child.args, cmd); - strvec_pushl(&child.args, "/create", "/tn", NULL); - strvec_pushf(&child.args, SCHTASKS_NAME_FMT, frequency); - strvec_pushl(&child.args, "/f", "/xml", - get_tempfile_path(tfile), NULL); + strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml", + get_tempfile_path(tfile), NULL); close_tempfile_gently(tfile); child.no_stdout = 1; @@ -2032,6 +2055,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority result = finish_command(&child); delete_tempfile(&tfile); + free(name); return result; } diff --git a/builtin/merge-index.c b/builtin/merge-index.c index c0383fe9df9..012f52bd007 100644 --- a/builtin/merge-index.c +++ b/builtin/merge-index.c @@ -12,6 +12,7 @@ static int merge_entry(int pos, const char *path) const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL }; char hexbuf[4][GIT_MAX_HEXSZ + 1]; char ownbuf[4][60]; + struct child_process cmd = CHILD_PROCESS_INIT; if (pos >= active_nr) die("git merge-index: %s not in the cache", path); @@ -31,7 +32,8 @@ static int merge_entry(int pos, const char *path) if (!found) die("git merge-index: %s not in the cache", path); - if (run_command_v_opt(arguments, 0)) { + strvec_pushv(&cmd.args, arguments); + if (run_command(&cmd)) { if (one_shot) err++; else { diff --git a/builtin/pull.c b/builtin/pull.c index 2f36823c97e..b21edd767aa 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -515,73 +515,75 @@ static void parse_repo_refspecs(int argc, const char **argv, const char **repo, */ static int run_fetch(const char *repo, const char **refspecs) { - struct strvec args = STRVEC_INIT; + struct child_process cmd = CHILD_PROCESS_INIT; - strvec_pushl(&args, "fetch", "--update-head-ok", NULL); + strvec_pushl(&cmd.args, "fetch", "--update-head-ok", NULL); /* Shared options */ - argv_push_verbosity(&args); + argv_push_verbosity(&cmd.args); if (opt_progress) - strvec_push(&args, opt_progress); + strvec_push(&cmd.args, opt_progress); /* Options passed to git-fetch */ if (opt_all) - strvec_push(&args, opt_all); + strvec_push(&cmd.args, opt_all); if (opt_append) - strvec_push(&args, opt_append); + strvec_push(&cmd.args, opt_append); if (opt_upload_pack) - strvec_push(&args, opt_upload_pack); - argv_push_force(&args); + strvec_push(&cmd.args, opt_upload_pack); + argv_push_force(&cmd.args); if (opt_tags) - strvec_push(&args, opt_tags); + strvec_push(&cmd.args, opt_tags); if (opt_prune) - strvec_push(&args, opt_prune); + strvec_push(&cmd.args, opt_prune); if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT) switch (recurse_submodules_cli) { case RECURSE_SUBMODULES_ON: - strvec_push(&args, "--recurse-submodules=on"); + strvec_push(&cmd.args, "--recurse-submodules=on"); break; case RECURSE_SUBMODULES_OFF: - strvec_push(&args, "--recurse-submodules=no"); + strvec_push(&cmd.args, "--recurse-submodules=no"); break; case RECURSE_SUBMODULES_ON_DEMAND: - strvec_push(&args, "--recurse-submodules=on-demand"); + strvec_push(&cmd.args, "--recurse-submodules=on-demand"); break; default: BUG("submodule recursion option not understood"); } if (max_children) - strvec_push(&args, max_children); + strvec_push(&cmd.args, max_children); if (opt_dry_run) - strvec_push(&args, "--dry-run"); + strvec_push(&cmd.args, "--dry-run"); if (opt_keep) - strvec_push(&args, opt_keep); + strvec_push(&cmd.args, opt_keep); if (opt_depth) - strvec_push(&args, opt_depth); + strvec_push(&cmd.args, opt_depth); if (opt_unshallow) - strvec_push(&args, opt_unshallow); + strvec_push(&cmd.args, opt_unshallow); if (opt_update_shallow) - strvec_push(&args, opt_update_shallow); + strvec_push(&cmd.args, opt_update_shallow); if (opt_refmap) - strvec_push(&args, opt_refmap); + strvec_push(&cmd.args, opt_refmap); if (opt_ipv4) - strvec_push(&args, opt_ipv4); + strvec_push(&cmd.args, opt_ipv4); if (opt_ipv6) - strvec_push(&args, opt_ipv6); + strvec_push(&cmd.args, opt_ipv6); if (opt_show_forced_updates > 0) - strvec_push(&args, "--show-forced-updates"); + strvec_push(&cmd.args, "--show-forced-updates"); else if (opt_show_forced_updates == 0) - strvec_push(&args, "--no-show-forced-updates"); + strvec_push(&cmd.args, "--no-show-forced-updates"); if (set_upstream) - strvec_push(&args, set_upstream); - strvec_pushv(&args, opt_fetch.v); + strvec_push(&cmd.args, set_upstream); + strvec_pushv(&cmd.args, opt_fetch.v); if (repo) { - strvec_push(&args, repo); - strvec_pushv(&args, refspecs); + strvec_push(&cmd.args, repo); + strvec_pushv(&cmd.args, refspecs); } else if (*refspecs) BUG("refspecs without repo?"); - return run_command_sv_opt(&args, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE); + cmd.git_cmd = 1; + cmd.close_object_store = 1; + return run_command(&cmd); } /** @@ -650,49 +652,50 @@ static int update_submodules(void) */ static int run_merge(void) { - struct strvec args = STRVEC_INIT; + struct child_process cmd = CHILD_PROCESS_INIT; - strvec_pushl(&args, "merge", NULL); + strvec_pushl(&cmd.args, "merge", NULL); /* Shared options */ - argv_push_verbosity(&args); + argv_push_verbosity(&cmd.args); if (opt_progress) - strvec_push(&args, opt_progress); + strvec_push(&cmd.args, opt_progress); /* Options passed to git-merge */ if (opt_diffstat) - strvec_push(&args, opt_diffstat); + strvec_push(&cmd.args, opt_diffstat); if (opt_log) - strvec_push(&args, opt_log); + strvec_push(&cmd.args, opt_log); if (opt_signoff) - strvec_push(&args, opt_signoff); + strvec_push(&cmd.args, opt_signoff); if (opt_squash) - strvec_push(&args, opt_squash); + strvec_push(&cmd.args, opt_squash); if (opt_commit) - strvec_push(&args, opt_commit); + strvec_push(&cmd.args, opt_commit); if (opt_edit) - strvec_push(&args, opt_edit); + strvec_push(&cmd.args, opt_edit); if (cleanup_arg) - strvec_pushf(&args, "--cleanup=%s", cleanup_arg); + strvec_pushf(&cmd.args, "--cleanup=%s", cleanup_arg); if (opt_ff) - strvec_push(&args, opt_ff); + strvec_push(&cmd.args, opt_ff); if (opt_verify) - strvec_push(&args, opt_verify); + strvec_push(&cmd.args, opt_verify); if (opt_verify_signatures) - strvec_push(&args, opt_verify_signatures); - strvec_pushv(&args, opt_strategies.v); - strvec_pushv(&args, opt_strategy_opts.v); + strvec_push(&cmd.args, opt_verify_signatures); + strvec_pushv(&cmd.args, opt_strategies.v); + strvec_pushv(&cmd.args, opt_strategy_opts.v); if (opt_gpg_sign) - strvec_push(&args, opt_gpg_sign); + strvec_push(&cmd.args, opt_gpg_sign); if (opt_autostash == 0) - strvec_push(&args, "--no-autostash"); + strvec_push(&cmd.args, "--no-autostash"); else if (opt_autostash == 1) - strvec_push(&args, "--autostash"); + strvec_push(&cmd.args, "--autostash"); if (opt_allow_unrelated_histories > 0) - strvec_push(&args, "--allow-unrelated-histories"); + strvec_push(&cmd.args, "--allow-unrelated-histories"); - strvec_push(&args, "FETCH_HEAD"); - return run_command_sv_opt(&args, RUN_GIT_CMD); + strvec_push(&cmd.args, "FETCH_HEAD"); + cmd.git_cmd = 1; + return run_command(&cmd); } /** @@ -873,40 +876,41 @@ static int get_rebase_newbase_and_upstream(struct object_id *newbase, static int run_rebase(const struct object_id *newbase, const struct object_id *upstream) { - struct strvec args = STRVEC_INIT; + struct child_process cmd = CHILD_PROCESS_INIT; - strvec_push(&args, "rebase"); + strvec_push(&cmd.args, "rebase"); /* Shared options */ - argv_push_verbosity(&args); + argv_push_verbosity(&cmd.args); /* Options passed to git-rebase */ if (opt_rebase == REBASE_MERGES) - strvec_push(&args, "--rebase-merges"); + strvec_push(&cmd.args, "--rebase-merges"); else if (opt_rebase == REBASE_INTERACTIVE) - strvec_push(&args, "--interactive"); + strvec_push(&cmd.args, "--interactive"); if (opt_diffstat) - strvec_push(&args, opt_diffstat); - strvec_pushv(&args, opt_strategies.v); - strvec_pushv(&args, opt_strategy_opts.v); + strvec_push(&cmd.args, opt_diffstat); + strvec_pushv(&cmd.args, opt_strategies.v); + strvec_pushv(&cmd.args, opt_strategy_opts.v); if (opt_gpg_sign) - strvec_push(&args, opt_gpg_sign); + strvec_push(&cmd.args, opt_gpg_sign); if (opt_signoff) - strvec_push(&args, opt_signoff); + strvec_push(&cmd.args, opt_signoff); if (opt_autostash == 0) - strvec_push(&args, "--no-autostash"); + strvec_push(&cmd.args, "--no-autostash"); else if (opt_autostash == 1) - strvec_push(&args, "--autostash"); + strvec_push(&cmd.args, "--autostash"); if (opt_verify_signatures && !strcmp(opt_verify_signatures, "--verify-signatures")) warning(_("ignoring --verify-signatures for rebase")); - strvec_push(&args, "--onto"); - strvec_push(&args, oid_to_hex(newbase)); + strvec_push(&cmd.args, "--onto"); + strvec_push(&cmd.args, oid_to_hex(newbase)); - strvec_push(&args, oid_to_hex(upstream)); + strvec_push(&cmd.args, oid_to_hex(upstream)); - return run_command_sv_opt(&args, RUN_GIT_CMD); + cmd.git_cmd = 1; + return run_command(&cmd); } static int get_can_ff(struct object_id *orig_head, diff --git a/builtin/remote.c b/builtin/remote.c index 5d3534db19c..0cde2e244a4 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); printf_ln(_("Updating %s"), name); - if (run_command_v_opt(argv, RUN_GIT_CMD)) + cmd.git_cmd = 1; + if (run_command(&cmd)) return error(_("Could not fetch %s"), name); return 0; } @@ -1508,34 +1510,35 @@ static int update(int argc, const char **argv, const char *prefix) N_("prune remotes after fetching")), OPT_END() }; - struct strvec fetch_argv = STRVEC_INIT; + struct child_process cmd = CHILD_PROCESS_INIT; int default_defined = 0; argc = parse_options(argc, argv, prefix, options, builtin_remote_update_usage, PARSE_OPT_KEEP_ARGV0); - strvec_push(&fetch_argv, "fetch"); + strvec_push(&cmd.args, "fetch"); if (prune != -1) - strvec_push(&fetch_argv, prune ? "--prune" : "--no-prune"); + strvec_push(&cmd.args, prune ? "--prune" : "--no-prune"); if (verbose) - strvec_push(&fetch_argv, "-v"); - strvec_push(&fetch_argv, "--multiple"); + strvec_push(&cmd.args, "-v"); + strvec_push(&cmd.args, "--multiple"); if (argc < 2) - strvec_push(&fetch_argv, "default"); + strvec_push(&cmd.args, "default"); for (i = 1; i < argc; i++) - strvec_push(&fetch_argv, argv[i]); + strvec_push(&cmd.args, argv[i]); - if (strcmp(fetch_argv.v[fetch_argv.nr-1], "default") == 0) { + if (strcmp(cmd.args.v[cmd.args.nr-1], "default") == 0) { git_config(get_remote_default, &default_defined); if (!default_defined) { - strvec_pop(&fetch_argv); - strvec_push(&fetch_argv, "--all"); + strvec_pop(&cmd.args); + strvec_push(&cmd.args, "--all"); } } - return run_command_sv_opt(&fetch_argv, RUN_GIT_CMD); + cmd.git_cmd = 1; + return run_command(&cmd); } static int remove_all_fetch_refspecs(const char *key) diff --git a/compat/mingw.c b/compat/mingw.c index 4f5392c5796..d614f156df1 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -196,15 +196,20 @@ static int read_yes_no_answer(void) static int ask_yes_no_if_possible(const char *format, ...) { char question[4096]; - char *retry_hook; + const char *retry_hook; va_list args; va_start(args, format); vsnprintf(question, sizeof(question), format, args); va_end(args); - if ((retry_hook = mingw_getenv("GIT_ASK_YESNO"))) - return !run_command_l_opt(0, retry_hook, question, NULL); + 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))) return 0; diff --git a/diff.c b/diff.c index 392530016fa..8451c230d9e 100644 --- a/diff.c +++ b/diff.c @@ -4278,8 +4278,8 @@ static void run_external_diff(const char *pgm, const char *xfrm_msg, struct diff_options *o) { - struct diff_queue_struct *q = &diff_queued_diff; struct child_process cmd = CHILD_PROCESS_INIT; + struct diff_queue_struct *q = &diff_queued_diff; strvec_push(&cmd.args, pgm); strvec_push(&cmd.args, name); @@ -4295,7 +4295,8 @@ static void run_external_diff(const char *pgm, } } - strvec_pushf(&cmd.env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); + strvec_pushf(&cmd.env, "GIT_DIFF_PATH_COUNTER=%d", + ++o->diff_path_counter); strvec_pushf(&cmd.env, "GIT_DIFF_PATH_TOTAL=%d", q->nr); diff_free_filespec_data(one); diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c index 19d772f0f3a..96d8d37c06d 100644 --- a/fsmonitor-ipc.c +++ b/fsmonitor-ipc.c @@ -56,11 +56,11 @@ static int spawn_daemon(void) { struct child_process cmd = CHILD_PROCESS_INIT; + strvec_pushl(&cmd.args, "fsmonitor--daemon", "start", NULL); + cmd.git_cmd = 1; cmd.no_stdin = 1; cmd.trace2_child_class = "fsmonitor"; - strvec_pushl(&cmd.args, "fsmonitor--daemon", "start", NULL); - return run_command(&cmd); } diff --git a/ll-merge.c b/ll-merge.c index 740689b7bd6..b20491043e0 100644 --- a/ll-merge.c +++ b/ll-merge.c @@ -193,6 +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; + struct child_process child = CHILD_PROCESS_INIT; int status, fd, i; struct stat st; enum ll_merge_result ret; @@ -218,7 +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); - status = run_command_l_opt(RUN_USING_SHELL, cmd.buf, NULL); + strvec_push(&child.args, cmd.buf); + child.use_shell = 1; + status = run_command(&child); fd = open(temp[1], O_RDONLY); if (fd < 0) goto bad; diff --git a/merge.c b/merge.c index 487debacecb..445b4f19aa8 100644 --- a/merge.c +++ b/merge.c @@ -19,21 +19,22 @@ int try_merge_command(struct repository *r, const char **xopts, struct commit_list *common, const char *head_arg, struct commit_list *remotes) { - struct strvec args = STRVEC_INIT; + struct child_process cmd = CHILD_PROCESS_INIT; int i, ret; struct commit_list *j; - strvec_pushf(&args, "merge-%s", strategy); + strvec_pushf(&cmd.args, "merge-%s", strategy); for (i = 0; i < xopts_nr; i++) - strvec_pushf(&args, "--%s", xopts[i]); + strvec_pushf(&cmd.args, "--%s", xopts[i]); for (j = common; j; j = j->next) - strvec_push(&args, merge_argument(j->item)); - strvec_push(&args, "--"); - strvec_push(&args, head_arg); + strvec_push(&cmd.args, merge_argument(j->item)); + strvec_push(&cmd.args, "--"); + strvec_push(&cmd.args, head_arg); for (j = remotes; j; j = j->next) - strvec_push(&args, merge_argument(j->item)); + strvec_push(&cmd.args, merge_argument(j->item)); - ret = run_command_sv_opt(&args, RUN_GIT_CMD); + cmd.git_cmd = 1; + ret = run_command(&cmd); discard_index(r->index); if (repo_read_index(r) < 0) diff --git a/scalar.c b/scalar.c index 3480bf73cbd..03f9e480dd6 100644 --- a/scalar.c +++ b/scalar.c @@ -69,17 +69,18 @@ static void setup_enlistment_directory(int argc, const char **argv, static int run_git(const char *arg, ...) { - struct strvec argv = STRVEC_INIT; + struct child_process cmd = CHILD_PROCESS_INIT; va_list args; const char *p; va_start(args, arg); - strvec_push(&argv, arg); + strvec_push(&cmd.args, arg); while ((p = va_arg(args, const char *))) - strvec_push(&argv, p); + strvec_push(&cmd.args, p); va_end(args); - return run_command_sv_opt(&argv, RUN_GIT_CMD); + cmd.git_cmd = 1; + return run_command(&cmd); } struct scalar_config { diff --git a/sequencer.c b/sequencer.c index 7ee0e05512c..9b9d6a55828 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3183,14 +3183,15 @@ static int rollback_is_safe(void) static int reset_merge(const struct object_id *oid) { - struct strvec argv = STRVEC_INIT; + struct child_process cmd = CHILD_PROCESS_INIT; - strvec_pushl(&argv, "reset", "--merge", NULL); + strvec_pushl(&cmd.args, "reset", "--merge", NULL); if (!is_null_oid(oid)) - strvec_push(&argv, oid_to_hex(oid)); + strvec_push(&cmd.args, oid_to_hex(oid)); - return run_command_sv_opt(&argv, RUN_GIT_CMD); + cmd.git_cmd = 1; + return run_command(&cmd); } static int rollback_single_pick(struct repository *r) @@ -3554,10 +3555,13 @@ static int error_failed_squash(struct repository *r, static int do_exec(struct repository *r, const char *command_line) { + struct child_process cmd = CHILD_PROCESS_INIT; int dirty, status; fprintf(stderr, _("Executing: %s\n"), command_line); - status = run_command_l_opt(RUN_USING_SHELL, command_line, NULL); + strvec_push(&cmd.args, command_line); + cmd.use_shell = 1; + status = run_command(&cmd); /* force re-reading of the cache */ if (discard_index(r->index) < 0 || repo_read_index(r) < 0) @@ -4861,13 +4865,13 @@ static int pick_commits(struct repository *r, static int continue_single_pick(struct repository *r, struct replay_opts *opts) { - struct strvec argv = STRVEC_INIT; + struct child_process cmd = CHILD_PROCESS_INIT; if (!refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") && !refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD")) return error(_("no cherry-pick or revert in progress")); - strvec_push(&argv, "commit"); + strvec_push(&cmd.args, "commit"); /* * continue_single_pick() handles the case of recovering from a @@ -4880,9 +4884,10 @@ static int continue_single_pick(struct repository *r, struct replay_opts *opts) * Include --cleanup=strip as well because we don't want the * "# Conflicts:" messages. */ - strvec_pushl(&argv, "--no-edit", "--cleanup=strip", NULL); + strvec_pushl(&cmd.args, "--no-edit", "--cleanup=strip", NULL); - return run_command_sv_opt(&argv, RUN_GIT_CMD); + cmd.git_cmd = 1; + return run_command(&cmd); } static int commit_staged_changes(struct repository *r, diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c index 7967478a40a..42185f3fc05 100644 --- a/t/helper/test-fake-ssh.c +++ b/t/helper/test-fake-ssh.c @@ -8,6 +8,7 @@ int cmd_main(int argc, const char **argv) struct strbuf buf = STRBUF_INIT; FILE *f; int i; + struct child_process cmd = CHILD_PROCESS_INIT; /* First, print all parameters into $TRASH_DIRECTORY/ssh-output */ if (!trash_directory) @@ -24,5 +25,7 @@ int cmd_main(int argc, const char **argv) /* Now, evaluate the *last* parameter */ if (argc < 2) return 0; - return run_command_l_opt(RUN_USING_SHELL, argv[argc - 1], NULL); + strvec_push(&cmd.args, argv[argc - 1]); + cmd.use_shell = 1; + return run_command(&cmd); } diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c index a714130ece7..94fd8ccf513 100644 --- a/t/helper/test-trace2.c +++ b/t/helper/test-trace2.c @@ -132,6 +132,7 @@ static int ut_003error(int argc, const char **argv) */ static int ut_004child(int argc, const char **argv) { + struct child_process cmd = CHILD_PROCESS_INIT; int result; /* @@ -141,7 +142,8 @@ static int ut_004child(int argc, const char **argv) if (!argc) return 0; - result = run_command_v_opt(argv, 0); + strvec_pushv(&cmd.args, argv); + result = run_command(&cmd); exit(result); } diff --git a/tmp-objdir.h b/tmp-objdir.h index c96aa77396d..615b1885733 100644 --- a/tmp-objdir.h +++ b/tmp-objdir.h @@ -10,11 +10,9 @@ * * Example: * - * struct child_process cmd = CHILD_PROCESS_INIT; - * ... * struct tmp_objdir *t = tmp_objdir_create("incoming"); - * strvec_pushl(&cmd.env, tmp_objdir_env(t)); - * if (!run_command(&cmd) && !tmp_objdir_migrate(t)) + * strvec_pushv(&cmd.env, tmp_objdir_env(t)); + * if (!run_command(&cmd)) && !tmp_objdir_migrate(t)) * printf("success!\n"); * else * die("failed...tmp_objdir will clean up for us");
