{"thread":{"id":"58626","subject":"[PATCH 00/10] run-command API: add run_command_{l,sv}_opt()","startedAt":"2022-10-14T15:40:33Z","lastAt":"2022-10-20T23:50:41Z","messageCount":39,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","René Scharfe","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":10},"messages":[{"id":"464950","messageId":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":null,"subject":"[PATCH 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:12Z","receivedAt":"2022-10-14T15:40:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series starts out by addressing the issue Jeff King noted in\nhttps://lore.kernel.org/git/Y0TXTl0gSBOFQa9B@coredump.intra.peff.net/;\nI.e. that by moving away from a \"struct strvec\" for run_command()\nvariants we lose out on the \"warn_unused_result\" checking we'd get\nwith the strvec_pushl() (ensuring the last element is NULL).\n\nThe 3/10 here adds a run_command_l_opt() function, which provides an\neasy & safe one-shot API using warn_unused_result.\n\nThe 10/10 here then adds a run_command_sv_opt(), which is a\nrun_command_v_opt() taking a \"struct strvec\" that the API clears for\nthe user after it's called. This makes a lot of one-shot \"run this and\nfree the strvec\" users less verbose.\n\nThen, having converted to these helpers we had 1-2 users of other\nrun_command_*() variants, which could be moved to using run_command()\ndirectly, and thus getting rid of supporting so much shorthand API\nvariation.\n\nAt the end of this series the in-tree use of the 3x helpers is (by\nnumber of *.c file occurances):\n\n 19 run_command_l_opt\n 15 run_command_v_opt\n 12 run_command_sv_opt\n\nMore can be converted to run_command_{l,sv}_opt(), but I left out\ne.g. bisect--helper.c to avoid conflicts with anything in-flight.\n\nÆvar Arnfjörð Bjarmason (10):\n run-command.c: refactor run_command_*_tr2() to internal helpers\n merge: remove always-the-same \"verbose\" arguments\n run-command API: add and use a run_command_l_opt()\n am: use run_command_l_opt() for show_patch()\n run-command API docs: clarify & fleshen out run_command_v_opt*() docs\n run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag\n run-command API & diff.c: remove run_command_v_opt_cd_env()\n run-command API & users: remove run_command_v_opt_tr2()\n gc: use strvec_pushf(), avoid redundant strbuf_detach()\n run-command API: add and use a run_command_sv_opt()\n\n add-interactive.c | 3 +-\n bisect.c | 19 +++++------\n builtin/add.c | 6 ++--\n builtin/am.c | 14 +++-----\n builtin/clone.c | 19 ++++-------\n builtin/difftool.c | 14 ++++----\n builtin/gc.c | 49 +++++++++-----------------\n builtin/merge.c | 46 ++++++-------------------\n builtin/pull.c | 15 ++------\n builtin/remote.c | 15 +++-----\n compat/mingw.c | 8 ++---\n diff.c | 26 +++++++-------\n fsmonitor-ipc.c | 10 ++++--\n git.c | 15 ++++----\n ll-merge.c | 4 +--\n merge.c | 3 +-\n run-command.c | 43 ++++++++++++-----------\n run-command.h | 74 +++++++++++++++++++++++++++-------------\n scalar.c | 6 +---\n sequencer.c | 15 ++------\n t/helper/test-fake-ssh.c | 4 +--\n tmp-objdir.h | 6 ++--\n 22 files changed, 177 insertions(+), 237 deletions(-)\n\n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464951","messageId":"patch-01.10-c1f701af6e8-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH 01/10] run-command.c: refactor run_command_*_tr2() to internal helpers","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:13Z","receivedAt":"2022-10-14T15:40:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor the run_command_v_opt_cd_env_tr2() function to helpers which\ntakes a \"struct child_process *cmd\" argument. This will allow for\nadding a helper which won't need to copy a set of arguments to the\n\"cmd.args\" we'd otherwise have to populate from the \"argv\".\n\nSplitting out the part of the function that sets the various members\nfrom \"opt\" will help in the subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 40 +++++++++++++++++++++++++++-------------\n 1 file changed, 27 insertions(+), 13 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 5ec3a46dccf..cf4a431c839 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1004,6 +1004,18 @@ int run_command(struct child_process *cmd)\n \treturn finish_command(cmd);\n }\n \n+static void run_command_set_opts(struct child_process *cmd, int opt)\n+{\n+\tcmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;\n+\tcmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n+\tcmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n+\tcmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n+\tcmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n+\tcmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n+\tcmd->wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n+\tcmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n+}\n+\n int run_command_v_opt(const char **argv, int opt)\n {\n \treturn run_command_v_opt_cd_env(argv, opt, NULL, NULL);\n@@ -1019,24 +1031,26 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const\n \treturn run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);\n }\n \n+static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,\n+\t\t\t\t\t const char *dir,\n+\t\t\t\t\t const char *const *env,\n+\t\t\t\t\t const char *tr2_class)\n+{\n+\trun_command_set_opts(cmd, opt);\n+\tcmd->dir = dir;\n+\tif (env)\n+\t\tstrvec_pushv(&cmd->env, (const char **)env);\n+\tcmd->trace2_child_class = tr2_class;\n+\treturn run_command(cmd);\n+}\n+\n int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n \t\t\t\t const char *const *env, const char *tr2_class)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n \tstrvec_pushv(&cmd.args, argv);\n-\tcmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;\n-\tcmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n-\tcmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n-\tcmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n-\tcmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n-\tcmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n-\tcmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n-\tcmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n-\tcmd.dir = dir;\n-\tif (env)\n-\t\tstrvec_pushv(&cmd.env, (const char **)env);\n-\tcmd.trace2_child_class = tr2_class;\n-\treturn run_command(&cmd);\n+\treturn run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class);\n }\n \n #ifndef NO_PTHREADS\n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464952","messageId":"patch-02.10-543ccbb1ee1-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH 02/10] merge: remove always-the-same \"verbose\" arguments","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:14Z","receivedAt":"2022-10-14T15:40:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Simplify the code that builds the arguments for the \"read-tree\"\ninvocation in reset_hard() and read_empty() to remove the \"verbose\"\nparameter.\n\nBefore 172b6428d06 (do not overwrite untracked during merge from\nunborn branch, 2010-11-14) there was a \"reset_hard()\" function that\nwould be called in two places, one of those passed a \"verbose=1\", the\nother a \"verbose=0\".\n\nAfter 172b6428d06 when read_empty() was split off from reset_hard()\nboth of these functions only had one caller. The \"verbose\" in\nread_empty() would always be false, and the one in reset_hard() would\nalways be true.\n\nThere was never a good reason for the code to act this way, it\nhappened because the read_empty() function was a copy/pasted and\nadjusted version of reset_hard().\n\nSince we're no longer conditionally adding the \"-v\" parameter\nhere (and we'd only add it for \"reset_hard()\" we'll be able to move to\na simpler and safer run-command API in the subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/merge.c | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5900b81729d..3bb49d805b4 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -345,14 +345,12 @@ static int save_state(struct object_id *stash)\n \treturn rc;\n }\n \n-static void read_empty(const struct object_id *oid, int verbose)\n+static void read_empty(const struct object_id *oid)\n {\n \tint i = 0;\n \tconst char *args[7];\n \n \targs[i++] = \"read-tree\";\n-\tif (verbose)\n-\t\targs[i++] = \"-v\";\n \targs[i++] = \"-m\";\n \targs[i++] = \"-u\";\n \targs[i++] = empty_tree_oid_hex();\n@@ -363,14 +361,13 @@ static void read_empty(const struct object_id *oid, int verbose)\n \t\tdie(_(\"read-tree failed\"));\n }\n \n-static void reset_hard(const struct object_id *oid, int verbose)\n+static void reset_hard(const struct object_id *oid)\n {\n \tint i = 0;\n \tconst char *args[6];\n \n \targs[i++] = \"read-tree\";\n-\tif (verbose)\n-\t\targs[i++] = \"-v\";\n+\targs[i++] = \"-v\";\n \targs[i++] = \"--reset\";\n \targs[i++] = \"-u\";\n \targs[i++] = oid_to_hex(oid);\n@@ -385,7 +382,7 @@ static void restore_state(const struct object_id *head,\n {\n \tstruct strvec args = STRVEC_INIT;\n \n-\treset_hard(head, 1);\n+\treset_hard(head);\n \n \tif (is_null_oid(stash))\n \t\tgoto refresh_cache;\n@@ -1470,7 +1467,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t check_trust_level);\n \n \t\tremote_head_oid = &remoteheads->item->object.oid;\n-\t\tread_empty(remote_head_oid, 0);\n+\t\tread_empty(remote_head_oid);\n \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head_oid, NULL, 0,\n \t\t\t UPDATE_REFS_DIE_ON_ERR);\n \t\tgoto done;\n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464953","messageId":"patch-03.10-fd81d44f221-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH 03/10] run-command API: add and use a run_command_l_opt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:15Z","receivedAt":"2022-10-14T15:40:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When run_command() is used with strvec_pushl() as in the pre-image of\nb004c902827 (gc: simplify maintenance_task_pack_refs(), 2022-10-04) we\nget the benefit of the LAST_ARG_MUST_BE_NULL. See 9fe3edc47f1 (Add the\nLAST_ARG_MUST_BE_NULL macro, 2013-07-18).\n\nLet's provide and use a run_command_l_opt() function, which gives us\nthe best of both worlds. We'll now need less code to accomplish the\nsame thing, and this version's safer as we can assert that the\nterminating NULL is present.\n\nThe \"builtin/bisect--helper.c\" would be a large beneficiary of this\nAPI, with \"15 insertions(+), 26 deletions(-)\", but let's leave that\naside for now, as there's an outstanding topic that's extensively\nmodifying it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bisect.c | 19 +++++++++----------\n builtin/clone.c | 16 ++++++----------\n builtin/difftool.c | 14 ++++++--------\n builtin/gc.c | 22 ++++++++--------------\n builtin/merge.c | 35 ++++++-----------------------------\n builtin/remote.c | 10 ++++------\n compat/mingw.c | 8 +++-----\n ll-merge.c | 4 +---\n run-command.c | 15 +++++++++++++++\n run-command.h | 13 +++++++++++--\n sequencer.c | 4 +---\n t/helper/test-fake-ssh.c | 4 +---\n 12 files changed, 71 insertions(+), 93 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex fd581b85a72..415b0e7b0b2 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -738,18 +738,17 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,\n \tupdate_ref(NULL, \"BISECT_EXPECTED_REV\", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n \n \targv_checkout[2] = bisect_rev_hex;\n-\tif (no_checkout) {\n+\tif (no_checkout)\n \t\tupdate_ref(NULL, \"BISECT_HEAD\", bisect_rev, NULL, 0,\n \t\t\t UPDATE_REFS_DIE_ON_ERR);\n-\t} else {\n-\t\tif (run_command_v_opt(argv_checkout, RUN_GIT_CMD))\n-\t\t\t/*\n-\t\t\t * Errors in `run_command()` itself, signaled by res < 0,\n-\t\t\t * and errors in the child process, signaled by res > 0\n-\t\t\t * can both be treated as regular BISECT_FAILED (-1).\n-\t\t\t */\n-\t\t\treturn BISECT_FAILED;\n-\t}\n+\telse if (run_command_l_opt(RUN_GIT_CMD, \"checkout\", \"-q\",\n+\t\t\t\t bisect_rev_hex, \"--\", NULL))\n+\t\t/*\n+\t\t * Errors in `run_command()` itself, signaled by res < 0,\n+\t\t * and errors in the child process, signaled by res > 0\n+\t\t * can both be treated as regular BISECT_FAILED (-1).\n+\t\t */\n+\t\treturn BISECT_FAILED;\n \n \tcommit = lookup_commit_reference(the_repository, bisect_rev);\n \tformat_commit_message(commit, \"[%H] %s%n\", &commit_msg, &pp);\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ed8d44bb6ab..13fc7a4bc5d 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -651,23 +651,19 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \n static int git_sparse_checkout_init(const char *repo)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n-\tint result = 0;\n-\tstrvec_pushl(&argv, \"-C\", repo, \"sparse-checkout\", \"set\", NULL);\n-\n \t/*\n \t * We must apply the setting in the current process\n \t * for the later checkout to use the sparse-checkout file.\n \t */\n \tcore_apply_sparse_checkout = 1;\n \n-\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\tif (run_command_l_opt(RUN_GIT_CMD, \"-C\", repo, \"sparse-checkout\",\n+\t\t\t \"set\", NULL)) {\n \t\terror(_(\"failed to initialize sparse-checkout\"));\n-\t\tresult = 1;\n+\t\treturn 1;\n \t}\n \n-\tstrvec_clear(&argv);\n-\treturn result;\n+\treturn 0;\n }\n \n static int checkout(int submodule_progress, int filter_submodules)\n@@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,\n \n static void dissociate_from_references(void)\n {\n-\tstatic const char* argv[] = { \"repack\", \"-a\", \"-d\", NULL };\n \tchar *alternates = git_pathdup(\"objects/info/alternates\");\n \n \tif (!access(alternates, F_OK)) {\n-\t\tif (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))\n+\t\tif (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,\n+\t\t\t\t \"repack\", \"-a\", \"-d\", NULL))\n \t\t\tdie(_(\"cannot repack to clean up\"));\n \t\tif (unlink(alternates) && errno != ENOENT)\n \t\t\tdie_errno(_(\"cannot unlink temporary alternates file\"));\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 4b10ad1a369..ed211a87322 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -44,8 +44,8 @@ static int difftool_config(const char *var, const char *value, void *cb)\n \n static int print_tool_help(void)\n {\n-\tconst char *argv[] = { \"mergetool\", \"--tool-help=diff\", NULL };\n-\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n+\treturn run_command_l_opt(RUN_GIT_CMD, \"mergetool\", \"--tool-help=diff\",\n+\t\t\t\t NULL);\n }\n \n static int parse_index_info(char *p, int *mode1, int *mode2,\n@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct index_state wtindex;\n \tstruct checkout lstate, rstate;\n \tint flags = RUN_GIT_CMD, err = 0;\n-\tconst char *helper_argv[] = { \"difftool--helper\", NULL, NULL, NULL };\n+\tconst char *helper_command = \"difftool--helper\";\n \tstruct hashmap wt_modified, tmp_modified;\n \tint indices_loaded = 0;\n \n@@ -563,16 +563,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t}\n \n \tstrbuf_setlen(&ldir, ldir_len);\n-\thelper_argv[1] = ldir.buf;\n \tstrbuf_setlen(&rdir, rdir_len);\n-\thelper_argv[2] = rdir.buf;\n-\n \tif (extcmd) {\n-\t\thelper_argv[0] = extcmd;\n+\t\thelper_command = extcmd;\n \t\tflags = 0;\n \t} else\n \t\tsetenv(\"GIT_DIFFTOOL_DIRDIFF\", \"true\", 1);\n-\tret = run_command_v_opt(helper_argv, flags);\n+\tret = run_command_l_opt(flags, helper_command, ldir.buf, rdir.buf,\n+\t\t\t\tNULL);\n \n \t/* TODO: audit for interaction with sparse-index. */\n \tensure_full_index(&wtindex);\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 243ee85d283..075b4637660 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -58,8 +58,6 @@ static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;\n static struct strvec reflog = STRVEC_INIT;\n static struct strvec repack = STRVEC_INIT;\n static struct strvec prune = STRVEC_INIT;\n-static struct strvec prune_worktrees = STRVEC_INIT;\n-static struct strvec rerere = STRVEC_INIT;\n \n static struct tempfile *pidfile;\n static struct lock_file log_lock;\n@@ -167,9 +165,8 @@ static void gc_config(void)\n struct maintenance_run_opts;\n static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)\n {\n-\tconst char *argv[] = { \"pack-refs\", \"--all\", \"--prune\", NULL };\n-\n-\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n+\treturn run_command_l_opt(RUN_GIT_CMD, \"pack-refs\", \"--all\", \"--prune\",\n+\t\t\t\t NULL);\n }\n \n static int too_many_loose_objects(void)\n@@ -574,8 +571,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tstrvec_pushl(&reflog, \"reflog\", \"expire\", \"--all\", NULL);\n \tstrvec_pushl(&repack, \"repack\", \"-d\", \"-l\", NULL);\n \tstrvec_pushl(&prune, \"prune\", \"--expire\", NULL);\n-\tstrvec_pushl(&prune_worktrees, \"worktree\", \"prune\", \"--expire\", NULL);\n-\tstrvec_pushl(&rerere, \"rerere\", \"gc\", NULL);\n \n \t/* default expiry time, overwritten in gc_config */\n \tgc_config();\n@@ -688,14 +683,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (prune_worktrees_expire) {\n-\t\tstrvec_push(&prune_worktrees, prune_worktrees_expire);\n-\t\tif (run_command_v_opt(prune_worktrees.v, RUN_GIT_CMD))\n-\t\t\tdie(FAILED_RUN, prune_worktrees.v[0]);\n-\t}\n+\tif (prune_worktrees_expire &&\n+\t run_command_l_opt(RUN_GIT_CMD, \"worktree\", \"prune\", \"--expire\",\n+\t\t\t prune_worktrees_expire, NULL))\n+\t\tdie(FAILED_RUN, \"worktree\");\n \n-\tif (run_command_v_opt(rerere.v, RUN_GIT_CMD))\n-\t\tdie(FAILED_RUN, rerere.v[0]);\n+\tif (run_command_l_opt(RUN_GIT_CMD, \"rerere\", \"gc\", NULL))\n+\t\tdie(FAILED_RUN, \"rerere\");\n \n \treport_garbage = report_pack_garbage;\n \treprepare_packed_git(the_repository);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 3bb49d805b4..9c04b588f68 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -347,55 +347,32 @@ static int save_state(struct object_id *stash)\n \n static void read_empty(const struct object_id *oid)\n {\n-\tint i = 0;\n-\tconst char *args[7];\n-\n-\targs[i++] = \"read-tree\";\n-\targs[i++] = \"-m\";\n-\targs[i++] = \"-u\";\n-\targs[i++] = empty_tree_oid_hex();\n-\targs[i++] = oid_to_hex(oid);\n-\targs[i] = NULL;\n-\n-\tif (run_command_v_opt(args, RUN_GIT_CMD))\n+\tif (run_command_l_opt(RUN_GIT_CMD, \"read-tree\", \"-m\", \"-u\",\n+\t\t\t empty_tree_oid_hex(), oid_to_hex(oid), NULL))\n \t\tdie(_(\"read-tree failed\"));\n }\n \n static void reset_hard(const struct object_id *oid)\n {\n-\tint i = 0;\n-\tconst char *args[6];\n-\n-\targs[i++] = \"read-tree\";\n-\targs[i++] = \"-v\";\n-\targs[i++] = \"--reset\";\n-\targs[i++] = \"-u\";\n-\targs[i++] = oid_to_hex(oid);\n-\targs[i] = NULL;\n-\n-\tif (run_command_v_opt(args, RUN_GIT_CMD))\n+\tif (run_command_l_opt(RUN_GIT_CMD, \"read-tree\", \"-v\", \"--reset\", \"-u\",\n+\t\t\t oid_to_hex(oid), NULL))\n \t\tdie(_(\"read-tree failed\"));\n }\n \n static void restore_state(const struct object_id *head,\n \t\t\t const struct object_id *stash)\n {\n-\tstruct strvec args = STRVEC_INIT;\n-\n \treset_hard(head);\n \n \tif (is_null_oid(stash))\n \t\tgoto refresh_cache;\n \n-\tstrvec_pushl(&args, \"stash\", \"apply\", \"--index\", \"--quiet\", NULL);\n-\tstrvec_push(&args, oid_to_hex(stash));\n-\n \t/*\n \t * It is OK to ignore error here, for example when there was\n \t * nothing to restore.\n \t */\n-\trun_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n+\trun_command_l_opt(RUN_GIT_CMD, \"stash\", \"apply\", \"--index\", \"--quiet\",\n+\t\t\t oid_to_hex(stash), NULL);\n \n refresh_cache:\n \tif (discard_cache() < 0 || read_cache() < 0)\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 910f7b9316a..1d86c14297b 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -92,13 +92,11 @@ static int verbose;\n \n static int fetch_remote(const char *name)\n {\n-\tconst char *argv[] = { \"fetch\", name, NULL, NULL };\n-\tif (verbose) {\n-\t\targv[1] = \"-v\";\n-\t\targv[2] = name;\n-\t}\n \tprintf_ln(_(\"Updating %s\"), name);\n-\tif (run_command_v_opt(argv, RUN_GIT_CMD))\n+\tif (verbose && run_command_l_opt(RUN_GIT_CMD, \"-v\", \"fetch\", name,\n+\t\t\t\t\t NULL))\n+\t\treturn error(_(\"Could not fetch %s\"), name);\n+\telse if (run_command_l_opt(RUN_GIT_CMD, \"fetch\", name, NULL))\n \t\treturn error(_(\"Could not fetch %s\"), name);\n \treturn 0;\n }\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 901375d5841..4f5392c5796 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -196,17 +196,15 @@ static int read_yes_no_answer(void)\n static int ask_yes_no_if_possible(const char *format, ...)\n {\n \tchar question[4096];\n-\tconst char *retry_hook[] = { NULL, NULL, NULL };\n+\tchar *retry_hook;\n \tva_list args;\n \n \tva_start(args, format);\n \tvsnprintf(question, sizeof(question), format, args);\n \tva_end(args);\n \n-\tif ((retry_hook[0] = mingw_getenv(\"GIT_ASK_YESNO\"))) {\n-\t\tretry_hook[1] = question;\n-\t\treturn !run_command_v_opt(retry_hook, 0);\n-\t}\n+\tif ((retry_hook = mingw_getenv(\"GIT_ASK_YESNO\")))\n+\t\treturn !run_command_l_opt(0, retry_hook, question, NULL);\n \n \tif (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))\n \t\treturn 0;\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 8955d7e1f6e..740689b7bd6 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -193,7 +193,6 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tstruct strbuf cmd = STRBUF_INIT;\n \tstruct strbuf_expand_dict_entry dict[6];\n \tstruct strbuf path_sq = STRBUF_INIT;\n-\tconst char *args[] = { NULL, NULL };\n \tint status, fd, i;\n \tstruct stat st;\n \tenum ll_merge_result ret;\n@@ -219,8 +218,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \n \tstrbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);\n \n-\targs[0] = cmd.buf;\n-\tstatus = run_command_v_opt(args, RUN_USING_SHELL);\n+\tstatus = run_command_l_opt(RUN_USING_SHELL, cmd.buf, NULL);\n \tfd = open(temp[1], O_RDONLY);\n \tif (fd < 0)\n \t\tgoto bad;\ndiff --git a/run-command.c b/run-command.c\nindex cf4a431c839..0be5626f7f2 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1016,6 +1016,21 @@ static void run_command_set_opts(struct child_process *cmd, int opt)\n \tcmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n }\n \n+int run_command_l_opt(int opt, ...)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tva_list ap;\n+\tconst char *arg;\n+\n+\tva_start(ap, opt);\n+\twhile ((arg = va_arg(ap, const char *)))\n+\t\tstrvec_push(&cmd.args, arg);\n+\tva_end(ap);\n+\n+\trun_command_set_opts(&cmd, opt);\n+\treturn run_command(&cmd);\n+}\n+\n int run_command_v_opt(const char **argv, int opt)\n {\n \treturn run_command_v_opt_cd_env(argv, opt, NULL, NULL);\ndiff --git a/run-command.h b/run-command.h\nindex 0e85e5846a5..6320d70f062 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -151,8 +151,8 @@ struct child_process {\n \n /**\n * The functions: child_process_init, start_command, finish_command,\n- * run_command, run_command_v_opt, run_command_v_opt_cd_env, child_process_clear\n- * do the following:\n+ * run_command, run_command_l_opt, run_command_v_opt,\n+ * run_command_v_opt_cd_env, child_process_clear do the following:\n *\n * - If a system call failed, errno is set and -1 is returned. A diagnostic\n * is printed.\n@@ -254,6 +254,15 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const\n int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n \t\t\t\t const char *const *env, const char *tr2_class);\n \n+/**\n+ * The run_command_l_opt() function run_command_v_opt() takes a list\n+ * of strings terminated by a NULL. Use it instead of\n+ * run_command_v_opt() when possible, as it allows the compiler to\n+ * check that the \"argv\" is NULL-delimited.\n+ */\n+LAST_ARG_MUST_BE_NULL\n+int run_command_l_opt(int opt, ...);\n+\n /**\n * Execute the given command, sending \"in\" to its stdin, and capturing its\n * stdout and stderr in the \"out\" and \"err\" strbufs. Any of the three may\ndiff --git a/sequencer.c b/sequencer.c\nindex debb2ecbafe..20495db9de2 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3558,12 +3558,10 @@ static int error_failed_squash(struct repository *r,\n \n static int do_exec(struct repository *r, const char *command_line)\n {\n-\tconst char *child_argv[] = { NULL, NULL };\n \tint dirty, status;\n \n \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n-\tchild_argv[0] = command_line;\n-\tstatus = run_command_v_opt(child_argv, RUN_USING_SHELL);\n+\tstatus = run_command_l_opt(RUN_USING_SHELL, command_line, NULL);\n \n \t/* force re-reading of the cache */\n \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\ndiff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c\nindex 12beee99ad2..7967478a40a 100644\n--- a/t/helper/test-fake-ssh.c\n+++ b/t/helper/test-fake-ssh.c\n@@ -8,7 +8,6 @@ int cmd_main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tFILE *f;\n \tint i;\n-\tconst char *child_argv[] = { NULL, NULL };\n \n \t/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */\n \tif (!trash_directory)\n@@ -25,6 +24,5 @@ int cmd_main(int argc, const char **argv)\n \t/* Now, evaluate the *last* parameter */\n \tif (argc < 2)\n \t\treturn 0;\n-\tchild_argv[0] = argv[argc - 1];\n-\treturn run_command_v_opt(child_argv, RUN_USING_SHELL);\n+\treturn run_command_l_opt(RUN_USING_SHELL, argv[argc - 1], NULL);\n }\n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464954","messageId":"patch-04.10-4cd61aaa981-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH 04/10] am: use run_command_l_opt() for show_patch()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:16Z","receivedAt":"2022-10-14T15:40:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"git show\" invocation added in 66335298a47 (rebase: add\n--show-current-patch, 2018-02-11) is a one-off, and one where we're\nnot calling oid_to_hex() twice. So we can rely on the static buffer\nthat oid_to_hex() points to, rather than xstrdup()-ing it. As a result\nwe can use the run_command_l_opt() function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/am.c | 14 ++++----------\n 1 file changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 39fea248330..1d298a91306 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2186,16 +2186,10 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)\n \tconst char *patch_path;\n \tint len;\n \n-\tif (!is_null_oid(&state->orig_commit)) {\n-\t\tconst char *av[4] = { \"show\", NULL, \"--\", NULL };\n-\t\tchar *new_oid_str;\n-\t\tint ret;\n-\n-\t\tav[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));\n-\t\tret = run_command_v_opt(av, RUN_GIT_CMD);\n-\t\tfree(new_oid_str);\n-\t\treturn ret;\n-\t}\n+\tif (!is_null_oid(&state->orig_commit))\n+\t\treturn run_command_l_opt(RUN_GIT_CMD, \"show\",\n+\t\t\t\t\t oid_to_hex(&state->orig_commit),\n+\t\t\t\t\t \"--\", NULL);\n \n \tswitch (sub_mode) {\n \tcase SHOW_PATCH_RAW:\n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464955","messageId":"patch-05.10-b6a3c4c66f8-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH 05/10] run-command API docs: clarify & fleshen out run_command_v_opt*() docs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:17Z","receivedAt":"2022-10-14T15:40:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a discussion of the flags that were missing to the\nrun_command_v_opt*() docs, and in doing so format them such that we\ncan easily add or remove flags from a table in the future, rather than\nhaving them tied up in prose.\n\nLet's also clarify why the user might want to use this API over\nrun_command() itself.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.h | 29 ++++++++++++++++++++++-------\n 1 file changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/run-command.h b/run-command.h\nindex 6320d70f062..cf250e36426 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -234,13 +234,28 @@ int run_auto_maintenance(int quiet);\n #define RUN_CLOSE_OBJECT_STORE\t\t(1<<7)\n \n /**\n- * Convenience functions that encapsulate a sequence of\n- * start_command() followed by finish_command(). The argument argv\n- * specifies the program and its arguments. The argument opt is zero\n- * or more of the flags `RUN_COMMAND_NO_STDIN`, `RUN_GIT_CMD`,\n- * `RUN_COMMAND_STDOUT_TO_STDERR`, or `RUN_SILENT_EXEC_FAILURE`\n- * that correspond to the members .no_stdin, .git_cmd,\n- * .stdout_to_stderr, .silent_exec_failure of `struct child_process`.\n+ * The run_command_v_opt*() API is a convenience wrapper for an\n+ * underlying run_command().\n+ *\n+ * It's intended to be used when the user already has an \"argv\" they'd\n+ * like to use. As opposed to the \"struct child_process\"'s \"args\"\n+ * member, which will be strvec_clear()'d by calling run_command(),\n+ * the caller owns the \"argv\", which is not altered by invoking these\n+ * functions.\n+ *\n+ * The \"opt\" flags that will cause various underlying run_command()\n+ * members to be set. The flags and the corresponding struct members\n+ * are:\n+ *\n+ *\t- RUN_COMMAND_NO_STDIN: .no_stdin\n+ *\t- RUN_GIT_CMD: .git_cmd\n+ *\t- RUN_COMMAND_STDOUT_TO_STDERR: .stdout_to_stderr\n+ *\t- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure\n+ *\t- RUN_USING_SHELL: .use_shell\n+ *\t- RUN_CLEAN_ON_EXIT: .clean_on_exit\n+ *\t- RUN_WAIT_AFTER_CLEAN: .wait_after_clean\n+ *\t- RUN_CLOSE_OBJECT_STORE: .close_object_store\n+ *\n * The argument dir corresponds the member .dir. The argument env\n * corresponds to the member .env.\n */\n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464956","messageId":"patch-06.10-9d0286fbf64-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH 06/10] run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:18Z","receivedAt":"2022-10-14T15:40:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The run_command_v_opt*() API is meant to handle various common cases\nwhere we don't want the verbosity of calling the underlying\nrun_command(), but we're explicitly not trying to cover the full API\nsurface of run_command() itself.\n\nSo if we're not using some of these flags we probably won't need them\nagain, any future user who needs to set \"stdout_to_stderr\" can just\nuse run_command() itself, and the convenience API shouldn't be\ncluttered by trying to handle all the needs of various one-offs.\n\nSo let's remove the RUN_COMMAND_STDOUT_TO_STDERR flag, it hasn't been\nused since 860a2ebecd2 (receive-pack: send auto-gc output over\nsideband 2, 2016-06-05) when its last user started using the\nunderlying API directly.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 1 -\n run-command.h | 12 +++++-------\n 2 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 0be5626f7f2..fe1b4a0b650 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1008,7 +1008,6 @@ static void run_command_set_opts(struct child_process *cmd, int opt)\n {\n \tcmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;\n \tcmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n-\tcmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n \tcmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n \tcmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n \tcmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\ndiff --git a/run-command.h b/run-command.h\nindex cf250e36426..0af01bd9b67 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -226,12 +226,11 @@ int run_auto_maintenance(int quiet);\n \n #define RUN_COMMAND_NO_STDIN\t\t(1<<0)\n #define RUN_GIT_CMD\t\t\t(1<<1)\n-#define RUN_COMMAND_STDOUT_TO_STDERR\t(1<<2)\n-#define RUN_SILENT_EXEC_FAILURE\t\t(1<<3)\n-#define RUN_USING_SHELL\t\t\t(1<<4)\n-#define RUN_CLEAN_ON_EXIT\t\t(1<<5)\n-#define RUN_WAIT_AFTER_CLEAN\t\t(1<<6)\n-#define RUN_CLOSE_OBJECT_STORE\t\t(1<<7)\n+#define RUN_SILENT_EXEC_FAILURE\t\t(1<<2)\n+#define RUN_USING_SHELL\t\t\t(1<<3)\n+#define RUN_CLEAN_ON_EXIT\t\t(1<<4)\n+#define RUN_WAIT_AFTER_CLEAN\t\t(1<<5)\n+#define RUN_CLOSE_OBJECT_STORE\t\t(1<<6)\n \n /**\n * The run_command_v_opt*() API is a convenience wrapper for an\n@@ -249,7 +248,6 @@ int run_auto_maintenance(int quiet);\n *\n *\t- RUN_COMMAND_NO_STDIN: .no_stdin\n *\t- RUN_GIT_CMD: .git_cmd\n- *\t- RUN_COMMAND_STDOUT_TO_STDERR: .stdout_to_stderr\n *\t- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure\n *\t- RUN_USING_SHELL: .use_shell\n *\t- RUN_CLEAN_ON_EXIT: .clean_on_exit\n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464957","messageId":"patch-08.10-88e063f3b05-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH 08/10] run-command API & users: remove run_command_v_opt_tr2()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:20Z","receivedAt":"2022-10-14T15:41:01Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in the preceding commit for run_command_v_opt_cd_env() that\nfunction's users could more easily use the underlying run_command()\ndirectly.\n\nIn the case of the \"git.c\" user that can be argued the other way,\ngiven the slight line count increase here, but part of that's because\nthe \"args\" in \"git.c\" was being leaked, which we now don't have to\nworry about.\n\nThat just leaves the spawn_daemon() user in \"fsmonitor-ipc.c\", it's\nlikewise slightly more verbose as a result, but by making it use\nrun_command() we can remove this entire part of the API. As noted in a\npreceding commit run_command_v_opt*() should be aimed at handling\nvarious common cases, not these one-offs.\n\nThe \"fsmonitor-ipc.c\" caller would be nicer with a hypothetical\nrun_command_l_opt_tr2(), but let's not maintain such a thing only for\nit, as it would be its only user.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fsmonitor-ipc.c | 10 +++++++---\n git.c | 15 +++++++++------\n run-command.c | 28 ++--------------------------\n run-command.h | 19 +++----------------\n 4 files changed, 21 insertions(+), 51 deletions(-)\n\ndiff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c\nindex 789e7397baa..7251f48e456 100644\n--- a/fsmonitor-ipc.c\n+++ b/fsmonitor-ipc.c\n@@ -56,10 +56,14 @@ enum ipc_active_state fsmonitor_ipc__get_state(void)\n \n static int spawn_daemon(void)\n {\n-\tconst char *args[] = { \"fsmonitor--daemon\", \"start\", NULL };\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n-\treturn run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,\n-\t\t\t\t \"fsmonitor\");\n+\tcmd.git_cmd = 1;\n+\tcmd.no_stdin = 1;\n+\tcmd.trace2_child_class = \"fsmonitor\";\n+\tstrvec_pushl(&cmd.args, \"fsmonitor--daemon\", \"start\", NULL);\n+\n+\treturn run_command(&cmd);\n }\n \n int fsmonitor_ipc__send_query(const char *since_token,\ndiff --git a/git.c b/git.c\nindex da411c53822..ccf444417b5 100644\n--- a/git.c\n+++ b/git.c\n@@ -787,7 +787,7 @@ static int run_argv(int *argcp, const char ***argv)\n \t\tif (!done_alias)\n \t\t\thandle_builtin(*argcp, *argv);\n \t\telse if (get_builtin(**argv)) {\n-\t\t\tstruct strvec args = STRVEC_INIT;\n+\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \t\t\tint i;\n \n \t\t\t/*\n@@ -804,18 +804,21 @@ static int run_argv(int *argcp, const char ***argv)\n \n \t\t\tcommit_pager_choice();\n \n-\t\t\tstrvec_push(&args, \"git\");\n+\t\t\tstrvec_push(&cmd.args, \"git\");\n \t\t\tfor (i = 0; i < *argcp; i++)\n-\t\t\t\tstrvec_push(&args, (*argv)[i]);\n+\t\t\t\tstrvec_push(&cmd.args, (*argv)[i]);\n \n-\t\t\ttrace_argv_printf(args.v, \"trace: exec:\");\n+\t\t\ttrace_argv_printf(cmd.args.v, \"trace: exec:\");\n \n \t\t\t/*\n \t\t\t * if we fail because the command is not found, it is\n \t\t\t * OK to return. Otherwise, we just pass along the status code.\n \t\t\t */\n-\t\t\ti = run_command_v_opt_tr2(args.v, RUN_SILENT_EXEC_FAILURE |\n-\t\t\t\t\t\t RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, \"git_alias\");\n+\t\t\tcmd.silent_exec_failure = 1;\n+\t\t\tcmd.clean_on_exit = 1;\n+\t\t\tcmd.wait_after_clean = 1;\n+\t\t\tcmd.trace2_child_class = \"git_alias\";\n+\t\t\ti = run_command(&cmd);\n \t\t\tif (i >= 0 || errno != ENOENT)\n \t\t\t\texit(i);\n \t\t\tdie(\"could not execute builtin %s\", **argv);\ndiff --git a/run-command.c b/run-command.c\nindex 6132a9f19f0..0066ace85fa 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1010,7 +1010,6 @@ static void run_command_set_opts(struct child_process *cmd, int opt)\n \tcmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n \tcmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n \tcmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n-\tcmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n \tcmd->wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n \tcmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n }\n@@ -1031,35 +1030,12 @@ int run_command_l_opt(int opt, ...)\n }\n \n int run_command_v_opt(const char **argv, int opt)\n-{\n-\treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);\n-}\n-\n-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)\n-{\n-\treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);\n-}\n-\n-static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,\n-\t\t\t\t\t const char *dir,\n-\t\t\t\t\t const char *const *env,\n-\t\t\t\t\t const char *tr2_class)\n-{\n-\trun_command_set_opts(cmd, opt);\n-\tcmd->dir = dir;\n-\tif (env)\n-\t\tstrvec_pushv(&cmd->env, (const char **)env);\n-\tcmd->trace2_child_class = tr2_class;\n-\treturn run_command(cmd);\n-}\n-\n-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n-\t\t\t\t const char *const *env, const char *tr2_class)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n \tstrvec_pushv(&cmd.args, argv);\n-\treturn run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class);\n+\trun_command_set_opts(&cmd, opt);\n+\treturn run_command(&cmd);\n }\n \n #ifndef NO_PTHREADS\ndiff --git a/run-command.h b/run-command.h\nindex 2574d46cb70..2b1fe3cde5c 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -228,12 +228,11 @@ int run_auto_maintenance(int quiet);\n #define RUN_GIT_CMD\t\t\t(1<<1)\n #define RUN_SILENT_EXEC_FAILURE\t\t(1<<2)\n #define RUN_USING_SHELL\t\t\t(1<<3)\n-#define RUN_CLEAN_ON_EXIT\t\t(1<<4)\n-#define RUN_WAIT_AFTER_CLEAN\t\t(1<<5)\n-#define RUN_CLOSE_OBJECT_STORE\t\t(1<<6)\n+#define RUN_WAIT_AFTER_CLEAN\t\t(1<<4)\n+#define RUN_CLOSE_OBJECT_STORE\t\t(1<<5)\n \n /**\n- * The run_command_v_opt*() API is a convenience wrapper for an\n+ * The run_command_v_opt() function is a convenience wrapper for an\n * underlying run_command().\n *\n * It's intended to be used when the user already has an \"argv\" they'd\n@@ -250,21 +249,9 @@ int run_auto_maintenance(int quiet);\n *\t- RUN_GIT_CMD: .git_cmd\n *\t- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure\n *\t- RUN_USING_SHELL: .use_shell\n- *\t- RUN_CLEAN_ON_EXIT: .clean_on_exit\n- *\t- RUN_WAIT_AFTER_CLEAN: .wait_after_clean\n *\t- RUN_CLOSE_OBJECT_STORE: .close_object_store\n- *\n- * The argument dir corresponds the member .dir. The argument env\n- * corresponds to the member .env.\n */\n int run_command_v_opt(const char **argv, int opt);\n-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);\n-/*\n- * env (the environment) is to be formatted like environ: \"VAR=VALUE\".\n- * To unset an environment variable use just \"VAR\".\n- */\n-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n-\t\t\t\t const char *const *env, const char *tr2_class);\n \n /**\n * The run_command_l_opt() function run_command_v_opt() takes a list\n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464958","messageId":"patch-07.10-31e8536f28c-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH 07/10] run-command API & diff.c: remove run_command_v_opt_cd_env()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:19Z","receivedAt":"2022-10-14T15:41:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As we'll see in subsequent commits most of the run_command_v_opt_*()\nAPI users are a bad fit for what we'd like to do with that API. The\nideal use-case for it is something where we already have an \"argv\",\nand don't want the one-shot semantics of run_command().\n\nIn the case of run_command_v_opt_cd_env() there was only user of it,\nand that user was just accumulating complexity by not using\nrun_command() directly.\n\nBy not having to maintain its own \"argv\" and \"env\" it doesn't have to\nstrvec_clear() them both, which in the case of run_command() will be\ndone by child_process_clear() before it returns.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n diff.c | 26 ++++++++++++--------------\n run-command.c | 7 +------\n run-command.h | 3 +--\n tmp-objdir.h | 6 ++++--\n 4 files changed, 18 insertions(+), 24 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 648f6717a55..392530016fa 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4278,35 +4278,33 @@ static void run_external_diff(const char *pgm,\n \t\t\t const char *xfrm_msg,\n \t\t\t struct diff_options *o)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n-\tstruct strvec env = STRVEC_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n-\tstrvec_push(&argv, pgm);\n-\tstrvec_push(&argv, name);\n+\tstrvec_push(&cmd.args, pgm);\n+\tstrvec_push(&cmd.args, name);\n \n \tif (one && two) {\n-\t\tadd_external_diff_name(o->repo, &argv, name, one);\n+\t\tadd_external_diff_name(o->repo, &cmd.args, name, one);\n \t\tif (!other)\n-\t\t\tadd_external_diff_name(o->repo, &argv, name, two);\n+\t\t\tadd_external_diff_name(o->repo, &cmd.args, name, two);\n \t\telse {\n-\t\t\tadd_external_diff_name(o->repo, &argv, other, two);\n-\t\t\tstrvec_push(&argv, other);\n-\t\t\tstrvec_push(&argv, xfrm_msg);\n+\t\t\tadd_external_diff_name(o->repo, &cmd.args, other, two);\n+\t\t\tstrvec_push(&cmd.args, other);\n+\t\t\tstrvec_push(&cmd.args, xfrm_msg);\n \t\t}\n \t}\n \n-\tstrvec_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n-\tstrvec_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n+\tstrvec_pushf(&cmd.env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n+\tstrvec_pushf(&cmd.env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n \n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n-\tif (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))\n+\tcmd.use_shell = 1;\n+\tif (run_command(&cmd))\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n \n \tremove_tempfile();\n-\tstrvec_clear(&argv);\n-\tstrvec_clear(&env);\n }\n \n static int similarity_index(struct diff_filepair *p)\ndiff --git a/run-command.c b/run-command.c\nindex fe1b4a0b650..6132a9f19f0 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1032,7 +1032,7 @@ int run_command_l_opt(int opt, ...)\n \n int run_command_v_opt(const char **argv, int opt)\n {\n-\treturn run_command_v_opt_cd_env(argv, opt, NULL, NULL);\n+\treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);\n }\n \n int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)\n@@ -1040,11 +1040,6 @@ int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)\n \treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);\n }\n \n-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env)\n-{\n-\treturn run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);\n-}\n-\n static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,\n \t\t\t\t\t const char *dir,\n \t\t\t\t\t const char *const *env,\ndiff --git a/run-command.h b/run-command.h\nindex 0af01bd9b67..2574d46cb70 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -152,7 +152,7 @@ struct child_process {\n /**\n * The functions: child_process_init, start_command, finish_command,\n * run_command, run_command_l_opt, run_command_v_opt,\n- * run_command_v_opt_cd_env, child_process_clear do the following:\n+ * child_process_clear do the following:\n *\n * - If a system call failed, errno is set and -1 is returned. A diagnostic\n * is printed.\n@@ -263,7 +263,6 @@ int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);\n * env (the environment) is to be formatted like environ: \"VAR=VALUE\".\n * To unset an environment variable use just \"VAR\".\n */\n-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);\n int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n \t\t\t\t const char *const *env, const char *tr2_class);\n \ndiff --git a/tmp-objdir.h b/tmp-objdir.h\nindex 76efc7edee5..c96aa77396d 100644\n--- a/tmp-objdir.h\n+++ b/tmp-objdir.h\n@@ -10,9 +10,11 @@\n *\n * Example:\n *\n+ *\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+ *\t...\n *\tstruct tmp_objdir *t = tmp_objdir_create(\"incoming\");\n- *\tif (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&\n- *\t !tmp_objdir_migrate(t))\n+ * strvec_pushl(&cmd.env, tmp_objdir_env(t));\n+ *\tif (!run_command(&cmd) && !tmp_objdir_migrate(t))\n *\t\tprintf(\"success!\\n\");\n *\telse\n *\t\tdie(\"failed...tmp_objdir will clean up for us\");\n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464959","messageId":"patch-09.10-0f5524e40ad-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH 09/10] gc: use strvec_pushf(), avoid redundant strbuf_detach()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:21Z","receivedAt":"2022-10-14T15:41:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust code added in 3797a0a7b7a (maintenance: use Windows scheduled\ntasks, 2021-01-05) to use strvec_pushf() directly. Rather than having\na function that returns a strbuf_detach() we need to free and have the\n\"strvec_pushl()\" do its own \"xstrdup()\" we can format this in-place.\n\nBy changing this we only have the \"strvec_clear()\" between the\n\"run_command_v_opt()\" and the \"return result\" in\n\"schtasks_remove_task()\". In the subsequent commit we'll start using a\nhelper which'll allow us to skip the \"strvec_clear()\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 22 ++++++++--------------\n 1 file changed, 8 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 075b4637660..519e64e86ee 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1894,12 +1894,7 @@ static int is_schtasks_available(void)\n #endif\n }\n \n-static char *schtasks_task_name(const char *frequency)\n-{\n-\tstruct strbuf label = STRBUF_INIT;\n-\tstrbuf_addf(&label, \"Git Maintenance (%s)\", frequency);\n-\treturn strbuf_detach(&label, NULL);\n-}\n+#define SCHTASKS_NAME_FMT \"Git Maintenance (%s)\"\n \n static int schtasks_remove_task(enum schedule_priority schedule)\n {\n@@ -1907,16 +1902,15 @@ static int schtasks_remove_task(enum schedule_priority schedule)\n \tint result;\n \tstruct strvec args = STRVEC_INIT;\n \tconst char *frequency = get_frequency(schedule);\n-\tchar *name = schtasks_task_name(frequency);\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&args, cmd);\n-\tstrvec_pushl(&args, \"/delete\", \"/tn\", name, \"/f\", NULL);\n+\tstrvec_pushl(&args, \"/delete\", \"/tn\", NULL);\n+\tstrvec_pushf(&args, SCHTASKS_NAME_FMT, frequency);\n+\tstrvec_pushl(&args, \"/f\", NULL);\n \n \tresult = run_command_v_opt(args.v, 0);\n-\n \tstrvec_clear(&args);\n-\tfree(name);\n \treturn result;\n }\n \n@@ -1935,7 +1929,6 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tconst char *xml;\n \tstruct tempfile *tfile;\n \tconst char *frequency = get_frequency(schedule);\n-\tchar *name = schtasks_task_name(frequency);\n \tstruct strbuf tfilename = STRBUF_INIT;\n \n \tget_schedule_cmd(&cmd, NULL);\n@@ -2028,8 +2021,10 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \t \"\\n\";\n \tfprintf(tfile->fp, xml, exec_path, exec_path, frequency);\n \tstrvec_split(&child.args, cmd);\n-\tstrvec_pushl(&child.args, \"/create\", \"/tn\", name, \"/f\", \"/xml\",\n-\t\t\t\t get_tempfile_path(tfile), NULL);\n+\tstrvec_pushl(&child.args, \"/create\", \"/tn\", NULL);\n+\tstrvec_pushf(&child.args, SCHTASKS_NAME_FMT, frequency);\n+\tstrvec_pushl(&child.args, \"/f\", \"/xml\",\n+\t\t get_tempfile_path(tfile), NULL);\n \tclose_tempfile_gently(tfile);\n \n \tchild.no_stdout = 1;\n@@ -2040,7 +2035,6 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tresult = finish_command(&child);\n \n \tdelete_tempfile(&tfile);\n-\tfree(name);\n \treturn result;\n }\n \n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464960","messageId":"patch-10.10-874cb72c2f4-20221014T153426Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH 10/10] run-command API: add and use a run_command_sv_opt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-14T15:40:22Z","receivedAt":"2022-10-14T15:41:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a run_command_sv_opt() convenience wrapper for\nrun_command_v_opt(), as noted in the API documentation this is for the\ncommon case of wanting to construct a \"struct strvec\" to pass to\nrun_command_v_opt(), and as it's a one-shot to strvec_clear() it\nafterwards.\n\nLet's convert those API users that were using a \"ret\" variable to\ncarry over to \"return\" after a \"strvec_clear()\" to use this new\nfunction instead.\n\nLet's leave aside the user in \"builtin/bisect--helper.c\"'s\nbisect_visualize(). There's an outstanding topic that's extensively\nmodifying it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n add-interactive.c | 3 +--\n builtin/add.c | 6 ++----\n builtin/clone.c | 3 +--\n builtin/gc.c | 5 +----\n builtin/pull.c | 15 +++------------\n builtin/remote.c | 5 +----\n merge.c | 3 +--\n run-command.h | 20 +++++++++++++++++++-\n scalar.c | 6 +-----\n sequencer.c | 11 ++---------\n 10 files changed, 32 insertions(+), 45 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex f071b2a1b4f..9c86f3b9532 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -1007,8 +1007,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,\n \t\t\tif (files->selected[i])\n \t\t\t\tstrvec_push(&args,\n \t\t\t\t\t files->items.items[i].string);\n-\t\tres = run_command_v_opt(args.v, 0);\n-\t\tstrvec_clear(&args);\n+\t\tres = run_command_sv_opt(&args, 0);\n \t}\n \n \tputchar('\\n');\ndiff --git a/builtin/add.c b/builtin/add.c\nindex f84372964c8..7c783eebc0e 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -240,7 +240,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)\n int run_add_interactive(const char *revision, const char *patch_mode,\n \t\t\tconst struct pathspec *pathspec)\n {\n-\tint status, i;\n+\tint i;\n \tstruct strvec argv = STRVEC_INIT;\n \tint use_builtin_add_i =\n \t\tgit_env_bool(\"GIT_TEST_ADD_I_USE_BUILTIN\", -1);\n@@ -282,9 +282,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,\n \t\t/* pass original pathspec, to be re-parsed */\n \t\tstrvec_push(&argv, pathspec->items[i].original);\n \n-\tstatus = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&argv);\n-\treturn status;\n+\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n }\n \n int interactive_add(const char **argv, const char *prefix, int patch)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 13fc7a4bc5d..eabf8e0f196 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -756,8 +756,7 @@ static int checkout(int submodule_progress, int filter_submodules)\n \t\t\t\t\t \"--single-branch\" :\n \t\t\t\t\t \"--no-single-branch\");\n \n-\t\terr = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\t\tstrvec_clear(&args);\n+\t\treturn run_command_sv_opt(&args, RUN_GIT_CMD);\n \t}\n \n \treturn err;\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 519e64e86ee..8393e19b504 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1899,7 +1899,6 @@ static int is_schtasks_available(void)\n static int schtasks_remove_task(enum schedule_priority schedule)\n {\n \tconst char *cmd = \"schtasks\";\n-\tint result;\n \tstruct strvec args = STRVEC_INIT;\n \tconst char *frequency = get_frequency(schedule);\n \n@@ -1909,9 +1908,7 @@ static int schtasks_remove_task(enum schedule_priority schedule)\n \tstrvec_pushf(&args, SCHTASKS_NAME_FMT, frequency);\n \tstrvec_pushl(&args, \"/f\", NULL);\n \n-\tresult = run_command_v_opt(args.v, 0);\n-\tstrvec_clear(&args);\n-\treturn result;\n+\treturn run_command_sv_opt(&args, 0);\n }\n \n static int schtasks_remove_tasks(void)\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 403a24d7ca6..2f36823c97e 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -516,7 +516,6 @@ static void parse_repo_refspecs(int argc, const char **argv, const char **repo,\n static int run_fetch(const char *repo, const char **refspecs)\n {\n \tstruct strvec args = STRVEC_INIT;\n-\tint ret;\n \n \tstrvec_pushl(&args, \"fetch\", \"--update-head-ok\", NULL);\n \n@@ -582,9 +581,7 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\tstrvec_pushv(&args, refspecs);\n \t} else if (*refspecs)\n \t\tBUG(\"refspecs without repo?\");\n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);\n-\tstrvec_clear(&args);\n-\treturn ret;\n+\treturn run_command_sv_opt(&args, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);\n }\n \n /**\n@@ -653,7 +650,6 @@ static int update_submodules(void)\n */\n static int run_merge(void)\n {\n-\tint ret;\n \tstruct strvec args = STRVEC_INIT;\n \n \tstrvec_pushl(&args, \"merge\", NULL);\n@@ -696,9 +692,7 @@ static int run_merge(void)\n \t\tstrvec_push(&args, \"--allow-unrelated-histories\");\n \n \tstrvec_push(&args, \"FETCH_HEAD\");\n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n-\treturn ret;\n+\treturn run_command_sv_opt(&args, RUN_GIT_CMD);\n }\n \n /**\n@@ -879,7 +873,6 @@ static int get_rebase_newbase_and_upstream(struct object_id *newbase,\n static int run_rebase(const struct object_id *newbase,\n \t\tconst struct object_id *upstream)\n {\n-\tint ret;\n \tstruct strvec args = STRVEC_INIT;\n \n \tstrvec_push(&args, \"rebase\");\n@@ -913,9 +906,7 @@ static int run_rebase(const struct object_id *newbase,\n \n \tstrvec_push(&args, oid_to_hex(upstream));\n \n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n-\treturn ret;\n+\treturn run_command_sv_opt(&args, RUN_GIT_CMD);\n }\n \n static int get_can_ff(struct object_id *orig_head,\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 1d86c14297b..07de21a624e 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1508,7 +1508,6 @@ static int update(int argc, const char **argv, const char *prefix)\n \t};\n \tstruct strvec fetch_argv = STRVEC_INIT;\n \tint default_defined = 0;\n-\tint retval;\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_remote_update_usage,\n@@ -1534,9 +1533,7 @@ static int update(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tretval = run_command_v_opt(fetch_argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&fetch_argv);\n-\treturn retval;\n+\treturn run_command_sv_opt(&fetch_argv, RUN_GIT_CMD);\n }\n \n static int remove_all_fetch_refspecs(const char *key)\ndiff --git a/merge.c b/merge.c\nindex 2382ff66d35..487debacecb 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -33,8 +33,7 @@ int try_merge_command(struct repository *r,\n \tfor (j = remotes; j; j = j->next)\n \t\tstrvec_push(&args, merge_argument(j->item));\n \n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n+\tret = run_command_sv_opt(&args, RUN_GIT_CMD);\n \n \tdiscard_index(r->index);\n \tif (repo_read_index(r) < 0)\ndiff --git a/run-command.h b/run-command.h\nindex 2b1fe3cde5c..639cee4f4fb 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -151,7 +151,7 @@ struct child_process {\n \n /**\n * The functions: child_process_init, start_command, finish_command,\n- * run_command, run_command_l_opt, run_command_v_opt,\n+ * run_command, run_command_l_opt, run_command_v_opt, run_command_sv_opt,\n * child_process_clear do the following:\n *\n * - If a system call failed, errno is set and -1 is returned. A diagnostic\n@@ -262,6 +262,24 @@ int run_command_v_opt(const char **argv, int opt);\n LAST_ARG_MUST_BE_NULL\n int run_command_l_opt(int opt, ...);\n \n+/**\n+ * The run_command_sv_opt() function is a wrapper for\n+ * run_command_v_opt(). It takes a \"struct strvec *args\" which\n+ * similarly to run_command() (but not run_command_sv_opt()) will be\n+ * strvec_clear()'d before returning.\n+ *\n+ * Use it for the common case of constructing a \"struct strvec\" for a\n+ * one-shot run_command_v_opt() invocation.\n+ */\n+RESULT_MUST_BE_USED\n+static inline int run_command_sv_opt(struct strvec *args, int opt)\n+{\n+\tint ret = run_command_v_opt(args->v, opt);\n+\n+\tstrvec_clear(args);\n+\treturn ret;\n+}\n+\n /**\n * Execute the given command, sending \"in\" to its stdin, and capturing its\n * stdout and stderr in the \"out\" and \"err\" strbufs. Any of the three may\ndiff --git a/scalar.c b/scalar.c\nindex 6de9c0ee523..3480bf73cbd 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -72,7 +72,6 @@ static int run_git(const char *arg, ...)\n \tstruct strvec argv = STRVEC_INIT;\n \tva_list args;\n \tconst char *p;\n-\tint res;\n \n \tva_start(args, arg);\n \tstrvec_push(&argv, arg);\n@@ -80,10 +79,7 @@ static int run_git(const char *arg, ...)\n \t\tstrvec_push(&argv, p);\n \tva_end(args);\n \n-\tres = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\n-\tstrvec_clear(&argv);\n-\treturn res;\n+\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n }\n \n struct scalar_config {\ndiff --git a/sequencer.c b/sequencer.c\nindex 20495db9de2..7ee0e05512c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3183,7 +3183,6 @@ static int rollback_is_safe(void)\n \n static int reset_merge(const struct object_id *oid)\n {\n-\tint ret;\n \tstruct strvec argv = STRVEC_INIT;\n \n \tstrvec_pushl(&argv, \"reset\", \"--merge\", NULL);\n@@ -3191,10 +3190,7 @@ static int reset_merge(const struct object_id *oid)\n \tif (!is_null_oid(oid))\n \t\tstrvec_push(&argv, oid_to_hex(oid));\n \n-\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&argv);\n-\n-\treturn ret;\n+\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n }\n \n static int rollback_single_pick(struct repository *r)\n@@ -4866,7 +4862,6 @@ static int pick_commits(struct repository *r,\n static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n {\n \tstruct strvec argv = STRVEC_INIT;\n-\tint ret;\n \n \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n \t !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n@@ -4887,9 +4882,7 @@ static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n \t\t */\n \t\tstrvec_pushl(&argv, \"--no-edit\", \"--cleanup=strip\", NULL);\n \n-\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&argv);\n-\treturn ret;\n+\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n }\n \n static int commit_staged_changes(struct repository *r,\n-- \n2.38.0.1092.g8c0298861b0\n\n"},{"id":"464970","messageId":"xmqq4jw69sf9.fsf@gitster.g","threadId":"58626","inReplyTo":"patch-01.10-c1f701af6e8-20221014T153426Z-avarab@gmail.com","subject":"Re: [PATCH 01/10] run-command.c: refactor run_command_*_tr2() to internal helpers","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T18:22:18Z","receivedAt":"2022-10-14T18:22:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +static void run_command_set_opts(struct child_process *cmd, int opt)\n> +{\n> +\tcmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;\n> +\tcmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n> +\tcmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n> +\tcmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n> +\tcmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n> +\tcmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n> +\tcmd->wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n> +\tcmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n> +}\n\nThis, combined with the change in the caller that loses the\ncorresponding lines, does make sense. But\n\n\n> @@ -1019,24 +1031,26 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const\n> \treturn run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);\n> }\n> \n> +static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,\n> +\t\t\t\t\t const char *dir,\n> +\t\t\t\t\t const char *const *env,\n> +\t\t\t\t\t const char *tr2_class)\n> +{\n> +\trun_command_set_opts(cmd, opt);\n> +\tcmd->dir = dir;\n> +\tif (env)\n> +\t\tstrvec_pushv(&cmd->env, (const char **)env);\n> +\tcmd->trace2_child_class = tr2_class;\n> +\treturn run_command(cmd);\n> +}\n\nThis?\n\nThe original caller took argv and copied it into cmd.args itself and\nthe updated caller still does so because this new helper doesn't do\nso for it, but it no longer does the copying of env because this\nhelper does it.\n\nUnless we will add several more callers of this helper in later\nsteps, this sounds a bit too specialized to the way the first single\ncaller used to do it.\n\nBut let's keep reading to see if it is worth adding it.\n\n> int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n> \t\t\t\t const char *const *env, const char *tr2_class)\n> {\n> \tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\n> \tstrvec_pushv(&cmd.args, argv);\n> -\tcmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;\n> -\tcmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n> -\tcmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n> -\tcmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n> -\tcmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n> -\tcmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n> -\tcmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n> -\tcmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n> -\tcmd.dir = dir;\n> -\tif (env)\n> -\t\tstrvec_pushv(&cmd.env, (const char **)env);\n> -\tcmd.trace2_child_class = tr2_class;\n> -\treturn run_command(&cmd);\n> +\treturn run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class);\n> }\n> \n> #ifndef NO_PTHREADS\n"},{"id":"464972","messageId":"xmqqzgdy8dfz.fsf@gitster.g","threadId":"58626","inReplyTo":"patch-02.10-543ccbb1ee1-20221014T153426Z-avarab@gmail.com","subject":"Re: [PATCH 02/10] merge: remove always-the-same \"verbose\" arguments","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T18:31:12Z","receivedAt":"2022-10-14T18:31:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Simplify the code that builds the arguments for the \"read-tree\"\n> invocation in reset_hard() and read_empty() to remove the \"verbose\"\n> parameter.\n>\n> Before 172b6428d06 (do not overwrite untracked during merge from\n> unborn branch, 2010-11-14) there was a \"reset_hard()\" function that\n> would be called in two places, one of those passed a \"verbose=1\", the\n> other a \"verbose=0\".\n>\n> After 172b6428d06 when read_empty() was split off from reset_hard()\n> both of these functions only had one caller. The \"verbose\" in\n> read_empty() would always be false, and the one in reset_hard() would\n> always be true.\n>\n> There was never a good reason for the code to act this way, it\n> happened because the read_empty() function was a copy/pasted and\n> adjusted version of reset_hard().\n>\n> Since we're no longer conditionally adding the \"-v\" parameter\n> here (and we'd only add it for \"reset_hard()\" we'll be able to move to\n> a simpler and safer run-command API in the subsequent commit.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> builtin/merge.c | 13 +++++--------\n> 1 file changed, 5 insertions(+), 8 deletions(-)\n\nI haven't checked the topics in flight that touch the same file, but\nas these are file-scope static, it is easy to check the correctness,\nand the change of function signature will mean that compilers will\nnotice after a merge if there is somebody else who still wants them\nto be conditionally verbose.\n\nI wonder if these were always unused, or we lost different callers\nover time, though.\n\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 5900b81729d..3bb49d805b4 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -345,14 +345,12 @@ static int save_state(struct object_id *stash)\n> \treturn rc;\n> }\n> \n> -static void read_empty(const struct object_id *oid, int verbose)\n> +static void read_empty(const struct object_id *oid)\n> {\n> \tint i = 0;\n> \tconst char *args[7];\n> \n> \targs[i++] = \"read-tree\";\n> -\tif (verbose)\n> -\t\targs[i++] = \"-v\";\n> \targs[i++] = \"-m\";\n> \targs[i++] = \"-u\";\n> \targs[i++] = empty_tree_oid_hex();\n> @@ -363,14 +361,13 @@ static void read_empty(const struct object_id *oid, int verbose)\n> \t\tdie(_(\"read-tree failed\"));\n> }\n> \n> -static void reset_hard(const struct object_id *oid, int verbose)\n> +static void reset_hard(const struct object_id *oid)\n> {\n> \tint i = 0;\n> \tconst char *args[6];\n> \n> \targs[i++] = \"read-tree\";\n> -\tif (verbose)\n> -\t\targs[i++] = \"-v\";\n> +\targs[i++] = \"-v\";\n> \targs[i++] = \"--reset\";\n> \targs[i++] = \"-u\";\n> \targs[i++] = oid_to_hex(oid);\n> @@ -385,7 +382,7 @@ static void restore_state(const struct object_id *head,\n> {\n> \tstruct strvec args = STRVEC_INIT;\n> \n> -\treset_hard(head, 1);\n> +\treset_hard(head);\n> \n> \tif (is_null_oid(stash))\n> \t\tgoto refresh_cache;\n> @@ -1470,7 +1467,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \t\t\t\t\t check_trust_level);\n> \n> \t\tremote_head_oid = &remoteheads->item->object.oid;\n> -\t\tread_empty(remote_head_oid, 0);\n> +\t\tread_empty(remote_head_oid);\n> \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head_oid, NULL, 0,\n> \t\t\t UPDATE_REFS_DIE_ON_ERR);\n> \t\tgoto done;\n"},{"id":"464973","messageId":"xmqqtu468d0k.fsf@gitster.g","threadId":"58626","inReplyTo":"patch-03.10-fd81d44f221-20221014T153426Z-avarab@gmail.com","subject":"Re: [PATCH 03/10] run-command API: add and use a run_command_l_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T18:40:27Z","receivedAt":"2022-10-14T18:40:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> bisect.c | 19 +++++++++----------\n> builtin/clone.c | 16 ++++++----------\n> builtin/difftool.c | 14 ++++++--------\n> builtin/gc.c | 22 ++++++++--------------\n> builtin/merge.c | 35 ++++++-----------------------------\n> builtin/remote.c | 10 ++++------\n> compat/mingw.c | 8 +++-----\n> ll-merge.c | 4 +---\n> run-command.c | 15 +++++++++++++++\n> run-command.h | 13 +++++++++++--\n> sequencer.c | 4 +---\n> t/helper/test-fake-ssh.c | 4 +---\n> 12 files changed, 71 insertions(+), 93 deletions(-)\n\nNice.\n\n> @@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,\n> \n> static void dissociate_from_references(void)\n> {\n> -\tstatic const char* argv[] = { \"repack\", \"-a\", \"-d\", NULL };\n\nGood to see that this one in a wrong scope can now go away.\n\n> \tchar *alternates = git_pathdup(\"objects/info/alternates\");\n> \n> \tif (!access(alternates, F_OK)) {\n> -\t\tif (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))\n> +\t\tif (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,\n> +\t\t\t\t \"repack\", \"-a\", \"-d\", NULL))\n> \t\t\tdie(_(\"cannot repack to clean up\"));\n\n\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 910f7b9316a..1d86c14297b 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -92,13 +92,11 @@ static int verbose;\n> \n> static int fetch_remote(const char *name)\n> {\n> -\tconst char *argv[] = { \"fetch\", name, NULL, NULL };\n> -\tif (verbose) {\n> -\t\targv[1] = \"-v\";\n> -\t\targv[2] = name;\n> -\t}\n> \tprintf_ln(_(\"Updating %s\"), name);\n> -\tif (run_command_v_opt(argv, RUN_GIT_CMD))\n> +\tif (verbose && run_command_l_opt(RUN_GIT_CMD, \"-v\", \"fetch\", name,\n> +\t\t\t\t\t NULL))\n> +\t\treturn error(_(\"Could not fetch %s\"), name);\n> +\telse if (run_command_l_opt(RUN_GIT_CMD, \"fetch\", name, NULL))\n> \t\treturn error(_(\"Could not fetch %s\"), name);\n> \treturn 0;\n> }\n\nThis, together with the \"merge\" one that used to be conditional\n(which I take as a sign that new callers can legitimately wish it to\nbe conditional again), is where the new l_opt() variant is weak.\n\nAnd the above is wrong. If verbose option is given and run command\nsucceeds in running \"fetch -v\" (another bug is that the updated code\nis running \"git -v fetch \"), we will try running \"fetch\" without\n\"-v\" option after that.\n\nSo, for simplest things (i.e. the majority of places this patch\ntouches), addition of l_opt() is great. Use of it for nontrivial\nthings like this is not. We need to repeat ourselves, and the use\nof API is error prone.\n\n> diff --git a/compat/mingw.c b/compat/mingw.c\n> index 901375d5841..4f5392c5796 100644\n> --- a/compat/mingw.c\n> +++ b/compat/mingw.c\n> @@ -196,17 +196,15 @@ static int read_yes_no_answer(void)\n> static int ask_yes_no_if_possible(const char *format, ...)\n> {\n> \tchar question[4096];\n> -\tconst char *retry_hook[] = { NULL, NULL, NULL };\n\nGood to see that this one in a wrong scope can now go away.\n\n> +\tchar *retry_hook;\n> \tva_list args;\n> \n> \tva_start(args, format);\n> \tvsnprintf(question, sizeof(question), format, args);\n> \tva_end(args);\n> \n> -\tif ((retry_hook[0] = mingw_getenv(\"GIT_ASK_YESNO\"))) {\n> -\t\tretry_hook[1] = question;\n> -\t\treturn !run_command_v_opt(retry_hook, 0);\n> -\t}\n> +\tif ((retry_hook = mingw_getenv(\"GIT_ASK_YESNO\")))\n> +\t\treturn !run_command_l_opt(0, retry_hook, question, NULL);\n> \n> \tif (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))\n> \t\treturn 0;\n"},{"id":"464974","messageId":"xmqqpmeu8cuo.fsf@gitster.g","threadId":"58626","inReplyTo":"patch-04.10-4cd61aaa981-20221014T153426Z-avarab@gmail.com","subject":"Re: [PATCH 04/10] am: use run_command_l_opt() for show_patch()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T18:43:59Z","receivedAt":"2022-10-14T18:44:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The \"git show\" invocation added in 66335298a47 (rebase: add\n> --show-current-patch, 2018-02-11) is a one-off, and one where we're\n> not calling oid_to_hex() twice.\n\nI am not sure what relevance it has being a one-off in justifying\nthis change.\n\nI agree with the latter half of the explanation, though. As long as\nrun_command_l_opt() does not reuse the static oid_to_hex() buffers,\nthis is safe.\n\n> -\tif (!is_null_oid(&state->orig_commit)) {\n> -\t\tconst char *av[4] = { \"show\", NULL, \"--\", NULL };\n> -\t\tchar *new_oid_str;\n> -\t\tint ret;\n> -\n> -\t\tav[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));\n> -\t\tret = run_command_v_opt(av, RUN_GIT_CMD);\n> -\t\tfree(new_oid_str);\n> -\t\treturn ret;\n> -\t}\n> +\tif (!is_null_oid(&state->orig_commit))\n> +\t\treturn run_command_l_opt(RUN_GIT_CMD, \"show\",\n> +\t\t\t\t\t oid_to_hex(&state->orig_commit),\n> +\t\t\t\t\t \"--\", NULL);\n> \n> \tswitch (sub_mode) {\n> \tcase SHOW_PATCH_RAW:\n"},{"id":"464978","messageId":"2afd907d-5d9c-4968-901a-e88e3c0d4729@web.de","threadId":"58626","inReplyTo":"patch-10.10-874cb72c2f4-20221014T153426Z-avarab@gmail.com","subject":"Re: [PATCH 10/10] run-command API: add and use a run_command_sv_opt()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-14T19:21:32Z","receivedAt":"2022-10-14T19:22:36Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.10.22 um 17:40 schrieb Ævar Arnfjörð Bjarmason:\n> Add a run_command_sv_opt() convenience wrapper for\n> run_command_v_opt(), as noted in the API documentation this is for the\n> common case of wanting to construct a \"struct strvec\" to pass to\n> run_command_v_opt(), and as it's a one-shot to strvec_clear() it\n> afterwards.\n\nInteresting idea. It wastes memory by allocating the argument vector\ntwice, but for most call-sites this won't be noticeable. I suspect it\nmight fit at least some of the use cases for run_command_l_opt() better,\ne.g. those that previously used a strvec or that need some flexibility.\n\n> diff --git a/run-command.h b/run-command.h\n> index 2b1fe3cde5c..639cee4f4fb 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -151,7 +151,7 @@ struct child_process {\n>\n> /**\n> * The functions: child_process_init, start_command, finish_command,\n> - * run_command, run_command_l_opt, run_command_v_opt,\n> + * run_command, run_command_l_opt, run_command_v_opt, run_command_sv_opt,\n> * child_process_clear do the following:\n> *\n> * - If a system call failed, errno is set and -1 is returned. A diagnostic\n> @@ -262,6 +262,24 @@ int run_command_v_opt(const char **argv, int opt);\n> LAST_ARG_MUST_BE_NULL\n> int run_command_l_opt(int opt, ...);\n>\n> +/**\n> + * The run_command_sv_opt() function is a wrapper for\n> + * run_command_v_opt(). It takes a \"struct strvec *args\" which\n> + * similarly to run_command() (but not run_command_sv_opt()) will be\n ^^^^^^^^^^^^^^^^^^^^\nDo you mean run_command_v_opt()?\n\nRené\n"},{"id":"464982","messageId":"Y0nAm4+KL71vyYB8@coredump.intra.peff.net","threadId":"58626","inReplyTo":"xmqqtu468d0k.fsf@gitster.g","subject":"Re: [PATCH 03/10] run-command API: add and use a run_command_l_opt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-14T20:03:39Z","receivedAt":"2022-10-14T20:03:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 14, 2022 at 11:40:27AM -0700, Junio C Hamano wrote:\n\n> > @@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,\n> > \n> > static void dissociate_from_references(void)\n> > {\n> > -\tstatic const char* argv[] = { \"repack\", \"-a\", \"-d\", NULL };\n> \n> Good to see that this one in a wrong scope can now go away.\n\nIt definitely is broader scope than is necessary. I wonder if it makes\nthings easier to read, though, the way we would sometimes hoist things\nout of a function into a static-global to make them more obvious. I\ndunno.\n\n> > \tchar *alternates = git_pathdup(\"objects/info/alternates\");\n> > \n> > \tif (!access(alternates, F_OK)) {\n> > -\t\tif (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))\n> > +\t\tif (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,\n> > +\t\t\t\t \"repack\", \"-a\", \"-d\", NULL))\n> > \t\t\tdie(_(\"cannot repack to clean up\"));\n\nI just happened to notice in this one there is a weird extra space\nbefore \"-a\".\n\n-Peff\n"},{"id":"464986","messageId":"xmqq8rli881x.fsf@gitster.g","threadId":"58626","inReplyTo":"Y0nAm4+KL71vyYB8@coredump.intra.peff.net","subject":"Re: [PATCH 03/10] run-command API: add and use a run_command_l_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T20:27:38Z","receivedAt":"2022-10-14T20:27:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Oct 14, 2022 at 11:40:27AM -0700, Junio C Hamano wrote:\n>\n>> > @@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,\n>> > \n>> > static void dissociate_from_references(void)\n>> > {\n>> > -\tstatic const char* argv[] = { \"repack\", \"-a\", \"-d\", NULL };\n>> \n>> Good to see that this one in a wrong scope can now go away.\n>\n> It definitely is broader scope than is necessary. I wonder if it makes\n> things easier to read, though, the way we would sometimes hoist things\n> out of a function into a static-global to make them more obvious. I\n> dunno.\n\nDidn't think about it, but it is a worthy point to make. Unlike the\ncall to l_opt() buried inside a conditional, it makes it stand out\nthat what we see upfront is one of the commands the function will\nrun. It is especially valuable when a function is almost exclusively\nabout running a single command, but then we will have a single call\nto l_opt() without much preparations or clean-ups around it, so the\nvisual noise that detracts our eyes away from the actual commands\nmay not be all that bad, though.\n\n>> > \tchar *alternates = git_pathdup(\"objects/info/alternates\");\n>> > \n>> > \tif (!access(alternates, F_OK)) {\n>> > -\t\tif (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))\n>> > +\t\tif (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,\n>> > +\t\t\t\t \"repack\", \"-a\", \"-d\", NULL))\n>> > \t\t\tdie(_(\"cannot repack to clean up\"));\n>\n> I just happened to notice in this one there is a weird extra space\n> before \"-a\".\n\nYeah, good eyes.\n\nThanks.\n"},{"id":"465120","messageId":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com","subject":"[PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:11Z","receivedAt":"2022-10-17T17:49:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series provides a more idiomatic set of run-command API helpers\nto match our current use-cases for run_command_v_opt(). See v1[1] for\na more general overview.\n\nChanges since v1:\n\n * Fixed a migration bug in builtin/remote.c noted by Junio (just\n skipping that case).\n\n * Fixed an indentation issue noted by Jeff.\n\n * Changed run_command_sv_opt() so that we take full advantage of\n having the \"struct strvec *\", and move it to \"cmd.args\", rather\n than copying its contents.\n\n * Rewrote how 1/10 uses the \"opts\" helper, in response to Junio's\n comment.\n\n * Small commit message touch-ups.\n\n1. https://lore.kernel.org/git/cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com/\n\nCI & branch for this topic available at\nhttps://github.com/avar/git/tree/avar/run-command-wrapper-API-simplification-2\n\nÆvar Arnfjörð Bjarmason (10):\n run-command.c: refactor run_command_*_tr2() to internal helpers\n merge: remove always-the-same \"verbose\" arguments\n run-command API: add and use a run_command_l_opt()\n am: use run_command_l_opt() for show_patch()\n run-command API docs: clarify & fleshen out run_command_v_opt*() docs\n run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag\n run-command API & diff.c: remove run_command_v_opt_cd_env()\n run-command API & users: remove run_command_v_opt_tr2()\n gc: use strvec_pushf(), avoid redundant strbuf_detach()\n run-command API: add and use a run_command_sv_opt()\n\n add-interactive.c | 3 +-\n bisect.c | 19 ++++++-----\n builtin/add.c | 6 ++--\n builtin/am.c | 14 +++-----\n builtin/clone.c | 19 ++++-------\n builtin/difftool.c | 14 ++++----\n builtin/gc.c | 49 ++++++++++------------------\n builtin/merge.c | 46 ++++++--------------------\n builtin/pull.c | 15 ++-------\n builtin/remote.c | 5 +--\n compat/mingw.c | 8 ++---\n diff.c | 26 +++++++--------\n fsmonitor-ipc.c | 10 ++++--\n git.c | 15 +++++----\n ll-merge.c | 4 +--\n merge.c | 3 +-\n run-command.c | 52 +++++++++++++++++------------\n run-command.h | 70 ++++++++++++++++++++++++++--------------\n scalar.c | 6 +---\n sequencer.c | 15 ++-------\n t/helper/test-fake-ssh.c | 4 +--\n tmp-objdir.h | 6 ++--\n 22 files changed, 179 insertions(+), 230 deletions(-)\n\nRange-diff against v1:\n 1: c1f701af6e8 ! 1: 3842204371e run-command.c: refactor run_command_*_tr2() to internal helpers\n @@ run-command.c: int run_command(struct child_process *cmd)\n +\tcmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n +}\n +\n - int run_command_v_opt(const char **argv, int opt)\n - {\n - \treturn run_command_v_opt_cd_env(argv, opt, NULL, NULL);\n -@@ run-command.c: int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const\n - \treturn run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);\n - }\n - \n -+static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,\n -+\t\t\t\t\t const char *dir,\n -+\t\t\t\t\t const char *const *env,\n -+\t\t\t\t\t const char *tr2_class)\n ++static int run_command_v_opt_1(struct child_process *cmd, int opt)\n +{\n +\trun_command_set_opts(cmd, opt);\n -+\tcmd->dir = dir;\n -+\tif (env)\n -+\t\tstrvec_pushv(&cmd->env, (const char **)env);\n -+\tcmd->trace2_child_class = tr2_class;\n +\treturn run_command(cmd);\n +}\n +\n - int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n - \t\t\t\t const char *const *env, const char *tr2_class)\n + int run_command_v_opt(const char **argv, int opt)\n + {\n + \treturn run_command_v_opt_cd_env(argv, opt, NULL, NULL);\n +@@ run-command.c: int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n -+\n \tstrvec_pushv(&cmd.args, argv);\n -\tcmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;\n -\tcmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n @@ run-command.c: int run_command_v_opt_cd_env(const char **argv, int opt, const ch\n -\tcmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n -\tcmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n -\tcmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n --\tcmd.dir = dir;\n --\tif (env)\n --\t\tstrvec_pushv(&cmd.env, (const char **)env);\n --\tcmd.trace2_child_class = tr2_class;\n + \tcmd.dir = dir;\n + \tif (env)\n + \t\tstrvec_pushv(&cmd.env, (const char **)env);\n + \tcmd.trace2_child_class = tr2_class;\n -\treturn run_command(&cmd);\n -+\treturn run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class);\n ++\treturn run_command_v_opt_1(&cmd, opt);\n }\n \n #ifndef NO_PTHREADS\n 2: 543ccbb1ee1 = 2: 8b00172ef83 merge: remove always-the-same \"verbose\" arguments\n 3: fd81d44f221 ! 3: 680a42a878e run-command API: add and use a run_command_l_opt()\n @@ builtin/clone.c: static void write_refspec_config(const char *src_ref_prefix,\n \tif (!access(alternates, F_OK)) {\n -\t\tif (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))\n +\t\tif (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,\n -+\t\t\t\t \"repack\", \"-a\", \"-d\", NULL))\n ++\t\t\t\t \"repack\", \"-a\", \"-d\", NULL))\n \t\t\tdie(_(\"cannot repack to clean up\"));\n \t\tif (unlink(alternates) && errno != ENOENT)\n \t\t\tdie_errno(_(\"cannot unlink temporary alternates file\"));\n @@ builtin/merge.c: static int save_state(struct object_id *stash)\n refresh_cache:\n \tif (discard_cache() < 0 || read_cache() < 0)\n \n - ## builtin/remote.c ##\n -@@ builtin/remote.c: static int verbose;\n - \n - static int fetch_remote(const char *name)\n - {\n --\tconst char *argv[] = { \"fetch\", name, NULL, NULL };\n --\tif (verbose) {\n --\t\targv[1] = \"-v\";\n --\t\targv[2] = name;\n --\t}\n - \tprintf_ln(_(\"Updating %s\"), name);\n --\tif (run_command_v_opt(argv, RUN_GIT_CMD))\n -+\tif (verbose && run_command_l_opt(RUN_GIT_CMD, \"-v\", \"fetch\", name,\n -+\t\t\t\t\t NULL))\n -+\t\treturn error(_(\"Could not fetch %s\"), name);\n -+\telse if (run_command_l_opt(RUN_GIT_CMD, \"fetch\", name, NULL))\n - \t\treturn error(_(\"Could not fetch %s\"), name);\n - \treturn 0;\n - }\n -\n ## compat/mingw.c ##\n @@ compat/mingw.c: static int read_yes_no_answer(void)\n static int ask_yes_no_if_possible(const char *format, ...)\n @@ run-command.c: static void run_command_set_opts(struct child_process *cmd, int o\n +\treturn run_command(&cmd);\n +}\n +\n - int run_command_v_opt(const char **argv, int opt)\n + static int run_command_v_opt_1(struct child_process *cmd, int opt)\n {\n - \treturn run_command_v_opt_cd_env(argv, opt, NULL, NULL);\n + \trun_command_set_opts(cmd, opt);\n \n ## run-command.h ##\n @@ run-command.h: struct child_process {\n 4: 4cd61aaa981 ! 4: 5cfd6a94ce3 am: use run_command_l_opt() for show_patch()\n @@ Commit message\n am: use run_command_l_opt() for show_patch()\n \n The \"git show\" invocation added in 66335298a47 (rebase: add\n - --show-current-patch, 2018-02-11) is a one-off, and one where we're\n - not calling oid_to_hex() twice. So we can rely on the static buffer\n - that oid_to_hex() points to, rather than xstrdup()-ing it. As a result\n - we can use the run_command_l_opt() function.\n + --show-current-patch, 2018-02-11) is one where we're not calling\n + oid_to_hex() twice. So we can rely on the static buffer that\n + oid_to_hex() points to, rather than xstrdup()-ing it. As a result we\n + can use the run_command_l_opt() function.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n 5: b6a3c4c66f8 = 5: 4fca38bb4d6 run-command API docs: clarify & fleshen out run_command_v_opt*() docs\n 6: 9d0286fbf64 = 6: 75eccc152ad run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag\n 7: 31e8536f28c ! 7: 3b3d3777232 run-command API & diff.c: remove run_command_v_opt_cd_env()\n @@ diff.c: static void run_external_diff(const char *pgm,\n static int similarity_index(struct diff_filepair *p)\n \n ## run-command.c ##\n -@@ run-command.c: int run_command_l_opt(int opt, ...)\n +@@ run-command.c: static int run_command_v_opt_1(struct child_process *cmd, int opt)\n \n int run_command_v_opt(const char **argv, int opt)\n {\n @@ run-command.c: int run_command_v_opt_tr2(const char **argv, int opt, const char\n -\treturn run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);\n -}\n -\n - static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,\n - \t\t\t\t\t const char *dir,\n - \t\t\t\t\t const char *const *env,\n + int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n + \t\t\t\t const char *const *env, const char *tr2_class)\n + {\n \n ## run-command.h ##\n @@ run-command.h: struct child_process {\n 8: 88e063f3b05 ! 8: 4f1a051823f run-command API & users: remove run_command_v_opt_tr2()\n @@ run-command.c: static void run_command_set_opts(struct child_process *cmd, int o\n \tcmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n }\n @@ run-command.c: int run_command_l_opt(int opt, ...)\n + \treturn run_command(&cmd);\n }\n \n +-static int run_command_v_opt_1(struct child_process *cmd, int opt)\n +-{\n +-\trun_command_set_opts(cmd, opt);\n +-\treturn run_command(cmd);\n +-}\n +-\n int run_command_v_opt(const char **argv, int opt)\n -{\n -\treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);\n @@ run-command.c: int run_command_l_opt(int opt, ...)\n -\treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);\n -}\n -\n --static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,\n --\t\t\t\t\t const char *dir,\n --\t\t\t\t\t const char *const *env,\n --\t\t\t\t\t const char *tr2_class)\n --{\n --\trun_command_set_opts(cmd, opt);\n --\tcmd->dir = dir;\n --\tif (env)\n --\t\tstrvec_pushv(&cmd->env, (const char **)env);\n --\tcmd->trace2_child_class = tr2_class;\n --\treturn run_command(cmd);\n --}\n --\n -int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n -\t\t\t\t const char *const *env, const char *tr2_class)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n - \n ++\n \tstrvec_pushv(&cmd.args, argv);\n --\treturn run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class);\n +-\tcmd.dir = dir;\n +-\tif (env)\n +-\t\tstrvec_pushv(&cmd.env, (const char **)env);\n +-\tcmd.trace2_child_class = tr2_class;\n +-\treturn run_command_v_opt_1(&cmd, opt);\n +\trun_command_set_opts(&cmd, opt);\n +\treturn run_command(&cmd);\n }\n 9: 0f5524e40ad = 9: 99c5688797a gc: use strvec_pushf(), avoid redundant strbuf_detach()\n10: 874cb72c2f4 ! 10: 138af632a36 run-command API: add and use a run_command_sv_opt()\n @@ Commit message\n carry over to \"return\" after a \"strvec_clear()\" to use this new\n function instead.\n \n + Because we pass the \"struct strvec *\" to the function we can also\n + avoid copying the arguments to the \"args\" member of the \"struct\n + child_process\", as we were doing with run_command_v_opt().\n +\n + Instead we can use memcpy() and strvec_clear() to do the moral\n + equivalent of a strbuf_{detach,attach}(). The strvec API doesn't have\n + a strvec_attach(), we could add it here while at it, but let's avoid\n + generalizing the interface for now and migrate the \"struct strvec *\"\n + in the \"run_command_sv_opt()\" instead.\n +\n Let's leave aside the user in \"builtin/bisect--helper.c\"'s\n bisect_visualize(). There's an outstanding topic that's extensively\n modifying it.\n @@ merge.c: int try_merge_command(struct repository *r,\n \tdiscard_index(r->index);\n \tif (repo_read_index(r) < 0)\n \n + ## run-command.c ##\n +@@ run-command.c: int run_command_v_opt(const char **argv, int opt)\n + \treturn run_command(&cmd);\n + }\n + \n ++int run_command_sv_opt(struct strvec *args, int opt)\n ++{\n ++\tstruct child_process cmd = CHILD_PROCESS_INIT;\n ++\n ++\t/* TODO: We could encapsulate this with a strvec_attach() */\n ++\tmemcpy(&cmd.args, args, sizeof(*args));\n ++\tstrvec_init(args);\n ++\trun_command_set_opts(&cmd, opt);\n ++\treturn run_command(&cmd);\n ++}\n ++\n + #ifndef NO_PTHREADS\n + static pthread_t main_thread;\n + static int main_thread_set;\n +\n ## run-command.h ##\n @@ run-command.h: struct child_process {\n \n @@ run-command.h: int run_command_v_opt(const char **argv, int opt);\n +/**\n + * The run_command_sv_opt() function is a wrapper for\n + * run_command_v_opt(). It takes a \"struct strvec *args\" which\n -+ * similarly to run_command() (but not run_command_sv_opt()) will be\n -+ * strvec_clear()'d before returning.\n ++ * similarly will be strvec_clear()'d before returning.\n + *\n + * Use it for the common case of constructing a \"struct strvec\" for a\n + * one-shot run_command_v_opt() invocation.\n ++ *\n ++ * The \"args\" will migrated the \"cmd.args\" member of an underlying\n ++ * \"struct child_process\", in a way that avoids making an extra copy.\n + */\n +RESULT_MUST_BE_USED\n -+static inline int run_command_sv_opt(struct strvec *args, int opt)\n -+{\n -+\tint ret = run_command_v_opt(args->v, opt);\n -+\n -+\tstrvec_clear(args);\n -+\treturn ret;\n -+}\n ++int run_command_sv_opt(struct strvec *args, int opt);\n +\n /**\n * Execute the given command, sending \"in\" to its stdin, and capturing its\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465121","messageId":"patch-v2-01.10-3842204371e-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"[PATCH v2 01/10] run-command.c: refactor run_command_*_tr2() to internal helpers","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:12Z","receivedAt":"2022-10-17T17:49:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor the run_command_v_opt_cd_env_tr2() function to helpers which\ntakes a \"struct child_process *cmd\" argument. This will allow for\nadding a helper which won't need to copy a set of arguments to the\n\"cmd.args\" we'd otherwise have to populate from the \"argv\".\n\nSplitting out the part of the function that sets the various members\nfrom \"opt\" will help in the subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 28 +++++++++++++++++++---------\n 1 file changed, 19 insertions(+), 9 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 5ec3a46dccf..4e75f83b5e7 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1004,6 +1004,24 @@ int run_command(struct child_process *cmd)\n \treturn finish_command(cmd);\n }\n \n+static void run_command_set_opts(struct child_process *cmd, int opt)\n+{\n+\tcmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;\n+\tcmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n+\tcmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n+\tcmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n+\tcmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n+\tcmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n+\tcmd->wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n+\tcmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n+}\n+\n+static int run_command_v_opt_1(struct child_process *cmd, int opt)\n+{\n+\trun_command_set_opts(cmd, opt);\n+\treturn run_command(cmd);\n+}\n+\n int run_command_v_opt(const char **argv, int opt)\n {\n \treturn run_command_v_opt_cd_env(argv, opt, NULL, NULL);\n@@ -1024,19 +1042,11 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstrvec_pushv(&cmd.args, argv);\n-\tcmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;\n-\tcmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n-\tcmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n-\tcmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n-\tcmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n-\tcmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n-\tcmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n-\tcmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n \tcmd.dir = dir;\n \tif (env)\n \t\tstrvec_pushv(&cmd.env, (const char **)env);\n \tcmd.trace2_child_class = tr2_class;\n-\treturn run_command(&cmd);\n+\treturn run_command_v_opt_1(&cmd, opt);\n }\n \n #ifndef NO_PTHREADS\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465122","messageId":"patch-v2-02.10-8b00172ef83-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"[PATCH v2 02/10] merge: remove always-the-same \"verbose\" arguments","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:13Z","receivedAt":"2022-10-17T17:49:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Simplify the code that builds the arguments for the \"read-tree\"\ninvocation in reset_hard() and read_empty() to remove the \"verbose\"\nparameter.\n\nBefore 172b6428d06 (do not overwrite untracked during merge from\nunborn branch, 2010-11-14) there was a \"reset_hard()\" function that\nwould be called in two places, one of those passed a \"verbose=1\", the\nother a \"verbose=0\".\n\nAfter 172b6428d06 when read_empty() was split off from reset_hard()\nboth of these functions only had one caller. The \"verbose\" in\nread_empty() would always be false, and the one in reset_hard() would\nalways be true.\n\nThere was never a good reason for the code to act this way, it\nhappened because the read_empty() function was a copy/pasted and\nadjusted version of reset_hard().\n\nSince we're no longer conditionally adding the \"-v\" parameter\nhere (and we'd only add it for \"reset_hard()\" we'll be able to move to\na simpler and safer run-command API in the subsequent commit.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/merge.c | 13 +++++--------\n 1 file changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5900b81729d..3bb49d805b4 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -345,14 +345,12 @@ static int save_state(struct object_id *stash)\n \treturn rc;\n }\n \n-static void read_empty(const struct object_id *oid, int verbose)\n+static void read_empty(const struct object_id *oid)\n {\n \tint i = 0;\n \tconst char *args[7];\n \n \targs[i++] = \"read-tree\";\n-\tif (verbose)\n-\t\targs[i++] = \"-v\";\n \targs[i++] = \"-m\";\n \targs[i++] = \"-u\";\n \targs[i++] = empty_tree_oid_hex();\n@@ -363,14 +361,13 @@ static void read_empty(const struct object_id *oid, int verbose)\n \t\tdie(_(\"read-tree failed\"));\n }\n \n-static void reset_hard(const struct object_id *oid, int verbose)\n+static void reset_hard(const struct object_id *oid)\n {\n \tint i = 0;\n \tconst char *args[6];\n \n \targs[i++] = \"read-tree\";\n-\tif (verbose)\n-\t\targs[i++] = \"-v\";\n+\targs[i++] = \"-v\";\n \targs[i++] = \"--reset\";\n \targs[i++] = \"-u\";\n \targs[i++] = oid_to_hex(oid);\n@@ -385,7 +382,7 @@ static void restore_state(const struct object_id *head,\n {\n \tstruct strvec args = STRVEC_INIT;\n \n-\treset_hard(head, 1);\n+\treset_hard(head);\n \n \tif (is_null_oid(stash))\n \t\tgoto refresh_cache;\n@@ -1470,7 +1467,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t check_trust_level);\n \n \t\tremote_head_oid = &remoteheads->item->object.oid;\n-\t\tread_empty(remote_head_oid, 0);\n+\t\tread_empty(remote_head_oid);\n \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head_oid, NULL, 0,\n \t\t\t UPDATE_REFS_DIE_ON_ERR);\n \t\tgoto done;\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465123","messageId":"patch-v2-04.10-5cfd6a94ce3-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"[PATCH v2 04/10] am: use run_command_l_opt() for show_patch()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:15Z","receivedAt":"2022-10-17T17:49:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The \"git show\" invocation added in 66335298a47 (rebase: add\n--show-current-patch, 2018-02-11) is one where we're not calling\noid_to_hex() twice. So we can rely on the static buffer that\noid_to_hex() points to, rather than xstrdup()-ing it. As a result we\ncan use the run_command_l_opt() function.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/am.c | 14 ++++----------\n 1 file changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 39fea248330..1d298a91306 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2186,16 +2186,10 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)\n \tconst char *patch_path;\n \tint len;\n \n-\tif (!is_null_oid(&state->orig_commit)) {\n-\t\tconst char *av[4] = { \"show\", NULL, \"--\", NULL };\n-\t\tchar *new_oid_str;\n-\t\tint ret;\n-\n-\t\tav[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));\n-\t\tret = run_command_v_opt(av, RUN_GIT_CMD);\n-\t\tfree(new_oid_str);\n-\t\treturn ret;\n-\t}\n+\tif (!is_null_oid(&state->orig_commit))\n+\t\treturn run_command_l_opt(RUN_GIT_CMD, \"show\",\n+\t\t\t\t\t oid_to_hex(&state->orig_commit),\n+\t\t\t\t\t \"--\", NULL);\n \n \tswitch (sub_mode) {\n \tcase SHOW_PATCH_RAW:\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465125","messageId":"patch-v2-03.10-680a42a878e-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"[PATCH v2 03/10] run-command API: add and use a run_command_l_opt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:14Z","receivedAt":"2022-10-17T17:49:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When run_command() is used with strvec_pushl() as in the pre-image of\nb004c902827 (gc: simplify maintenance_task_pack_refs(), 2022-10-04) we\nget the benefit of the LAST_ARG_MUST_BE_NULL. See 9fe3edc47f1 (Add the\nLAST_ARG_MUST_BE_NULL macro, 2013-07-18).\n\nLet's provide and use a run_command_l_opt() function, which gives us\nthe best of both worlds. We'll now need less code to accomplish the\nsame thing, and this version's safer as we can assert that the\nterminating NULL is present.\n\nThe \"builtin/bisect--helper.c\" would be a large beneficiary of this\nAPI, with \"15 insertions(+), 26 deletions(-)\", but let's leave that\naside for now, as there's an outstanding topic that's extensively\nmodifying it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n bisect.c | 19 +++++++++----------\n builtin/clone.c | 16 ++++++----------\n builtin/difftool.c | 14 ++++++--------\n builtin/gc.c | 22 ++++++++--------------\n builtin/merge.c | 35 ++++++-----------------------------\n compat/mingw.c | 8 +++-----\n ll-merge.c | 4 +---\n run-command.c | 15 +++++++++++++++\n run-command.h | 13 +++++++++++--\n sequencer.c | 4 +---\n t/helper/test-fake-ssh.c | 4 +---\n 11 files changed, 67 insertions(+), 87 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex fd581b85a72..415b0e7b0b2 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -738,18 +738,17 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,\n \tupdate_ref(NULL, \"BISECT_EXPECTED_REV\", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n \n \targv_checkout[2] = bisect_rev_hex;\n-\tif (no_checkout) {\n+\tif (no_checkout)\n \t\tupdate_ref(NULL, \"BISECT_HEAD\", bisect_rev, NULL, 0,\n \t\t\t UPDATE_REFS_DIE_ON_ERR);\n-\t} else {\n-\t\tif (run_command_v_opt(argv_checkout, RUN_GIT_CMD))\n-\t\t\t/*\n-\t\t\t * Errors in `run_command()` itself, signaled by res < 0,\n-\t\t\t * and errors in the child process, signaled by res > 0\n-\t\t\t * can both be treated as regular BISECT_FAILED (-1).\n-\t\t\t */\n-\t\t\treturn BISECT_FAILED;\n-\t}\n+\telse if (run_command_l_opt(RUN_GIT_CMD, \"checkout\", \"-q\",\n+\t\t\t\t bisect_rev_hex, \"--\", NULL))\n+\t\t/*\n+\t\t * Errors in `run_command()` itself, signaled by res < 0,\n+\t\t * and errors in the child process, signaled by res > 0\n+\t\t * can both be treated as regular BISECT_FAILED (-1).\n+\t\t */\n+\t\treturn BISECT_FAILED;\n \n \tcommit = lookup_commit_reference(the_repository, bisect_rev);\n \tformat_commit_message(commit, \"[%H] %s%n\", &commit_msg, &pp);\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ed8d44bb6ab..a1514363190 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -651,23 +651,19 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \n static int git_sparse_checkout_init(const char *repo)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n-\tint result = 0;\n-\tstrvec_pushl(&argv, \"-C\", repo, \"sparse-checkout\", \"set\", NULL);\n-\n \t/*\n \t * We must apply the setting in the current process\n \t * for the later checkout to use the sparse-checkout file.\n \t */\n \tcore_apply_sparse_checkout = 1;\n \n-\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\tif (run_command_l_opt(RUN_GIT_CMD, \"-C\", repo, \"sparse-checkout\",\n+\t\t\t \"set\", NULL)) {\n \t\terror(_(\"failed to initialize sparse-checkout\"));\n-\t\tresult = 1;\n+\t\treturn 1;\n \t}\n \n-\tstrvec_clear(&argv);\n-\treturn result;\n+\treturn 0;\n }\n \n static int checkout(int submodule_progress, int filter_submodules)\n@@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,\n \n static void dissociate_from_references(void)\n {\n-\tstatic const char* argv[] = { \"repack\", \"-a\", \"-d\", NULL };\n \tchar *alternates = git_pathdup(\"objects/info/alternates\");\n \n \tif (!access(alternates, F_OK)) {\n-\t\tif (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))\n+\t\tif (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,\n+\t\t\t\t \"repack\", \"-a\", \"-d\", NULL))\n \t\t\tdie(_(\"cannot repack to clean up\"));\n \t\tif (unlink(alternates) && errno != ENOENT)\n \t\t\tdie_errno(_(\"cannot unlink temporary alternates file\"));\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 4b10ad1a369..ed211a87322 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -44,8 +44,8 @@ static int difftool_config(const char *var, const char *value, void *cb)\n \n static int print_tool_help(void)\n {\n-\tconst char *argv[] = { \"mergetool\", \"--tool-help=diff\", NULL };\n-\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n+\treturn run_command_l_opt(RUN_GIT_CMD, \"mergetool\", \"--tool-help=diff\",\n+\t\t\t\t NULL);\n }\n \n static int parse_index_info(char *p, int *mode1, int *mode2,\n@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct index_state wtindex;\n \tstruct checkout lstate, rstate;\n \tint flags = RUN_GIT_CMD, err = 0;\n-\tconst char *helper_argv[] = { \"difftool--helper\", NULL, NULL, NULL };\n+\tconst char *helper_command = \"difftool--helper\";\n \tstruct hashmap wt_modified, tmp_modified;\n \tint indices_loaded = 0;\n \n@@ -563,16 +563,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t}\n \n \tstrbuf_setlen(&ldir, ldir_len);\n-\thelper_argv[1] = ldir.buf;\n \tstrbuf_setlen(&rdir, rdir_len);\n-\thelper_argv[2] = rdir.buf;\n-\n \tif (extcmd) {\n-\t\thelper_argv[0] = extcmd;\n+\t\thelper_command = extcmd;\n \t\tflags = 0;\n \t} else\n \t\tsetenv(\"GIT_DIFFTOOL_DIRDIFF\", \"true\", 1);\n-\tret = run_command_v_opt(helper_argv, flags);\n+\tret = run_command_l_opt(flags, helper_command, ldir.buf, rdir.buf,\n+\t\t\t\tNULL);\n \n \t/* TODO: audit for interaction with sparse-index. */\n \tensure_full_index(&wtindex);\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 243ee85d283..075b4637660 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -58,8 +58,6 @@ static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;\n static struct strvec reflog = STRVEC_INIT;\n static struct strvec repack = STRVEC_INIT;\n static struct strvec prune = STRVEC_INIT;\n-static struct strvec prune_worktrees = STRVEC_INIT;\n-static struct strvec rerere = STRVEC_INIT;\n \n static struct tempfile *pidfile;\n static struct lock_file log_lock;\n@@ -167,9 +165,8 @@ static void gc_config(void)\n struct maintenance_run_opts;\n static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)\n {\n-\tconst char *argv[] = { \"pack-refs\", \"--all\", \"--prune\", NULL };\n-\n-\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n+\treturn run_command_l_opt(RUN_GIT_CMD, \"pack-refs\", \"--all\", \"--prune\",\n+\t\t\t\t NULL);\n }\n \n static int too_many_loose_objects(void)\n@@ -574,8 +571,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tstrvec_pushl(&reflog, \"reflog\", \"expire\", \"--all\", NULL);\n \tstrvec_pushl(&repack, \"repack\", \"-d\", \"-l\", NULL);\n \tstrvec_pushl(&prune, \"prune\", \"--expire\", NULL);\n-\tstrvec_pushl(&prune_worktrees, \"worktree\", \"prune\", \"--expire\", NULL);\n-\tstrvec_pushl(&rerere, \"rerere\", \"gc\", NULL);\n \n \t/* default expiry time, overwritten in gc_config */\n \tgc_config();\n@@ -688,14 +683,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tif (prune_worktrees_expire) {\n-\t\tstrvec_push(&prune_worktrees, prune_worktrees_expire);\n-\t\tif (run_command_v_opt(prune_worktrees.v, RUN_GIT_CMD))\n-\t\t\tdie(FAILED_RUN, prune_worktrees.v[0]);\n-\t}\n+\tif (prune_worktrees_expire &&\n+\t run_command_l_opt(RUN_GIT_CMD, \"worktree\", \"prune\", \"--expire\",\n+\t\t\t prune_worktrees_expire, NULL))\n+\t\tdie(FAILED_RUN, \"worktree\");\n \n-\tif (run_command_v_opt(rerere.v, RUN_GIT_CMD))\n-\t\tdie(FAILED_RUN, rerere.v[0]);\n+\tif (run_command_l_opt(RUN_GIT_CMD, \"rerere\", \"gc\", NULL))\n+\t\tdie(FAILED_RUN, \"rerere\");\n \n \treport_garbage = report_pack_garbage;\n \treprepare_packed_git(the_repository);\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 3bb49d805b4..9c04b588f68 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -347,55 +347,32 @@ static int save_state(struct object_id *stash)\n \n static void read_empty(const struct object_id *oid)\n {\n-\tint i = 0;\n-\tconst char *args[7];\n-\n-\targs[i++] = \"read-tree\";\n-\targs[i++] = \"-m\";\n-\targs[i++] = \"-u\";\n-\targs[i++] = empty_tree_oid_hex();\n-\targs[i++] = oid_to_hex(oid);\n-\targs[i] = NULL;\n-\n-\tif (run_command_v_opt(args, RUN_GIT_CMD))\n+\tif (run_command_l_opt(RUN_GIT_CMD, \"read-tree\", \"-m\", \"-u\",\n+\t\t\t empty_tree_oid_hex(), oid_to_hex(oid), NULL))\n \t\tdie(_(\"read-tree failed\"));\n }\n \n static void reset_hard(const struct object_id *oid)\n {\n-\tint i = 0;\n-\tconst char *args[6];\n-\n-\targs[i++] = \"read-tree\";\n-\targs[i++] = \"-v\";\n-\targs[i++] = \"--reset\";\n-\targs[i++] = \"-u\";\n-\targs[i++] = oid_to_hex(oid);\n-\targs[i] = NULL;\n-\n-\tif (run_command_v_opt(args, RUN_GIT_CMD))\n+\tif (run_command_l_opt(RUN_GIT_CMD, \"read-tree\", \"-v\", \"--reset\", \"-u\",\n+\t\t\t oid_to_hex(oid), NULL))\n \t\tdie(_(\"read-tree failed\"));\n }\n \n static void restore_state(const struct object_id *head,\n \t\t\t const struct object_id *stash)\n {\n-\tstruct strvec args = STRVEC_INIT;\n-\n \treset_hard(head);\n \n \tif (is_null_oid(stash))\n \t\tgoto refresh_cache;\n \n-\tstrvec_pushl(&args, \"stash\", \"apply\", \"--index\", \"--quiet\", NULL);\n-\tstrvec_push(&args, oid_to_hex(stash));\n-\n \t/*\n \t * It is OK to ignore error here, for example when there was\n \t * nothing to restore.\n \t */\n-\trun_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n+\trun_command_l_opt(RUN_GIT_CMD, \"stash\", \"apply\", \"--index\", \"--quiet\",\n+\t\t\t oid_to_hex(stash), NULL);\n \n refresh_cache:\n \tif (discard_cache() < 0 || read_cache() < 0)\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 901375d5841..4f5392c5796 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -196,17 +196,15 @@ static int read_yes_no_answer(void)\n static int ask_yes_no_if_possible(const char *format, ...)\n {\n \tchar question[4096];\n-\tconst char *retry_hook[] = { NULL, NULL, NULL };\n+\tchar *retry_hook;\n \tva_list args;\n \n \tva_start(args, format);\n \tvsnprintf(question, sizeof(question), format, args);\n \tva_end(args);\n \n-\tif ((retry_hook[0] = mingw_getenv(\"GIT_ASK_YESNO\"))) {\n-\t\tretry_hook[1] = question;\n-\t\treturn !run_command_v_opt(retry_hook, 0);\n-\t}\n+\tif ((retry_hook = mingw_getenv(\"GIT_ASK_YESNO\")))\n+\t\treturn !run_command_l_opt(0, retry_hook, question, NULL);\n \n \tif (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))\n \t\treturn 0;\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 8955d7e1f6e..740689b7bd6 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -193,7 +193,6 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tstruct strbuf cmd = STRBUF_INIT;\n \tstruct strbuf_expand_dict_entry dict[6];\n \tstruct strbuf path_sq = STRBUF_INIT;\n-\tconst char *args[] = { NULL, NULL };\n \tint status, fd, i;\n \tstruct stat st;\n \tenum ll_merge_result ret;\n@@ -219,8 +218,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \n \tstrbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);\n \n-\targs[0] = cmd.buf;\n-\tstatus = run_command_v_opt(args, RUN_USING_SHELL);\n+\tstatus = run_command_l_opt(RUN_USING_SHELL, cmd.buf, NULL);\n \tfd = open(temp[1], O_RDONLY);\n \tif (fd < 0)\n \t\tgoto bad;\ndiff --git a/run-command.c b/run-command.c\nindex 4e75f83b5e7..fa23fd0b803 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1016,6 +1016,21 @@ static void run_command_set_opts(struct child_process *cmd, int opt)\n \tcmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n }\n \n+int run_command_l_opt(int opt, ...)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tva_list ap;\n+\tconst char *arg;\n+\n+\tva_start(ap, opt);\n+\twhile ((arg = va_arg(ap, const char *)))\n+\t\tstrvec_push(&cmd.args, arg);\n+\tva_end(ap);\n+\n+\trun_command_set_opts(&cmd, opt);\n+\treturn run_command(&cmd);\n+}\n+\n static int run_command_v_opt_1(struct child_process *cmd, int opt)\n {\n \trun_command_set_opts(cmd, opt);\ndiff --git a/run-command.h b/run-command.h\nindex 0e85e5846a5..6320d70f062 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -151,8 +151,8 @@ struct child_process {\n \n /**\n * The functions: child_process_init, start_command, finish_command,\n- * run_command, run_command_v_opt, run_command_v_opt_cd_env, child_process_clear\n- * do the following:\n+ * run_command, run_command_l_opt, run_command_v_opt,\n+ * run_command_v_opt_cd_env, child_process_clear do the following:\n *\n * - If a system call failed, errno is set and -1 is returned. A diagnostic\n * is printed.\n@@ -254,6 +254,15 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const\n int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n \t\t\t\t const char *const *env, const char *tr2_class);\n \n+/**\n+ * The run_command_l_opt() function run_command_v_opt() takes a list\n+ * of strings terminated by a NULL. Use it instead of\n+ * run_command_v_opt() when possible, as it allows the compiler to\n+ * check that the \"argv\" is NULL-delimited.\n+ */\n+LAST_ARG_MUST_BE_NULL\n+int run_command_l_opt(int opt, ...);\n+\n /**\n * Execute the given command, sending \"in\" to its stdin, and capturing its\n * stdout and stderr in the \"out\" and \"err\" strbufs. Any of the three may\ndiff --git a/sequencer.c b/sequencer.c\nindex debb2ecbafe..20495db9de2 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3558,12 +3558,10 @@ static int error_failed_squash(struct repository *r,\n \n static int do_exec(struct repository *r, const char *command_line)\n {\n-\tconst char *child_argv[] = { NULL, NULL };\n \tint dirty, status;\n \n \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n-\tchild_argv[0] = command_line;\n-\tstatus = run_command_v_opt(child_argv, RUN_USING_SHELL);\n+\tstatus = run_command_l_opt(RUN_USING_SHELL, command_line, NULL);\n \n \t/* force re-reading of the cache */\n \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\ndiff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c\nindex 12beee99ad2..7967478a40a 100644\n--- a/t/helper/test-fake-ssh.c\n+++ b/t/helper/test-fake-ssh.c\n@@ -8,7 +8,6 @@ int cmd_main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tFILE *f;\n \tint i;\n-\tconst char *child_argv[] = { NULL, NULL };\n \n \t/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */\n \tif (!trash_directory)\n@@ -25,6 +24,5 @@ int cmd_main(int argc, const char **argv)\n \t/* Now, evaluate the *last* parameter */\n \tif (argc < 2)\n \t\treturn 0;\n-\tchild_argv[0] = argv[argc - 1];\n-\treturn run_command_v_opt(child_argv, RUN_USING_SHELL);\n+\treturn run_command_l_opt(RUN_USING_SHELL, argv[argc - 1], NULL);\n }\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465124","messageId":"patch-v2-05.10-4fca38bb4d6-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"[PATCH v2 05/10] run-command API docs: clarify & fleshen out run_command_v_opt*() docs","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:16Z","receivedAt":"2022-10-17T17:49:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a discussion of the flags that were missing to the\nrun_command_v_opt*() docs, and in doing so format them such that we\ncan easily add or remove flags from a table in the future, rather than\nhaving them tied up in prose.\n\nLet's also clarify why the user might want to use this API over\nrun_command() itself.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.h | 29 ++++++++++++++++++++++-------\n 1 file changed, 22 insertions(+), 7 deletions(-)\n\ndiff --git a/run-command.h b/run-command.h\nindex 6320d70f062..cf250e36426 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -234,13 +234,28 @@ int run_auto_maintenance(int quiet);\n #define RUN_CLOSE_OBJECT_STORE\t\t(1<<7)\n \n /**\n- * Convenience functions that encapsulate a sequence of\n- * start_command() followed by finish_command(). The argument argv\n- * specifies the program and its arguments. The argument opt is zero\n- * or more of the flags `RUN_COMMAND_NO_STDIN`, `RUN_GIT_CMD`,\n- * `RUN_COMMAND_STDOUT_TO_STDERR`, or `RUN_SILENT_EXEC_FAILURE`\n- * that correspond to the members .no_stdin, .git_cmd,\n- * .stdout_to_stderr, .silent_exec_failure of `struct child_process`.\n+ * The run_command_v_opt*() API is a convenience wrapper for an\n+ * underlying run_command().\n+ *\n+ * It's intended to be used when the user already has an \"argv\" they'd\n+ * like to use. As opposed to the \"struct child_process\"'s \"args\"\n+ * member, which will be strvec_clear()'d by calling run_command(),\n+ * the caller owns the \"argv\", which is not altered by invoking these\n+ * functions.\n+ *\n+ * The \"opt\" flags that will cause various underlying run_command()\n+ * members to be set. The flags and the corresponding struct members\n+ * are:\n+ *\n+ *\t- RUN_COMMAND_NO_STDIN: .no_stdin\n+ *\t- RUN_GIT_CMD: .git_cmd\n+ *\t- RUN_COMMAND_STDOUT_TO_STDERR: .stdout_to_stderr\n+ *\t- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure\n+ *\t- RUN_USING_SHELL: .use_shell\n+ *\t- RUN_CLEAN_ON_EXIT: .clean_on_exit\n+ *\t- RUN_WAIT_AFTER_CLEAN: .wait_after_clean\n+ *\t- RUN_CLOSE_OBJECT_STORE: .close_object_store\n+ *\n * The argument dir corresponds the member .dir. The argument env\n * corresponds to the member .env.\n */\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465126","messageId":"patch-v2-06.10-75eccc152ad-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"[PATCH v2 06/10] run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:17Z","receivedAt":"2022-10-17T17:49:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The run_command_v_opt*() API is meant to handle various common cases\nwhere we don't want the verbosity of calling the underlying\nrun_command(), but we're explicitly not trying to cover the full API\nsurface of run_command() itself.\n\nSo if we're not using some of these flags we probably won't need them\nagain, any future user who needs to set \"stdout_to_stderr\" can just\nuse run_command() itself, and the convenience API shouldn't be\ncluttered by trying to handle all the needs of various one-offs.\n\nSo let's remove the RUN_COMMAND_STDOUT_TO_STDERR flag, it hasn't been\nused since 860a2ebecd2 (receive-pack: send auto-gc output over\nsideband 2, 2016-06-05) when its last user started using the\nunderlying API directly.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n run-command.c | 1 -\n run-command.h | 12 +++++-------\n 2 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex fa23fd0b803..f9a0b13f62f 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1008,7 +1008,6 @@ static void run_command_set_opts(struct child_process *cmd, int opt)\n {\n \tcmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;\n \tcmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n-\tcmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n \tcmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n \tcmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n \tcmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\ndiff --git a/run-command.h b/run-command.h\nindex cf250e36426..0af01bd9b67 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -226,12 +226,11 @@ int run_auto_maintenance(int quiet);\n \n #define RUN_COMMAND_NO_STDIN\t\t(1<<0)\n #define RUN_GIT_CMD\t\t\t(1<<1)\n-#define RUN_COMMAND_STDOUT_TO_STDERR\t(1<<2)\n-#define RUN_SILENT_EXEC_FAILURE\t\t(1<<3)\n-#define RUN_USING_SHELL\t\t\t(1<<4)\n-#define RUN_CLEAN_ON_EXIT\t\t(1<<5)\n-#define RUN_WAIT_AFTER_CLEAN\t\t(1<<6)\n-#define RUN_CLOSE_OBJECT_STORE\t\t(1<<7)\n+#define RUN_SILENT_EXEC_FAILURE\t\t(1<<2)\n+#define RUN_USING_SHELL\t\t\t(1<<3)\n+#define RUN_CLEAN_ON_EXIT\t\t(1<<4)\n+#define RUN_WAIT_AFTER_CLEAN\t\t(1<<5)\n+#define RUN_CLOSE_OBJECT_STORE\t\t(1<<6)\n \n /**\n * The run_command_v_opt*() API is a convenience wrapper for an\n@@ -249,7 +248,6 @@ int run_auto_maintenance(int quiet);\n *\n *\t- RUN_COMMAND_NO_STDIN: .no_stdin\n *\t- RUN_GIT_CMD: .git_cmd\n- *\t- RUN_COMMAND_STDOUT_TO_STDERR: .stdout_to_stderr\n *\t- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure\n *\t- RUN_USING_SHELL: .use_shell\n *\t- RUN_CLEAN_ON_EXIT: .clean_on_exit\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465127","messageId":"patch-v2-07.10-3b3d3777232-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"[PATCH v2 07/10] run-command API & diff.c: remove run_command_v_opt_cd_env()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:18Z","receivedAt":"2022-10-17T17:50:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As we'll see in subsequent commits most of the run_command_v_opt_*()\nAPI users are a bad fit for what we'd like to do with that API. The\nideal use-case for it is something where we already have an \"argv\",\nand don't want the one-shot semantics of run_command().\n\nIn the case of run_command_v_opt_cd_env() there was only user of it,\nand that user was just accumulating complexity by not using\nrun_command() directly.\n\nBy not having to maintain its own \"argv\" and \"env\" it doesn't have to\nstrvec_clear() them both, which in the case of run_command() will be\ndone by child_process_clear() before it returns.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n diff.c | 26 ++++++++++++--------------\n run-command.c | 7 +------\n run-command.h | 3 +--\n tmp-objdir.h | 6 ++++--\n 4 files changed, 18 insertions(+), 24 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 648f6717a55..392530016fa 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4278,35 +4278,33 @@ static void run_external_diff(const char *pgm,\n \t\t\t const char *xfrm_msg,\n \t\t\t struct diff_options *o)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n-\tstruct strvec env = STRVEC_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n-\tstrvec_push(&argv, pgm);\n-\tstrvec_push(&argv, name);\n+\tstrvec_push(&cmd.args, pgm);\n+\tstrvec_push(&cmd.args, name);\n \n \tif (one && two) {\n-\t\tadd_external_diff_name(o->repo, &argv, name, one);\n+\t\tadd_external_diff_name(o->repo, &cmd.args, name, one);\n \t\tif (!other)\n-\t\t\tadd_external_diff_name(o->repo, &argv, name, two);\n+\t\t\tadd_external_diff_name(o->repo, &cmd.args, name, two);\n \t\telse {\n-\t\t\tadd_external_diff_name(o->repo, &argv, other, two);\n-\t\t\tstrvec_push(&argv, other);\n-\t\t\tstrvec_push(&argv, xfrm_msg);\n+\t\t\tadd_external_diff_name(o->repo, &cmd.args, other, two);\n+\t\t\tstrvec_push(&cmd.args, other);\n+\t\t\tstrvec_push(&cmd.args, xfrm_msg);\n \t\t}\n \t}\n \n-\tstrvec_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n-\tstrvec_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n+\tstrvec_pushf(&cmd.env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n+\tstrvec_pushf(&cmd.env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n \n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n-\tif (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))\n+\tcmd.use_shell = 1;\n+\tif (run_command(&cmd))\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n \n \tremove_tempfile();\n-\tstrvec_clear(&argv);\n-\tstrvec_clear(&env);\n }\n \n static int similarity_index(struct diff_filepair *p)\ndiff --git a/run-command.c b/run-command.c\nindex f9a0b13f62f..25a978fb027 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1038,7 +1038,7 @@ static int run_command_v_opt_1(struct child_process *cmd, int opt)\n \n int run_command_v_opt(const char **argv, int opt)\n {\n-\treturn run_command_v_opt_cd_env(argv, opt, NULL, NULL);\n+\treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);\n }\n \n int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)\n@@ -1046,11 +1046,6 @@ int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)\n \treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);\n }\n \n-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env)\n-{\n-\treturn run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);\n-}\n-\n int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n \t\t\t\t const char *const *env, const char *tr2_class)\n {\ndiff --git a/run-command.h b/run-command.h\nindex 0af01bd9b67..2574d46cb70 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -152,7 +152,7 @@ struct child_process {\n /**\n * The functions: child_process_init, start_command, finish_command,\n * run_command, run_command_l_opt, run_command_v_opt,\n- * run_command_v_opt_cd_env, child_process_clear do the following:\n+ * child_process_clear do the following:\n *\n * - If a system call failed, errno is set and -1 is returned. A diagnostic\n * is printed.\n@@ -263,7 +263,6 @@ int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);\n * env (the environment) is to be formatted like environ: \"VAR=VALUE\".\n * To unset an environment variable use just \"VAR\".\n */\n-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);\n int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n \t\t\t\t const char *const *env, const char *tr2_class);\n \ndiff --git a/tmp-objdir.h b/tmp-objdir.h\nindex 76efc7edee5..c96aa77396d 100644\n--- a/tmp-objdir.h\n+++ b/tmp-objdir.h\n@@ -10,9 +10,11 @@\n *\n * Example:\n *\n+ *\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+ *\t...\n *\tstruct tmp_objdir *t = tmp_objdir_create(\"incoming\");\n- *\tif (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&\n- *\t !tmp_objdir_migrate(t))\n+ * strvec_pushl(&cmd.env, tmp_objdir_env(t));\n+ *\tif (!run_command(&cmd) && !tmp_objdir_migrate(t))\n *\t\tprintf(\"success!\\n\");\n *\telse\n *\t\tdie(\"failed...tmp_objdir will clean up for us\");\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465128","messageId":"patch-v2-08.10-4f1a051823f-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"[PATCH v2 08/10] run-command API & users: remove run_command_v_opt_tr2()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:19Z","receivedAt":"2022-10-17T17:50:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in the preceding commit for run_command_v_opt_cd_env() that\nfunction's users could more easily use the underlying run_command()\ndirectly.\n\nIn the case of the \"git.c\" user that can be argued the other way,\ngiven the slight line count increase here, but part of that's because\nthe \"args\" in \"git.c\" was being leaked, which we now don't have to\nworry about.\n\nThat just leaves the spawn_daemon() user in \"fsmonitor-ipc.c\", it's\nlikewise slightly more verbose as a result, but by making it use\nrun_command() we can remove this entire part of the API. As noted in a\npreceding commit run_command_v_opt*() should be aimed at handling\nvarious common cases, not these one-offs.\n\nThe \"fsmonitor-ipc.c\" caller would be nicer with a hypothetical\nrun_command_l_opt_tr2(), but let's not maintain such a thing only for\nit, as it would be its only user.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n fsmonitor-ipc.c | 10 +++++++---\n git.c | 15 +++++++++------\n run-command.c | 26 +++-----------------------\n run-command.h | 19 +++----------------\n 4 files changed, 22 insertions(+), 48 deletions(-)\n\ndiff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c\nindex 789e7397baa..7251f48e456 100644\n--- a/fsmonitor-ipc.c\n+++ b/fsmonitor-ipc.c\n@@ -56,10 +56,14 @@ enum ipc_active_state fsmonitor_ipc__get_state(void)\n \n static int spawn_daemon(void)\n {\n-\tconst char *args[] = { \"fsmonitor--daemon\", \"start\", NULL };\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n-\treturn run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,\n-\t\t\t\t \"fsmonitor\");\n+\tcmd.git_cmd = 1;\n+\tcmd.no_stdin = 1;\n+\tcmd.trace2_child_class = \"fsmonitor\";\n+\tstrvec_pushl(&cmd.args, \"fsmonitor--daemon\", \"start\", NULL);\n+\n+\treturn run_command(&cmd);\n }\n \n int fsmonitor_ipc__send_query(const char *since_token,\ndiff --git a/git.c b/git.c\nindex da411c53822..ccf444417b5 100644\n--- a/git.c\n+++ b/git.c\n@@ -787,7 +787,7 @@ static int run_argv(int *argcp, const char ***argv)\n \t\tif (!done_alias)\n \t\t\thandle_builtin(*argcp, *argv);\n \t\telse if (get_builtin(**argv)) {\n-\t\t\tstruct strvec args = STRVEC_INIT;\n+\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \t\t\tint i;\n \n \t\t\t/*\n@@ -804,18 +804,21 @@ static int run_argv(int *argcp, const char ***argv)\n \n \t\t\tcommit_pager_choice();\n \n-\t\t\tstrvec_push(&args, \"git\");\n+\t\t\tstrvec_push(&cmd.args, \"git\");\n \t\t\tfor (i = 0; i < *argcp; i++)\n-\t\t\t\tstrvec_push(&args, (*argv)[i]);\n+\t\t\t\tstrvec_push(&cmd.args, (*argv)[i]);\n \n-\t\t\ttrace_argv_printf(args.v, \"trace: exec:\");\n+\t\t\ttrace_argv_printf(cmd.args.v, \"trace: exec:\");\n \n \t\t\t/*\n \t\t\t * if we fail because the command is not found, it is\n \t\t\t * OK to return. Otherwise, we just pass along the status code.\n \t\t\t */\n-\t\t\ti = run_command_v_opt_tr2(args.v, RUN_SILENT_EXEC_FAILURE |\n-\t\t\t\t\t\t RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, \"git_alias\");\n+\t\t\tcmd.silent_exec_failure = 1;\n+\t\t\tcmd.clean_on_exit = 1;\n+\t\t\tcmd.wait_after_clean = 1;\n+\t\t\tcmd.trace2_child_class = \"git_alias\";\n+\t\t\ti = run_command(&cmd);\n \t\t\tif (i >= 0 || errno != ENOENT)\n \t\t\t\texit(i);\n \t\t\tdie(\"could not execute builtin %s\", **argv);\ndiff --git a/run-command.c b/run-command.c\nindex 25a978fb027..0066ace85fa 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1010,7 +1010,6 @@ static void run_command_set_opts(struct child_process *cmd, int opt)\n \tcmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n \tcmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n \tcmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n-\tcmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n \tcmd->wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n \tcmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n }\n@@ -1030,32 +1029,13 @@ int run_command_l_opt(int opt, ...)\n \treturn run_command(&cmd);\n }\n \n-static int run_command_v_opt_1(struct child_process *cmd, int opt)\n-{\n-\trun_command_set_opts(cmd, opt);\n-\treturn run_command(cmd);\n-}\n-\n int run_command_v_opt(const char **argv, int opt)\n-{\n-\treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);\n-}\n-\n-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)\n-{\n-\treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);\n-}\n-\n-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n-\t\t\t\t const char *const *env, const char *tr2_class)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n \tstrvec_pushv(&cmd.args, argv);\n-\tcmd.dir = dir;\n-\tif (env)\n-\t\tstrvec_pushv(&cmd.env, (const char **)env);\n-\tcmd.trace2_child_class = tr2_class;\n-\treturn run_command_v_opt_1(&cmd, opt);\n+\trun_command_set_opts(&cmd, opt);\n+\treturn run_command(&cmd);\n }\n \n #ifndef NO_PTHREADS\ndiff --git a/run-command.h b/run-command.h\nindex 2574d46cb70..2b1fe3cde5c 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -228,12 +228,11 @@ int run_auto_maintenance(int quiet);\n #define RUN_GIT_CMD\t\t\t(1<<1)\n #define RUN_SILENT_EXEC_FAILURE\t\t(1<<2)\n #define RUN_USING_SHELL\t\t\t(1<<3)\n-#define RUN_CLEAN_ON_EXIT\t\t(1<<4)\n-#define RUN_WAIT_AFTER_CLEAN\t\t(1<<5)\n-#define RUN_CLOSE_OBJECT_STORE\t\t(1<<6)\n+#define RUN_WAIT_AFTER_CLEAN\t\t(1<<4)\n+#define RUN_CLOSE_OBJECT_STORE\t\t(1<<5)\n \n /**\n- * The run_command_v_opt*() API is a convenience wrapper for an\n+ * The run_command_v_opt() function is a convenience wrapper for an\n * underlying run_command().\n *\n * It's intended to be used when the user already has an \"argv\" they'd\n@@ -250,21 +249,9 @@ int run_auto_maintenance(int quiet);\n *\t- RUN_GIT_CMD: .git_cmd\n *\t- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure\n *\t- RUN_USING_SHELL: .use_shell\n- *\t- RUN_CLEAN_ON_EXIT: .clean_on_exit\n- *\t- RUN_WAIT_AFTER_CLEAN: .wait_after_clean\n *\t- RUN_CLOSE_OBJECT_STORE: .close_object_store\n- *\n- * The argument dir corresponds the member .dir. The argument env\n- * corresponds to the member .env.\n */\n int run_command_v_opt(const char **argv, int opt);\n-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);\n-/*\n- * env (the environment) is to be formatted like environ: \"VAR=VALUE\".\n- * To unset an environment variable use just \"VAR\".\n- */\n-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n-\t\t\t\t const char *const *env, const char *tr2_class);\n \n /**\n * The run_command_l_opt() function run_command_v_opt() takes a list\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465129","messageId":"patch-v2-10.10-138af632a36-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"[PATCH v2 10/10] run-command API: add and use a run_command_sv_opt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:21Z","receivedAt":"2022-10-17T17:50:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add a run_command_sv_opt() convenience wrapper for\nrun_command_v_opt(), as noted in the API documentation this is for the\ncommon case of wanting to construct a \"struct strvec\" to pass to\nrun_command_v_opt(), and as it's a one-shot to strvec_clear() it\nafterwards.\n\nLet's convert those API users that were using a \"ret\" variable to\ncarry over to \"return\" after a \"strvec_clear()\" to use this new\nfunction instead.\n\nBecause we pass the \"struct strvec *\" to the function we can also\navoid copying the arguments to the \"args\" member of the \"struct\nchild_process\", as we were doing with run_command_v_opt().\n\nInstead we can use memcpy() and strvec_clear() to do the moral\nequivalent of a strbuf_{detach,attach}(). The strvec API doesn't have\na strvec_attach(), we could add it here while at it, but let's avoid\ngeneralizing the interface for now and migrate the \"struct strvec *\"\nin the \"run_command_sv_opt()\" instead.\n\nLet's leave aside the user in \"builtin/bisect--helper.c\"'s\nbisect_visualize(). There's an outstanding topic that's extensively\nmodifying it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n add-interactive.c | 3 +--\n builtin/add.c | 6 ++----\n builtin/clone.c | 3 +--\n builtin/gc.c | 5 +----\n builtin/pull.c | 15 +++------------\n builtin/remote.c | 5 +----\n merge.c | 3 +--\n run-command.c | 11 +++++++++++\n run-command.h | 16 +++++++++++++++-\n scalar.c | 6 +-----\n sequencer.c | 11 ++---------\n 11 files changed, 39 insertions(+), 45 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex f071b2a1b4f..9c86f3b9532 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -1007,8 +1007,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,\n \t\t\tif (files->selected[i])\n \t\t\t\tstrvec_push(&args,\n \t\t\t\t\t files->items.items[i].string);\n-\t\tres = run_command_v_opt(args.v, 0);\n-\t\tstrvec_clear(&args);\n+\t\tres = run_command_sv_opt(&args, 0);\n \t}\n \n \tputchar('\\n');\ndiff --git a/builtin/add.c b/builtin/add.c\nindex f84372964c8..7c783eebc0e 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -240,7 +240,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)\n int run_add_interactive(const char *revision, const char *patch_mode,\n \t\t\tconst struct pathspec *pathspec)\n {\n-\tint status, i;\n+\tint i;\n \tstruct strvec argv = STRVEC_INIT;\n \tint use_builtin_add_i =\n \t\tgit_env_bool(\"GIT_TEST_ADD_I_USE_BUILTIN\", -1);\n@@ -282,9 +282,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,\n \t\t/* pass original pathspec, to be re-parsed */\n \t\tstrvec_push(&argv, pathspec->items[i].original);\n \n-\tstatus = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&argv);\n-\treturn status;\n+\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n }\n \n int interactive_add(const char **argv, const char *prefix, int patch)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex a1514363190..8e43781e147 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -756,8 +756,7 @@ static int checkout(int submodule_progress, int filter_submodules)\n \t\t\t\t\t \"--single-branch\" :\n \t\t\t\t\t \"--no-single-branch\");\n \n-\t\terr = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\t\tstrvec_clear(&args);\n+\t\treturn run_command_sv_opt(&args, RUN_GIT_CMD);\n \t}\n \n \treturn err;\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 519e64e86ee..8393e19b504 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1899,7 +1899,6 @@ static int is_schtasks_available(void)\n static int schtasks_remove_task(enum schedule_priority schedule)\n {\n \tconst char *cmd = \"schtasks\";\n-\tint result;\n \tstruct strvec args = STRVEC_INIT;\n \tconst char *frequency = get_frequency(schedule);\n \n@@ -1909,9 +1908,7 @@ static int schtasks_remove_task(enum schedule_priority schedule)\n \tstrvec_pushf(&args, SCHTASKS_NAME_FMT, frequency);\n \tstrvec_pushl(&args, \"/f\", NULL);\n \n-\tresult = run_command_v_opt(args.v, 0);\n-\tstrvec_clear(&args);\n-\treturn result;\n+\treturn run_command_sv_opt(&args, 0);\n }\n \n static int schtasks_remove_tasks(void)\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 403a24d7ca6..2f36823c97e 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -516,7 +516,6 @@ static void parse_repo_refspecs(int argc, const char **argv, const char **repo,\n static int run_fetch(const char *repo, const char **refspecs)\n {\n \tstruct strvec args = STRVEC_INIT;\n-\tint ret;\n \n \tstrvec_pushl(&args, \"fetch\", \"--update-head-ok\", NULL);\n \n@@ -582,9 +581,7 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\tstrvec_pushv(&args, refspecs);\n \t} else if (*refspecs)\n \t\tBUG(\"refspecs without repo?\");\n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);\n-\tstrvec_clear(&args);\n-\treturn ret;\n+\treturn run_command_sv_opt(&args, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);\n }\n \n /**\n@@ -653,7 +650,6 @@ static int update_submodules(void)\n */\n static int run_merge(void)\n {\n-\tint ret;\n \tstruct strvec args = STRVEC_INIT;\n \n \tstrvec_pushl(&args, \"merge\", NULL);\n@@ -696,9 +692,7 @@ static int run_merge(void)\n \t\tstrvec_push(&args, \"--allow-unrelated-histories\");\n \n \tstrvec_push(&args, \"FETCH_HEAD\");\n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n-\treturn ret;\n+\treturn run_command_sv_opt(&args, RUN_GIT_CMD);\n }\n \n /**\n@@ -879,7 +873,6 @@ static int get_rebase_newbase_and_upstream(struct object_id *newbase,\n static int run_rebase(const struct object_id *newbase,\n \t\tconst struct object_id *upstream)\n {\n-\tint ret;\n \tstruct strvec args = STRVEC_INIT;\n \n \tstrvec_push(&args, \"rebase\");\n@@ -913,9 +906,7 @@ static int run_rebase(const struct object_id *newbase,\n \n \tstrvec_push(&args, oid_to_hex(upstream));\n \n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n-\treturn ret;\n+\treturn run_command_sv_opt(&args, RUN_GIT_CMD);\n }\n \n static int get_can_ff(struct object_id *orig_head,\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 910f7b9316a..5d3534db19c 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1510,7 +1510,6 @@ static int update(int argc, const char **argv, const char *prefix)\n \t};\n \tstruct strvec fetch_argv = STRVEC_INIT;\n \tint default_defined = 0;\n-\tint retval;\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_remote_update_usage,\n@@ -1536,9 +1535,7 @@ static int update(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tretval = run_command_v_opt(fetch_argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&fetch_argv);\n-\treturn retval;\n+\treturn run_command_sv_opt(&fetch_argv, RUN_GIT_CMD);\n }\n \n static int remove_all_fetch_refspecs(const char *key)\ndiff --git a/merge.c b/merge.c\nindex 2382ff66d35..487debacecb 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -33,8 +33,7 @@ int try_merge_command(struct repository *r,\n \tfor (j = remotes; j; j = j->next)\n \t\tstrvec_push(&args, merge_argument(j->item));\n \n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n+\tret = run_command_sv_opt(&args, RUN_GIT_CMD);\n \n \tdiscard_index(r->index);\n \tif (repo_read_index(r) < 0)\ndiff --git a/run-command.c b/run-command.c\nindex 0066ace85fa..724fc581c89 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1038,6 +1038,17 @@ int run_command_v_opt(const char **argv, int opt)\n \treturn run_command(&cmd);\n }\n \n+int run_command_sv_opt(struct strvec *args, int opt)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\t/* TODO: We could encapsulate this with a strvec_attach() */\n+\tmemcpy(&cmd.args, args, sizeof(*args));\n+\tstrvec_init(args);\n+\trun_command_set_opts(&cmd, opt);\n+\treturn run_command(&cmd);\n+}\n+\n #ifndef NO_PTHREADS\n static pthread_t main_thread;\n static int main_thread_set;\ndiff --git a/run-command.h b/run-command.h\nindex 2b1fe3cde5c..8e8403f3bb3 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -151,7 +151,7 @@ struct child_process {\n \n /**\n * The functions: child_process_init, start_command, finish_command,\n- * run_command, run_command_l_opt, run_command_v_opt,\n+ * run_command, run_command_l_opt, run_command_v_opt, run_command_sv_opt,\n * child_process_clear do the following:\n *\n * - If a system call failed, errno is set and -1 is returned. A diagnostic\n@@ -262,6 +262,20 @@ int run_command_v_opt(const char **argv, int opt);\n LAST_ARG_MUST_BE_NULL\n int run_command_l_opt(int opt, ...);\n \n+/**\n+ * The run_command_sv_opt() function is a wrapper for\n+ * run_command_v_opt(). It takes a \"struct strvec *args\" which\n+ * similarly will be strvec_clear()'d before returning.\n+ *\n+ * Use it for the common case of constructing a \"struct strvec\" for a\n+ * one-shot run_command_v_opt() invocation.\n+ *\n+ * The \"args\" will migrated the \"cmd.args\" member of an underlying\n+ * \"struct child_process\", in a way that avoids making an extra copy.\n+ */\n+RESULT_MUST_BE_USED\n+int run_command_sv_opt(struct strvec *args, int opt);\n+\n /**\n * Execute the given command, sending \"in\" to its stdin, and capturing its\n * stdout and stderr in the \"out\" and \"err\" strbufs. Any of the three may\ndiff --git a/scalar.c b/scalar.c\nindex 6de9c0ee523..3480bf73cbd 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -72,7 +72,6 @@ static int run_git(const char *arg, ...)\n \tstruct strvec argv = STRVEC_INIT;\n \tva_list args;\n \tconst char *p;\n-\tint res;\n \n \tva_start(args, arg);\n \tstrvec_push(&argv, arg);\n@@ -80,10 +79,7 @@ static int run_git(const char *arg, ...)\n \t\tstrvec_push(&argv, p);\n \tva_end(args);\n \n-\tres = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\n-\tstrvec_clear(&argv);\n-\treturn res;\n+\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n }\n \n struct scalar_config {\ndiff --git a/sequencer.c b/sequencer.c\nindex 20495db9de2..7ee0e05512c 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3183,7 +3183,6 @@ static int rollback_is_safe(void)\n \n static int reset_merge(const struct object_id *oid)\n {\n-\tint ret;\n \tstruct strvec argv = STRVEC_INIT;\n \n \tstrvec_pushl(&argv, \"reset\", \"--merge\", NULL);\n@@ -3191,10 +3190,7 @@ static int reset_merge(const struct object_id *oid)\n \tif (!is_null_oid(oid))\n \t\tstrvec_push(&argv, oid_to_hex(oid));\n \n-\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&argv);\n-\n-\treturn ret;\n+\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n }\n \n static int rollback_single_pick(struct repository *r)\n@@ -4866,7 +4862,6 @@ static int pick_commits(struct repository *r,\n static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n {\n \tstruct strvec argv = STRVEC_INIT;\n-\tint ret;\n \n \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n \t !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n@@ -4887,9 +4882,7 @@ static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n \t\t */\n \t\tstrvec_pushl(&argv, \"--no-edit\", \"--cleanup=strip\", NULL);\n \n-\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&argv);\n-\treturn ret;\n+\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n }\n \n static int commit_staged_changes(struct repository *r,\n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465130","messageId":"patch-v2-09.10-99c5688797a-20221017T170316Z-avarab@gmail.com","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"[PATCH v2 09/10] gc: use strvec_pushf(), avoid redundant strbuf_detach()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-17T17:49:20Z","receivedAt":"2022-10-17T17:50:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Adjust code added in 3797a0a7b7a (maintenance: use Windows scheduled\ntasks, 2021-01-05) to use strvec_pushf() directly. Rather than having\na function that returns a strbuf_detach() we need to free and have the\n\"strvec_pushl()\" do its own \"xstrdup()\" we can format this in-place.\n\nBy changing this we only have the \"strvec_clear()\" between the\n\"run_command_v_opt()\" and the \"return result\" in\n\"schtasks_remove_task()\". In the subsequent commit we'll start using a\nhelper which'll allow us to skip the \"strvec_clear()\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/gc.c | 22 ++++++++--------------\n 1 file changed, 8 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 075b4637660..519e64e86ee 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -1894,12 +1894,7 @@ static int is_schtasks_available(void)\n #endif\n }\n \n-static char *schtasks_task_name(const char *frequency)\n-{\n-\tstruct strbuf label = STRBUF_INIT;\n-\tstrbuf_addf(&label, \"Git Maintenance (%s)\", frequency);\n-\treturn strbuf_detach(&label, NULL);\n-}\n+#define SCHTASKS_NAME_FMT \"Git Maintenance (%s)\"\n \n static int schtasks_remove_task(enum schedule_priority schedule)\n {\n@@ -1907,16 +1902,15 @@ static int schtasks_remove_task(enum schedule_priority schedule)\n \tint result;\n \tstruct strvec args = STRVEC_INIT;\n \tconst char *frequency = get_frequency(schedule);\n-\tchar *name = schtasks_task_name(frequency);\n \n \tget_schedule_cmd(&cmd, NULL);\n \tstrvec_split(&args, cmd);\n-\tstrvec_pushl(&args, \"/delete\", \"/tn\", name, \"/f\", NULL);\n+\tstrvec_pushl(&args, \"/delete\", \"/tn\", NULL);\n+\tstrvec_pushf(&args, SCHTASKS_NAME_FMT, frequency);\n+\tstrvec_pushl(&args, \"/f\", NULL);\n \n \tresult = run_command_v_opt(args.v, 0);\n-\n \tstrvec_clear(&args);\n-\tfree(name);\n \treturn result;\n }\n \n@@ -1935,7 +1929,6 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tconst char *xml;\n \tstruct tempfile *tfile;\n \tconst char *frequency = get_frequency(schedule);\n-\tchar *name = schtasks_task_name(frequency);\n \tstruct strbuf tfilename = STRBUF_INIT;\n \n \tget_schedule_cmd(&cmd, NULL);\n@@ -2028,8 +2021,10 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \t \"\\n\";\n \tfprintf(tfile->fp, xml, exec_path, exec_path, frequency);\n \tstrvec_split(&child.args, cmd);\n-\tstrvec_pushl(&child.args, \"/create\", \"/tn\", name, \"/f\", \"/xml\",\n-\t\t\t\t get_tempfile_path(tfile), NULL);\n+\tstrvec_pushl(&child.args, \"/create\", \"/tn\", NULL);\n+\tstrvec_pushf(&child.args, SCHTASKS_NAME_FMT, frequency);\n+\tstrvec_pushl(&child.args, \"/f\", \"/xml\",\n+\t\t get_tempfile_path(tfile), NULL);\n \tclose_tempfile_gently(tfile);\n \n \tchild.no_stdout = 1;\n@@ -2040,7 +2035,6 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tresult = finish_command(&child);\n \n \tdelete_tempfile(&tfile);\n-\tfree(name);\n \treturn result;\n }\n \n-- \n2.38.0.1091.gf9d18265e59\n\n"},{"id":"465178","messageId":"xmqq4jw1wl6z.fsf@gitster.g","threadId":"58626","inReplyTo":"cover-v2-00.10-00000000000-20221017T170316Z-avarab@gmail.com","subject":"Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-18T09:11:16Z","receivedAt":"2022-10-18T09:11:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> This series provides a more idiomatic set of run-command API helpers\n> to match our current use-cases for run_command_v_opt(). See v1[1] for\n> a more general overview.\n\nHmph... I somehow thought that the concensus is rather try the\ncomplete opposite approach shown by René's patch to lose the use of\nrun_command_v_opt() by replacing it with run_command(&args), with\nargs.v populated using strvec_pushl() and other strvec API\nfunctions.\n\nOne of the reasons I would prefer to see us moving in that direction\nwas because the first iteration of this series was a good\ndemonstration of the relatively limited usefulness of _l_opt()\nvariant and also it seemed to be error prone to use it.\n"},{"id":"465193","messageId":"221018.86a65ti70m.gmgdl@evledraar.gmail.com","threadId":"58626","inReplyTo":"xmqq4jw1wl6z.fsf@gitster.g","subject":"Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-18T13:28:43Z","receivedAt":"2022-10-18T13:41:30Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 18 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> This series provides a more idiomatic set of run-command API helpers\n>> to match our current use-cases for run_command_v_opt(). See v1[1] for\n>> a more general overview.\n>\n> Hmph... I somehow thought that the concensus is rather try the\n> complete opposite approach shown by René's patch to lose the use of\n> run_command_v_opt() by replacing it with run_command(&args), with\n> args.v populated using strvec_pushl() and other strvec API\n> functions.\n>\n> One of the reasons I would prefer to see us moving in that direction\n> was because the first iteration of this series was a good\n> demonstration of the relatively limited usefulness of _l_opt()\n> variant and also it seemed to be error prone to use it.\n\nI'm getting somewhat mixed messages. Jeff seemed to like the end-to-end\nsafety of run_command_l_opt() before I wrote it. I think the\nrun_command_l_opt() still really shines for the simple cases.\n\nI don't see how *_l_opt() is particularly error prone, I just had a\nstupid think-o in v1 of this, but that if/else if bug is something that\ncould have snuck in with run_command() given the same stupidity :)\n\nI checked out René's [1] and diff'd it with mine, excluding various\nparts that inflated the diff for no good explanatory purpose (e.g. the\nAPI itself, or where I omitted some conversions).\n\nI think the diffstat is a good argument for *some* version of my series,\nbut as e.g. the first hunk shows we could also just convert\ne.g. run_diff() to run_command() instead of run_command_sv_opt().\n\nI wonder if a run_command() that just had the prototype (struct\nchild_process *cmd, ...) might not be the best of both worlds (or a\nrun_commandl() alternative). I.e. to do away with the whole custom way\nof specifying the flag(s), and just take the passed-in arguments and\nappend them to \"&cmd.args\".\n\nThat would give us run_command_l_opt() behavior, which is handy in some\ncases, and gives us compile-time checks. We could BUG() out if\n\"cmd.args.nr > 0\" if we use it, to ensure we use one or the other\n(although a combination would be handy in some cases, so maybe not).\n\nWhat do you all think?\n\nIt's also interesting to consider adding a --noop option to be supported\nby parse-options.c. The reason we can't use a run_command_l_opt() in\nsome cases is because we're doing e.g.:\n\n\tif (progress)\n\t\tstrvec_push(&args, \"--progress\");\n\nWe have a --no-progress, but in those cases the recipient at the end\noften cares about a default of -1 for a bool variable, or similar. So if\nwe could do:\n\n\trun_command_l_opt(0, command,\n\t\t(progress ? \"--progress\" : \"--noop\"),\n\t\t...,\n\t\tNULL\n\t);\n\nWe could benefit from compile-time checks, and wouldn't have to allocate\na strvec just for building up the arguments in some cases. Just food for\nthought...\n\n1. https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/\n\n add-interactive.c | 8 +--\n builtin/add.c | 15 +++---\n builtin/am.c | 12 +++--\n builtin/clone.c | 44 +++++++++------\n builtin/difftool.c | 24 +++++----\n builtin/fetch.c | 9 ++--\n builtin/gc.c | 74 ++++++++++++++++---------\n builtin/merge-index.c | 4 +-\n builtin/pull.c | 138 ++++++++++++++++++++++++-----------------------\n builtin/remote.c | 37 +++++++------\n compat/mingw.c | 11 ++--\n diff.c | 5 +-\n fsmonitor-ipc.c | 4 +-\n ll-merge.c | 5 +-\n merge.c | 17 +++---\n scalar.c | 9 ++--\n sequencer.c | 23 ++++----\n t/helper/test-fake-ssh.c | 5 +-\n t/helper/test-trace2.c | 4 +-\n tmp-objdir.h | 6 +--\n 20 files changed, 265 insertions(+), 189 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex 9c86f3b9532..ecc5ae1b249 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -997,17 +997,17 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,\n \tcount = list_and_choose(s, files, opts);\n \topts->flags = 0;\n \tif (count > 0) {\n-\t\tstruct strvec args = STRVEC_INIT;\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n-\t\tstrvec_pushl(&args, \"git\", \"diff\", \"-p\", \"--cached\",\n+\t\tstrvec_pushl(&cmd.args, \"git\", \"diff\", \"-p\", \"--cached\",\n \t\t\t oid_to_hex(!is_initial ? &oid :\n \t\t\t\t\ts->r->hash_algo->empty_tree),\n \t\t\t \"--\", NULL);\n \t\tfor (i = 0; i < files->items.nr; i++)\n \t\t\tif (files->selected[i])\n-\t\t\t\tstrvec_push(&args,\n+\t\t\t\tstrvec_push(&cmd.args,\n \t\t\t\t\t files->items.items[i].string);\n-\t\tres = run_command_sv_opt(&args, 0);\n+\t\tres = run_command(&cmd);\n \t}\n \n \tputchar('\\n');\ndiff --git a/builtin/add.c b/builtin/add.c\nindex 7c783eebc0e..626c71ec6aa 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -241,7 +241,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,\n \t\t\tconst struct pathspec *pathspec)\n {\n \tint i;\n-\tstruct strvec argv = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint use_builtin_add_i =\n \t\tgit_env_bool(\"GIT_TEST_ADD_I_USE_BUILTIN\", -1);\n \n@@ -272,17 +272,18 @@ int run_add_interactive(const char *revision, const char *patch_mode,\n \t\treturn !!run_add_p(the_repository, mode, revision, pathspec);\n \t}\n \n-\tstrvec_push(&argv, \"add--interactive\");\n+\tstrvec_push(&cmd.args, \"add--interactive\");\n \tif (patch_mode)\n-\t\tstrvec_push(&argv, patch_mode);\n+\t\tstrvec_push(&cmd.args, patch_mode);\n \tif (revision)\n-\t\tstrvec_push(&argv, revision);\n-\tstrvec_push(&argv, \"--\");\n+\t\tstrvec_push(&cmd.args, revision);\n+\tstrvec_push(&cmd.args, \"--\");\n \tfor (i = 0; i < pathspec->nr; i++)\n \t\t/* pass original pathspec, to be re-parsed */\n-\t\tstrvec_push(&argv, pathspec->items[i].original);\n+\t\tstrvec_push(&cmd.args, pathspec->items[i].original);\n \n-\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n \n int interactive_add(const char **argv, const char *prefix, int patch)\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1d298a91306..20aea0d2487 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2186,10 +2186,14 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)\n \tconst char *patch_path;\n \tint len;\n \n-\tif (!is_null_oid(&state->orig_commit))\n-\t\treturn run_command_l_opt(RUN_GIT_CMD, \"show\",\n-\t\t\t\t\t oid_to_hex(&state->orig_commit),\n-\t\t\t\t\t \"--\", NULL);\n+\tif (!is_null_oid(&state->orig_commit)) {\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\t\tstrvec_pushl(&cmd.args, \"show\", oid_to_hex(&state->orig_commit),\n+\t\t\t \"--\", NULL);\n+\t\tcmd.git_cmd = 1;\n+\t\treturn run_command(&cmd);\n+\t}\n \n \tswitch (sub_mode) {\n \tcase SHOW_PATCH_RAW:\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 8e43781e147..219cfbd7355 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -651,19 +651,23 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \n static int git_sparse_checkout_init(const char *repo)\n {\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tint result = 0;\n+\tstrvec_pushl(&cmd.args, \"-C\", repo, \"sparse-checkout\", \"set\", NULL);\n+\n \t/*\n \t * We must apply the setting in the current process\n \t * for the later checkout to use the sparse-checkout file.\n \t */\n \tcore_apply_sparse_checkout = 1;\n \n-\tif (run_command_l_opt(RUN_GIT_CMD, \"-C\", repo, \"sparse-checkout\",\n-\t\t\t \"set\", NULL)) {\n+\tcmd.git_cmd = 1;\n+\tif (run_command(&cmd)) {\n \t\terror(_(\"failed to initialize sparse-checkout\"));\n-\t\treturn 1;\n+\t\tresult = 1;\n \t}\n \n-\treturn 0;\n+\treturn result;\n }\n \n static int checkout(int submodule_progress, int filter_submodules)\n@@ -727,36 +731,38 @@ static int checkout(int submodule_progress, int filter_submodules)\n \t\t\t oid_to_hex(&oid), \"1\", NULL);\n \n \tif (!err && (option_recurse_submodules.nr > 0)) {\n-\t\tstruct strvec args = STRVEC_INIT;\n-\t\tstrvec_pushl(&args, \"submodule\", \"update\", \"--require-init\", \"--recursive\", NULL);\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\t\tstrvec_pushl(&cmd.args, \"submodule\", \"update\", \"--require-init\",\n+\t\t\t \"--recursive\", NULL);\n \n \t\tif (option_shallow_submodules == 1)\n-\t\t\tstrvec_push(&args, \"--depth=1\");\n+\t\t\tstrvec_push(&cmd.args, \"--depth=1\");\n \n \t\tif (max_jobs != -1)\n-\t\t\tstrvec_pushf(&args, \"--jobs=%d\", max_jobs);\n+\t\t\tstrvec_pushf(&cmd.args, \"--jobs=%d\", max_jobs);\n \n \t\tif (submodule_progress)\n-\t\t\tstrvec_push(&args, \"--progress\");\n+\t\t\tstrvec_push(&cmd.args, \"--progress\");\n \n \t\tif (option_verbosity < 0)\n-\t\t\tstrvec_push(&args, \"--quiet\");\n+\t\t\tstrvec_push(&cmd.args, \"--quiet\");\n \n \t\tif (option_remote_submodules) {\n-\t\t\tstrvec_push(&args, \"--remote\");\n-\t\t\tstrvec_push(&args, \"--no-fetch\");\n+\t\t\tstrvec_push(&cmd.args, \"--remote\");\n+\t\t\tstrvec_push(&cmd.args, \"--no-fetch\");\n \t\t}\n \n \t\tif (filter_submodules && filter_options.choice)\n-\t\t\tstrvec_pushf(&args, \"--filter=%s\",\n+\t\t\tstrvec_pushf(&cmd.args, \"--filter=%s\",\n \t\t\t\t expand_list_objects_filter_spec(&filter_options));\n \n \t\tif (option_single_branch >= 0)\n-\t\t\tstrvec_push(&args, option_single_branch ?\n+\t\t\tstrvec_push(&cmd.args, option_single_branch ?\n \t\t\t\t\t \"--single-branch\" :\n \t\t\t\t\t \"--no-single-branch\");\n \n-\t\treturn run_command_sv_opt(&args, RUN_GIT_CMD);\n+\t\tcmd.git_cmd = 1;\n+\t\terr = run_command(&cmd);\n \t}\n \n \treturn err;\n@@ -860,8 +866,12 @@ static void dissociate_from_references(void)\n \tchar *alternates = git_pathdup(\"objects/info/alternates\");\n \n \tif (!access(alternates, F_OK)) {\n-\t\tif (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,\n-\t\t\t\t \"repack\", \"-a\", \"-d\", NULL))\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\t\tstrvec_pushl(&cmd.args, \"repack\", \"-a\", \"-d\", NULL);\n+\t\tcmd.git_cmd = 1;\n+\t\tcmd.no_stdin = 1;\n+\t\tif (run_command(&cmd))\n \t\t\tdie(_(\"cannot repack to clean up\"));\n \t\tif (unlink(alternates) && errno != ENOENT)\n \t\t\tdie_errno(_(\"cannot unlink temporary alternates file\"));\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex ed211a87322..d6c262e15ff 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -44,8 +44,10 @@ static int difftool_config(const char *var, const char *value, void *cb)\n \n static int print_tool_help(void)\n {\n-\treturn run_command_l_opt(RUN_GIT_CMD, \"mergetool\", \"--tool-help=diff\",\n-\t\t\t\t NULL);\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstrvec_pushl(&cmd.args, \"mergetool\", \"--tool-help=diff\", NULL);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n \n static int parse_index_info(char *p, int *mode1, int *mode2,\n@@ -360,8 +362,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct pair_entry *entry;\n \tstruct index_state wtindex;\n \tstruct checkout lstate, rstate;\n-\tint flags = RUN_GIT_CMD, err = 0;\n-\tconst char *helper_command = \"difftool--helper\";\n+\tint err = 0;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct hashmap wt_modified, tmp_modified;\n \tint indices_loaded = 0;\n \n@@ -564,13 +566,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \n \tstrbuf_setlen(&ldir, ldir_len);\n \tstrbuf_setlen(&rdir, rdir_len);\n+\n \tif (extcmd) {\n-\t\thelper_command = extcmd;\n-\t\tflags = 0;\n-\t} else\n+\t\tstrvec_push(&cmd.args, extcmd);\n+\t} else {\n+\t\tstrvec_push(&cmd.args, \"difftool--helper\");\n+\t\tcmd.git_cmd = 1;\n \t\tsetenv(\"GIT_DIFFTOOL_DIRDIFF\", \"true\", 1);\n-\tret = run_command_l_opt(flags, helper_command, ldir.buf, rdir.buf,\n-\t\t\t\tNULL);\n+\t}\n+\tstrvec_pushl(&cmd.args, ldir.buf, rdir.buf, NULL);\n+\n+\tret = run_command(&cmd);\n \n \t/* TODO: audit for interaction with sparse-index. */\n \tensure_full_index(&wtindex);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a0fca93bb6a..dd060dd65af 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1965,14 +1965,17 @@ static int fetch_multiple(struct string_list *list, int max_children)\n \t} else\n \t\tfor (i = 0; i < list->nr; i++) {\n \t\t\tconst char *name = list->items[i].string;\n-\t\t\tstrvec_push(&argv, name);\n+\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\t\t\tstrvec_pushv(&cmd.args, argv.v);\n+\t\t\tstrvec_push(&cmd.args, name);\n \t\t\tif (verbosity >= 0)\n \t\t\t\tprintf(_(\"Fetching %s\\n\"), name);\n-\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\t\t\tcmd.git_cmd = 1;\n+\t\t\tif (run_command(&cmd)) {\n \t\t\t\terror(_(\"could not fetch %s\"), name);\n \t\t\t\tresult = 1;\n \t\t\t}\n-\t\t\tstrvec_pop(&argv);\n \t\t}\n \n \tstrvec_clear(&argv);\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 8393e19b504..b47e53c2101 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -58,6 +58,8 @@ static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;\n static struct strvec reflog = STRVEC_INIT;\n static struct strvec repack = STRVEC_INIT;\n static struct strvec prune = STRVEC_INIT;\n+static struct strvec prune_worktrees = STRVEC_INIT;\n+static struct strvec rerere = STRVEC_INIT;\n \n static struct tempfile *pidfile;\n static struct lock_file log_lock;\n@@ -165,8 +167,11 @@ static void gc_config(void)\n struct maintenance_run_opts;\n static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)\n {\n-\treturn run_command_l_opt(RUN_GIT_CMD, \"pack-refs\", \"--all\", \"--prune\",\n-\t\t\t\t NULL);\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tstrvec_pushl(&cmd.args, \"pack-refs\", \"--all\", \"--prune\", NULL);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n \n static int too_many_loose_objects(void)\n@@ -518,6 +523,16 @@ static int report_last_gc_error(void)\n \treturn ret;\n }\n \n+static void run_git_or_die(const char **argv)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tstrvec_pushv(&cmd.args, argv);\n+\tcmd.git_cmd = 1;\n+\tif (run_command(&cmd))\n+\t\tdie(FAILED_RUN, argv[0]);\n+}\n+\n static void gc_before_repack(void)\n {\n \t/*\n@@ -532,8 +547,8 @@ static void gc_before_repack(void)\n \tif (pack_refs && maintenance_task_pack_refs(NULL))\n \t\tdie(FAILED_RUN, \"pack-refs\");\n \n-\tif (prune_reflogs && run_command_v_opt(reflog.v, RUN_GIT_CMD))\n-\t\tdie(FAILED_RUN, reflog.v[0]);\n+\tif (prune_reflogs)\n+\t\trun_git_or_die(reflog.v);\n }\n \n int cmd_gc(int argc, const char **argv, const char *prefix)\n@@ -571,6 +586,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tstrvec_pushl(&reflog, \"reflog\", \"expire\", \"--all\", NULL);\n \tstrvec_pushl(&repack, \"repack\", \"-d\", \"-l\", NULL);\n \tstrvec_pushl(&prune, \"prune\", \"--expire\", NULL);\n+\tstrvec_pushl(&prune_worktrees, \"worktree\", \"prune\", \"--expire\", NULL);\n+\tstrvec_pushl(&rerere, \"rerere\", \"gc\", NULL);\n \n \t/* default expiry time, overwritten in gc_config */\n \tgc_config();\n@@ -666,8 +683,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tgc_before_repack();\n \n \tif (!repository_format_precious_objects) {\n-\t\tif (run_command_v_opt(repack.v,\n-\t\t\t\t RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE))\n+\t\tstruct child_process repack_cmd = CHILD_PROCESS_INIT;\n+\n+\t\tstrvec_pushv(&repack_cmd.args, repack.v);\n+\t\trepack_cmd.git_cmd = 1;\n+\t\trepack_cmd.close_object_store = 1;\n+\t\tif (run_command(&repack_cmd))\n \t\t\tdie(FAILED_RUN, repack.v[0]);\n \n \t\tif (prune_expire) {\n@@ -678,18 +699,16 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tif (has_promisor_remote())\n \t\t\t\tstrvec_push(&prune,\n \t\t\t\t\t \"--exclude-promisor-objects\");\n-\t\t\tif (run_command_v_opt(prune.v, RUN_GIT_CMD))\n-\t\t\t\tdie(FAILED_RUN, prune.v[0]);\n+\t\t\trun_git_or_die(prune.v);\n \t\t}\n \t}\n \n-\tif (prune_worktrees_expire &&\n-\t run_command_l_opt(RUN_GIT_CMD, \"worktree\", \"prune\", \"--expire\",\n-\t\t\t prune_worktrees_expire, NULL))\n-\t\tdie(FAILED_RUN, \"worktree\");\n+\tif (prune_worktrees_expire) {\n+\t\tstrvec_push(&prune_worktrees, prune_worktrees_expire);\n+\t\trun_git_or_die(prune_worktrees.v);\n+\t}\n \n-\tif (run_command_l_opt(RUN_GIT_CMD, \"rerere\", \"gc\", NULL))\n-\t\tdie(FAILED_RUN, \"rerere\");\n+\trun_git_or_die(rerere.v);\n \n \treport_garbage = report_pack_garbage;\n \treprepare_packed_git(the_repository);\n@@ -1894,21 +1913,26 @@ static int is_schtasks_available(void)\n #endif\n }\n \n-#define SCHTASKS_NAME_FMT \"Git Maintenance (%s)\"\n+static char *schtasks_task_name(const char *frequency)\n+{\n+\tstruct strbuf label = STRBUF_INIT;\n+\tstrbuf_addf(&label, \"Git Maintenance (%s)\", frequency);\n+\treturn strbuf_detach(&label, NULL);\n+}\n \n static int schtasks_remove_task(enum schedule_priority schedule)\n {\n \tconst char *cmd = \"schtasks\";\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n \tconst char *frequency = get_frequency(schedule);\n+\tchar *name = schtasks_task_name(frequency);\n \n \tget_schedule_cmd(&cmd, NULL);\n-\tstrvec_split(&args, cmd);\n-\tstrvec_pushl(&args, \"/delete\", \"/tn\", NULL);\n-\tstrvec_pushf(&args, SCHTASKS_NAME_FMT, frequency);\n-\tstrvec_pushl(&args, \"/f\", NULL);\n+\tstrvec_split(&child.args, cmd);\n+\tstrvec_pushl(&child.args, \"/delete\", \"/tn\", name, \"/f\", NULL);\n+\tfree(name);\n \n-\treturn run_command_sv_opt(&args, 0);\n+\treturn run_command(&child);\n }\n \n static int schtasks_remove_tasks(void)\n@@ -1926,6 +1950,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tconst char *xml;\n \tstruct tempfile *tfile;\n \tconst char *frequency = get_frequency(schedule);\n+\tchar *name = schtasks_task_name(frequency);\n \tstruct strbuf tfilename = STRBUF_INIT;\n \n \tget_schedule_cmd(&cmd, NULL);\n@@ -2018,10 +2043,8 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \t \"\\n\";\n \tfprintf(tfile->fp, xml, exec_path, exec_path, frequency);\n \tstrvec_split(&child.args, cmd);\n-\tstrvec_pushl(&child.args, \"/create\", \"/tn\", NULL);\n-\tstrvec_pushf(&child.args, SCHTASKS_NAME_FMT, frequency);\n-\tstrvec_pushl(&child.args, \"/f\", \"/xml\",\n-\t\t get_tempfile_path(tfile), NULL);\n+\tstrvec_pushl(&child.args, \"/create\", \"/tn\", name, \"/f\", \"/xml\",\n+\t\t\t\t get_tempfile_path(tfile), NULL);\n \tclose_tempfile_gently(tfile);\n \n \tchild.no_stdout = 1;\n@@ -2032,6 +2055,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority\n \tresult = finish_command(&child);\n \n \tdelete_tempfile(&tfile);\n+\tfree(name);\n \treturn result;\n }\n \ndiff --git a/builtin/merge-index.c b/builtin/merge-index.c\nindex c0383fe9df9..012f52bd007 100644\n--- a/builtin/merge-index.c\n+++ b/builtin/merge-index.c\n@@ -12,6 +12,7 @@ static int merge_entry(int pos, const char *path)\n \tconst char *arguments[] = { pgm, \"\", \"\", \"\", path, \"\", \"\", \"\", NULL };\n \tchar hexbuf[4][GIT_MAX_HEXSZ + 1];\n \tchar ownbuf[4][60];\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n \tif (pos >= active_nr)\n \t\tdie(\"git merge-index: %s not in the cache\", path);\n@@ -31,7 +32,8 @@ static int merge_entry(int pos, const char *path)\n \tif (!found)\n \t\tdie(\"git merge-index: %s not in the cache\", path);\n \n-\tif (run_command_v_opt(arguments, 0)) {\n+\tstrvec_pushv(&cmd.args, arguments);\n+\tif (run_command(&cmd)) {\n \t\tif (one_shot)\n \t\t\terr++;\n \t\telse {\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 2f36823c97e..b21edd767aa 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -515,73 +515,75 @@ static void parse_repo_refspecs(int argc, const char **argv, const char **repo,\n */\n static int run_fetch(const char *repo, const char **refspecs)\n {\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n-\tstrvec_pushl(&args, \"fetch\", \"--update-head-ok\", NULL);\n+\tstrvec_pushl(&cmd.args, \"fetch\", \"--update-head-ok\", NULL);\n \n \t/* Shared options */\n-\targv_push_verbosity(&args);\n+\targv_push_verbosity(&cmd.args);\n \tif (opt_progress)\n-\t\tstrvec_push(&args, opt_progress);\n+\t\tstrvec_push(&cmd.args, opt_progress);\n \n \t/* Options passed to git-fetch */\n \tif (opt_all)\n-\t\tstrvec_push(&args, opt_all);\n+\t\tstrvec_push(&cmd.args, opt_all);\n \tif (opt_append)\n-\t\tstrvec_push(&args, opt_append);\n+\t\tstrvec_push(&cmd.args, opt_append);\n \tif (opt_upload_pack)\n-\t\tstrvec_push(&args, opt_upload_pack);\n-\targv_push_force(&args);\n+\t\tstrvec_push(&cmd.args, opt_upload_pack);\n+\targv_push_force(&cmd.args);\n \tif (opt_tags)\n-\t\tstrvec_push(&args, opt_tags);\n+\t\tstrvec_push(&cmd.args, opt_tags);\n \tif (opt_prune)\n-\t\tstrvec_push(&args, opt_prune);\n+\t\tstrvec_push(&cmd.args, opt_prune);\n \tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n \t\tswitch (recurse_submodules_cli) {\n \t\tcase RECURSE_SUBMODULES_ON:\n-\t\t\tstrvec_push(&args, \"--recurse-submodules=on\");\n+\t\t\tstrvec_push(&cmd.args, \"--recurse-submodules=on\");\n \t\t\tbreak;\n \t\tcase RECURSE_SUBMODULES_OFF:\n-\t\t\tstrvec_push(&args, \"--recurse-submodules=no\");\n+\t\t\tstrvec_push(&cmd.args, \"--recurse-submodules=no\");\n \t\t\tbreak;\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n-\t\t\tstrvec_push(&args, \"--recurse-submodules=on-demand\");\n+\t\t\tstrvec_push(&cmd.args, \"--recurse-submodules=on-demand\");\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tBUG(\"submodule recursion option not understood\");\n \t\t}\n \tif (max_children)\n-\t\tstrvec_push(&args, max_children);\n+\t\tstrvec_push(&cmd.args, max_children);\n \tif (opt_dry_run)\n-\t\tstrvec_push(&args, \"--dry-run\");\n+\t\tstrvec_push(&cmd.args, \"--dry-run\");\n \tif (opt_keep)\n-\t\tstrvec_push(&args, opt_keep);\n+\t\tstrvec_push(&cmd.args, opt_keep);\n \tif (opt_depth)\n-\t\tstrvec_push(&args, opt_depth);\n+\t\tstrvec_push(&cmd.args, opt_depth);\n \tif (opt_unshallow)\n-\t\tstrvec_push(&args, opt_unshallow);\n+\t\tstrvec_push(&cmd.args, opt_unshallow);\n \tif (opt_update_shallow)\n-\t\tstrvec_push(&args, opt_update_shallow);\n+\t\tstrvec_push(&cmd.args, opt_update_shallow);\n \tif (opt_refmap)\n-\t\tstrvec_push(&args, opt_refmap);\n+\t\tstrvec_push(&cmd.args, opt_refmap);\n \tif (opt_ipv4)\n-\t\tstrvec_push(&args, opt_ipv4);\n+\t\tstrvec_push(&cmd.args, opt_ipv4);\n \tif (opt_ipv6)\n-\t\tstrvec_push(&args, opt_ipv6);\n+\t\tstrvec_push(&cmd.args, opt_ipv6);\n \tif (opt_show_forced_updates > 0)\n-\t\tstrvec_push(&args, \"--show-forced-updates\");\n+\t\tstrvec_push(&cmd.args, \"--show-forced-updates\");\n \telse if (opt_show_forced_updates == 0)\n-\t\tstrvec_push(&args, \"--no-show-forced-updates\");\n+\t\tstrvec_push(&cmd.args, \"--no-show-forced-updates\");\n \tif (set_upstream)\n-\t\tstrvec_push(&args, set_upstream);\n-\tstrvec_pushv(&args, opt_fetch.v);\n+\t\tstrvec_push(&cmd.args, set_upstream);\n+\tstrvec_pushv(&cmd.args, opt_fetch.v);\n \n \tif (repo) {\n-\t\tstrvec_push(&args, repo);\n-\t\tstrvec_pushv(&args, refspecs);\n+\t\tstrvec_push(&cmd.args, repo);\n+\t\tstrvec_pushv(&cmd.args, refspecs);\n \t} else if (*refspecs)\n \t\tBUG(\"refspecs without repo?\");\n-\treturn run_command_sv_opt(&args, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);\n+\tcmd.git_cmd = 1;\n+\tcmd.close_object_store = 1;\n+\treturn run_command(&cmd);\n }\n \n /**\n@@ -650,49 +652,50 @@ static int update_submodules(void)\n */\n static int run_merge(void)\n {\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n-\tstrvec_pushl(&args, \"merge\", NULL);\n+\tstrvec_pushl(&cmd.args, \"merge\", NULL);\n \n \t/* Shared options */\n-\targv_push_verbosity(&args);\n+\targv_push_verbosity(&cmd.args);\n \tif (opt_progress)\n-\t\tstrvec_push(&args, opt_progress);\n+\t\tstrvec_push(&cmd.args, opt_progress);\n \n \t/* Options passed to git-merge */\n \tif (opt_diffstat)\n-\t\tstrvec_push(&args, opt_diffstat);\n+\t\tstrvec_push(&cmd.args, opt_diffstat);\n \tif (opt_log)\n-\t\tstrvec_push(&args, opt_log);\n+\t\tstrvec_push(&cmd.args, opt_log);\n \tif (opt_signoff)\n-\t\tstrvec_push(&args, opt_signoff);\n+\t\tstrvec_push(&cmd.args, opt_signoff);\n \tif (opt_squash)\n-\t\tstrvec_push(&args, opt_squash);\n+\t\tstrvec_push(&cmd.args, opt_squash);\n \tif (opt_commit)\n-\t\tstrvec_push(&args, opt_commit);\n+\t\tstrvec_push(&cmd.args, opt_commit);\n \tif (opt_edit)\n-\t\tstrvec_push(&args, opt_edit);\n+\t\tstrvec_push(&cmd.args, opt_edit);\n \tif (cleanup_arg)\n-\t\tstrvec_pushf(&args, \"--cleanup=%s\", cleanup_arg);\n+\t\tstrvec_pushf(&cmd.args, \"--cleanup=%s\", cleanup_arg);\n \tif (opt_ff)\n-\t\tstrvec_push(&args, opt_ff);\n+\t\tstrvec_push(&cmd.args, opt_ff);\n \tif (opt_verify)\n-\t\tstrvec_push(&args, opt_verify);\n+\t\tstrvec_push(&cmd.args, opt_verify);\n \tif (opt_verify_signatures)\n-\t\tstrvec_push(&args, opt_verify_signatures);\n-\tstrvec_pushv(&args, opt_strategies.v);\n-\tstrvec_pushv(&args, opt_strategy_opts.v);\n+\t\tstrvec_push(&cmd.args, opt_verify_signatures);\n+\tstrvec_pushv(&cmd.args, opt_strategies.v);\n+\tstrvec_pushv(&cmd.args, opt_strategy_opts.v);\n \tif (opt_gpg_sign)\n-\t\tstrvec_push(&args, opt_gpg_sign);\n+\t\tstrvec_push(&cmd.args, opt_gpg_sign);\n \tif (opt_autostash == 0)\n-\t\tstrvec_push(&args, \"--no-autostash\");\n+\t\tstrvec_push(&cmd.args, \"--no-autostash\");\n \telse if (opt_autostash == 1)\n-\t\tstrvec_push(&args, \"--autostash\");\n+\t\tstrvec_push(&cmd.args, \"--autostash\");\n \tif (opt_allow_unrelated_histories > 0)\n-\t\tstrvec_push(&args, \"--allow-unrelated-histories\");\n+\t\tstrvec_push(&cmd.args, \"--allow-unrelated-histories\");\n \n-\tstrvec_push(&args, \"FETCH_HEAD\");\n-\treturn run_command_sv_opt(&args, RUN_GIT_CMD);\n+\tstrvec_push(&cmd.args, \"FETCH_HEAD\");\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n \n /**\n@@ -873,40 +876,41 @@ static int get_rebase_newbase_and_upstream(struct object_id *newbase,\n static int run_rebase(const struct object_id *newbase,\n \t\tconst struct object_id *upstream)\n {\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n-\tstrvec_push(&args, \"rebase\");\n+\tstrvec_push(&cmd.args, \"rebase\");\n \n \t/* Shared options */\n-\targv_push_verbosity(&args);\n+\targv_push_verbosity(&cmd.args);\n \n \t/* Options passed to git-rebase */\n \tif (opt_rebase == REBASE_MERGES)\n-\t\tstrvec_push(&args, \"--rebase-merges\");\n+\t\tstrvec_push(&cmd.args, \"--rebase-merges\");\n \telse if (opt_rebase == REBASE_INTERACTIVE)\n-\t\tstrvec_push(&args, \"--interactive\");\n+\t\tstrvec_push(&cmd.args, \"--interactive\");\n \tif (opt_diffstat)\n-\t\tstrvec_push(&args, opt_diffstat);\n-\tstrvec_pushv(&args, opt_strategies.v);\n-\tstrvec_pushv(&args, opt_strategy_opts.v);\n+\t\tstrvec_push(&cmd.args, opt_diffstat);\n+\tstrvec_pushv(&cmd.args, opt_strategies.v);\n+\tstrvec_pushv(&cmd.args, opt_strategy_opts.v);\n \tif (opt_gpg_sign)\n-\t\tstrvec_push(&args, opt_gpg_sign);\n+\t\tstrvec_push(&cmd.args, opt_gpg_sign);\n \tif (opt_signoff)\n-\t\tstrvec_push(&args, opt_signoff);\n+\t\tstrvec_push(&cmd.args, opt_signoff);\n \tif (opt_autostash == 0)\n-\t\tstrvec_push(&args, \"--no-autostash\");\n+\t\tstrvec_push(&cmd.args, \"--no-autostash\");\n \telse if (opt_autostash == 1)\n-\t\tstrvec_push(&args, \"--autostash\");\n+\t\tstrvec_push(&cmd.args, \"--autostash\");\n \tif (opt_verify_signatures &&\n \t !strcmp(opt_verify_signatures, \"--verify-signatures\"))\n \t\twarning(_(\"ignoring --verify-signatures for rebase\"));\n \n-\tstrvec_push(&args, \"--onto\");\n-\tstrvec_push(&args, oid_to_hex(newbase));\n+\tstrvec_push(&cmd.args, \"--onto\");\n+\tstrvec_push(&cmd.args, oid_to_hex(newbase));\n \n-\tstrvec_push(&args, oid_to_hex(upstream));\n+\tstrvec_push(&cmd.args, oid_to_hex(upstream));\n \n-\treturn run_command_sv_opt(&args, RUN_GIT_CMD);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n \n static int get_can_ff(struct object_id *orig_head,\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 5d3534db19c..0cde2e244a4 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -92,13 +92,15 @@ static int verbose;\n \n static int fetch_remote(const char *name)\n {\n-\tconst char *argv[] = { \"fetch\", name, NULL, NULL };\n-\tif (verbose) {\n-\t\targv[1] = \"-v\";\n-\t\targv[2] = name;\n-\t}\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tstrvec_push(&cmd.args, \"fetch\");\n+\tif (verbose)\n+\t\tstrvec_push(&cmd.args, \"-v\");\n+\tstrvec_push(&cmd.args, name);\n \tprintf_ln(_(\"Updating %s\"), name);\n-\tif (run_command_v_opt(argv, RUN_GIT_CMD))\n+\tcmd.git_cmd = 1;\n+\tif (run_command(&cmd))\n \t\treturn error(_(\"Could not fetch %s\"), name);\n \treturn 0;\n }\n@@ -1508,34 +1510,35 @@ static int update(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"prune remotes after fetching\")),\n \t\tOPT_END()\n \t};\n-\tstruct strvec fetch_argv = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint default_defined = 0;\n \n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_remote_update_usage,\n \t\t\t PARSE_OPT_KEEP_ARGV0);\n \n-\tstrvec_push(&fetch_argv, \"fetch\");\n+\tstrvec_push(&cmd.args, \"fetch\");\n \n \tif (prune != -1)\n-\t\tstrvec_push(&fetch_argv, prune ? \"--prune\" : \"--no-prune\");\n+\t\tstrvec_push(&cmd.args, prune ? \"--prune\" : \"--no-prune\");\n \tif (verbose)\n-\t\tstrvec_push(&fetch_argv, \"-v\");\n-\tstrvec_push(&fetch_argv, \"--multiple\");\n+\t\tstrvec_push(&cmd.args, \"-v\");\n+\tstrvec_push(&cmd.args, \"--multiple\");\n \tif (argc < 2)\n-\t\tstrvec_push(&fetch_argv, \"default\");\n+\t\tstrvec_push(&cmd.args, \"default\");\n \tfor (i = 1; i < argc; i++)\n-\t\tstrvec_push(&fetch_argv, argv[i]);\n+\t\tstrvec_push(&cmd.args, argv[i]);\n \n-\tif (strcmp(fetch_argv.v[fetch_argv.nr-1], \"default\") == 0) {\n+\tif (strcmp(cmd.args.v[cmd.args.nr-1], \"default\") == 0) {\n \t\tgit_config(get_remote_default, &default_defined);\n \t\tif (!default_defined) {\n-\t\t\tstrvec_pop(&fetch_argv);\n-\t\t\tstrvec_push(&fetch_argv, \"--all\");\n+\t\t\tstrvec_pop(&cmd.args);\n+\t\t\tstrvec_push(&cmd.args, \"--all\");\n \t\t}\n \t}\n \n-\treturn run_command_sv_opt(&fetch_argv, RUN_GIT_CMD);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n \n static int remove_all_fetch_refspecs(const char *key)\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 4f5392c5796..d614f156df1 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -196,15 +196,20 @@ static int read_yes_no_answer(void)\n static int ask_yes_no_if_possible(const char *format, ...)\n {\n \tchar question[4096];\n-\tchar *retry_hook;\n+\tconst char *retry_hook;\n \tva_list args;\n \n \tva_start(args, format);\n \tvsnprintf(question, sizeof(question), format, args);\n \tva_end(args);\n \n-\tif ((retry_hook = mingw_getenv(\"GIT_ASK_YESNO\")))\n-\t\treturn !run_command_l_opt(0, retry_hook, question, NULL);\n+\tretry_hook = mingw_getenv(\"GIT_ASK_YESNO\");\n+\tif (retry_hook) {\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\t\tstrvec_pushl(&cmd.args, retry_hook, question, NULL);\n+\t\treturn !run_command(&cmd);\n+\t}\n \n \tif (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))\n \t\treturn 0;\ndiff --git a/diff.c b/diff.c\nindex 392530016fa..8451c230d9e 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4278,8 +4278,8 @@ static void run_external_diff(const char *pgm,\n \t\t\t const char *xfrm_msg,\n \t\t\t struct diff_options *o)\n {\n-\tstruct diff_queue_struct *q = &diff_queued_diff;\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstruct diff_queue_struct *q = &diff_queued_diff;\n \n \tstrvec_push(&cmd.args, pgm);\n \tstrvec_push(&cmd.args, name);\n@@ -4295,7 +4295,8 @@ static void run_external_diff(const char *pgm,\n \t\t}\n \t}\n \n-\tstrvec_pushf(&cmd.env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n+\tstrvec_pushf(&cmd.env, \"GIT_DIFF_PATH_COUNTER=%d\",\n+\t\t ++o->diff_path_counter);\n \tstrvec_pushf(&cmd.env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n \n \tdiff_free_filespec_data(one);\ndiff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c\nindex 19d772f0f3a..96d8d37c06d 100644\n--- a/fsmonitor-ipc.c\n+++ b/fsmonitor-ipc.c\n@@ -56,11 +56,11 @@ static int spawn_daemon(void)\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n+\tstrvec_pushl(&cmd.args, \"fsmonitor--daemon\", \"start\", NULL);\n+\n \tcmd.git_cmd = 1;\n \tcmd.no_stdin = 1;\n \tcmd.trace2_child_class = \"fsmonitor\";\n-\tstrvec_pushl(&cmd.args, \"fsmonitor--daemon\", \"start\", NULL);\n-\n \treturn run_command(&cmd);\n }\n \ndiff --git a/ll-merge.c b/ll-merge.c\nindex 740689b7bd6..b20491043e0 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -193,6 +193,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tstruct strbuf cmd = STRBUF_INIT;\n \tstruct strbuf_expand_dict_entry dict[6];\n \tstruct strbuf path_sq = STRBUF_INIT;\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n \tint status, fd, i;\n \tstruct stat st;\n \tenum ll_merge_result ret;\n@@ -218,7 +219,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \n \tstrbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);\n \n-\tstatus = run_command_l_opt(RUN_USING_SHELL, cmd.buf, NULL);\n+\tstrvec_push(&child.args, cmd.buf);\n+\tchild.use_shell = 1;\n+\tstatus = run_command(&child);\n \tfd = open(temp[1], O_RDONLY);\n \tif (fd < 0)\n \t\tgoto bad;\ndiff --git a/merge.c b/merge.c\nindex 487debacecb..445b4f19aa8 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -19,21 +19,22 @@ int try_merge_command(struct repository *r,\n \t\t const char **xopts, struct commit_list *common,\n \t\t const char *head_arg, struct commit_list *remotes)\n {\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint i, ret;\n \tstruct commit_list *j;\n \n-\tstrvec_pushf(&args, \"merge-%s\", strategy);\n+\tstrvec_pushf(&cmd.args, \"merge-%s\", strategy);\n \tfor (i = 0; i < xopts_nr; i++)\n-\t\tstrvec_pushf(&args, \"--%s\", xopts[i]);\n+\t\tstrvec_pushf(&cmd.args, \"--%s\", xopts[i]);\n \tfor (j = common; j; j = j->next)\n-\t\tstrvec_push(&args, merge_argument(j->item));\n-\tstrvec_push(&args, \"--\");\n-\tstrvec_push(&args, head_arg);\n+\t\tstrvec_push(&cmd.args, merge_argument(j->item));\n+\tstrvec_push(&cmd.args, \"--\");\n+\tstrvec_push(&cmd.args, head_arg);\n \tfor (j = remotes; j; j = j->next)\n-\t\tstrvec_push(&args, merge_argument(j->item));\n+\t\tstrvec_push(&cmd.args, merge_argument(j->item));\n \n-\tret = run_command_sv_opt(&args, RUN_GIT_CMD);\n+\tcmd.git_cmd = 1;\n+\tret = run_command(&cmd);\n \n \tdiscard_index(r->index);\n \tif (repo_read_index(r) < 0)\ndiff --git a/scalar.c b/scalar.c\nindex 3480bf73cbd..03f9e480dd6 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -69,17 +69,18 @@ static void setup_enlistment_directory(int argc, const char **argv,\n \n static int run_git(const char *arg, ...)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tva_list args;\n \tconst char *p;\n \n \tva_start(args, arg);\n-\tstrvec_push(&argv, arg);\n+\tstrvec_push(&cmd.args, arg);\n \twhile ((p = va_arg(args, const char *)))\n-\t\tstrvec_push(&argv, p);\n+\t\tstrvec_push(&cmd.args, p);\n \tva_end(args);\n \n-\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n \n struct scalar_config {\ndiff --git a/sequencer.c b/sequencer.c\nindex 7ee0e05512c..9b9d6a55828 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3183,14 +3183,15 @@ static int rollback_is_safe(void)\n \n static int reset_merge(const struct object_id *oid)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n-\tstrvec_pushl(&argv, \"reset\", \"--merge\", NULL);\n+\tstrvec_pushl(&cmd.args, \"reset\", \"--merge\", NULL);\n \n \tif (!is_null_oid(oid))\n-\t\tstrvec_push(&argv, oid_to_hex(oid));\n+\t\tstrvec_push(&cmd.args, oid_to_hex(oid));\n \n-\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n \n static int rollback_single_pick(struct repository *r)\n@@ -3554,10 +3555,13 @@ static int error_failed_squash(struct repository *r,\n \n static int do_exec(struct repository *r, const char *command_line)\n {\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint dirty, status;\n \n \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n-\tstatus = run_command_l_opt(RUN_USING_SHELL, command_line, NULL);\n+\tstrvec_push(&cmd.args, command_line);\n+\tcmd.use_shell = 1;\n+\tstatus = run_command(&cmd);\n \n \t/* force re-reading of the cache */\n \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n@@ -4861,13 +4865,13 @@ static int pick_commits(struct repository *r,\n \n static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n \t !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n \n-\tstrvec_push(&argv, \"commit\");\n+\tstrvec_push(&cmd.args, \"commit\");\n \n \t/*\n \t * continue_single_pick() handles the case of recovering from a\n@@ -4880,9 +4884,10 @@ static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n \t\t * Include --cleanup=strip as well because we don't want the\n \t\t * \"# Conflicts:\" messages.\n \t\t */\n-\t\tstrvec_pushl(&argv, \"--no-edit\", \"--cleanup=strip\", NULL);\n+\t\tstrvec_pushl(&cmd.args, \"--no-edit\", \"--cleanup=strip\", NULL);\n \n-\treturn run_command_sv_opt(&argv, RUN_GIT_CMD);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n \n static int commit_staged_changes(struct repository *r,\ndiff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c\nindex 7967478a40a..42185f3fc05 100644\n--- a/t/helper/test-fake-ssh.c\n+++ b/t/helper/test-fake-ssh.c\n@@ -8,6 +8,7 @@ int cmd_main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tFILE *f;\n \tint i;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \n \t/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */\n \tif (!trash_directory)\n@@ -24,5 +25,7 @@ int cmd_main(int argc, const char **argv)\n \t/* Now, evaluate the *last* parameter */\n \tif (argc < 2)\n \t\treturn 0;\n-\treturn run_command_l_opt(RUN_USING_SHELL, argv[argc - 1], NULL);\n+\tstrvec_push(&cmd.args, argv[argc - 1]);\n+\tcmd.use_shell = 1;\n+\treturn run_command(&cmd);\n }\ndiff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\nindex a714130ece7..94fd8ccf513 100644\n--- a/t/helper/test-trace2.c\n+++ b/t/helper/test-trace2.c\n@@ -132,6 +132,7 @@ static int ut_003error(int argc, const char **argv)\n */\n static int ut_004child(int argc, const char **argv)\n {\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint result;\n \n \t/*\n@@ -141,7 +142,8 @@ static int ut_004child(int argc, const char **argv)\n \tif (!argc)\n \t\treturn 0;\n \n-\tresult = run_command_v_opt(argv, 0);\n+\tstrvec_pushv(&cmd.args, argv);\n+\tresult = run_command(&cmd);\n \texit(result);\n }\n \ndiff --git a/tmp-objdir.h b/tmp-objdir.h\nindex c96aa77396d..615b1885733 100644\n--- a/tmp-objdir.h\n+++ b/tmp-objdir.h\n@@ -10,11 +10,9 @@\n *\n * Example:\n *\n- *\tstruct child_process cmd = CHILD_PROCESS_INIT;\n- *\t...\n *\tstruct tmp_objdir *t = tmp_objdir_create(\"incoming\");\n- * strvec_pushl(&cmd.env, tmp_objdir_env(t));\n- *\tif (!run_command(&cmd) && !tmp_objdir_migrate(t))\n+ *\tstrvec_pushv(&cmd.env, tmp_objdir_env(t));\n+ *\tif (!run_command(&cmd)) && !tmp_objdir_migrate(t))\n *\t\tprintf(\"success!\\n\");\n *\telse\n *\t\tdie(\"failed...tmp_objdir will clean up for us\");\n"},{"id":"465222","messageId":"Y08P0G1Be+5hCVML@coredump.intra.peff.net","threadId":"58626","inReplyTo":"221018.86a65ti70m.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-18T20:42:56Z","receivedAt":"2022-10-18T20:43:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 18, 2022 at 03:28:43PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > Hmph... I somehow thought that the concensus is rather try the\n> > complete opposite approach shown by René's patch to lose the use of\n> > run_command_v_opt() by replacing it with run_command(&args), with\n> > args.v populated using strvec_pushl() and other strvec API\n> > functions.\n> >\n> > One of the reasons I would prefer to see us moving in that direction\n> > was because the first iteration of this series was a good\n> > demonstration of the relatively limited usefulness of _l_opt()\n> > variant and also it seemed to be error prone to use it.\n> \n> I'm getting somewhat mixed messages. Jeff seemed to like the end-to-end\n> safety of run_command_l_opt() before I wrote it. I think the\n> run_command_l_opt() still really shines for the simple cases.\n\nSorry if I gave that impression. I like the safety of strvec_pushl(),\nbut I think using it with a \"struct child_process\" is just fine. It's\nmore flexible, and as René's patch showed, doesn't really make the\ncallers more complex (that definitely _wasn't_ the case long ago, when\nmost of these callers were written).\n\nI do think Junio saying \"consensus\" may have been premature. I expressed\nmy opinion and he agreed, but I think that is as far as it got. :)\n\n> I don't see how *_l_opt() is particularly error prone, I just had a\n> stupid think-o in v1 of this, but that if/else if bug is something that\n> could have snuck in with run_command() given the same stupidity :)\n\nI don't think it's error-prone. It just seems like it complicates an API\nfor little gain, and causes us to have a lot of boilerplate mapping\nRUN_* flags into cmd.* fields.\n\n> I wonder if a run_command() that just had the prototype (struct\n> child_process *cmd, ...) might not be the best of both worlds (or a\n> run_commandl() alternative). I.e. to do away with the whole custom way\n> of specifying the flag(s), and just take the passed-in arguments and\n> append them to \"&cmd.args\".\n\nThat would work, but is it buying much? You still have to declare the\nchild_process at that point, which means you have:\n\n struct child_process cmd = CHILD_PROCESS_INIT;\n cmd.git_cmd = 1;\n run_command(&cmd, \"log\", \"--\", \"HEAD\", NULL);\n\ninstead of:\n\n struct child_process cmd = CHILD_PROCESS_INIT;\n cmd.git_cmd = 1;\n strvec_pushl(&cmd.args, \"log\", \"--\", \"HEAD\", NULL);\n run_command(&cmd);\n\nSaving one line doesn't seem worth complicating the API to me. Plus\nexisting users have to say \"run_command(&cmd, NULL)\", or we need to\nignore varargs when \"cmd.args.nr > 0\" (which papers over errors).\n\nAnd most of the time run_command() is inside an if() anyway. Just\nstyle-wise, keeping the long command name on its own line is a\nreadability improvement (IMHO, anyway).\n\n> It's also interesting to consider adding a --noop option to be supported\n> by parse-options.c. The reason we can't use a run_command_l_opt() in\n> some cases is because we're doing e.g.:\n> \n> \tif (progress)\n> \t\tstrvec_push(&args, \"--progress\");\n> \n> We have a --no-progress, but in those cases the recipient at the end\n> often cares about a default of -1 for a bool variable, or similar. So if\n> we could do:\n> \n> \trun_command_l_opt(0, command,\n> \t\t(progress ? \"--progress\" : \"--noop\"),\n> \t\t...,\n> \t\tNULL\n> \t);\n\nI dunno. That does not seem more readable to me, and would end up with\nGIT_TRACE lines like:\n\n git foo --noop --noop --real-option --noop\n\n> We could benefit from compile-time checks, and wouldn't have to allocate\n> a strvec just for building up the arguments in some cases. Just food for\n> thought...\n\nKeep in mind we allocate a strvec either way. And IMHO seeing:\n\n if (foo)\n strvec_push(&cmd.args, \"foo\");\n\nis the most readable form. Not to mention that it is more flexible. Many\ncases use \"pushf\" for the same thing.\n\n-Peff\n"},{"id":"465252","messageId":"xmqq7d0vu8cw.fsf@gitster.g","threadId":"58626","inReplyTo":"Y08P0G1Be+5hCVML@coredump.intra.peff.net","subject":"Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-19T15:43:43Z","receivedAt":"2022-10-19T15:56:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do think Junio saying \"consensus\" may have been premature. I expressed\n> my opinion and he agreed, but I think that is as far as it got. :)\n\nMaybe. This is a tangent, but as far as I am concerned, when Réne\nwrites something that looks to me very straight-forward and correct,\nand it passes your taste buds, then that is enough consensus to move\nahead. As Linus often said and I concur, some people got good\ntaste, that is hard to quantify and probably hard to teach, and\nthere are a handful folks here with good taste. And when two who\nhave demonstrated they are with good taste agrees, that is good\nenough to me.\n\n>> I don't see how *_l_opt() is particularly error prone, I just had a\n>> stupid think-o in v1 of this, but that if/else if bug is something that\n>> could have snuck in with run_command() given the same stupidity :)\n>\n> I don't think it's error-prone. It just seems like it complicates an API\n> for little gain, and causes us to have a lot of boilerplate mapping\n> RUN_* flags into cmd.* fields.\n\nTrue. run_command() needs the RUN_* flags twiddling, too, so it is\nnot a point against _l_opt() variant. What I see as its biggest\nproblem is it is a bit too rigid for many of the current callers\nthat use _v_opt() to replace them, and can easily tempt developers\nto write almost-duplicate _l_opt() calls, leading to an easily\navoidable bug like we saw in the if/else if/else cascade.\n\nThanks.\n"},{"id":"465259","messageId":"Y1AujKKPUJFepph5@coredump.intra.peff.net","threadId":"58626","inReplyTo":"xmqq7d0vu8cw.fsf@gitster.g","subject":"Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-19T17:06:20Z","receivedAt":"2022-10-19T17:06:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 19, 2022 at 08:43:43AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I do think Junio saying \"consensus\" may have been premature. I expressed\n> > my opinion and he agreed, but I think that is as far as it got. :)\n> \n> Maybe. This is a tangent, but as far as I am concerned, when Réne\n> writes something that looks to me very straight-forward and correct,\n> and it passes your taste buds, then that is enough consensus to move\n> ahead. As Linus often said and I concur, some people got good\n> taste, that is hard to quantify and probably hard to teach, and\n> there are a handful folks here with good taste. And when two who\n> have demonstrated they are with good taste agrees, that is good\n> enough to me.\n\nI pretty much agree with that worldview, but I try hard not to assume\n\"my taste is good\" when going into a technical discussion. If only\nbecause I've embarrassed myself a sufficient number of times. :)\n\n> >> I don't see how *_l_opt() is particularly error prone, I just had a\n> >> stupid think-o in v1 of this, but that if/else if bug is something that\n> >> could have snuck in with run_command() given the same stupidity :)\n> >\n> > I don't think it's error-prone. It just seems like it complicates an API\n> > for little gain, and causes us to have a lot of boilerplate mapping\n> > RUN_* flags into cmd.* fields.\n> \n> True. run_command() needs the RUN_* flags twiddling, too, so it is\n> not a point against _l_opt() variant.\n\nDid you mean run_command_v() here? If so, then yes, it requires the\nflags. But if we are going to drop it in favor of just run_command(),\nthen those flags go away (and moving to the _l() variant is a step in\nthe opposite direction).\n\n-Peff\n"},{"id":"465260","messageId":"xmqq7d0vsngp.fsf@gitster.g","threadId":"58626","inReplyTo":"Y1AujKKPUJFepph5@coredump.intra.peff.net","subject":"Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-19T18:00:22Z","receivedAt":"2022-10-19T18:00:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> True. run_command() needs the RUN_* flags twiddling, too, so it is\n>> not a point against _l_opt() variant.\n>\n> Did you mean run_command_v() here? If so, then yes, it requires the\n> flags. But if we are going to drop it in favor of just run_command(),\n> then those flags go away (and moving to the _l() variant is a step in\n> the opposite direction).\n\nAh, but we'd still need to prepare individual bits in the child\nstructure (e.g. RUN_GIT_CMD vs .git_cmd) anyway. Even though we may\nnot have to work with the flags, somehow we need to set it up. So\nit is not all that strong argument against the _l_opt().\n\nThe above assessment is primarily because I do not have too much\nagainst RUN_GIT_CMD and friends, compared to setting the individual\nmembers in the child_process struct. Setting individual members in\nthe struct is more direct and it may be an improvement use\nrun_command() directly over using _v_opt() and _l_opt() variants.\n\n"},{"id":"465262","messageId":"Y1BIfEN5ds2kKjjk@coredump.intra.peff.net","threadId":"58626","inReplyTo":"xmqq7d0vsngp.fsf@gitster.g","subject":"Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-19T18:57:00Z","receivedAt":"2022-10-19T18:57:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 19, 2022 at 11:00:22AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> True. run_command() needs the RUN_* flags twiddling, too, so it is\n> >> not a point against _l_opt() variant.\n> >\n> > Did you mean run_command_v() here? If so, then yes, it requires the\n> > flags. But if we are going to drop it in favor of just run_command(),\n> > then those flags go away (and moving to the _l() variant is a step in\n> > the opposite direction).\n> \n> Ah, but we'd still need to prepare individual bits in the child\n> structure (e.g. RUN_GIT_CMD vs .git_cmd) anyway. Even though we may\n> not have to work with the flags, somehow we need to set it up. So\n> it is not all that strong argument against the _l_opt().\n\nRight, to the callers it is no different; they must use the flags or the\nstruct fields. What I meant is that we do not need to support _both_.\nI.e., the big mapping in run_command_v_opt_cd_env_tr2() goes away.\n\n> The above assessment is primarily because I do not have too much\n> against RUN_GIT_CMD and friends, compared to setting the individual\n> members in the child_process struct. Setting individual members in\n> the struct is more direct and it may be an improvement use\n> run_command() directly over using _v_opt() and _l_opt() variants.\n\nYeah. I do not find RUN_GIT_CMD all that bad myself; I was mainly\npointing out that we do not need to support both of them (and of the\ntwo, the struct fields are by far the more flexible, so that's what we\nshould keep).\n\nThat said, I am not too sad to have both. I would not have bothered to\ndo the work to remove the _v() versions with flags. But since René\nalready did so...\n\n-Peff\n"},{"id":"465265","messageId":"xmqq5ygfr48b.fsf@gitster.g","threadId":"58626","inReplyTo":"Y1BIfEN5ds2kKjjk@coredump.intra.peff.net","subject":"Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-19T19:41:08Z","receivedAt":"2022-10-19T19:41:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> That said, I am not too sad to have both. I would not have bothered to\n> do the work to remove the _v() versions with flags. But since René\n> already did so...\n\nIt makes two of us ;-)\n"},{"id":"465324","messageId":"84971a3a-4c5c-5b79-582e-88b3bd8c624f@web.de","threadId":"58626","inReplyTo":"xmqq5ygfr48b.fsf@gitster.g","subject":"Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-20T18:34:10Z","receivedAt":"2022-10-20T18:34:30Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.10.22 um 21:41 schrieb Junio C Hamano:\n> Jeff King writes:\n>\n>> That said, I am not too sad to have both. I would not have bothered to\n>> do the work to remove the _v() versions with flags. But since René\n>> already did so...\n>\n> It makes two of us ;-)\n\nI wrote that patch out of curiosity and was pleased by the total line\ncount reduction, getting rid of the on-stack array construction sites\nwith their magic numbers, not having to manage strvecs explicitly\nanymore and the callers staying readable.\n\nOne weak spot is the new helper builtin/gc.c::run_git_or_die() that I\nadded because it was easier than replacing all those strvecs that are\nprepared before deciding whether their commands are even needed.\n\nStripping down the central API to a single shared object (a struct and\nfunctions that get it passed) simplifies it for programmers. It takes\nthe idea of d3b2159712 (run-command API: remove \"argv\" member, always\nuse \"args\", 2021-11-25) and c7c4bdeccf (run-command API: remove \"env\"\nmember, always use \"env_array\", 2021-11-25) to its logical conclusion\nof going fully dynamic and using standard strvec functions everywhere.\nLocal shortcuts like builtin/gc.c::run_git_or_die() may still be\ndefensible.\n\nBut still: Is all of that code churn worth it? Not sure.\n\nRené\n"},{"id":"465388","messageId":"xmqqilkeoy0q.fsf@gitster.g","threadId":"58626","inReplyTo":"84971a3a-4c5c-5b79-582e-88b3bd8c624f@web.de","subject":"Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-20T23:50:29Z","receivedAt":"2022-10-20T23:50:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> I wrote that patch out of curiosity and was pleased by the total line\n> count reduction, getting rid of the on-stack array construction sites\n> with their magic numbers, not having to manage strvecs explicitly\n> anymore and the callers staying readable.\n> ...\n> But still: Is all of that code churn worth it? Not sure.\n\nCompared to doing nothing? The result did look easier to grok,\nespecially as we no longer need to worry about another way to do the\nsame thing (i.e. run_command() vs run_command_v_*() variants) and\ncan get rid of the flags constants that need to be kept in sync with\nthe members of the child_process struct.\n\nCompared to adding the _l_opt() variant? Surely, as that goes the\nother direction to add more callers that use the flags constants,\nwithout much practical gain (which was a bit sad, but we only found\nthat out after seeing the result).\n"}]}