| Message ID | cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | run-command API: add run_command_{l,sv}_opt() | expand |
Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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.
On Tue, Oct 18 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> 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@web.de/ 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");
On Tue, Oct 18, 2022 at 03:28:43PM +0200, Ævar Arnfjörð Bjarmason wrote: > > 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. Sorry if I gave that impression. I like the safety of strvec_pushl(), but I think using it with a "struct child_process" is just fine. It's more flexible, and as René's patch showed, doesn't really make the callers more complex (that definitely _wasn't_ the case long ago, when most of these callers were written). I do think Junio saying "consensus" may have been premature. I expressed my opinion and he agreed, but I think that is as far as it got. :) > 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 don't think it's error-prone. It just seems like it complicates an API for little gain, and causes us to have a lot of boilerplate mapping RUN_* flags into cmd.* fields. > 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 work, but is it buying much? You still have to declare the child_process at that point, which means you have: struct child_process cmd = CHILD_PROCESS_INIT; cmd.git_cmd = 1; run_command(&cmd, "log", "--", "HEAD", NULL); instead of: struct child_process cmd = CHILD_PROCESS_INIT; cmd.git_cmd = 1; strvec_pushl(&cmd.args, "log", "--", "HEAD", NULL); run_command(&cmd); Saving one line doesn't seem worth complicating the API to me. Plus existing users have to say "run_command(&cmd, NULL)", or we need to ignore varargs when "cmd.args.nr > 0" (which papers over errors). And most of the time run_command() is inside an if() anyway. Just style-wise, keeping the long command name on its own line is a readability improvement (IMHO, anyway). > 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 > ); I dunno. That does not seem more readable to me, and would end up with GIT_TRACE lines like: git foo --noop --noop --real-option --noop > 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... Keep in mind we allocate a strvec either way. And IMHO seeing: if (foo) strvec_push(&cmd.args, "foo"); is the most readable form. Not to mention that it is more flexible. Many cases use "pushf" for the same thing. -Peff
Jeff King <peff@peff.net> writes: > I do think Junio saying "consensus" may have been premature. I expressed > my opinion and he agreed, but I think that is as far as it got. :) Maybe. This is a tangent, but as far as I am concerned, when Réne writes something that looks to me very straight-forward and correct, and it passes your taste buds, then that is enough consensus to move ahead. As Linus often said and I concur, some people got good taste, that is hard to quantify and probably hard to teach, and there are a handful folks here with good taste. And when two who have demonstrated they are with good taste agrees, that is good enough to me. >> 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 don't think it's error-prone. It just seems like it complicates an API > for little gain, and causes us to have a lot of boilerplate mapping > RUN_* flags into cmd.* fields. True. run_command() needs the RUN_* flags twiddling, too, so it is not a point against _l_opt() variant. What I see as its biggest problem is it is a bit too rigid for many of the current callers that use _v_opt() to replace them, and can easily tempt developers to write almost-duplicate _l_opt() calls, leading to an easily avoidable bug like we saw in the if/else if/else cascade. Thanks.
On Wed, Oct 19, 2022 at 08:43:43AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > I do think Junio saying "consensus" may have been premature. I expressed > > my opinion and he agreed, but I think that is as far as it got. :) > > Maybe. This is a tangent, but as far as I am concerned, when Réne > writes something that looks to me very straight-forward and correct, > and it passes your taste buds, then that is enough consensus to move > ahead. As Linus often said and I concur, some people got good > taste, that is hard to quantify and probably hard to teach, and > there are a handful folks here with good taste. And when two who > have demonstrated they are with good taste agrees, that is good > enough to me. I pretty much agree with that worldview, but I try hard not to assume "my taste is good" when going into a technical discussion. If only because I've embarrassed myself a sufficient number of times. :) > >> 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 don't think it's error-prone. It just seems like it complicates an API > > for little gain, and causes us to have a lot of boilerplate mapping > > RUN_* flags into cmd.* fields. > > True. run_command() needs the RUN_* flags twiddling, too, so it is > not a point against _l_opt() variant. Did you mean run_command_v() here? If so, then yes, it requires the flags. But if we are going to drop it in favor of just run_command(), then those flags go away (and moving to the _l() variant is a step in the opposite direction). -Peff
Jeff King <peff@peff.net> writes: >> True. run_command() needs the RUN_* flags twiddling, too, so it is >> not a point against _l_opt() variant. > > Did you mean run_command_v() here? If so, then yes, it requires the > flags. But if we are going to drop it in favor of just run_command(), > then those flags go away (and moving to the _l() variant is a step in > the opposite direction). Ah, but we'd still need to prepare individual bits in the child structure (e.g. RUN_GIT_CMD vs .git_cmd) anyway. Even though we may not have to work with the flags, somehow we need to set it up. So it is not all that strong argument against the _l_opt(). The above assessment is primarily because I do not have too much against RUN_GIT_CMD and friends, compared to setting the individual members in the child_process struct. Setting individual members in the struct is more direct and it may be an improvement use run_command() directly over using _v_opt() and _l_opt() variants.
On Wed, Oct 19, 2022 at 11:00:22AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > >> True. run_command() needs the RUN_* flags twiddling, too, so it is > >> not a point against _l_opt() variant. > > > > Did you mean run_command_v() here? If so, then yes, it requires the > > flags. But if we are going to drop it in favor of just run_command(), > > then those flags go away (and moving to the _l() variant is a step in > > the opposite direction). > > Ah, but we'd still need to prepare individual bits in the child > structure (e.g. RUN_GIT_CMD vs .git_cmd) anyway. Even though we may > not have to work with the flags, somehow we need to set it up. So > it is not all that strong argument against the _l_opt(). Right, to the callers it is no different; they must use the flags or the struct fields. What I meant is that we do not need to support _both_. I.e., the big mapping in run_command_v_opt_cd_env_tr2() goes away. > The above assessment is primarily because I do not have too much > against RUN_GIT_CMD and friends, compared to setting the individual > members in the child_process struct. Setting individual members in > the struct is more direct and it may be an improvement use > run_command() directly over using _v_opt() and _l_opt() variants. Yeah. I do not find RUN_GIT_CMD all that bad myself; I was mainly pointing out that we do not need to support both of them (and of the two, the struct fields are by far the more flexible, so that's what we should keep). That said, I am not too sad to have both. I would not have bothered to do the work to remove the _v() versions with flags. But since René already did so... -Peff
Jeff King <peff@peff.net> writes: > That said, I am not too sad to have both. I would not have bothered to > do the work to remove the _v() versions with flags. But since René > already did so... It makes two of us ;-)
Am 19.10.22 um 21:41 schrieb Junio C Hamano: > Jeff King <peff@peff.net> writes: > >> That said, I am not too sad to have both. I would not have bothered to >> do the work to remove the _v() versions with flags. But since René >> already did so... > > It makes two of us ;-) I wrote that patch out of curiosity and was pleased by the total line count reduction, getting rid of the on-stack array construction sites with their magic numbers, not having to manage strvecs explicitly anymore and the callers staying readable. One weak spot is the new helper builtin/gc.c::run_git_or_die() that I added because it was easier than replacing all those strvecs that are prepared before deciding whether their commands are even needed. Stripping down the central API to a single shared object (a struct and functions that get it passed) simplifies it for programmers. It takes the idea of d3b2159712 (run-command API: remove "argv" member, always use "args", 2021-11-25) and c7c4bdeccf (run-command API: remove "env" member, always use "env_array", 2021-11-25) to its logical conclusion of going fully dynamic and using standard strvec functions everywhere. Local shortcuts like builtin/gc.c::run_git_or_die() may still be defensible. But still: Is all of that code churn worth it? Not sure. René
René Scharfe <l.s.r@web.de> writes: > I wrote that patch out of curiosity and was pleased by the total line > count reduction, getting rid of the on-stack array construction sites > with their magic numbers, not having to manage strvecs explicitly > anymore and the callers staying readable. > ... > But still: Is all of that code churn worth it? Not sure. Compared to doing nothing? The result did look easier to grok, especially as we no longer need to worry about another way to do the same thing (i.e. run_command() vs run_command_v_*() variants) and can get rid of the flags constants that need to be kept in sync with the members of the child_process struct. Compared to adding the _l_opt() variant? Surely, as that goes the other direction to add more callers that use the flags constants, without much practical gain (which was a bit sad, but we only found that out after seeing the result).
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. Changes since v1: * Fixed a migration bug in builtin/remote.c noted by Junio (just skipping that case). * Fixed an indentation issue noted by Jeff. * Changed run_command_sv_opt() so that we take full advantage of having the "struct strvec *", and move it to "cmd.args", rather than copying its contents. * Rewrote how 1/10 uses the "opts" helper, in response to Junio's comment. * Small commit message touch-ups. 1. https://lore.kernel.org/git/cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com/ CI & branch for this topic available at https://github.com/avar/git/tree/avar/run-command-wrapper-API-simplification-2 Ævar Arnfjörð Bjarmason (10): run-command.c: refactor run_command_*_tr2() to internal helpers merge: remove always-the-same "verbose" arguments run-command API: add and use a run_command_l_opt() am: use run_command_l_opt() for show_patch() run-command API docs: clarify & fleshen out run_command_v_opt*() docs run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag run-command API & diff.c: remove run_command_v_opt_cd_env() run-command API & users: remove run_command_v_opt_tr2() gc: use strvec_pushf(), avoid redundant strbuf_detach() run-command API: add and use a run_command_sv_opt() add-interactive.c | 3 +- bisect.c | 19 ++++++----- builtin/add.c | 6 ++-- builtin/am.c | 14 +++----- builtin/clone.c | 19 ++++------- builtin/difftool.c | 14 ++++---- builtin/gc.c | 49 ++++++++++------------------ builtin/merge.c | 46 ++++++-------------------- builtin/pull.c | 15 ++------- builtin/remote.c | 5 +-- compat/mingw.c | 8 ++--- diff.c | 26 +++++++-------- fsmonitor-ipc.c | 10 ++++-- git.c | 15 +++++---- ll-merge.c | 4 +-- merge.c | 3 +- run-command.c | 52 +++++++++++++++++------------ run-command.h | 70 ++++++++++++++++++++++++++-------------- scalar.c | 6 +--- sequencer.c | 15 ++------- t/helper/test-fake-ssh.c | 4 +-- tmp-objdir.h | 6 ++-- 22 files changed, 179 insertions(+), 230 deletions(-) Range-diff against v1: 1: c1f701af6e8 ! 1: 3842204371e run-command.c: refactor run_command_*_tr2() to internal helpers @@ run-command.c: int run_command(struct child_process *cmd) + cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0; +} + - int run_command_v_opt(const char **argv, int opt) - { - return run_command_v_opt_cd_env(argv, opt, NULL, NULL); -@@ run-command.c: int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const - return run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL); - } - -+static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt, -+ const char *dir, -+ const char *const *env, -+ const char *tr2_class) ++static int run_command_v_opt_1(struct child_process *cmd, int opt) +{ + run_command_set_opts(cmd, opt); -+ cmd->dir = dir; -+ if (env) -+ strvec_pushv(&cmd->env, (const char **)env); -+ cmd->trace2_child_class = tr2_class; + return run_command(cmd); +} + - int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir, - const char *const *env, const char *tr2_class) + int run_command_v_opt(const char **argv, int opt) + { + return run_command_v_opt_cd_env(argv, opt, NULL, NULL); +@@ run-command.c: int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir, { struct child_process cmd = CHILD_PROCESS_INIT; -+ strvec_pushv(&cmd.args, argv); - cmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0; - cmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0; @@ run-command.c: int run_command_v_opt_cd_env(const char **argv, int opt, const ch - cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0; - cmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0; - cmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0; -- cmd.dir = dir; -- if (env) -- strvec_pushv(&cmd.env, (const char **)env); -- cmd.trace2_child_class = tr2_class; + cmd.dir = dir; + if (env) + strvec_pushv(&cmd.env, (const char **)env); + cmd.trace2_child_class = tr2_class; - return run_command(&cmd); -+ return run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class); ++ return run_command_v_opt_1(&cmd, opt); } #ifndef NO_PTHREADS 2: 543ccbb1ee1 = 2: 8b00172ef83 merge: remove always-the-same "verbose" arguments 3: fd81d44f221 ! 3: 680a42a878e run-command API: add and use a run_command_l_opt() @@ builtin/clone.c: static void write_refspec_config(const char *src_ref_prefix, if (!access(alternates, F_OK)) { - if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN)) + if (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN, -+ "repack", "-a", "-d", NULL)) ++ "repack", "-a", "-d", NULL)) die(_("cannot repack to clean up")); if (unlink(alternates) && errno != ENOENT) die_errno(_("cannot unlink temporary alternates file")); @@ builtin/merge.c: static int save_state(struct object_id *stash) refresh_cache: if (discard_cache() < 0 || read_cache() < 0) - ## builtin/remote.c ## -@@ builtin/remote.c: 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; -- } - printf_ln(_("Updating %s"), name); -- if (run_command_v_opt(argv, RUN_GIT_CMD)) -+ if (verbose && run_command_l_opt(RUN_GIT_CMD, "-v", "fetch", name, -+ NULL)) -+ return error(_("Could not fetch %s"), name); -+ else if (run_command_l_opt(RUN_GIT_CMD, "fetch", name, NULL)) - return error(_("Could not fetch %s"), name); - return 0; - } - ## compat/mingw.c ## @@ compat/mingw.c: static int read_yes_no_answer(void) static int ask_yes_no_if_possible(const char *format, ...) @@ run-command.c: static void run_command_set_opts(struct child_process *cmd, int o + return run_command(&cmd); +} + - int run_command_v_opt(const char **argv, int opt) + static int run_command_v_opt_1(struct child_process *cmd, int opt) { - return run_command_v_opt_cd_env(argv, opt, NULL, NULL); + run_command_set_opts(cmd, opt); ## run-command.h ## @@ run-command.h: struct child_process { 4: 4cd61aaa981 ! 4: 5cfd6a94ce3 am: use run_command_l_opt() for show_patch() @@ Commit message am: use run_command_l_opt() for show_patch() The "git show" invocation added in 66335298a47 (rebase: add - --show-current-patch, 2018-02-11) is a one-off, and one where we're - not calling oid_to_hex() twice. So we can rely on the static buffer - that oid_to_hex() points to, rather than xstrdup()-ing it. As a result - we can use the run_command_l_opt() function. + --show-current-patch, 2018-02-11) is one where we're not calling + oid_to_hex() twice. So we can rely on the static buffer that + oid_to_hex() points to, rather than xstrdup()-ing it. As a result we + can use the run_command_l_opt() function. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> 5: b6a3c4c66f8 = 5: 4fca38bb4d6 run-command API docs: clarify & fleshen out run_command_v_opt*() docs 6: 9d0286fbf64 = 6: 75eccc152ad run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag 7: 31e8536f28c ! 7: 3b3d3777232 run-command API & diff.c: remove run_command_v_opt_cd_env() @@ diff.c: static void run_external_diff(const char *pgm, static int similarity_index(struct diff_filepair *p) ## run-command.c ## -@@ run-command.c: int run_command_l_opt(int opt, ...) +@@ run-command.c: static int run_command_v_opt_1(struct child_process *cmd, int opt) int run_command_v_opt(const char **argv, int opt) { @@ run-command.c: int run_command_v_opt_tr2(const char **argv, int opt, const char - return run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL); -} - - static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt, - const char *dir, - const char *const *env, + int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir, + const char *const *env, const char *tr2_class) + { ## run-command.h ## @@ run-command.h: struct child_process { 8: 88e063f3b05 ! 8: 4f1a051823f run-command API & users: remove run_command_v_opt_tr2() @@ run-command.c: static void run_command_set_opts(struct child_process *cmd, int o cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0; } @@ run-command.c: int run_command_l_opt(int opt, ...) + return run_command(&cmd); } +-static int run_command_v_opt_1(struct child_process *cmd, int opt) +-{ +- run_command_set_opts(cmd, opt); +- return run_command(cmd); +-} +- int run_command_v_opt(const char **argv, int opt) -{ - return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL); @@ run-command.c: int run_command_l_opt(int opt, ...) - return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class); -} - --static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt, -- const char *dir, -- const char *const *env, -- const char *tr2_class) --{ -- run_command_set_opts(cmd, opt); -- cmd->dir = dir; -- if (env) -- strvec_pushv(&cmd->env, (const char **)env); -- cmd->trace2_child_class = tr2_class; -- return run_command(cmd); --} -- -int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir, - const char *const *env, const char *tr2_class) { struct child_process cmd = CHILD_PROCESS_INIT; - ++ strvec_pushv(&cmd.args, argv); -- return run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class); +- cmd.dir = dir; +- if (env) +- strvec_pushv(&cmd.env, (const char **)env); +- cmd.trace2_child_class = tr2_class; +- return run_command_v_opt_1(&cmd, opt); + run_command_set_opts(&cmd, opt); + return run_command(&cmd); } 9: 0f5524e40ad = 9: 99c5688797a gc: use strvec_pushf(), avoid redundant strbuf_detach() 10: 874cb72c2f4 ! 10: 138af632a36 run-command API: add and use a run_command_sv_opt() @@ Commit message carry over to "return" after a "strvec_clear()" to use this new function instead. + Because we pass the "struct strvec *" to the function we can also + avoid copying the arguments to the "args" member of the "struct + child_process", as we were doing with run_command_v_opt(). + + Instead we can use memcpy() and strvec_clear() to do the moral + equivalent of a strbuf_{detach,attach}(). The strvec API doesn't have + a strvec_attach(), we could add it here while at it, but let's avoid + generalizing the interface for now and migrate the "struct strvec *" + in the "run_command_sv_opt()" instead. + Let's leave aside the user in "builtin/bisect--helper.c"'s bisect_visualize(). There's an outstanding topic that's extensively modifying it. @@ merge.c: int try_merge_command(struct repository *r, discard_index(r->index); if (repo_read_index(r) < 0) + ## run-command.c ## +@@ run-command.c: int run_command_v_opt(const char **argv, int opt) + return run_command(&cmd); + } + ++int run_command_sv_opt(struct strvec *args, int opt) ++{ ++ struct child_process cmd = CHILD_PROCESS_INIT; ++ ++ /* TODO: We could encapsulate this with a strvec_attach() */ ++ memcpy(&cmd.args, args, sizeof(*args)); ++ strvec_init(args); ++ run_command_set_opts(&cmd, opt); ++ return run_command(&cmd); ++} ++ + #ifndef NO_PTHREADS + static pthread_t main_thread; + static int main_thread_set; + ## run-command.h ## @@ run-command.h: struct child_process { @@ run-command.h: int run_command_v_opt(const char **argv, int opt); +/** + * The run_command_sv_opt() function is a wrapper for + * run_command_v_opt(). It takes a "struct strvec *args" which -+ * similarly to run_command() (but not run_command_sv_opt()) will be -+ * strvec_clear()'d before returning. ++ * similarly will be strvec_clear()'d before returning. + * + * Use it for the common case of constructing a "struct strvec" for a + * one-shot run_command_v_opt() invocation. ++ * ++ * The "args" will migrated the "cmd.args" member of an underlying ++ * "struct child_process", in a way that avoids making an extra copy. + */ +RESULT_MUST_BE_USED -+static inline int run_command_sv_opt(struct strvec *args, int opt) -+{ -+ int ret = run_command_v_opt(args->v, opt); -+ -+ strvec_clear(args); -+ return ret; -+} ++int run_command_sv_opt(struct strvec *args, int opt); + /** * Execute the given command, sending "in" to its stdin, and capturing its