[PATCH 00/10] run-command API: add run_command_{l,sv}_opt()

[PATCH 00/10] run-command API: add run_command_{l,sv}_opt()

[Ævar Arnfjörð Bjarmason]

This series starts out by addressing the issue Jeff King noted in
https://lore.kernel.org/git/Y0TXTl0gSBOFQa9B@coredump.intra.peff.net/;
I.e. that by moving away from a "struct strvec" for run_command()
variants we lose out on the "warn_unused_result" checking we'd get
with the strvec_pushl() (ensuring the last element is NULL).

The 3/10 here adds a run_command_l_opt() function, which provides an
easy & safe one-shot API using warn_unused_result.

The 10/10 here then adds a run_command_sv_opt(), which is a
run_command_v_opt() taking a "struct strvec" that the API clears for
the user after it's called. This makes a lot of one-shot "run this and
free the strvec" users less verbose.

Then, having converted to these helpers we had 1-2 users of other
run_command_*() variants, which could be moved to using run_command()
directly, and thus getting rid of supporting so much shorthand API
variation.

At the end of this series the in-tree use of the 3x helpers is (by
number of *.c file occurances):

     19 run_command_l_opt
     15 run_command_v_opt
     12 run_command_sv_opt

More can be converted to run_command_{l,sv}_opt(), but I left out
e.g. bisect--helper.c to avoid conflicts with anything in-flight.

Ævar Arnfjörð Bjarmason (10):
  run-command.c: refactor run_command_*_tr2() to internal helpers
  merge: remove always-the-same "verbose" arguments
  run-command API: add and use a run_command_l_opt()
  am: use run_command_l_opt() for show_patch()
  run-command API docs: clarify & fleshen out run_command_v_opt*() docs
  run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag
  run-command API & diff.c: remove run_command_v_opt_cd_env()
  run-command API & users: remove run_command_v_opt_tr2()
  gc: use strvec_pushf(), avoid redundant strbuf_detach()
  run-command API: add and use a run_command_sv_opt()

 add-interactive.c        |  3 +-
 bisect.c                 | 19 +++++------
 builtin/add.c            |  6 ++--
 builtin/am.c             | 14 +++-----
 builtin/clone.c          | 19 ++++-------
 builtin/difftool.c       | 14 ++++----
 builtin/gc.c             | 49 +++++++++-----------------
 builtin/merge.c          | 46 ++++++-------------------
 builtin/pull.c           | 15 ++------
 builtin/remote.c         | 15 +++-----
 compat/mingw.c           |  8 ++---
 diff.c                   | 26 +++++++-------
 fsmonitor-ipc.c          | 10 ++++--
 git.c                    | 15 ++++----
 ll-merge.c               |  4 +--
 merge.c                  |  3 +-
 run-command.c            | 43 ++++++++++++-----------
 run-command.h            | 74 +++++++++++++++++++++++++++-------------
 scalar.c                 |  6 +---
 sequencer.c              | 15 ++------
 t/helper/test-fake-ssh.c |  4 +--
 tmp-objdir.h             |  6 ++--
 22 files changed, 177 insertions(+), 237 deletions(-)

-- 
2.38.0.1092.g8c0298861b0

[PATCH 01/10] run-command.c: refactor run_command_*_tr2() to internal helpers

[Ævar Arnfjörð Bjarmason]

Refactor the run_command_v_opt_cd_env_tr2() function to helpers which
takes a "struct child_process *cmd" argument. This will allow for
adding a helper which won't need to copy a set of arguments to the
"cmd.args" we'd otherwise have to populate from the "argv".

Splitting out the part of the function that sets the various members
from "opt" will help in the subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c | 40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/run-command.c b/run-command.c
index 5ec3a46dccf..cf4a431c839 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1004,6 +1004,18 @@ int run_command(struct child_process *cmd)
 	return finish_command(cmd);
 }
 
+static void run_command_set_opts(struct child_process *cmd, int opt)
+{
+	cmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
+	cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
+	cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
+	cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
+	cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
+	cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
+	cmd->wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
+	cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
+}
+
 int run_command_v_opt(const char **argv, int opt)
 {
 	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
@@ -1019,24 +1031,26 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
 	return run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);
 }
 
+static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,
+					  const char *dir,
+					  const char *const *env,
+					  const char *tr2_class)
+{
+	run_command_set_opts(cmd, opt);
+	cmd->dir = dir;
+	if (env)
+		strvec_pushv(&cmd->env, (const char **)env);
+	cmd->trace2_child_class = tr2_class;
+	return run_command(cmd);
+}
+
 int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 				 const char *const *env, const char *tr2_class)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
+
 	strvec_pushv(&cmd.args, argv);
-	cmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
-	cmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
-	cmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
-	cmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
-	cmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;
-	cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
-	cmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
-	cmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
-	cmd.dir = dir;
-	if (env)
-		strvec_pushv(&cmd.env, (const char **)env);
-	cmd.trace2_child_class = tr2_class;
-	return run_command(&cmd);
+	return run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class);
 }
 
 #ifndef NO_PTHREADS
-- 
2.38.0.1092.g8c0298861b0

Re: [PATCH 01/10] run-command.c: refactor run_command_*_tr2() to internal helpers

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> +static void run_command_set_opts(struct child_process *cmd, int opt)
> +{
> +	cmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
> +	cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
> +	cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
> +	cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
> +	cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
> +	cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
> +	cmd->wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
> +	cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
> +}

This, combined with the change in the caller that loses the
corresponding lines, does make sense.  But


> @@ -1019,24 +1031,26 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
>  	return run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);
>  }
>  
> +static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,
> +					  const char *dir,
> +					  const char *const *env,
> +					  const char *tr2_class)
> +{
> +	run_command_set_opts(cmd, opt);
> +	cmd->dir = dir;
> +	if (env)
> +		strvec_pushv(&cmd->env, (const char **)env);
> +	cmd->trace2_child_class = tr2_class;
> +	return run_command(cmd);
> +}

This?

The original caller took argv and copied it into cmd.args itself and
the updated caller still does so because this new helper doesn't do
so for it, but it no longer does the copying of env because this
helper does it.

Unless we will add several more callers of this helper in later
steps, this sounds a bit too specialized to the way the first single
caller used to do it.

But let's keep reading to see if it is worth adding it.

>  int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
>  				 const char *const *env, const char *tr2_class)
>  {
>  	struct child_process cmd = CHILD_PROCESS_INIT;
> +
>  	strvec_pushv(&cmd.args, argv);
> -	cmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
> -	cmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
> -	cmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
> -	cmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
> -	cmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;
> -	cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
> -	cmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
> -	cmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
> -	cmd.dir = dir;
> -	if (env)
> -		strvec_pushv(&cmd.env, (const char **)env);
> -	cmd.trace2_child_class = tr2_class;
> -	return run_command(&cmd);
> +	return run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class);
>  }
>  
>  #ifndef NO_PTHREADS

[PATCH 02/10] merge: remove always-the-same "verbose" arguments

[Ævar Arnfjörð Bjarmason]

Simplify the code that builds the arguments for the "read-tree"
invocation in reset_hard() and read_empty() to remove the "verbose"
parameter.

Before 172b6428d06 (do not overwrite untracked during merge from
unborn branch, 2010-11-14) there was a "reset_hard()" function that
would be called in two places, one of those passed a "verbose=1", the
other a "verbose=0".

After 172b6428d06 when read_empty() was split off from reset_hard()
both of these functions only had one caller. The "verbose" in
read_empty() would always be false, and the one in reset_hard() would
always be true.

There was never a good reason for the code to act this way, it
happened because the read_empty() function was a copy/pasted and
adjusted version of reset_hard().

Since we're no longer conditionally adding the "-v" parameter
here (and we'd only add it for "reset_hard()" we'll be able to move to
a simpler and safer run-command API in the subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 5900b81729d..3bb49d805b4 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -345,14 +345,12 @@ static int save_state(struct object_id *stash)
 	return rc;
 }
 
-static void read_empty(const struct object_id *oid, int verbose)
+static void read_empty(const struct object_id *oid)
 {
 	int i = 0;
 	const char *args[7];
 
 	args[i++] = "read-tree";
-	if (verbose)
-		args[i++] = "-v";
 	args[i++] = "-m";
 	args[i++] = "-u";
 	args[i++] = empty_tree_oid_hex();
@@ -363,14 +361,13 @@ static void read_empty(const struct object_id *oid, int verbose)
 		die(_("read-tree failed"));
 }
 
-static void reset_hard(const struct object_id *oid, int verbose)
+static void reset_hard(const struct object_id *oid)
 {
 	int i = 0;
 	const char *args[6];
 
 	args[i++] = "read-tree";
-	if (verbose)
-		args[i++] = "-v";
+	args[i++] = "-v";
 	args[i++] = "--reset";
 	args[i++] = "-u";
 	args[i++] = oid_to_hex(oid);
@@ -385,7 +382,7 @@ static void restore_state(const struct object_id *head,
 {
 	struct strvec args = STRVEC_INIT;
 
-	reset_hard(head, 1);
+	reset_hard(head);
 
 	if (is_null_oid(stash))
 		goto refresh_cache;
@@ -1470,7 +1467,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 					       check_trust_level);
 
 		remote_head_oid = &remoteheads->item->object.oid;
-		read_empty(remote_head_oid, 0);
+		read_empty(remote_head_oid);
 		update_ref("initial pull", "HEAD", remote_head_oid, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
 		goto done;
-- 
2.38.0.1092.g8c0298861b0

Re: [PATCH 02/10] merge: remove always-the-same "verbose" arguments

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Simplify the code that builds the arguments for the "read-tree"
> invocation in reset_hard() and read_empty() to remove the "verbose"
> parameter.
>
> Before 172b6428d06 (do not overwrite untracked during merge from
> unborn branch, 2010-11-14) there was a "reset_hard()" function that
> would be called in two places, one of those passed a "verbose=1", the
> other a "verbose=0".
>
> After 172b6428d06 when read_empty() was split off from reset_hard()
> both of these functions only had one caller. The "verbose" in
> read_empty() would always be false, and the one in reset_hard() would
> always be true.
>
> There was never a good reason for the code to act this way, it
> happened because the read_empty() function was a copy/pasted and
> adjusted version of reset_hard().
>
> Since we're no longer conditionally adding the "-v" parameter
> here (and we'd only add it for "reset_hard()" we'll be able to move to
> a simpler and safer run-command API in the subsequent commit.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/merge.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)

I haven't checked the topics in flight that touch the same file, but
as these are file-scope static, it is easy to check the correctness,
and the change of function signature will mean that compilers will
notice after a merge if there is somebody else who still wants them
to be conditionally verbose.

I wonder if these were always unused, or we lost different callers
over time, though.

>
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 5900b81729d..3bb49d805b4 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -345,14 +345,12 @@ static int save_state(struct object_id *stash)
>  	return rc;
>  }
>  
> -static void read_empty(const struct object_id *oid, int verbose)
> +static void read_empty(const struct object_id *oid)
>  {
>  	int i = 0;
>  	const char *args[7];
>  
>  	args[i++] = "read-tree";
> -	if (verbose)
> -		args[i++] = "-v";
>  	args[i++] = "-m";
>  	args[i++] = "-u";
>  	args[i++] = empty_tree_oid_hex();
> @@ -363,14 +361,13 @@ static void read_empty(const struct object_id *oid, int verbose)
>  		die(_("read-tree failed"));
>  }
>  
> -static void reset_hard(const struct object_id *oid, int verbose)
> +static void reset_hard(const struct object_id *oid)
>  {
>  	int i = 0;
>  	const char *args[6];
>  
>  	args[i++] = "read-tree";
> -	if (verbose)
> -		args[i++] = "-v";
> +	args[i++] = "-v";
>  	args[i++] = "--reset";
>  	args[i++] = "-u";
>  	args[i++] = oid_to_hex(oid);
> @@ -385,7 +382,7 @@ static void restore_state(const struct object_id *head,
>  {
>  	struct strvec args = STRVEC_INIT;
>  
> -	reset_hard(head, 1);
> +	reset_hard(head);
>  
>  	if (is_null_oid(stash))
>  		goto refresh_cache;
> @@ -1470,7 +1467,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  					       check_trust_level);
>  
>  		remote_head_oid = &remoteheads->item->object.oid;
> -		read_empty(remote_head_oid, 0);
> +		read_empty(remote_head_oid);
>  		update_ref("initial pull", "HEAD", remote_head_oid, NULL, 0,
>  			   UPDATE_REFS_DIE_ON_ERR);
>  		goto done;

[PATCH 03/10] run-command API: add and use a run_command_l_opt()

[Ævar Arnfjörð Bjarmason]

When run_command() is used with strvec_pushl() as in the pre-image of
b004c902827 (gc: simplify maintenance_task_pack_refs(), 2022-10-04) we
get the benefit of the LAST_ARG_MUST_BE_NULL. See 9fe3edc47f1 (Add the
LAST_ARG_MUST_BE_NULL macro, 2013-07-18).

Let's provide and use a run_command_l_opt() function, which gives us
the best of both worlds. We'll now need less code to accomplish the
same thing, and this version's safer as we can assert that the
terminating NULL is present.

The "builtin/bisect--helper.c" would be a large beneficiary of this
API, with "15 insertions(+), 26 deletions(-)", but let's leave that
aside for now, as there's an outstanding topic that's extensively
modifying it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 bisect.c                 | 19 +++++++++----------
 builtin/clone.c          | 16 ++++++----------
 builtin/difftool.c       | 14 ++++++--------
 builtin/gc.c             | 22 ++++++++--------------
 builtin/merge.c          | 35 ++++++-----------------------------
 builtin/remote.c         | 10 ++++------
 compat/mingw.c           |  8 +++-----
 ll-merge.c               |  4 +---
 run-command.c            | 15 +++++++++++++++
 run-command.h            | 13 +++++++++++--
 sequencer.c              |  4 +---
 t/helper/test-fake-ssh.c |  4 +---
 12 files changed, 71 insertions(+), 93 deletions(-)

diff --git a/bisect.c b/bisect.c
index fd581b85a72..415b0e7b0b2 100644
--- a/bisect.c
+++ b/bisect.c
@@ -738,18 +738,17 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
 	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 
 	argv_checkout[2] = bisect_rev_hex;
-	if (no_checkout) {
+	if (no_checkout)
 		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
-	} else {
-		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
-			/*
-			 * Errors in `run_command()` itself, signaled by res < 0,
-			 * and errors in the child process, signaled by res > 0
-			 * can both be treated as regular BISECT_FAILED (-1).
-			 */
-			return BISECT_FAILED;
-	}
+	else if (run_command_l_opt(RUN_GIT_CMD, "checkout", "-q",
+				   bisect_rev_hex, "--", NULL))
+		/*
+		 * Errors in `run_command()` itself, signaled by res < 0,
+		 * and errors in the child process, signaled by res > 0
+		 * can both be treated as regular BISECT_FAILED (-1).
+		 */
+		return BISECT_FAILED;
 
 	commit = lookup_commit_reference(the_repository, bisect_rev);
 	format_commit_message(commit, "[%H] %s%n", &commit_msg, &pp);
diff --git a/builtin/clone.c b/builtin/clone.c
index ed8d44bb6ab..13fc7a4bc5d 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -651,23 +651,19 @@ static void update_head(const struct ref *our, const struct ref *remote,
 
 static int git_sparse_checkout_init(const char *repo)
 {
-	struct strvec argv = STRVEC_INIT;
-	int result = 0;
-	strvec_pushl(&argv, "-C", repo, "sparse-checkout", "set", NULL);
-
 	/*
 	 * We must apply the setting in the current process
 	 * for the later checkout to use the sparse-checkout file.
 	 */
 	core_apply_sparse_checkout = 1;
 
-	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+	if (run_command_l_opt(RUN_GIT_CMD, "-C", repo, "sparse-checkout",
+			      "set", NULL)) {
 		error(_("failed to initialize sparse-checkout"));
-		result = 1;
+		return 1;
 	}
 
-	strvec_clear(&argv);
-	return result;
+	return 0;
 }
 
 static int checkout(int submodule_progress, int filter_submodules)
@@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,
 
 static void dissociate_from_references(void)
 {
-	static const char* argv[] = { "repack", "-a", "-d", NULL };
 	char *alternates = git_pathdup("objects/info/alternates");
 
 	if (!access(alternates, F_OK)) {
-		if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))
+		if (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,
+				      "repack",  "-a", "-d", NULL))
 			die(_("cannot repack to clean up"));
 		if (unlink(alternates) && errno != ENOENT)
 			die_errno(_("cannot unlink temporary alternates file"));
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4b10ad1a369..ed211a87322 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -44,8 +44,8 @@ static int difftool_config(const char *var, const char *value, void *cb)
 
 static int print_tool_help(void)
 {
-	const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
-	return run_command_v_opt(argv, RUN_GIT_CMD);
+	return run_command_l_opt(RUN_GIT_CMD, "mergetool", "--tool-help=diff",
+				 NULL);
 }
 
 static int parse_index_info(char *p, int *mode1, int *mode2,
@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct index_state wtindex;
 	struct checkout lstate, rstate;
 	int flags = RUN_GIT_CMD, err = 0;
-	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
+	const char *helper_command = "difftool--helper";
 	struct hashmap wt_modified, tmp_modified;
 	int indices_loaded = 0;
 
@@ -563,16 +563,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	}
 
 	strbuf_setlen(&ldir, ldir_len);
-	helper_argv[1] = ldir.buf;
 	strbuf_setlen(&rdir, rdir_len);
-	helper_argv[2] = rdir.buf;
-
 	if (extcmd) {
-		helper_argv[0] = extcmd;
+		helper_command = extcmd;
 		flags = 0;
 	} else
 		setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
-	ret = run_command_v_opt(helper_argv, flags);
+	ret = run_command_l_opt(flags, helper_command, ldir.buf, rdir.buf,
+				NULL);
 
 	/* TODO: audit for interaction with sparse-index. */
 	ensure_full_index(&wtindex);
diff --git a/builtin/gc.c b/builtin/gc.c
index 243ee85d283..075b4637660 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -58,8 +58,6 @@ static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;
 static struct strvec reflog = STRVEC_INIT;
 static struct strvec repack = STRVEC_INIT;
 static struct strvec prune = STRVEC_INIT;
-static struct strvec prune_worktrees = STRVEC_INIT;
-static struct strvec rerere = STRVEC_INIT;
 
 static struct tempfile *pidfile;
 static struct lock_file log_lock;
@@ -167,9 +165,8 @@ static void gc_config(void)
 struct maintenance_run_opts;
 static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
 {
-	const char *argv[] = { "pack-refs", "--all", "--prune", NULL };
-
-	return run_command_v_opt(argv, RUN_GIT_CMD);
+	return run_command_l_opt(RUN_GIT_CMD, "pack-refs", "--all", "--prune",
+				 NULL);
 }
 
 static int too_many_loose_objects(void)
@@ -574,8 +571,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	strvec_pushl(&reflog, "reflog", "expire", "--all", NULL);
 	strvec_pushl(&repack, "repack", "-d", "-l", NULL);
 	strvec_pushl(&prune, "prune", "--expire", NULL);
-	strvec_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL);
-	strvec_pushl(&rerere, "rerere", "gc", NULL);
 
 	/* default expiry time, overwritten in gc_config */
 	gc_config();
@@ -688,14 +683,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (prune_worktrees_expire) {
-		strvec_push(&prune_worktrees, prune_worktrees_expire);
-		if (run_command_v_opt(prune_worktrees.v, RUN_GIT_CMD))
-			die(FAILED_RUN, prune_worktrees.v[0]);
-	}
+	if (prune_worktrees_expire &&
+	    run_command_l_opt(RUN_GIT_CMD, "worktree", "prune", "--expire",
+			      prune_worktrees_expire, NULL))
+		die(FAILED_RUN, "worktree");
 
-	if (run_command_v_opt(rerere.v, RUN_GIT_CMD))
-		die(FAILED_RUN, rerere.v[0]);
+	if (run_command_l_opt(RUN_GIT_CMD, "rerere", "gc", NULL))
+		die(FAILED_RUN, "rerere");
 
 	report_garbage = report_pack_garbage;
 	reprepare_packed_git(the_repository);
diff --git a/builtin/merge.c b/builtin/merge.c
index 3bb49d805b4..9c04b588f68 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -347,55 +347,32 @@ static int save_state(struct object_id *stash)
 
 static void read_empty(const struct object_id *oid)
 {
-	int i = 0;
-	const char *args[7];
-
-	args[i++] = "read-tree";
-	args[i++] = "-m";
-	args[i++] = "-u";
-	args[i++] = empty_tree_oid_hex();
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
-
-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	if (run_command_l_opt(RUN_GIT_CMD, "read-tree", "-m", "-u",
+			      empty_tree_oid_hex(), oid_to_hex(oid), NULL))
 		die(_("read-tree failed"));
 }
 
 static void reset_hard(const struct object_id *oid)
 {
-	int i = 0;
-	const char *args[6];
-
-	args[i++] = "read-tree";
-	args[i++] = "-v";
-	args[i++] = "--reset";
-	args[i++] = "-u";
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
-
-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	if (run_command_l_opt(RUN_GIT_CMD, "read-tree", "-v", "--reset", "-u",
+			      oid_to_hex(oid), NULL))
 		die(_("read-tree failed"));
 }
 
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	struct strvec args = STRVEC_INIT;
-
 	reset_hard(head);
 
 	if (is_null_oid(stash))
 		goto refresh_cache;
 
-	strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
-	strvec_push(&args, oid_to_hex(stash));
-
 	/*
 	 * It is OK to ignore error here, for example when there was
 	 * nothing to restore.
 	 */
-	run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
+	run_command_l_opt(RUN_GIT_CMD, "stash", "apply", "--index", "--quiet",
+			  oid_to_hex(stash), NULL);
 
 refresh_cache:
 	if (discard_cache() < 0 || read_cache() < 0)
diff --git a/builtin/remote.c b/builtin/remote.c
index 910f7b9316a..1d86c14297b 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -92,13 +92,11 @@ static int verbose;
 
 static int fetch_remote(const char *name)
 {
-	const char *argv[] = { "fetch", name, NULL, NULL };
-	if (verbose) {
-		argv[1] = "-v";
-		argv[2] = name;
-	}
 	printf_ln(_("Updating %s"), name);
-	if (run_command_v_opt(argv, RUN_GIT_CMD))
+	if (verbose && run_command_l_opt(RUN_GIT_CMD, "-v", "fetch", name,
+					 NULL))
+		return error(_("Could not fetch %s"), name);
+	else if (run_command_l_opt(RUN_GIT_CMD, "fetch", name, NULL))
 		return error(_("Could not fetch %s"), name);
 	return 0;
 }
diff --git a/compat/mingw.c b/compat/mingw.c
index 901375d5841..4f5392c5796 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -196,17 +196,15 @@ static int read_yes_no_answer(void)
 static int ask_yes_no_if_possible(const char *format, ...)
 {
 	char question[4096];
-	const char *retry_hook[] = { NULL, NULL, NULL };
+	char *retry_hook;
 	va_list args;
 
 	va_start(args, format);
 	vsnprintf(question, sizeof(question), format, args);
 	va_end(args);
 
-	if ((retry_hook[0] = mingw_getenv("GIT_ASK_YESNO"))) {
-		retry_hook[1] = question;
-		return !run_command_v_opt(retry_hook, 0);
-	}
+	if ((retry_hook = mingw_getenv("GIT_ASK_YESNO")))
+		return !run_command_l_opt(0, retry_hook, question, NULL);
 
 	if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
 		return 0;
diff --git a/ll-merge.c b/ll-merge.c
index 8955d7e1f6e..740689b7bd6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -193,7 +193,6 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf_expand_dict_entry dict[6];
 	struct strbuf path_sq = STRBUF_INIT;
-	const char *args[] = { NULL, NULL };
 	int status, fd, i;
 	struct stat st;
 	enum ll_merge_result ret;
@@ -219,8 +218,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 
 	strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);
 
-	args[0] = cmd.buf;
-	status = run_command_v_opt(args, RUN_USING_SHELL);
+	status = run_command_l_opt(RUN_USING_SHELL, cmd.buf, NULL);
 	fd = open(temp[1], O_RDONLY);
 	if (fd < 0)
 		goto bad;
diff --git a/run-command.c b/run-command.c
index cf4a431c839..0be5626f7f2 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1016,6 +1016,21 @@ static void run_command_set_opts(struct child_process *cmd, int opt)
 	cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
 }
 
+int run_command_l_opt(int opt, ...)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	va_list ap;
+	const char *arg;
+
+	va_start(ap, opt);
+	while ((arg = va_arg(ap, const char *)))
+		strvec_push(&cmd.args, arg);
+	va_end(ap);
+
+	run_command_set_opts(&cmd, opt);
+	return run_command(&cmd);
+}
+
 int run_command_v_opt(const char **argv, int opt)
 {
 	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
diff --git a/run-command.h b/run-command.h
index 0e85e5846a5..6320d70f062 100644
--- a/run-command.h
+++ b/run-command.h
@@ -151,8 +151,8 @@ struct child_process {
 
 /**
  * The functions: child_process_init, start_command, finish_command,
- * run_command, run_command_v_opt, run_command_v_opt_cd_env, child_process_clear
- * do the following:
+ * run_command, run_command_l_opt, run_command_v_opt,
+ * run_command_v_opt_cd_env, child_process_clear do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
  *   is printed.
@@ -254,6 +254,15 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
 int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 				 const char *const *env, const char *tr2_class);
 
+/**
+ * The run_command_l_opt() function run_command_v_opt() takes a list
+ * of strings terminated by a NULL. Use it instead of
+ * run_command_v_opt() when possible, as it allows the compiler to
+ * check that the "argv" is NULL-delimited.
+ */
+LAST_ARG_MUST_BE_NULL
+int run_command_l_opt(int opt, ...);
+
 /**
  * Execute the given command, sending "in" to its stdin, and capturing its
  * stdout and stderr in the "out" and "err" strbufs. Any of the three may
diff --git a/sequencer.c b/sequencer.c
index debb2ecbafe..20495db9de2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3558,12 +3558,10 @@ static int error_failed_squash(struct repository *r,
 
 static int do_exec(struct repository *r, const char *command_line)
 {
-	const char *child_argv[] = { NULL, NULL };
 	int dirty, status;
 
 	fprintf(stderr, _("Executing: %s\n"), command_line);
-	child_argv[0] = command_line;
-	status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+	status = run_command_l_opt(RUN_USING_SHELL, command_line, NULL);
 
 	/* force re-reading of the cache */
 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c
index 12beee99ad2..7967478a40a 100644
--- a/t/helper/test-fake-ssh.c
+++ b/t/helper/test-fake-ssh.c
@@ -8,7 +8,6 @@ int cmd_main(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f;
 	int i;
-	const char *child_argv[] = { NULL, NULL };
 
 	/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */
 	if (!trash_directory)
@@ -25,6 +24,5 @@ int cmd_main(int argc, const char **argv)
 	/* Now, evaluate the *last* parameter */
 	if (argc < 2)
 		return 0;
-	child_argv[0] = argv[argc - 1];
-	return run_command_v_opt(child_argv, RUN_USING_SHELL);
+	return run_command_l_opt(RUN_USING_SHELL, argv[argc - 1], NULL);
 }
-- 
2.38.0.1092.g8c0298861b0

Re: [PATCH 03/10] run-command API: add and use a run_command_l_opt()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

>  bisect.c                 | 19 +++++++++----------
>  builtin/clone.c          | 16 ++++++----------
>  builtin/difftool.c       | 14 ++++++--------
>  builtin/gc.c             | 22 ++++++++--------------
>  builtin/merge.c          | 35 ++++++-----------------------------
>  builtin/remote.c         | 10 ++++------
>  compat/mingw.c           |  8 +++-----
>  ll-merge.c               |  4 +---
>  run-command.c            | 15 +++++++++++++++
>  run-command.h            | 13 +++++++++++--
>  sequencer.c              |  4 +---
>  t/helper/test-fake-ssh.c |  4 +---
>  12 files changed, 71 insertions(+), 93 deletions(-)

Nice.

> @@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,
>  
>  static void dissociate_from_references(void)
>  {
> -	static const char* argv[] = { "repack", "-a", "-d", NULL };

Good to see that this one in a wrong scope can now go away.

>  	char *alternates = git_pathdup("objects/info/alternates");
>  
>  	if (!access(alternates, F_OK)) {
> -		if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))
> +		if (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,
> +				      "repack",  "-a", "-d", NULL))
>  			die(_("cannot repack to clean up"));


> diff --git a/builtin/remote.c b/builtin/remote.c
> index 910f7b9316a..1d86c14297b 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -92,13 +92,11 @@ static int verbose;
>  
>  static int fetch_remote(const char *name)
>  {
> -	const char *argv[] = { "fetch", name, NULL, NULL };
> -	if (verbose) {
> -		argv[1] = "-v";
> -		argv[2] = name;
> -	}
>  	printf_ln(_("Updating %s"), name);
> -	if (run_command_v_opt(argv, RUN_GIT_CMD))
> +	if (verbose && run_command_l_opt(RUN_GIT_CMD, "-v", "fetch", name,
> +					 NULL))
> +		return error(_("Could not fetch %s"), name);
> +	else if (run_command_l_opt(RUN_GIT_CMD, "fetch", name, NULL))
>  		return error(_("Could not fetch %s"), name);
>  	return 0;
>  }

This, together with the "merge" one that used to be conditional
(which I take as a sign that new callers can legitimately wish it to
be conditional again), is where the new l_opt() variant is weak.

And the above is wrong.  If verbose option is given and run command
succeeds in running "fetch -v" (another bug is that the updated code
is running "git -v fetch <name>"), we will try running "fetch" without
"-v" option after that.

So, for simplest things (i.e. the majority of places this patch
touches), addition of l_opt() is great.  Use of it for nontrivial
things like this is not.  We need to repeat ourselves, and the use
of API is error prone.

> diff --git a/compat/mingw.c b/compat/mingw.c
> index 901375d5841..4f5392c5796 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -196,17 +196,15 @@ static int read_yes_no_answer(void)
>  static int ask_yes_no_if_possible(const char *format, ...)
>  {
>  	char question[4096];
> -	const char *retry_hook[] = { NULL, NULL, NULL };

Good to see that this one in a wrong scope can now go away.

> +	char *retry_hook;
>  	va_list args;
>  
>  	va_start(args, format);
>  	vsnprintf(question, sizeof(question), format, args);
>  	va_end(args);
>  
> -	if ((retry_hook[0] = mingw_getenv("GIT_ASK_YESNO"))) {
> -		retry_hook[1] = question;
> -		return !run_command_v_opt(retry_hook, 0);
> -	}
> +	if ((retry_hook = mingw_getenv("GIT_ASK_YESNO")))
> +		return !run_command_l_opt(0, retry_hook, question, NULL);
>  
>  	if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
>  		return 0;

Re: [PATCH 03/10] run-command API: add and use a run_command_l_opt()

[Jeff King]

On Fri, Oct 14, 2022 at 11:40:27AM -0700, Junio C Hamano wrote:

> > @@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,
> >  
> >  static void dissociate_from_references(void)
> >  {
> > -	static const char* argv[] = { "repack", "-a", "-d", NULL };
> 
> Good to see that this one in a wrong scope can now go away.

It definitely is broader scope than is necessary. I wonder if it makes
things easier to read, though, the way we would sometimes hoist things
out of a function into a static-global to make them more obvious. I
dunno.

> >  	char *alternates = git_pathdup("objects/info/alternates");
> >  
> >  	if (!access(alternates, F_OK)) {
> > -		if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))
> > +		if (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,
> > +				      "repack",  "-a", "-d", NULL))
> >  			die(_("cannot repack to clean up"));

I just happened to notice in this one there is a weird extra space
before "-a".

-Peff

Re: [PATCH 03/10] run-command API: add and use a run_command_l_opt()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Oct 14, 2022 at 11:40:27AM -0700, Junio C Hamano wrote:
>
>> > @@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,
>> >  
>> >  static void dissociate_from_references(void)
>> >  {
>> > -	static const char* argv[] = { "repack", "-a", "-d", NULL };
>> 
>> Good to see that this one in a wrong scope can now go away.
>
> It definitely is broader scope than is necessary. I wonder if it makes
> things easier to read, though, the way we would sometimes hoist things
> out of a function into a static-global to make them more obvious. I
> dunno.

Didn't think about it, but it is a worthy point to make.  Unlike the
call to l_opt() buried inside a conditional, it makes it stand out
that what we see upfront is one of the commands the function will
run.  It is especially valuable when a function is almost exclusively
about running a single command, but then we will have a single call
to l_opt() without much preparations or clean-ups around it, so the
visual noise that detracts our eyes away from the actual commands
may not be all that bad, though.

>> >  	char *alternates = git_pathdup("objects/info/alternates");
>> >  
>> >  	if (!access(alternates, F_OK)) {
>> > -		if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))
>> > +		if (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,
>> > +				      "repack",  "-a", "-d", NULL))
>> >  			die(_("cannot repack to clean up"));
>
> I just happened to notice in this one there is a weird extra space
> before "-a".

Yeah, good eyes.

Thanks.

[PATCH 04/10] am: use run_command_l_opt() for show_patch()

[Ævar Arnfjörð Bjarmason]

The "git show" invocation added in 66335298a47 (rebase: add
--show-current-patch, 2018-02-11) is a one-off, and one where we're
not calling oid_to_hex() twice. So we can rely on the static buffer
that oid_to_hex() points to, rather than xstrdup()-ing it. As a result
we can use the run_command_l_opt() function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/am.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 39fea248330..1d298a91306 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2186,16 +2186,10 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
 	const char *patch_path;
 	int len;
 
-	if (!is_null_oid(&state->orig_commit)) {
-		const char *av[4] = { "show", NULL, "--", NULL };
-		char *new_oid_str;
-		int ret;
-
-		av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));
-		ret = run_command_v_opt(av, RUN_GIT_CMD);
-		free(new_oid_str);
-		return ret;
-	}
+	if (!is_null_oid(&state->orig_commit))
+		return run_command_l_opt(RUN_GIT_CMD, "show",
+					 oid_to_hex(&state->orig_commit),
+					 "--", NULL);
 
 	switch (sub_mode) {
 	case SHOW_PATCH_RAW:
-- 
2.38.0.1092.g8c0298861b0

Re: [PATCH 04/10] am: use run_command_l_opt() for show_patch()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> The "git show" invocation added in 66335298a47 (rebase: add
> --show-current-patch, 2018-02-11) is a one-off, and one where we're
> not calling oid_to_hex() twice.

I am not sure what relevance it has being a one-off in justifying
this change.

I agree with the latter half of the explanation, though.  As long as
run_command_l_opt() does not reuse the static oid_to_hex() buffers,
this is safe.

> -	if (!is_null_oid(&state->orig_commit)) {
> -		const char *av[4] = { "show", NULL, "--", NULL };
> -		char *new_oid_str;
> -		int ret;
> -
> -		av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));
> -		ret = run_command_v_opt(av, RUN_GIT_CMD);
> -		free(new_oid_str);
> -		return ret;
> -	}
> +	if (!is_null_oid(&state->orig_commit))
> +		return run_command_l_opt(RUN_GIT_CMD, "show",
> +					 oid_to_hex(&state->orig_commit),
> +					 "--", NULL);
>  
>  	switch (sub_mode) {
>  	case SHOW_PATCH_RAW:

[PATCH 05/10] run-command API docs: clarify & fleshen out run_command_v_opt*() docs

[Ævar Arnfjörð Bjarmason]

Add a discussion of the flags that were missing to the
run_command_v_opt*() docs, and in doing so format them such that we
can easily add or remove flags from a table in the future, rather than
having them tied up in prose.

Let's also clarify why the user might want to use this API over
run_command() itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.h | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/run-command.h b/run-command.h
index 6320d70f062..cf250e36426 100644
--- a/run-command.h
+++ b/run-command.h
@@ -234,13 +234,28 @@ int run_auto_maintenance(int quiet);
 #define RUN_CLOSE_OBJECT_STORE		(1<<7)
 
 /**
- * Convenience functions that encapsulate a sequence of
- * start_command() followed by finish_command(). The argument argv
- * specifies the program and its arguments. The argument opt is zero
- * or more of the flags `RUN_COMMAND_NO_STDIN`, `RUN_GIT_CMD`,
- * `RUN_COMMAND_STDOUT_TO_STDERR`, or `RUN_SILENT_EXEC_FAILURE`
- * that correspond to the members .no_stdin, .git_cmd,
- * .stdout_to_stderr, .silent_exec_failure of `struct child_process`.
+ * The run_command_v_opt*() API is a convenience wrapper for an
+ * underlying run_command().
+ *
+ * It's intended to be used when the user already has an "argv" they'd
+ * like to use. As opposed to the "struct child_process"'s "args"
+ * member, which will be strvec_clear()'d by calling run_command(),
+ * the caller owns the "argv", which is not altered by invoking these
+ * functions.
+ *
+ * The "opt" flags that will cause various underlying run_command()
+ * members to be set. The flags and the corresponding struct members
+ * are:
+ *
+ *	- RUN_COMMAND_NO_STDIN: .no_stdin
+ *	- RUN_GIT_CMD: .git_cmd
+ *	- RUN_COMMAND_STDOUT_TO_STDERR: .stdout_to_stderr
+ *	- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure
+ *	- RUN_USING_SHELL: .use_shell
+ *	- RUN_CLEAN_ON_EXIT: .clean_on_exit
+ *	- RUN_WAIT_AFTER_CLEAN: .wait_after_clean
+ *	- RUN_CLOSE_OBJECT_STORE: .close_object_store
+ *
  * The argument dir corresponds the member .dir. The argument env
  * corresponds to the member .env.
  */
-- 
2.38.0.1092.g8c0298861b0

[PATCH 06/10] run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag

[Ævar Arnfjörð Bjarmason]

The run_command_v_opt*() API is meant to handle various common cases
where we don't want the verbosity of calling the underlying
run_command(), but we're explicitly not trying to cover the full API
surface of run_command() itself.

So if we're not using some of these flags we probably won't need them
again, any future user who needs to set "stdout_to_stderr" can just
use run_command() itself, and the convenience API shouldn't be
cluttered by trying to handle all the needs of various one-offs.

So let's remove the RUN_COMMAND_STDOUT_TO_STDERR flag, it hasn't been
used since 860a2ebecd2 (receive-pack: send auto-gc output over
sideband 2, 2016-06-05) when its last user started using the
underlying API directly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c |  1 -
 run-command.h | 12 +++++-------
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/run-command.c b/run-command.c
index 0be5626f7f2..fe1b4a0b650 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1008,7 +1008,6 @@ static void run_command_set_opts(struct child_process *cmd, int opt)
 {
 	cmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
 	cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
-	cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
 	cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
 	cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
 	cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
diff --git a/run-command.h b/run-command.h
index cf250e36426..0af01bd9b67 100644
--- a/run-command.h
+++ b/run-command.h
@@ -226,12 +226,11 @@ int run_auto_maintenance(int quiet);
 
 #define RUN_COMMAND_NO_STDIN		(1<<0)
 #define RUN_GIT_CMD			(1<<1)
-#define RUN_COMMAND_STDOUT_TO_STDERR	(1<<2)
-#define RUN_SILENT_EXEC_FAILURE		(1<<3)
-#define RUN_USING_SHELL			(1<<4)
-#define RUN_CLEAN_ON_EXIT		(1<<5)
-#define RUN_WAIT_AFTER_CLEAN		(1<<6)
-#define RUN_CLOSE_OBJECT_STORE		(1<<7)
+#define RUN_SILENT_EXEC_FAILURE		(1<<2)
+#define RUN_USING_SHELL			(1<<3)
+#define RUN_CLEAN_ON_EXIT		(1<<4)
+#define RUN_WAIT_AFTER_CLEAN		(1<<5)
+#define RUN_CLOSE_OBJECT_STORE		(1<<6)
 
 /**
  * The run_command_v_opt*() API is a convenience wrapper for an
@@ -249,7 +248,6 @@ int run_auto_maintenance(int quiet);
  *
  *	- RUN_COMMAND_NO_STDIN: .no_stdin
  *	- RUN_GIT_CMD: .git_cmd
- *	- RUN_COMMAND_STDOUT_TO_STDERR: .stdout_to_stderr
  *	- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure
  *	- RUN_USING_SHELL: .use_shell
  *	- RUN_CLEAN_ON_EXIT: .clean_on_exit
-- 
2.38.0.1092.g8c0298861b0

[PATCH 07/10] run-command API & diff.c: remove run_command_v_opt_cd_env()

[Ævar Arnfjörð Bjarmason]

As we'll see in subsequent commits most of the run_command_v_opt_*()
API users are a bad fit for what we'd like to do with that API. The
ideal use-case for it is something where we already have an "argv",
and don't want the one-shot semantics of run_command().

In the case of run_command_v_opt_cd_env() there was only user of it,
and that user was just accumulating complexity by not using
run_command() directly.

By not having to maintain its own "argv" and "env" it doesn't have to
strvec_clear() them both, which in the case of run_command() will be
done by child_process_clear() before it returns.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 diff.c        | 26 ++++++++++++--------------
 run-command.c |  7 +------
 run-command.h |  3 +--
 tmp-objdir.h  |  6 ++++--
 4 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/diff.c b/diff.c
index 648f6717a55..392530016fa 100644
--- a/diff.c
+++ b/diff.c
@@ -4278,35 +4278,33 @@ static void run_external_diff(const char *pgm,
 			      const char *xfrm_msg,
 			      struct diff_options *o)
 {
-	struct strvec argv = STRVEC_INIT;
-	struct strvec env = STRVEC_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
-	strvec_push(&argv, pgm);
-	strvec_push(&argv, name);
+	strvec_push(&cmd.args, pgm);
+	strvec_push(&cmd.args, name);
 
 	if (one && two) {
-		add_external_diff_name(o->repo, &argv, name, one);
+		add_external_diff_name(o->repo, &cmd.args, name, one);
 		if (!other)
-			add_external_diff_name(o->repo, &argv, name, two);
+			add_external_diff_name(o->repo, &cmd.args, name, two);
 		else {
-			add_external_diff_name(o->repo, &argv, other, two);
-			strvec_push(&argv, other);
-			strvec_push(&argv, xfrm_msg);
+			add_external_diff_name(o->repo, &cmd.args, other, two);
+			strvec_push(&cmd.args, other);
+			strvec_push(&cmd.args, xfrm_msg);
 		}
 	}
 
-	strvec_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
-	strvec_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
+	strvec_pushf(&cmd.env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
+	strvec_pushf(&cmd.env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
 
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
-	if (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))
+	cmd.use_shell = 1;
+	if (run_command(&cmd))
 		die(_("external diff died, stopping at %s"), name);
 
 	remove_tempfile();
-	strvec_clear(&argv);
-	strvec_clear(&env);
 }
 
 static int similarity_index(struct diff_filepair *p)
diff --git a/run-command.c b/run-command.c
index fe1b4a0b650..6132a9f19f0 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1032,7 +1032,7 @@ int run_command_l_opt(int opt, ...)
 
 int run_command_v_opt(const char **argv, int opt)
 {
-	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
+	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);
 }
 
 int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)
@@ -1040,11 +1040,6 @@ int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)
 	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);
 }
 
-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env)
-{
-	return run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);
-}
-
 static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,
 					  const char *dir,
 					  const char *const *env,
diff --git a/run-command.h b/run-command.h
index 0af01bd9b67..2574d46cb70 100644
--- a/run-command.h
+++ b/run-command.h
@@ -152,7 +152,7 @@ struct child_process {
 /**
  * The functions: child_process_init, start_command, finish_command,
  * run_command, run_command_l_opt, run_command_v_opt,
- * run_command_v_opt_cd_env, child_process_clear do the following:
+ * child_process_clear do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
  *   is printed.
@@ -263,7 +263,6 @@ int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);
  * env (the environment) is to be formatted like environ: "VAR=VALUE".
  * To unset an environment variable use just "VAR".
  */
-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);
 int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 				 const char *const *env, const char *tr2_class);
 
diff --git a/tmp-objdir.h b/tmp-objdir.h
index 76efc7edee5..c96aa77396d 100644
--- a/tmp-objdir.h
+++ b/tmp-objdir.h
@@ -10,9 +10,11 @@
  *
  * Example:
  *
+ *	struct child_process cmd = CHILD_PROCESS_INIT;
+ *	...
  *	struct tmp_objdir *t = tmp_objdir_create("incoming");
- *	if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
- *	    !tmp_objdir_migrate(t))
+ *      strvec_pushl(&cmd.env, tmp_objdir_env(t));
+ *	if (!run_command(&cmd) && !tmp_objdir_migrate(t))
  *		printf("success!\n");
  *	else
  *		die("failed...tmp_objdir will clean up for us");
-- 
2.38.0.1092.g8c0298861b0

[PATCH 08/10] run-command API & users: remove run_command_v_opt_tr2()

[Ævar Arnfjörð Bjarmason]

As noted in the preceding commit for run_command_v_opt_cd_env() that
function's users could more easily use the underlying run_command()
directly.

In the case of the "git.c" user that can be argued the other way,
given the slight line count increase here, but part of that's because
the "args" in "git.c" was being leaked, which we now don't have to
worry about.

That just leaves the spawn_daemon() user in "fsmonitor-ipc.c", it's
likewise slightly more verbose as a result, but by making it use
run_command() we can remove this entire part of the API. As noted in a
preceding commit run_command_v_opt*() should be aimed at handling
various common cases, not these one-offs.

The "fsmonitor-ipc.c" caller would be nicer with a hypothetical
run_command_l_opt_tr2(), but let's not maintain such a thing only for
it, as it would be its only user.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 fsmonitor-ipc.c | 10 +++++++---
 git.c           | 15 +++++++++------
 run-command.c   | 28 ++--------------------------
 run-command.h   | 19 +++----------------
 4 files changed, 21 insertions(+), 51 deletions(-)

diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index 789e7397baa..7251f48e456 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -56,10 +56,14 @@ enum ipc_active_state fsmonitor_ipc__get_state(void)
 
 static int spawn_daemon(void)
 {
-	const char *args[] = { "fsmonitor--daemon", "start", NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
-	return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
-				    "fsmonitor");
+	cmd.git_cmd = 1;
+	cmd.no_stdin = 1;
+	cmd.trace2_child_class = "fsmonitor";
+	strvec_pushl(&cmd.args, "fsmonitor--daemon", "start", NULL);
+
+	return run_command(&cmd);
 }
 
 int fsmonitor_ipc__send_query(const char *since_token,
diff --git a/git.c b/git.c
index da411c53822..ccf444417b5 100644
--- a/git.c
+++ b/git.c
@@ -787,7 +787,7 @@ static int run_argv(int *argcp, const char ***argv)
 		if (!done_alias)
 			handle_builtin(*argcp, *argv);
 		else if (get_builtin(**argv)) {
-			struct strvec args = STRVEC_INIT;
+			struct child_process cmd = CHILD_PROCESS_INIT;
 			int i;
 
 			/*
@@ -804,18 +804,21 @@ static int run_argv(int *argcp, const char ***argv)
 
 			commit_pager_choice();
 
-			strvec_push(&args, "git");
+			strvec_push(&cmd.args, "git");
 			for (i = 0; i < *argcp; i++)
-				strvec_push(&args, (*argv)[i]);
+				strvec_push(&cmd.args, (*argv)[i]);
 
-			trace_argv_printf(args.v, "trace: exec:");
+			trace_argv_printf(cmd.args.v, "trace: exec:");
 
 			/*
 			 * if we fail because the command is not found, it is
 			 * OK to return. Otherwise, we just pass along the status code.
 			 */
-			i = run_command_v_opt_tr2(args.v, RUN_SILENT_EXEC_FAILURE |
-						  RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, "git_alias");
+			cmd.silent_exec_failure = 1;
+			cmd.clean_on_exit = 1;
+			cmd.wait_after_clean = 1;
+			cmd.trace2_child_class = "git_alias";
+			i = run_command(&cmd);
 			if (i >= 0 || errno != ENOENT)
 				exit(i);
 			die("could not execute builtin %s", **argv);
diff --git a/run-command.c b/run-command.c
index 6132a9f19f0..0066ace85fa 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1010,7 +1010,6 @@ static void run_command_set_opts(struct child_process *cmd, int opt)
 	cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
 	cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
 	cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
-	cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
 	cmd->wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
 	cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
 }
@@ -1031,35 +1030,12 @@ int run_command_l_opt(int opt, ...)
 }
 
 int run_command_v_opt(const char **argv, int opt)
-{
-	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);
-}
-
-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)
-{
-	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);
-}
-
-static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,
-					  const char *dir,
-					  const char *const *env,
-					  const char *tr2_class)
-{
-	run_command_set_opts(cmd, opt);
-	cmd->dir = dir;
-	if (env)
-		strvec_pushv(&cmd->env, (const char **)env);
-	cmd->trace2_child_class = tr2_class;
-	return run_command(cmd);
-}
-
-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
-				 const char *const *env, const char *tr2_class)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 
 	strvec_pushv(&cmd.args, argv);
-	return run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class);
+	run_command_set_opts(&cmd, opt);
+	return run_command(&cmd);
 }
 
 #ifndef NO_PTHREADS
diff --git a/run-command.h b/run-command.h
index 2574d46cb70..2b1fe3cde5c 100644
--- a/run-command.h
+++ b/run-command.h
@@ -228,12 +228,11 @@ int run_auto_maintenance(int quiet);
 #define RUN_GIT_CMD			(1<<1)
 #define RUN_SILENT_EXEC_FAILURE		(1<<2)
 #define RUN_USING_SHELL			(1<<3)
-#define RUN_CLEAN_ON_EXIT		(1<<4)
-#define RUN_WAIT_AFTER_CLEAN		(1<<5)
-#define RUN_CLOSE_OBJECT_STORE		(1<<6)
+#define RUN_WAIT_AFTER_CLEAN		(1<<4)
+#define RUN_CLOSE_OBJECT_STORE		(1<<5)
 
 /**
- * The run_command_v_opt*() API is a convenience wrapper for an
+ * The run_command_v_opt() function is a convenience wrapper for an
  * underlying run_command().
  *
  * It's intended to be used when the user already has an "argv" they'd
@@ -250,21 +249,9 @@ int run_auto_maintenance(int quiet);
  *	- RUN_GIT_CMD: .git_cmd
  *	- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure
  *	- RUN_USING_SHELL: .use_shell
- *	- RUN_CLEAN_ON_EXIT: .clean_on_exit
- *	- RUN_WAIT_AFTER_CLEAN: .wait_after_clean
  *	- RUN_CLOSE_OBJECT_STORE: .close_object_store
- *
- * The argument dir corresponds the member .dir. The argument env
- * corresponds to the member .env.
  */
 int run_command_v_opt(const char **argv, int opt);
-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);
-/*
- * env (the environment) is to be formatted like environ: "VAR=VALUE".
- * To unset an environment variable use just "VAR".
- */
-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
-				 const char *const *env, const char *tr2_class);
 
 /**
  * The run_command_l_opt() function run_command_v_opt() takes a list
-- 
2.38.0.1092.g8c0298861b0

[PATCH 09/10] gc: use strvec_pushf(), avoid redundant strbuf_detach()

[Ævar Arnfjörð Bjarmason]

Adjust code added in 3797a0a7b7a (maintenance: use Windows scheduled
tasks, 2021-01-05) to use strvec_pushf() directly. Rather than having
a function that returns a strbuf_detach() we need to free and have the
"strvec_pushl()" do its own "xstrdup()" we can format this in-place.

By changing this we only have the "strvec_clear()" between the
"run_command_v_opt()" and the "return result" in
"schtasks_remove_task()". In the subsequent commit we'll start using a
helper which'll allow us to skip the "strvec_clear()".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 075b4637660..519e64e86ee 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1894,12 +1894,7 @@ static int is_schtasks_available(void)
 #endif
 }
 
-static char *schtasks_task_name(const char *frequency)
-{
-	struct strbuf label = STRBUF_INIT;
-	strbuf_addf(&label, "Git Maintenance (%s)", frequency);
-	return strbuf_detach(&label, NULL);
-}
+#define SCHTASKS_NAME_FMT "Git Maintenance (%s)"
 
 static int schtasks_remove_task(enum schedule_priority schedule)
 {
@@ -1907,16 +1902,15 @@ static int schtasks_remove_task(enum schedule_priority schedule)
 	int result;
 	struct strvec args = STRVEC_INIT;
 	const char *frequency = get_frequency(schedule);
-	char *name = schtasks_task_name(frequency);
 
 	get_schedule_cmd(&cmd, NULL);
 	strvec_split(&args, cmd);
-	strvec_pushl(&args, "/delete", "/tn", name, "/f", NULL);
+	strvec_pushl(&args, "/delete", "/tn", NULL);
+	strvec_pushf(&args, SCHTASKS_NAME_FMT, frequency);
+	strvec_pushl(&args, "/f", NULL);
 
 	result = run_command_v_opt(args.v, 0);
-
 	strvec_clear(&args);
-	free(name);
 	return result;
 }
 
@@ -1935,7 +1929,6 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	const char *xml;
 	struct tempfile *tfile;
 	const char *frequency = get_frequency(schedule);
-	char *name = schtasks_task_name(frequency);
 	struct strbuf tfilename = STRBUF_INIT;
 
 	get_schedule_cmd(&cmd, NULL);
@@ -2028,8 +2021,10 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	      "</Task>\n";
 	fprintf(tfile->fp, xml, exec_path, exec_path, frequency);
 	strvec_split(&child.args, cmd);
-	strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml",
-				  get_tempfile_path(tfile), NULL);
+	strvec_pushl(&child.args, "/create", "/tn", NULL);
+	strvec_pushf(&child.args, SCHTASKS_NAME_FMT, frequency);
+	strvec_pushl(&child.args, "/f", "/xml",
+		     get_tempfile_path(tfile), NULL);
 	close_tempfile_gently(tfile);
 
 	child.no_stdout = 1;
@@ -2040,7 +2035,6 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	result = finish_command(&child);
 
 	delete_tempfile(&tfile);
-	free(name);
 	return result;
 }
 
-- 
2.38.0.1092.g8c0298861b0

[PATCH 10/10] run-command API: add and use a run_command_sv_opt()

[Ævar Arnfjörð Bjarmason]

Add a run_command_sv_opt() convenience wrapper for
run_command_v_opt(), as noted in the API documentation this is for the
common case of wanting to construct a "struct strvec" to pass to
run_command_v_opt(), and as it's a one-shot to strvec_clear() it
afterwards.

Let's convert those API users that were using a "ret" variable to
carry over to "return" after a "strvec_clear()" to use this new
function instead.

Let's leave aside the user in "builtin/bisect--helper.c"'s
bisect_visualize(). There's an outstanding topic that's extensively
modifying it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 add-interactive.c |  3 +--
 builtin/add.c     |  6 ++----
 builtin/clone.c   |  3 +--
 builtin/gc.c      |  5 +----
 builtin/pull.c    | 15 +++------------
 builtin/remote.c  |  5 +----
 merge.c           |  3 +--
 run-command.h     | 20 +++++++++++++++++++-
 scalar.c          |  6 +-----
 sequencer.c       | 11 ++---------
 10 files changed, 32 insertions(+), 45 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index f071b2a1b4f..9c86f3b9532 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -1007,8 +1007,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
 			if (files->selected[i])
 				strvec_push(&args,
 					    files->items.items[i].string);
-		res = run_command_v_opt(args.v, 0);
-		strvec_clear(&args);
+		res = run_command_sv_opt(&args, 0);
 	}
 
 	putchar('\n');
diff --git a/builtin/add.c b/builtin/add.c
index f84372964c8..7c783eebc0e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -240,7 +240,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
 int run_add_interactive(const char *revision, const char *patch_mode,
 			const struct pathspec *pathspec)
 {
-	int status, i;
+	int i;
 	struct strvec argv = STRVEC_INIT;
 	int use_builtin_add_i =
 		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
@@ -282,9 +282,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 		/* pass original pathspec, to be re-parsed */
 		strvec_push(&argv, pathspec->items[i].original);
 
-	status = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-	return status;
+	return run_command_sv_opt(&argv, RUN_GIT_CMD);
 }
 
 int interactive_add(const char **argv, const char *prefix, int patch)
diff --git a/builtin/clone.c b/builtin/clone.c
index 13fc7a4bc5d..eabf8e0f196 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -756,8 +756,7 @@ static int checkout(int submodule_progress, int filter_submodules)
 					       "--single-branch" :
 					       "--no-single-branch");
 
-		err = run_command_v_opt(args.v, RUN_GIT_CMD);
-		strvec_clear(&args);
+		return run_command_sv_opt(&args, RUN_GIT_CMD);
 	}
 
 	return err;
diff --git a/builtin/gc.c b/builtin/gc.c
index 519e64e86ee..8393e19b504 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1899,7 +1899,6 @@ static int is_schtasks_available(void)
 static int schtasks_remove_task(enum schedule_priority schedule)
 {
 	const char *cmd = "schtasks";
-	int result;
 	struct strvec args = STRVEC_INIT;
 	const char *frequency = get_frequency(schedule);
 
@@ -1909,9 +1908,7 @@ static int schtasks_remove_task(enum schedule_priority schedule)
 	strvec_pushf(&args, SCHTASKS_NAME_FMT, frequency);
 	strvec_pushl(&args, "/f", NULL);
 
-	result = run_command_v_opt(args.v, 0);
-	strvec_clear(&args);
-	return result;
+	return run_command_sv_opt(&args, 0);
 }
 
 static int schtasks_remove_tasks(void)
diff --git a/builtin/pull.c b/builtin/pull.c
index 403a24d7ca6..2f36823c97e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -516,7 +516,6 @@ static void parse_repo_refspecs(int argc, const char **argv, const char **repo,
 static int run_fetch(const char *repo, const char **refspecs)
 {
 	struct strvec args = STRVEC_INIT;
-	int ret;
 
 	strvec_pushl(&args, "fetch", "--update-head-ok", NULL);
 
@@ -582,9 +581,7 @@ static int run_fetch(const char *repo, const char **refspecs)
 		strvec_pushv(&args, refspecs);
 	} else if (*refspecs)
 		BUG("refspecs without repo?");
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);
-	strvec_clear(&args);
-	return ret;
+	return run_command_sv_opt(&args, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);
 }
 
 /**
@@ -653,7 +650,6 @@ static int update_submodules(void)
  */
 static int run_merge(void)
 {
-	int ret;
 	struct strvec args = STRVEC_INIT;
 
 	strvec_pushl(&args, "merge", NULL);
@@ -696,9 +692,7 @@ static int run_merge(void)
 		strvec_push(&args, "--allow-unrelated-histories");
 
 	strvec_push(&args, "FETCH_HEAD");
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
-	return ret;
+	return run_command_sv_opt(&args, RUN_GIT_CMD);
 }
 
 /**
@@ -879,7 +873,6 @@ static int get_rebase_newbase_and_upstream(struct object_id *newbase,
 static int run_rebase(const struct object_id *newbase,
 		const struct object_id *upstream)
 {
-	int ret;
 	struct strvec args = STRVEC_INIT;
 
 	strvec_push(&args, "rebase");
@@ -913,9 +906,7 @@ static int run_rebase(const struct object_id *newbase,
 
 	strvec_push(&args, oid_to_hex(upstream));
 
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
-	return ret;
+	return run_command_sv_opt(&args, RUN_GIT_CMD);
 }
 
 static int get_can_ff(struct object_id *orig_head,
diff --git a/builtin/remote.c b/builtin/remote.c
index 1d86c14297b..07de21a624e 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1508,7 +1508,6 @@ static int update(int argc, const char **argv, const char *prefix)
 	};
 	struct strvec fetch_argv = STRVEC_INIT;
 	int default_defined = 0;
-	int retval;
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_remote_update_usage,
@@ -1534,9 +1533,7 @@ static int update(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	retval = run_command_v_opt(fetch_argv.v, RUN_GIT_CMD);
-	strvec_clear(&fetch_argv);
-	return retval;
+	return run_command_sv_opt(&fetch_argv, RUN_GIT_CMD);
 }
 
 static int remove_all_fetch_refspecs(const char *key)
diff --git a/merge.c b/merge.c
index 2382ff66d35..487debacecb 100644
--- a/merge.c
+++ b/merge.c
@@ -33,8 +33,7 @@ int try_merge_command(struct repository *r,
 	for (j = remotes; j; j = j->next)
 		strvec_push(&args, merge_argument(j->item));
 
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
+	ret = run_command_sv_opt(&args, RUN_GIT_CMD);
 
 	discard_index(r->index);
 	if (repo_read_index(r) < 0)
diff --git a/run-command.h b/run-command.h
index 2b1fe3cde5c..639cee4f4fb 100644
--- a/run-command.h
+++ b/run-command.h
@@ -151,7 +151,7 @@ struct child_process {
 
 /**
  * The functions: child_process_init, start_command, finish_command,
- * run_command, run_command_l_opt, run_command_v_opt,
+ * run_command, run_command_l_opt, run_command_v_opt, run_command_sv_opt,
  * child_process_clear do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
@@ -262,6 +262,24 @@ int run_command_v_opt(const char **argv, int opt);
 LAST_ARG_MUST_BE_NULL
 int run_command_l_opt(int opt, ...);
 
+/**
+ * The run_command_sv_opt() function is a wrapper for
+ * run_command_v_opt(). It takes a "struct strvec *args" which
+ * similarly to run_command() (but not run_command_sv_opt()) will be
+ * strvec_clear()'d before returning.
+ *
+ * Use it for the common case of constructing a "struct strvec" for a
+ * one-shot run_command_v_opt() invocation.
+ */
+RESULT_MUST_BE_USED
+static inline int run_command_sv_opt(struct strvec *args, int opt)
+{
+	int ret = run_command_v_opt(args->v, opt);
+
+	strvec_clear(args);
+	return ret;
+}
+
 /**
  * Execute the given command, sending "in" to its stdin, and capturing its
  * stdout and stderr in the "out" and "err" strbufs. Any of the three may
diff --git a/scalar.c b/scalar.c
index 6de9c0ee523..3480bf73cbd 100644
--- a/scalar.c
+++ b/scalar.c
@@ -72,7 +72,6 @@ static int run_git(const char *arg, ...)
 	struct strvec argv = STRVEC_INIT;
 	va_list args;
 	const char *p;
-	int res;
 
 	va_start(args, arg);
 	strvec_push(&argv, arg);
@@ -80,10 +79,7 @@ static int run_git(const char *arg, ...)
 		strvec_push(&argv, p);
 	va_end(args);
 
-	res = run_command_v_opt(argv.v, RUN_GIT_CMD);
-
-	strvec_clear(&argv);
-	return res;
+	return run_command_sv_opt(&argv, RUN_GIT_CMD);
 }
 
 struct scalar_config {
diff --git a/sequencer.c b/sequencer.c
index 20495db9de2..7ee0e05512c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3183,7 +3183,6 @@ static int rollback_is_safe(void)
 
 static int reset_merge(const struct object_id *oid)
 {
-	int ret;
 	struct strvec argv = STRVEC_INIT;
 
 	strvec_pushl(&argv, "reset", "--merge", NULL);
@@ -3191,10 +3190,7 @@ static int reset_merge(const struct object_id *oid)
 	if (!is_null_oid(oid))
 		strvec_push(&argv, oid_to_hex(oid));
 
-	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-
-	return ret;
+	return run_command_sv_opt(&argv, RUN_GIT_CMD);
 }
 
 static int rollback_single_pick(struct repository *r)
@@ -4866,7 +4862,6 @@ static int pick_commits(struct repository *r,
 static int continue_single_pick(struct repository *r, struct replay_opts *opts)
 {
 	struct strvec argv = STRVEC_INIT;
-	int ret;
 
 	if (!refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
 	    !refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD"))
@@ -4887,9 +4882,7 @@ static int continue_single_pick(struct repository *r, struct replay_opts *opts)
 		 */
 		strvec_pushl(&argv, "--no-edit", "--cleanup=strip", NULL);
 
-	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-	return ret;
+	return run_command_sv_opt(&argv, RUN_GIT_CMD);
 }
 
 static int commit_staged_changes(struct repository *r,
-- 
2.38.0.1092.g8c0298861b0

Re: [PATCH 10/10] run-command API: add and use a run_command_sv_opt()

[René Scharfe]

Am 14.10.22 um 17:40 schrieb Ævar Arnfjörð Bjarmason:
> Add a run_command_sv_opt() convenience wrapper for
> run_command_v_opt(), as noted in the API documentation this is for the
> common case of wanting to construct a "struct strvec" to pass to
> run_command_v_opt(), and as it's a one-shot to strvec_clear() it
> afterwards.

Interesting idea.  It wastes memory by allocating the argument vector
twice, but for most call-sites this won't be noticeable.  I suspect it
might fit at least some of the use cases for run_command_l_opt() better,
e.g. those that previously used a strvec or that need some flexibility.

> diff --git a/run-command.h b/run-command.h
> index 2b1fe3cde5c..639cee4f4fb 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -151,7 +151,7 @@ struct child_process {
>
>  /**
>   * The functions: child_process_init, start_command, finish_command,
> - * run_command, run_command_l_opt, run_command_v_opt,
> + * run_command, run_command_l_opt, run_command_v_opt, run_command_sv_opt,
>   * child_process_clear do the following:
>   *
>   * - If a system call failed, errno is set and -1 is returned. A diagnostic
> @@ -262,6 +262,24 @@ int run_command_v_opt(const char **argv, int opt);
>  LAST_ARG_MUST_BE_NULL
>  int run_command_l_opt(int opt, ...);
>
> +/**
> + * The run_command_sv_opt() function is a wrapper for
> + * run_command_v_opt(). It takes a "struct strvec *args" which
> + * similarly to run_command() (but not run_command_sv_opt()) will be
                                          ^^^^^^^^^^^^^^^^^^^^
Do you mean run_command_v_opt()?

René

[PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[Ævar Arnfjörð Bjarmason]

This series provides a more idiomatic set of run-command API helpers
to match our current use-cases for run_command_v_opt(). See v1[1] for
a more general overview.

Changes since v1:

 * Fixed a migration bug in builtin/remote.c noted by Junio (just
   skipping that case).

 * Fixed an indentation issue noted by Jeff.

 * Changed run_command_sv_opt() so that we take full advantage of
   having the "struct strvec *", and move it to "cmd.args", rather
   than copying its contents.

 * Rewrote how 1/10 uses the "opts" helper, in response to Junio's
   comment.

 * Small commit message touch-ups.

1. https://lore.kernel.org/git/cover-00.10-00000000000-20221014T153426Z-avarab@gmail.com/

CI & branch for this topic available at
https://github.com/avar/git/tree/avar/run-command-wrapper-API-simplification-2

Ævar Arnfjörð Bjarmason (10):
  run-command.c: refactor run_command_*_tr2() to internal helpers
  merge: remove always-the-same "verbose" arguments
  run-command API: add and use a run_command_l_opt()
  am: use run_command_l_opt() for show_patch()
  run-command API docs: clarify & fleshen out run_command_v_opt*() docs
  run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag
  run-command API & diff.c: remove run_command_v_opt_cd_env()
  run-command API & users: remove run_command_v_opt_tr2()
  gc: use strvec_pushf(), avoid redundant strbuf_detach()
  run-command API: add and use a run_command_sv_opt()

 add-interactive.c        |  3 +-
 bisect.c                 | 19 ++++++-----
 builtin/add.c            |  6 ++--
 builtin/am.c             | 14 +++-----
 builtin/clone.c          | 19 ++++-------
 builtin/difftool.c       | 14 ++++----
 builtin/gc.c             | 49 ++++++++++------------------
 builtin/merge.c          | 46 ++++++--------------------
 builtin/pull.c           | 15 ++-------
 builtin/remote.c         |  5 +--
 compat/mingw.c           |  8 ++---
 diff.c                   | 26 +++++++--------
 fsmonitor-ipc.c          | 10 ++++--
 git.c                    | 15 +++++----
 ll-merge.c               |  4 +--
 merge.c                  |  3 +-
 run-command.c            | 52 +++++++++++++++++------------
 run-command.h            | 70 ++++++++++++++++++++++++++--------------
 scalar.c                 |  6 +---
 sequencer.c              | 15 ++-------
 t/helper/test-fake-ssh.c |  4 +--
 tmp-objdir.h             |  6 ++--
 22 files changed, 179 insertions(+), 230 deletions(-)

Range-diff against v1:
 1:  c1f701af6e8 !  1:  3842204371e run-command.c: refactor run_command_*_tr2() to internal helpers
    @@ run-command.c: int run_command(struct child_process *cmd)
     +	cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
     +}
     +
    - int run_command_v_opt(const char **argv, int opt)
    - {
    - 	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
    -@@ run-command.c: int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
    - 	return run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);
    - }
    - 
    -+static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,
    -+					  const char *dir,
    -+					  const char *const *env,
    -+					  const char *tr2_class)
    ++static int run_command_v_opt_1(struct child_process *cmd, int opt)
     +{
     +	run_command_set_opts(cmd, opt);
    -+	cmd->dir = dir;
    -+	if (env)
    -+		strvec_pushv(&cmd->env, (const char **)env);
    -+	cmd->trace2_child_class = tr2_class;
     +	return run_command(cmd);
     +}
     +
    - int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
    - 				 const char *const *env, const char *tr2_class)
    + int run_command_v_opt(const char **argv, int opt)
    + {
    + 	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
    +@@ run-command.c: int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
      {
      	struct child_process cmd = CHILD_PROCESS_INIT;
    -+
      	strvec_pushv(&cmd.args, argv);
     -	cmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
     -	cmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
    @@ run-command.c: int run_command_v_opt_cd_env(const char **argv, int opt, const ch
     -	cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
     -	cmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
     -	cmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
    --	cmd.dir = dir;
    --	if (env)
    --		strvec_pushv(&cmd.env, (const char **)env);
    --	cmd.trace2_child_class = tr2_class;
    + 	cmd.dir = dir;
    + 	if (env)
    + 		strvec_pushv(&cmd.env, (const char **)env);
    + 	cmd.trace2_child_class = tr2_class;
     -	return run_command(&cmd);
    -+	return run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class);
    ++	return run_command_v_opt_1(&cmd, opt);
      }
      
      #ifndef NO_PTHREADS
 2:  543ccbb1ee1 =  2:  8b00172ef83 merge: remove always-the-same "verbose" arguments
 3:  fd81d44f221 !  3:  680a42a878e run-command API: add and use a run_command_l_opt()
    @@ builtin/clone.c: static void write_refspec_config(const char *src_ref_prefix,
      	if (!access(alternates, F_OK)) {
     -		if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))
     +		if (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,
    -+				      "repack",  "-a", "-d", NULL))
    ++				      "repack", "-a", "-d", NULL))
      			die(_("cannot repack to clean up"));
      		if (unlink(alternates) && errno != ENOENT)
      			die_errno(_("cannot unlink temporary alternates file"));
    @@ builtin/merge.c: static int save_state(struct object_id *stash)
      refresh_cache:
      	if (discard_cache() < 0 || read_cache() < 0)
     
    - ## builtin/remote.c ##
    -@@ builtin/remote.c: static int verbose;
    - 
    - static int fetch_remote(const char *name)
    - {
    --	const char *argv[] = { "fetch", name, NULL, NULL };
    --	if (verbose) {
    --		argv[1] = "-v";
    --		argv[2] = name;
    --	}
    - 	printf_ln(_("Updating %s"), name);
    --	if (run_command_v_opt(argv, RUN_GIT_CMD))
    -+	if (verbose && run_command_l_opt(RUN_GIT_CMD, "-v", "fetch", name,
    -+					 NULL))
    -+		return error(_("Could not fetch %s"), name);
    -+	else if (run_command_l_opt(RUN_GIT_CMD, "fetch", name, NULL))
    - 		return error(_("Could not fetch %s"), name);
    - 	return 0;
    - }
    -
      ## compat/mingw.c ##
     @@ compat/mingw.c: static int read_yes_no_answer(void)
      static int ask_yes_no_if_possible(const char *format, ...)
    @@ run-command.c: static void run_command_set_opts(struct child_process *cmd, int o
     +	return run_command(&cmd);
     +}
     +
    - int run_command_v_opt(const char **argv, int opt)
    + static int run_command_v_opt_1(struct child_process *cmd, int opt)
      {
    - 	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
    + 	run_command_set_opts(cmd, opt);
     
      ## run-command.h ##
     @@ run-command.h: struct child_process {
 4:  4cd61aaa981 !  4:  5cfd6a94ce3 am: use run_command_l_opt() for show_patch()
    @@ Commit message
         am: use run_command_l_opt() for show_patch()
     
         The "git show" invocation added in 66335298a47 (rebase: add
    -    --show-current-patch, 2018-02-11) is a one-off, and one where we're
    -    not calling oid_to_hex() twice. So we can rely on the static buffer
    -    that oid_to_hex() points to, rather than xstrdup()-ing it. As a result
    -    we can use the run_command_l_opt() function.
    +    --show-current-patch, 2018-02-11) is one where we're not calling
    +    oid_to_hex() twice. So we can rely on the static buffer that
    +    oid_to_hex() points to, rather than xstrdup()-ing it. As a result we
    +    can use the run_command_l_opt() function.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
 5:  b6a3c4c66f8 =  5:  4fca38bb4d6 run-command API docs: clarify & fleshen out run_command_v_opt*() docs
 6:  9d0286fbf64 =  6:  75eccc152ad run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag
 7:  31e8536f28c !  7:  3b3d3777232 run-command API & diff.c: remove run_command_v_opt_cd_env()
    @@ diff.c: static void run_external_diff(const char *pgm,
      static int similarity_index(struct diff_filepair *p)
     
      ## run-command.c ##
    -@@ run-command.c: int run_command_l_opt(int opt, ...)
    +@@ run-command.c: static int run_command_v_opt_1(struct child_process *cmd, int opt)
      
      int run_command_v_opt(const char **argv, int opt)
      {
    @@ run-command.c: int run_command_v_opt_tr2(const char **argv, int opt, const char
     -	return run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);
     -}
     -
    - static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,
    - 					  const char *dir,
    - 					  const char *const *env,
    + int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
    + 				 const char *const *env, const char *tr2_class)
    + {
     
      ## run-command.h ##
     @@ run-command.h: struct child_process {
 8:  88e063f3b05 !  8:  4f1a051823f run-command API & users: remove run_command_v_opt_tr2()
    @@ run-command.c: static void run_command_set_opts(struct child_process *cmd, int o
      	cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
      }
     @@ run-command.c: int run_command_l_opt(int opt, ...)
    + 	return run_command(&cmd);
      }
      
    +-static int run_command_v_opt_1(struct child_process *cmd, int opt)
    +-{
    +-	run_command_set_opts(cmd, opt);
    +-	return run_command(cmd);
    +-}
    +-
      int run_command_v_opt(const char **argv, int opt)
     -{
     -	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);
    @@ run-command.c: int run_command_l_opt(int opt, ...)
     -	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);
     -}
     -
    --static int run_command_v_opt_cd_env_tr2_1(struct child_process *cmd, int opt,
    --					  const char *dir,
    --					  const char *const *env,
    --					  const char *tr2_class)
    --{
    --	run_command_set_opts(cmd, opt);
    --	cmd->dir = dir;
    --	if (env)
    --		strvec_pushv(&cmd->env, (const char **)env);
    --	cmd->trace2_child_class = tr2_class;
    --	return run_command(cmd);
    --}
    --
     -int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
     -				 const char *const *env, const char *tr2_class)
      {
      	struct child_process cmd = CHILD_PROCESS_INIT;
    - 
    ++
      	strvec_pushv(&cmd.args, argv);
    --	return run_command_v_opt_cd_env_tr2_1(&cmd, opt, dir, env, tr2_class);
    +-	cmd.dir = dir;
    +-	if (env)
    +-		strvec_pushv(&cmd.env, (const char **)env);
    +-	cmd.trace2_child_class = tr2_class;
    +-	return run_command_v_opt_1(&cmd, opt);
     +	run_command_set_opts(&cmd, opt);
     +	return run_command(&cmd);
      }
 9:  0f5524e40ad =  9:  99c5688797a gc: use strvec_pushf(), avoid redundant strbuf_detach()
10:  874cb72c2f4 ! 10:  138af632a36 run-command API: add and use a run_command_sv_opt()
    @@ Commit message
         carry over to "return" after a "strvec_clear()" to use this new
         function instead.
     
    +    Because we pass the "struct strvec *" to the function we can also
    +    avoid copying the arguments to the "args" member of the "struct
    +    child_process", as we were doing with run_command_v_opt().
    +
    +    Instead we can use memcpy() and strvec_clear() to do the moral
    +    equivalent of a strbuf_{detach,attach}(). The strvec API doesn't have
    +    a strvec_attach(), we could add it here while at it, but let's avoid
    +    generalizing the interface for now and migrate the "struct strvec *"
    +    in the "run_command_sv_opt()" instead.
    +
         Let's leave aside the user in "builtin/bisect--helper.c"'s
         bisect_visualize(). There's an outstanding topic that's extensively
         modifying it.
    @@ merge.c: int try_merge_command(struct repository *r,
      	discard_index(r->index);
      	if (repo_read_index(r) < 0)
     
    + ## run-command.c ##
    +@@ run-command.c: int run_command_v_opt(const char **argv, int opt)
    + 	return run_command(&cmd);
    + }
    + 
    ++int run_command_sv_opt(struct strvec *args, int opt)
    ++{
    ++	struct child_process cmd = CHILD_PROCESS_INIT;
    ++
    ++	/* TODO: We could encapsulate this with a strvec_attach() */
    ++	memcpy(&cmd.args, args, sizeof(*args));
    ++	strvec_init(args);
    ++	run_command_set_opts(&cmd, opt);
    ++	return run_command(&cmd);
    ++}
    ++
    + #ifndef NO_PTHREADS
    + static pthread_t main_thread;
    + static int main_thread_set;
    +
      ## run-command.h ##
     @@ run-command.h: struct child_process {
      
    @@ run-command.h: int run_command_v_opt(const char **argv, int opt);
     +/**
     + * The run_command_sv_opt() function is a wrapper for
     + * run_command_v_opt(). It takes a "struct strvec *args" which
    -+ * similarly to run_command() (but not run_command_sv_opt()) will be
    -+ * strvec_clear()'d before returning.
    ++ * similarly will be strvec_clear()'d before returning.
     + *
     + * Use it for the common case of constructing a "struct strvec" for a
     + * one-shot run_command_v_opt() invocation.
    ++ *
    ++ * The "args" will migrated the "cmd.args" member of an underlying
    ++ * "struct child_process", in a way that avoids making an extra copy.
     + */
     +RESULT_MUST_BE_USED
    -+static inline int run_command_sv_opt(struct strvec *args, int opt)
    -+{
    -+	int ret = run_command_v_opt(args->v, opt);
    -+
    -+	strvec_clear(args);
    -+	return ret;
    -+}
    ++int run_command_sv_opt(struct strvec *args, int opt);
     +
      /**
       * Execute the given command, sending "in" to its stdin, and capturing its
-- 
2.38.0.1091.gf9d18265e59

[PATCH v2 01/10] run-command.c: refactor run_command_*_tr2() to internal helpers

[Ævar Arnfjörð Bjarmason]

Refactor the run_command_v_opt_cd_env_tr2() function to helpers which
takes a "struct child_process *cmd" argument. This will allow for
adding a helper which won't need to copy a set of arguments to the
"cmd.args" we'd otherwise have to populate from the "argv".

Splitting out the part of the function that sets the various members
from "opt" will help in the subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/run-command.c b/run-command.c
index 5ec3a46dccf..4e75f83b5e7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1004,6 +1004,24 @@ int run_command(struct child_process *cmd)
 	return finish_command(cmd);
 }
 
+static void run_command_set_opts(struct child_process *cmd, int opt)
+{
+	cmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
+	cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
+	cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
+	cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
+	cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
+	cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
+	cmd->wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
+	cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
+}
+
+static int run_command_v_opt_1(struct child_process *cmd, int opt)
+{
+	run_command_set_opts(cmd, opt);
+	return run_command(cmd);
+}
+
 int run_command_v_opt(const char **argv, int opt)
 {
 	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
@@ -1024,19 +1042,11 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	strvec_pushv(&cmd.args, argv);
-	cmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
-	cmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
-	cmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
-	cmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
-	cmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;
-	cmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
-	cmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
-	cmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
 	cmd.dir = dir;
 	if (env)
 		strvec_pushv(&cmd.env, (const char **)env);
 	cmd.trace2_child_class = tr2_class;
-	return run_command(&cmd);
+	return run_command_v_opt_1(&cmd, opt);
 }
 
 #ifndef NO_PTHREADS
-- 
2.38.0.1091.gf9d18265e59

[PATCH v2 02/10] merge: remove always-the-same "verbose" arguments

[Ævar Arnfjörð Bjarmason]

Simplify the code that builds the arguments for the "read-tree"
invocation in reset_hard() and read_empty() to remove the "verbose"
parameter.

Before 172b6428d06 (do not overwrite untracked during merge from
unborn branch, 2010-11-14) there was a "reset_hard()" function that
would be called in two places, one of those passed a "verbose=1", the
other a "verbose=0".

After 172b6428d06 when read_empty() was split off from reset_hard()
both of these functions only had one caller. The "verbose" in
read_empty() would always be false, and the one in reset_hard() would
always be true.

There was never a good reason for the code to act this way, it
happened because the read_empty() function was a copy/pasted and
adjusted version of reset_hard().

Since we're no longer conditionally adding the "-v" parameter
here (and we'd only add it for "reset_hard()" we'll be able to move to
a simpler and safer run-command API in the subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/merge.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 5900b81729d..3bb49d805b4 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -345,14 +345,12 @@ static int save_state(struct object_id *stash)
 	return rc;
 }
 
-static void read_empty(const struct object_id *oid, int verbose)
+static void read_empty(const struct object_id *oid)
 {
 	int i = 0;
 	const char *args[7];
 
 	args[i++] = "read-tree";
-	if (verbose)
-		args[i++] = "-v";
 	args[i++] = "-m";
 	args[i++] = "-u";
 	args[i++] = empty_tree_oid_hex();
@@ -363,14 +361,13 @@ static void read_empty(const struct object_id *oid, int verbose)
 		die(_("read-tree failed"));
 }
 
-static void reset_hard(const struct object_id *oid, int verbose)
+static void reset_hard(const struct object_id *oid)
 {
 	int i = 0;
 	const char *args[6];
 
 	args[i++] = "read-tree";
-	if (verbose)
-		args[i++] = "-v";
+	args[i++] = "-v";
 	args[i++] = "--reset";
 	args[i++] = "-u";
 	args[i++] = oid_to_hex(oid);
@@ -385,7 +382,7 @@ static void restore_state(const struct object_id *head,
 {
 	struct strvec args = STRVEC_INIT;
 
-	reset_hard(head, 1);
+	reset_hard(head);
 
 	if (is_null_oid(stash))
 		goto refresh_cache;
@@ -1470,7 +1467,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 					       check_trust_level);
 
 		remote_head_oid = &remoteheads->item->object.oid;
-		read_empty(remote_head_oid, 0);
+		read_empty(remote_head_oid);
 		update_ref("initial pull", "HEAD", remote_head_oid, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
 		goto done;
-- 
2.38.0.1091.gf9d18265e59

[PATCH v2 03/10] run-command API: add and use a run_command_l_opt()

[Ævar Arnfjörð Bjarmason]

When run_command() is used with strvec_pushl() as in the pre-image of
b004c902827 (gc: simplify maintenance_task_pack_refs(), 2022-10-04) we
get the benefit of the LAST_ARG_MUST_BE_NULL. See 9fe3edc47f1 (Add the
LAST_ARG_MUST_BE_NULL macro, 2013-07-18).

Let's provide and use a run_command_l_opt() function, which gives us
the best of both worlds. We'll now need less code to accomplish the
same thing, and this version's safer as we can assert that the
terminating NULL is present.

The "builtin/bisect--helper.c" would be a large beneficiary of this
API, with "15 insertions(+), 26 deletions(-)", but let's leave that
aside for now, as there's an outstanding topic that's extensively
modifying it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 bisect.c                 | 19 +++++++++----------
 builtin/clone.c          | 16 ++++++----------
 builtin/difftool.c       | 14 ++++++--------
 builtin/gc.c             | 22 ++++++++--------------
 builtin/merge.c          | 35 ++++++-----------------------------
 compat/mingw.c           |  8 +++-----
 ll-merge.c               |  4 +---
 run-command.c            | 15 +++++++++++++++
 run-command.h            | 13 +++++++++++--
 sequencer.c              |  4 +---
 t/helper/test-fake-ssh.c |  4 +---
 11 files changed, 67 insertions(+), 87 deletions(-)

diff --git a/bisect.c b/bisect.c
index fd581b85a72..415b0e7b0b2 100644
--- a/bisect.c
+++ b/bisect.c
@@ -738,18 +738,17 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
 	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 
 	argv_checkout[2] = bisect_rev_hex;
-	if (no_checkout) {
+	if (no_checkout)
 		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
-	} else {
-		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
-			/*
-			 * Errors in `run_command()` itself, signaled by res < 0,
-			 * and errors in the child process, signaled by res > 0
-			 * can both be treated as regular BISECT_FAILED (-1).
-			 */
-			return BISECT_FAILED;
-	}
+	else if (run_command_l_opt(RUN_GIT_CMD, "checkout", "-q",
+				   bisect_rev_hex, "--", NULL))
+		/*
+		 * Errors in `run_command()` itself, signaled by res < 0,
+		 * and errors in the child process, signaled by res > 0
+		 * can both be treated as regular BISECT_FAILED (-1).
+		 */
+		return BISECT_FAILED;
 
 	commit = lookup_commit_reference(the_repository, bisect_rev);
 	format_commit_message(commit, "[%H] %s%n", &commit_msg, &pp);
diff --git a/builtin/clone.c b/builtin/clone.c
index ed8d44bb6ab..a1514363190 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -651,23 +651,19 @@ static void update_head(const struct ref *our, const struct ref *remote,
 
 static int git_sparse_checkout_init(const char *repo)
 {
-	struct strvec argv = STRVEC_INIT;
-	int result = 0;
-	strvec_pushl(&argv, "-C", repo, "sparse-checkout", "set", NULL);
-
 	/*
 	 * We must apply the setting in the current process
 	 * for the later checkout to use the sparse-checkout file.
 	 */
 	core_apply_sparse_checkout = 1;
 
-	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+	if (run_command_l_opt(RUN_GIT_CMD, "-C", repo, "sparse-checkout",
+			      "set", NULL)) {
 		error(_("failed to initialize sparse-checkout"));
-		result = 1;
+		return 1;
 	}
 
-	strvec_clear(&argv);
-	return result;
+	return 0;
 }
 
 static int checkout(int submodule_progress, int filter_submodules)
@@ -862,11 +858,11 @@ static void write_refspec_config(const char *src_ref_prefix,
 
 static void dissociate_from_references(void)
 {
-	static const char* argv[] = { "repack", "-a", "-d", NULL };
 	char *alternates = git_pathdup("objects/info/alternates");
 
 	if (!access(alternates, F_OK)) {
-		if (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))
+		if (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,
+				      "repack", "-a", "-d", NULL))
 			die(_("cannot repack to clean up"));
 		if (unlink(alternates) && errno != ENOENT)
 			die_errno(_("cannot unlink temporary alternates file"));
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4b10ad1a369..ed211a87322 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -44,8 +44,8 @@ static int difftool_config(const char *var, const char *value, void *cb)
 
 static int print_tool_help(void)
 {
-	const char *argv[] = { "mergetool", "--tool-help=diff", NULL };
-	return run_command_v_opt(argv, RUN_GIT_CMD);
+	return run_command_l_opt(RUN_GIT_CMD, "mergetool", "--tool-help=diff",
+				 NULL);
 }
 
 static int parse_index_info(char *p, int *mode1, int *mode2,
@@ -361,7 +361,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct index_state wtindex;
 	struct checkout lstate, rstate;
 	int flags = RUN_GIT_CMD, err = 0;
-	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
+	const char *helper_command = "difftool--helper";
 	struct hashmap wt_modified, tmp_modified;
 	int indices_loaded = 0;
 
@@ -563,16 +563,14 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	}
 
 	strbuf_setlen(&ldir, ldir_len);
-	helper_argv[1] = ldir.buf;
 	strbuf_setlen(&rdir, rdir_len);
-	helper_argv[2] = rdir.buf;
-
 	if (extcmd) {
-		helper_argv[0] = extcmd;
+		helper_command = extcmd;
 		flags = 0;
 	} else
 		setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
-	ret = run_command_v_opt(helper_argv, flags);
+	ret = run_command_l_opt(flags, helper_command, ldir.buf, rdir.buf,
+				NULL);
 
 	/* TODO: audit for interaction with sparse-index. */
 	ensure_full_index(&wtindex);
diff --git a/builtin/gc.c b/builtin/gc.c
index 243ee85d283..075b4637660 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -58,8 +58,6 @@ static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;
 static struct strvec reflog = STRVEC_INIT;
 static struct strvec repack = STRVEC_INIT;
 static struct strvec prune = STRVEC_INIT;
-static struct strvec prune_worktrees = STRVEC_INIT;
-static struct strvec rerere = STRVEC_INIT;
 
 static struct tempfile *pidfile;
 static struct lock_file log_lock;
@@ -167,9 +165,8 @@ static void gc_config(void)
 struct maintenance_run_opts;
 static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
 {
-	const char *argv[] = { "pack-refs", "--all", "--prune", NULL };
-
-	return run_command_v_opt(argv, RUN_GIT_CMD);
+	return run_command_l_opt(RUN_GIT_CMD, "pack-refs", "--all", "--prune",
+				 NULL);
 }
 
 static int too_many_loose_objects(void)
@@ -574,8 +571,6 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	strvec_pushl(&reflog, "reflog", "expire", "--all", NULL);
 	strvec_pushl(&repack, "repack", "-d", "-l", NULL);
 	strvec_pushl(&prune, "prune", "--expire", NULL);
-	strvec_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL);
-	strvec_pushl(&rerere, "rerere", "gc", NULL);
 
 	/* default expiry time, overwritten in gc_config */
 	gc_config();
@@ -688,14 +683,13 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	if (prune_worktrees_expire) {
-		strvec_push(&prune_worktrees, prune_worktrees_expire);
-		if (run_command_v_opt(prune_worktrees.v, RUN_GIT_CMD))
-			die(FAILED_RUN, prune_worktrees.v[0]);
-	}
+	if (prune_worktrees_expire &&
+	    run_command_l_opt(RUN_GIT_CMD, "worktree", "prune", "--expire",
+			      prune_worktrees_expire, NULL))
+		die(FAILED_RUN, "worktree");
 
-	if (run_command_v_opt(rerere.v, RUN_GIT_CMD))
-		die(FAILED_RUN, rerere.v[0]);
+	if (run_command_l_opt(RUN_GIT_CMD, "rerere", "gc", NULL))
+		die(FAILED_RUN, "rerere");
 
 	report_garbage = report_pack_garbage;
 	reprepare_packed_git(the_repository);
diff --git a/builtin/merge.c b/builtin/merge.c
index 3bb49d805b4..9c04b588f68 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -347,55 +347,32 @@ static int save_state(struct object_id *stash)
 
 static void read_empty(const struct object_id *oid)
 {
-	int i = 0;
-	const char *args[7];
-
-	args[i++] = "read-tree";
-	args[i++] = "-m";
-	args[i++] = "-u";
-	args[i++] = empty_tree_oid_hex();
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
-
-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	if (run_command_l_opt(RUN_GIT_CMD, "read-tree", "-m", "-u",
+			      empty_tree_oid_hex(), oid_to_hex(oid), NULL))
 		die(_("read-tree failed"));
 }
 
 static void reset_hard(const struct object_id *oid)
 {
-	int i = 0;
-	const char *args[6];
-
-	args[i++] = "read-tree";
-	args[i++] = "-v";
-	args[i++] = "--reset";
-	args[i++] = "-u";
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
-
-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	if (run_command_l_opt(RUN_GIT_CMD, "read-tree", "-v", "--reset", "-u",
+			      oid_to_hex(oid), NULL))
 		die(_("read-tree failed"));
 }
 
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	struct strvec args = STRVEC_INIT;
-
 	reset_hard(head);
 
 	if (is_null_oid(stash))
 		goto refresh_cache;
 
-	strvec_pushl(&args, "stash", "apply", "--index", "--quiet", NULL);
-	strvec_push(&args, oid_to_hex(stash));
-
 	/*
 	 * It is OK to ignore error here, for example when there was
 	 * nothing to restore.
 	 */
-	run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
+	run_command_l_opt(RUN_GIT_CMD, "stash", "apply", "--index", "--quiet",
+			  oid_to_hex(stash), NULL);
 
 refresh_cache:
 	if (discard_cache() < 0 || read_cache() < 0)
diff --git a/compat/mingw.c b/compat/mingw.c
index 901375d5841..4f5392c5796 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -196,17 +196,15 @@ static int read_yes_no_answer(void)
 static int ask_yes_no_if_possible(const char *format, ...)
 {
 	char question[4096];
-	const char *retry_hook[] = { NULL, NULL, NULL };
+	char *retry_hook;
 	va_list args;
 
 	va_start(args, format);
 	vsnprintf(question, sizeof(question), format, args);
 	va_end(args);
 
-	if ((retry_hook[0] = mingw_getenv("GIT_ASK_YESNO"))) {
-		retry_hook[1] = question;
-		return !run_command_v_opt(retry_hook, 0);
-	}
+	if ((retry_hook = mingw_getenv("GIT_ASK_YESNO")))
+		return !run_command_l_opt(0, retry_hook, question, NULL);
 
 	if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
 		return 0;
diff --git a/ll-merge.c b/ll-merge.c
index 8955d7e1f6e..740689b7bd6 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -193,7 +193,6 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf_expand_dict_entry dict[6];
 	struct strbuf path_sq = STRBUF_INIT;
-	const char *args[] = { NULL, NULL };
 	int status, fd, i;
 	struct stat st;
 	enum ll_merge_result ret;
@@ -219,8 +218,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 
 	strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);
 
-	args[0] = cmd.buf;
-	status = run_command_v_opt(args, RUN_USING_SHELL);
+	status = run_command_l_opt(RUN_USING_SHELL, cmd.buf, NULL);
 	fd = open(temp[1], O_RDONLY);
 	if (fd < 0)
 		goto bad;
diff --git a/run-command.c b/run-command.c
index 4e75f83b5e7..fa23fd0b803 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1016,6 +1016,21 @@ static void run_command_set_opts(struct child_process *cmd, int opt)
 	cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
 }
 
+int run_command_l_opt(int opt, ...)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	va_list ap;
+	const char *arg;
+
+	va_start(ap, opt);
+	while ((arg = va_arg(ap, const char *)))
+		strvec_push(&cmd.args, arg);
+	va_end(ap);
+
+	run_command_set_opts(&cmd, opt);
+	return run_command(&cmd);
+}
+
 static int run_command_v_opt_1(struct child_process *cmd, int opt)
 {
 	run_command_set_opts(cmd, opt);
diff --git a/run-command.h b/run-command.h
index 0e85e5846a5..6320d70f062 100644
--- a/run-command.h
+++ b/run-command.h
@@ -151,8 +151,8 @@ struct child_process {
 
 /**
  * The functions: child_process_init, start_command, finish_command,
- * run_command, run_command_v_opt, run_command_v_opt_cd_env, child_process_clear
- * do the following:
+ * run_command, run_command_l_opt, run_command_v_opt,
+ * run_command_v_opt_cd_env, child_process_clear do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
  *   is printed.
@@ -254,6 +254,15 @@ int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const
 int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 				 const char *const *env, const char *tr2_class);
 
+/**
+ * The run_command_l_opt() function run_command_v_opt() takes a list
+ * of strings terminated by a NULL. Use it instead of
+ * run_command_v_opt() when possible, as it allows the compiler to
+ * check that the "argv" is NULL-delimited.
+ */
+LAST_ARG_MUST_BE_NULL
+int run_command_l_opt(int opt, ...);
+
 /**
  * Execute the given command, sending "in" to its stdin, and capturing its
  * stdout and stderr in the "out" and "err" strbufs. Any of the three may
diff --git a/sequencer.c b/sequencer.c
index debb2ecbafe..20495db9de2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3558,12 +3558,10 @@ static int error_failed_squash(struct repository *r,
 
 static int do_exec(struct repository *r, const char *command_line)
 {
-	const char *child_argv[] = { NULL, NULL };
 	int dirty, status;
 
 	fprintf(stderr, _("Executing: %s\n"), command_line);
-	child_argv[0] = command_line;
-	status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+	status = run_command_l_opt(RUN_USING_SHELL, command_line, NULL);
 
 	/* force re-reading of the cache */
 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c
index 12beee99ad2..7967478a40a 100644
--- a/t/helper/test-fake-ssh.c
+++ b/t/helper/test-fake-ssh.c
@@ -8,7 +8,6 @@ int cmd_main(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f;
 	int i;
-	const char *child_argv[] = { NULL, NULL };
 
 	/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */
 	if (!trash_directory)
@@ -25,6 +24,5 @@ int cmd_main(int argc, const char **argv)
 	/* Now, evaluate the *last* parameter */
 	if (argc < 2)
 		return 0;
-	child_argv[0] = argv[argc - 1];
-	return run_command_v_opt(child_argv, RUN_USING_SHELL);
+	return run_command_l_opt(RUN_USING_SHELL, argv[argc - 1], NULL);
 }
-- 
2.38.0.1091.gf9d18265e59

[PATCH v2 04/10] am: use run_command_l_opt() for show_patch()

[Ævar Arnfjörð Bjarmason]

The "git show" invocation added in 66335298a47 (rebase: add
--show-current-patch, 2018-02-11) is one where we're not calling
oid_to_hex() twice. So we can rely on the static buffer that
oid_to_hex() points to, rather than xstrdup()-ing it. As a result we
can use the run_command_l_opt() function.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/am.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 39fea248330..1d298a91306 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2186,16 +2186,10 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
 	const char *patch_path;
 	int len;
 
-	if (!is_null_oid(&state->orig_commit)) {
-		const char *av[4] = { "show", NULL, "--", NULL };
-		char *new_oid_str;
-		int ret;
-
-		av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));
-		ret = run_command_v_opt(av, RUN_GIT_CMD);
-		free(new_oid_str);
-		return ret;
-	}
+	if (!is_null_oid(&state->orig_commit))
+		return run_command_l_opt(RUN_GIT_CMD, "show",
+					 oid_to_hex(&state->orig_commit),
+					 "--", NULL);
 
 	switch (sub_mode) {
 	case SHOW_PATCH_RAW:
-- 
2.38.0.1091.gf9d18265e59

[PATCH v2 05/10] run-command API docs: clarify & fleshen out run_command_v_opt*() docs

[Ævar Arnfjörð Bjarmason]

Add a discussion of the flags that were missing to the
run_command_v_opt*() docs, and in doing so format them such that we
can easily add or remove flags from a table in the future, rather than
having them tied up in prose.

Let's also clarify why the user might want to use this API over
run_command() itself.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.h | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/run-command.h b/run-command.h
index 6320d70f062..cf250e36426 100644
--- a/run-command.h
+++ b/run-command.h
@@ -234,13 +234,28 @@ int run_auto_maintenance(int quiet);
 #define RUN_CLOSE_OBJECT_STORE		(1<<7)
 
 /**
- * Convenience functions that encapsulate a sequence of
- * start_command() followed by finish_command(). The argument argv
- * specifies the program and its arguments. The argument opt is zero
- * or more of the flags `RUN_COMMAND_NO_STDIN`, `RUN_GIT_CMD`,
- * `RUN_COMMAND_STDOUT_TO_STDERR`, or `RUN_SILENT_EXEC_FAILURE`
- * that correspond to the members .no_stdin, .git_cmd,
- * .stdout_to_stderr, .silent_exec_failure of `struct child_process`.
+ * The run_command_v_opt*() API is a convenience wrapper for an
+ * underlying run_command().
+ *
+ * It's intended to be used when the user already has an "argv" they'd
+ * like to use. As opposed to the "struct child_process"'s "args"
+ * member, which will be strvec_clear()'d by calling run_command(),
+ * the caller owns the "argv", which is not altered by invoking these
+ * functions.
+ *
+ * The "opt" flags that will cause various underlying run_command()
+ * members to be set. The flags and the corresponding struct members
+ * are:
+ *
+ *	- RUN_COMMAND_NO_STDIN: .no_stdin
+ *	- RUN_GIT_CMD: .git_cmd
+ *	- RUN_COMMAND_STDOUT_TO_STDERR: .stdout_to_stderr
+ *	- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure
+ *	- RUN_USING_SHELL: .use_shell
+ *	- RUN_CLEAN_ON_EXIT: .clean_on_exit
+ *	- RUN_WAIT_AFTER_CLEAN: .wait_after_clean
+ *	- RUN_CLOSE_OBJECT_STORE: .close_object_store
+ *
  * The argument dir corresponds the member .dir. The argument env
  * corresponds to the member .env.
  */
-- 
2.38.0.1091.gf9d18265e59

[PATCH v2 06/10] run-command API: remove RUN_COMMAND_STDOUT_TO_STDERR flag

[Ævar Arnfjörð Bjarmason]

The run_command_v_opt*() API is meant to handle various common cases
where we don't want the verbosity of calling the underlying
run_command(), but we're explicitly not trying to cover the full API
surface of run_command() itself.

So if we're not using some of these flags we probably won't need them
again, any future user who needs to set "stdout_to_stderr" can just
use run_command() itself, and the convenience API shouldn't be
cluttered by trying to handle all the needs of various one-offs.

So let's remove the RUN_COMMAND_STDOUT_TO_STDERR flag, it hasn't been
used since 860a2ebecd2 (receive-pack: send auto-gc output over
sideband 2, 2016-06-05) when its last user started using the
underlying API directly.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 run-command.c |  1 -
 run-command.h | 12 +++++-------
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/run-command.c b/run-command.c
index fa23fd0b803..f9a0b13f62f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1008,7 +1008,6 @@ static void run_command_set_opts(struct child_process *cmd, int opt)
 {
 	cmd->no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;
 	cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
-	cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
 	cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
 	cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
 	cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
diff --git a/run-command.h b/run-command.h
index cf250e36426..0af01bd9b67 100644
--- a/run-command.h
+++ b/run-command.h
@@ -226,12 +226,11 @@ int run_auto_maintenance(int quiet);
 
 #define RUN_COMMAND_NO_STDIN		(1<<0)
 #define RUN_GIT_CMD			(1<<1)
-#define RUN_COMMAND_STDOUT_TO_STDERR	(1<<2)
-#define RUN_SILENT_EXEC_FAILURE		(1<<3)
-#define RUN_USING_SHELL			(1<<4)
-#define RUN_CLEAN_ON_EXIT		(1<<5)
-#define RUN_WAIT_AFTER_CLEAN		(1<<6)
-#define RUN_CLOSE_OBJECT_STORE		(1<<7)
+#define RUN_SILENT_EXEC_FAILURE		(1<<2)
+#define RUN_USING_SHELL			(1<<3)
+#define RUN_CLEAN_ON_EXIT		(1<<4)
+#define RUN_WAIT_AFTER_CLEAN		(1<<5)
+#define RUN_CLOSE_OBJECT_STORE		(1<<6)
 
 /**
  * The run_command_v_opt*() API is a convenience wrapper for an
@@ -249,7 +248,6 @@ int run_auto_maintenance(int quiet);
  *
  *	- RUN_COMMAND_NO_STDIN: .no_stdin
  *	- RUN_GIT_CMD: .git_cmd
- *	- RUN_COMMAND_STDOUT_TO_STDERR: .stdout_to_stderr
  *	- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure
  *	- RUN_USING_SHELL: .use_shell
  *	- RUN_CLEAN_ON_EXIT: .clean_on_exit
-- 
2.38.0.1091.gf9d18265e59

[PATCH v2 07/10] run-command API & diff.c: remove run_command_v_opt_cd_env()

[Ævar Arnfjörð Bjarmason]

As we'll see in subsequent commits most of the run_command_v_opt_*()
API users are a bad fit for what we'd like to do with that API. The
ideal use-case for it is something where we already have an "argv",
and don't want the one-shot semantics of run_command().

In the case of run_command_v_opt_cd_env() there was only user of it,
and that user was just accumulating complexity by not using
run_command() directly.

By not having to maintain its own "argv" and "env" it doesn't have to
strvec_clear() them both, which in the case of run_command() will be
done by child_process_clear() before it returns.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 diff.c        | 26 ++++++++++++--------------
 run-command.c |  7 +------
 run-command.h |  3 +--
 tmp-objdir.h  |  6 ++++--
 4 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/diff.c b/diff.c
index 648f6717a55..392530016fa 100644
--- a/diff.c
+++ b/diff.c
@@ -4278,35 +4278,33 @@ static void run_external_diff(const char *pgm,
 			      const char *xfrm_msg,
 			      struct diff_options *o)
 {
-	struct strvec argv = STRVEC_INIT;
-	struct strvec env = STRVEC_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
-	strvec_push(&argv, pgm);
-	strvec_push(&argv, name);
+	strvec_push(&cmd.args, pgm);
+	strvec_push(&cmd.args, name);
 
 	if (one && two) {
-		add_external_diff_name(o->repo, &argv, name, one);
+		add_external_diff_name(o->repo, &cmd.args, name, one);
 		if (!other)
-			add_external_diff_name(o->repo, &argv, name, two);
+			add_external_diff_name(o->repo, &cmd.args, name, two);
 		else {
-			add_external_diff_name(o->repo, &argv, other, two);
-			strvec_push(&argv, other);
-			strvec_push(&argv, xfrm_msg);
+			add_external_diff_name(o->repo, &cmd.args, other, two);
+			strvec_push(&cmd.args, other);
+			strvec_push(&cmd.args, xfrm_msg);
 		}
 	}
 
-	strvec_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
-	strvec_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
+	strvec_pushf(&cmd.env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
+	strvec_pushf(&cmd.env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
 
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
-	if (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))
+	cmd.use_shell = 1;
+	if (run_command(&cmd))
 		die(_("external diff died, stopping at %s"), name);
 
 	remove_tempfile();
-	strvec_clear(&argv);
-	strvec_clear(&env);
 }
 
 static int similarity_index(struct diff_filepair *p)
diff --git a/run-command.c b/run-command.c
index f9a0b13f62f..25a978fb027 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1038,7 +1038,7 @@ static int run_command_v_opt_1(struct child_process *cmd, int opt)
 
 int run_command_v_opt(const char **argv, int opt)
 {
-	return run_command_v_opt_cd_env(argv, opt, NULL, NULL);
+	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);
 }
 
 int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)
@@ -1046,11 +1046,6 @@ int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)
 	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);
 }
 
-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env)
-{
-	return run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);
-}
-
 int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 				 const char *const *env, const char *tr2_class)
 {
diff --git a/run-command.h b/run-command.h
index 0af01bd9b67..2574d46cb70 100644
--- a/run-command.h
+++ b/run-command.h
@@ -152,7 +152,7 @@ struct child_process {
 /**
  * The functions: child_process_init, start_command, finish_command,
  * run_command, run_command_l_opt, run_command_v_opt,
- * run_command_v_opt_cd_env, child_process_clear do the following:
+ * child_process_clear do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
  *   is printed.
@@ -263,7 +263,6 @@ int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);
  * env (the environment) is to be formatted like environ: "VAR=VALUE".
  * To unset an environment variable use just "VAR".
  */
-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);
 int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 				 const char *const *env, const char *tr2_class);
 
diff --git a/tmp-objdir.h b/tmp-objdir.h
index 76efc7edee5..c96aa77396d 100644
--- a/tmp-objdir.h
+++ b/tmp-objdir.h
@@ -10,9 +10,11 @@
  *
  * Example:
  *
+ *	struct child_process cmd = CHILD_PROCESS_INIT;
+ *	...
  *	struct tmp_objdir *t = tmp_objdir_create("incoming");
- *	if (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&
- *	    !tmp_objdir_migrate(t))
+ *      strvec_pushl(&cmd.env, tmp_objdir_env(t));
+ *	if (!run_command(&cmd) && !tmp_objdir_migrate(t))
  *		printf("success!\n");
  *	else
  *		die("failed...tmp_objdir will clean up for us");
-- 
2.38.0.1091.gf9d18265e59

[PATCH v2 08/10] run-command API & users: remove run_command_v_opt_tr2()

[Ævar Arnfjörð Bjarmason]

As noted in the preceding commit for run_command_v_opt_cd_env() that
function's users could more easily use the underlying run_command()
directly.

In the case of the "git.c" user that can be argued the other way,
given the slight line count increase here, but part of that's because
the "args" in "git.c" was being leaked, which we now don't have to
worry about.

That just leaves the spawn_daemon() user in "fsmonitor-ipc.c", it's
likewise slightly more verbose as a result, but by making it use
run_command() we can remove this entire part of the API. As noted in a
preceding commit run_command_v_opt*() should be aimed at handling
various common cases, not these one-offs.

The "fsmonitor-ipc.c" caller would be nicer with a hypothetical
run_command_l_opt_tr2(), but let's not maintain such a thing only for
it, as it would be its only user.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 fsmonitor-ipc.c | 10 +++++++---
 git.c           | 15 +++++++++------
 run-command.c   | 26 +++-----------------------
 run-command.h   | 19 +++----------------
 4 files changed, 22 insertions(+), 48 deletions(-)

diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index 789e7397baa..7251f48e456 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -56,10 +56,14 @@ enum ipc_active_state fsmonitor_ipc__get_state(void)
 
 static int spawn_daemon(void)
 {
-	const char *args[] = { "fsmonitor--daemon", "start", NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
-	return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,
-				    "fsmonitor");
+	cmd.git_cmd = 1;
+	cmd.no_stdin = 1;
+	cmd.trace2_child_class = "fsmonitor";
+	strvec_pushl(&cmd.args, "fsmonitor--daemon", "start", NULL);
+
+	return run_command(&cmd);
 }
 
 int fsmonitor_ipc__send_query(const char *since_token,
diff --git a/git.c b/git.c
index da411c53822..ccf444417b5 100644
--- a/git.c
+++ b/git.c
@@ -787,7 +787,7 @@ static int run_argv(int *argcp, const char ***argv)
 		if (!done_alias)
 			handle_builtin(*argcp, *argv);
 		else if (get_builtin(**argv)) {
-			struct strvec args = STRVEC_INIT;
+			struct child_process cmd = CHILD_PROCESS_INIT;
 			int i;
 
 			/*
@@ -804,18 +804,21 @@ static int run_argv(int *argcp, const char ***argv)
 
 			commit_pager_choice();
 
-			strvec_push(&args, "git");
+			strvec_push(&cmd.args, "git");
 			for (i = 0; i < *argcp; i++)
-				strvec_push(&args, (*argv)[i]);
+				strvec_push(&cmd.args, (*argv)[i]);
 
-			trace_argv_printf(args.v, "trace: exec:");
+			trace_argv_printf(cmd.args.v, "trace: exec:");
 
 			/*
 			 * if we fail because the command is not found, it is
 			 * OK to return. Otherwise, we just pass along the status code.
 			 */
-			i = run_command_v_opt_tr2(args.v, RUN_SILENT_EXEC_FAILURE |
-						  RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, "git_alias");
+			cmd.silent_exec_failure = 1;
+			cmd.clean_on_exit = 1;
+			cmd.wait_after_clean = 1;
+			cmd.trace2_child_class = "git_alias";
+			i = run_command(&cmd);
 			if (i >= 0 || errno != ENOENT)
 				exit(i);
 			die("could not execute builtin %s", **argv);
diff --git a/run-command.c b/run-command.c
index 25a978fb027..0066ace85fa 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1010,7 +1010,6 @@ static void run_command_set_opts(struct child_process *cmd, int opt)
 	cmd->git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
 	cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;
 	cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;
-	cmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;
 	cmd->wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;
 	cmd->close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;
 }
@@ -1030,32 +1029,13 @@ int run_command_l_opt(int opt, ...)
 	return run_command(&cmd);
 }
 
-static int run_command_v_opt_1(struct child_process *cmd, int opt)
-{
-	run_command_set_opts(cmd, opt);
-	return run_command(cmd);
-}
-
 int run_command_v_opt(const char **argv, int opt)
-{
-	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, NULL);
-}
-
-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)
-{
-	return run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);
-}
-
-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
-				 const char *const *env, const char *tr2_class)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
+
 	strvec_pushv(&cmd.args, argv);
-	cmd.dir = dir;
-	if (env)
-		strvec_pushv(&cmd.env, (const char **)env);
-	cmd.trace2_child_class = tr2_class;
-	return run_command_v_opt_1(&cmd, opt);
+	run_command_set_opts(&cmd, opt);
+	return run_command(&cmd);
 }
 
 #ifndef NO_PTHREADS
diff --git a/run-command.h b/run-command.h
index 2574d46cb70..2b1fe3cde5c 100644
--- a/run-command.h
+++ b/run-command.h
@@ -228,12 +228,11 @@ int run_auto_maintenance(int quiet);
 #define RUN_GIT_CMD			(1<<1)
 #define RUN_SILENT_EXEC_FAILURE		(1<<2)
 #define RUN_USING_SHELL			(1<<3)
-#define RUN_CLEAN_ON_EXIT		(1<<4)
-#define RUN_WAIT_AFTER_CLEAN		(1<<5)
-#define RUN_CLOSE_OBJECT_STORE		(1<<6)
+#define RUN_WAIT_AFTER_CLEAN		(1<<4)
+#define RUN_CLOSE_OBJECT_STORE		(1<<5)
 
 /**
- * The run_command_v_opt*() API is a convenience wrapper for an
+ * The run_command_v_opt() function is a convenience wrapper for an
  * underlying run_command().
  *
  * It's intended to be used when the user already has an "argv" they'd
@@ -250,21 +249,9 @@ int run_auto_maintenance(int quiet);
  *	- RUN_GIT_CMD: .git_cmd
  *	- RUN_SILENT_EXEC_FAILURE: .silent_exec_failure
  *	- RUN_USING_SHELL: .use_shell
- *	- RUN_CLEAN_ON_EXIT: .clean_on_exit
- *	- RUN_WAIT_AFTER_CLEAN: .wait_after_clean
  *	- RUN_CLOSE_OBJECT_STORE: .close_object_store
- *
- * The argument dir corresponds the member .dir. The argument env
- * corresponds to the member .env.
  */
 int run_command_v_opt(const char **argv, int opt);
-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);
-/*
- * env (the environment) is to be formatted like environ: "VAR=VALUE".
- * To unset an environment variable use just "VAR".
- */
-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
-				 const char *const *env, const char *tr2_class);
 
 /**
  * The run_command_l_opt() function run_command_v_opt() takes a list
-- 
2.38.0.1091.gf9d18265e59

[PATCH v2 09/10] gc: use strvec_pushf(), avoid redundant strbuf_detach()

[Ævar Arnfjörð Bjarmason]

Adjust code added in 3797a0a7b7a (maintenance: use Windows scheduled
tasks, 2021-01-05) to use strvec_pushf() directly. Rather than having
a function that returns a strbuf_detach() we need to free and have the
"strvec_pushl()" do its own "xstrdup()" we can format this in-place.

By changing this we only have the "strvec_clear()" between the
"run_command_v_opt()" and the "return result" in
"schtasks_remove_task()". In the subsequent commit we'll start using a
helper which'll allow us to skip the "strvec_clear()".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/gc.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 075b4637660..519e64e86ee 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1894,12 +1894,7 @@ static int is_schtasks_available(void)
 #endif
 }
 
-static char *schtasks_task_name(const char *frequency)
-{
-	struct strbuf label = STRBUF_INIT;
-	strbuf_addf(&label, "Git Maintenance (%s)", frequency);
-	return strbuf_detach(&label, NULL);
-}
+#define SCHTASKS_NAME_FMT "Git Maintenance (%s)"
 
 static int schtasks_remove_task(enum schedule_priority schedule)
 {
@@ -1907,16 +1902,15 @@ static int schtasks_remove_task(enum schedule_priority schedule)
 	int result;
 	struct strvec args = STRVEC_INIT;
 	const char *frequency = get_frequency(schedule);
-	char *name = schtasks_task_name(frequency);
 
 	get_schedule_cmd(&cmd, NULL);
 	strvec_split(&args, cmd);
-	strvec_pushl(&args, "/delete", "/tn", name, "/f", NULL);
+	strvec_pushl(&args, "/delete", "/tn", NULL);
+	strvec_pushf(&args, SCHTASKS_NAME_FMT, frequency);
+	strvec_pushl(&args, "/f", NULL);
 
 	result = run_command_v_opt(args.v, 0);
-
 	strvec_clear(&args);
-	free(name);
 	return result;
 }
 
@@ -1935,7 +1929,6 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	const char *xml;
 	struct tempfile *tfile;
 	const char *frequency = get_frequency(schedule);
-	char *name = schtasks_task_name(frequency);
 	struct strbuf tfilename = STRBUF_INIT;
 
 	get_schedule_cmd(&cmd, NULL);
@@ -2028,8 +2021,10 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	      "</Task>\n";
 	fprintf(tfile->fp, xml, exec_path, exec_path, frequency);
 	strvec_split(&child.args, cmd);
-	strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml",
-				  get_tempfile_path(tfile), NULL);
+	strvec_pushl(&child.args, "/create", "/tn", NULL);
+	strvec_pushf(&child.args, SCHTASKS_NAME_FMT, frequency);
+	strvec_pushl(&child.args, "/f", "/xml",
+		     get_tempfile_path(tfile), NULL);
 	close_tempfile_gently(tfile);
 
 	child.no_stdout = 1;
@@ -2040,7 +2035,6 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	result = finish_command(&child);
 
 	delete_tempfile(&tfile);
-	free(name);
 	return result;
 }
 
-- 
2.38.0.1091.gf9d18265e59

[PATCH v2 10/10] run-command API: add and use a run_command_sv_opt()

[Ævar Arnfjörð Bjarmason]

Add a run_command_sv_opt() convenience wrapper for
run_command_v_opt(), as noted in the API documentation this is for the
common case of wanting to construct a "struct strvec" to pass to
run_command_v_opt(), and as it's a one-shot to strvec_clear() it
afterwards.

Let's convert those API users that were using a "ret" variable to
carry over to "return" after a "strvec_clear()" to use this new
function instead.

Because we pass the "struct strvec *" to the function we can also
avoid copying the arguments to the "args" member of the "struct
child_process", as we were doing with run_command_v_opt().

Instead we can use memcpy() and strvec_clear() to do the moral
equivalent of a strbuf_{detach,attach}(). The strvec API doesn't have
a strvec_attach(), we could add it here while at it, but let's avoid
generalizing the interface for now and migrate the "struct strvec *"
in the "run_command_sv_opt()" instead.

Let's leave aside the user in "builtin/bisect--helper.c"'s
bisect_visualize(). There's an outstanding topic that's extensively
modifying it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 add-interactive.c |  3 +--
 builtin/add.c     |  6 ++----
 builtin/clone.c   |  3 +--
 builtin/gc.c      |  5 +----
 builtin/pull.c    | 15 +++------------
 builtin/remote.c  |  5 +----
 merge.c           |  3 +--
 run-command.c     | 11 +++++++++++
 run-command.h     | 16 +++++++++++++++-
 scalar.c          |  6 +-----
 sequencer.c       | 11 ++---------
 11 files changed, 39 insertions(+), 45 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index f071b2a1b4f..9c86f3b9532 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -1007,8 +1007,7 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
 			if (files->selected[i])
 				strvec_push(&args,
 					    files->items.items[i].string);
-		res = run_command_v_opt(args.v, 0);
-		strvec_clear(&args);
+		res = run_command_sv_opt(&args, 0);
 	}
 
 	putchar('\n');
diff --git a/builtin/add.c b/builtin/add.c
index f84372964c8..7c783eebc0e 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -240,7 +240,7 @@ static int refresh(int verbose, const struct pathspec *pathspec)
 int run_add_interactive(const char *revision, const char *patch_mode,
 			const struct pathspec *pathspec)
 {
-	int status, i;
+	int i;
 	struct strvec argv = STRVEC_INIT;
 	int use_builtin_add_i =
 		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
@@ -282,9 +282,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 		/* pass original pathspec, to be re-parsed */
 		strvec_push(&argv, pathspec->items[i].original);
 
-	status = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-	return status;
+	return run_command_sv_opt(&argv, RUN_GIT_CMD);
 }
 
 int interactive_add(const char **argv, const char *prefix, int patch)
diff --git a/builtin/clone.c b/builtin/clone.c
index a1514363190..8e43781e147 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -756,8 +756,7 @@ static int checkout(int submodule_progress, int filter_submodules)
 					       "--single-branch" :
 					       "--no-single-branch");
 
-		err = run_command_v_opt(args.v, RUN_GIT_CMD);
-		strvec_clear(&args);
+		return run_command_sv_opt(&args, RUN_GIT_CMD);
 	}
 
 	return err;
diff --git a/builtin/gc.c b/builtin/gc.c
index 519e64e86ee..8393e19b504 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1899,7 +1899,6 @@ static int is_schtasks_available(void)
 static int schtasks_remove_task(enum schedule_priority schedule)
 {
 	const char *cmd = "schtasks";
-	int result;
 	struct strvec args = STRVEC_INIT;
 	const char *frequency = get_frequency(schedule);
 
@@ -1909,9 +1908,7 @@ static int schtasks_remove_task(enum schedule_priority schedule)
 	strvec_pushf(&args, SCHTASKS_NAME_FMT, frequency);
 	strvec_pushl(&args, "/f", NULL);
 
-	result = run_command_v_opt(args.v, 0);
-	strvec_clear(&args);
-	return result;
+	return run_command_sv_opt(&args, 0);
 }
 
 static int schtasks_remove_tasks(void)
diff --git a/builtin/pull.c b/builtin/pull.c
index 403a24d7ca6..2f36823c97e 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -516,7 +516,6 @@ static void parse_repo_refspecs(int argc, const char **argv, const char **repo,
 static int run_fetch(const char *repo, const char **refspecs)
 {
 	struct strvec args = STRVEC_INIT;
-	int ret;
 
 	strvec_pushl(&args, "fetch", "--update-head-ok", NULL);
 
@@ -582,9 +581,7 @@ static int run_fetch(const char *repo, const char **refspecs)
 		strvec_pushv(&args, refspecs);
 	} else if (*refspecs)
 		BUG("refspecs without repo?");
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);
-	strvec_clear(&args);
-	return ret;
+	return run_command_sv_opt(&args, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);
 }
 
 /**
@@ -653,7 +650,6 @@ static int update_submodules(void)
  */
 static int run_merge(void)
 {
-	int ret;
 	struct strvec args = STRVEC_INIT;
 
 	strvec_pushl(&args, "merge", NULL);
@@ -696,9 +692,7 @@ static int run_merge(void)
 		strvec_push(&args, "--allow-unrelated-histories");
 
 	strvec_push(&args, "FETCH_HEAD");
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
-	return ret;
+	return run_command_sv_opt(&args, RUN_GIT_CMD);
 }
 
 /**
@@ -879,7 +873,6 @@ static int get_rebase_newbase_and_upstream(struct object_id *newbase,
 static int run_rebase(const struct object_id *newbase,
 		const struct object_id *upstream)
 {
-	int ret;
 	struct strvec args = STRVEC_INIT;
 
 	strvec_push(&args, "rebase");
@@ -913,9 +906,7 @@ static int run_rebase(const struct object_id *newbase,
 
 	strvec_push(&args, oid_to_hex(upstream));
 
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
-	return ret;
+	return run_command_sv_opt(&args, RUN_GIT_CMD);
 }
 
 static int get_can_ff(struct object_id *orig_head,
diff --git a/builtin/remote.c b/builtin/remote.c
index 910f7b9316a..5d3534db19c 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1510,7 +1510,6 @@ static int update(int argc, const char **argv, const char *prefix)
 	};
 	struct strvec fetch_argv = STRVEC_INIT;
 	int default_defined = 0;
-	int retval;
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_remote_update_usage,
@@ -1536,9 +1535,7 @@ static int update(int argc, const char **argv, const char *prefix)
 		}
 	}
 
-	retval = run_command_v_opt(fetch_argv.v, RUN_GIT_CMD);
-	strvec_clear(&fetch_argv);
-	return retval;
+	return run_command_sv_opt(&fetch_argv, RUN_GIT_CMD);
 }
 
 static int remove_all_fetch_refspecs(const char *key)
diff --git a/merge.c b/merge.c
index 2382ff66d35..487debacecb 100644
--- a/merge.c
+++ b/merge.c
@@ -33,8 +33,7 @@ int try_merge_command(struct repository *r,
 	for (j = remotes; j; j = j->next)
 		strvec_push(&args, merge_argument(j->item));
 
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
+	ret = run_command_sv_opt(&args, RUN_GIT_CMD);
 
 	discard_index(r->index);
 	if (repo_read_index(r) < 0)
diff --git a/run-command.c b/run-command.c
index 0066ace85fa..724fc581c89 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1038,6 +1038,17 @@ int run_command_v_opt(const char **argv, int opt)
 	return run_command(&cmd);
 }
 
+int run_command_sv_opt(struct strvec *args, int opt)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	/* TODO: We could encapsulate this with a strvec_attach() */
+	memcpy(&cmd.args, args, sizeof(*args));
+	strvec_init(args);
+	run_command_set_opts(&cmd, opt);
+	return run_command(&cmd);
+}
+
 #ifndef NO_PTHREADS
 static pthread_t main_thread;
 static int main_thread_set;
diff --git a/run-command.h b/run-command.h
index 2b1fe3cde5c..8e8403f3bb3 100644
--- a/run-command.h
+++ b/run-command.h
@@ -151,7 +151,7 @@ struct child_process {
 
 /**
  * The functions: child_process_init, start_command, finish_command,
- * run_command, run_command_l_opt, run_command_v_opt,
+ * run_command, run_command_l_opt, run_command_v_opt, run_command_sv_opt,
  * child_process_clear do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
@@ -262,6 +262,20 @@ int run_command_v_opt(const char **argv, int opt);
 LAST_ARG_MUST_BE_NULL
 int run_command_l_opt(int opt, ...);
 
+/**
+ * The run_command_sv_opt() function is a wrapper for
+ * run_command_v_opt(). It takes a "struct strvec *args" which
+ * similarly will be strvec_clear()'d before returning.
+ *
+ * Use it for the common case of constructing a "struct strvec" for a
+ * one-shot run_command_v_opt() invocation.
+ *
+ * The "args" will migrated the "cmd.args" member of an underlying
+ * "struct child_process", in a way that avoids making an extra copy.
+ */
+RESULT_MUST_BE_USED
+int run_command_sv_opt(struct strvec *args, int opt);
+
 /**
  * Execute the given command, sending "in" to its stdin, and capturing its
  * stdout and stderr in the "out" and "err" strbufs. Any of the three may
diff --git a/scalar.c b/scalar.c
index 6de9c0ee523..3480bf73cbd 100644
--- a/scalar.c
+++ b/scalar.c
@@ -72,7 +72,6 @@ static int run_git(const char *arg, ...)
 	struct strvec argv = STRVEC_INIT;
 	va_list args;
 	const char *p;
-	int res;
 
 	va_start(args, arg);
 	strvec_push(&argv, arg);
@@ -80,10 +79,7 @@ static int run_git(const char *arg, ...)
 		strvec_push(&argv, p);
 	va_end(args);
 
-	res = run_command_v_opt(argv.v, RUN_GIT_CMD);
-
-	strvec_clear(&argv);
-	return res;
+	return run_command_sv_opt(&argv, RUN_GIT_CMD);
 }
 
 struct scalar_config {
diff --git a/sequencer.c b/sequencer.c
index 20495db9de2..7ee0e05512c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3183,7 +3183,6 @@ static int rollback_is_safe(void)
 
 static int reset_merge(const struct object_id *oid)
 {
-	int ret;
 	struct strvec argv = STRVEC_INIT;
 
 	strvec_pushl(&argv, "reset", "--merge", NULL);
@@ -3191,10 +3190,7 @@ static int reset_merge(const struct object_id *oid)
 	if (!is_null_oid(oid))
 		strvec_push(&argv, oid_to_hex(oid));
 
-	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-
-	return ret;
+	return run_command_sv_opt(&argv, RUN_GIT_CMD);
 }
 
 static int rollback_single_pick(struct repository *r)
@@ -4866,7 +4862,6 @@ static int pick_commits(struct repository *r,
 static int continue_single_pick(struct repository *r, struct replay_opts *opts)
 {
 	struct strvec argv = STRVEC_INIT;
-	int ret;
 
 	if (!refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
 	    !refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD"))
@@ -4887,9 +4882,7 @@ static int continue_single_pick(struct repository *r, struct replay_opts *opts)
 		 */
 		strvec_pushl(&argv, "--no-edit", "--cleanup=strip", NULL);
 
-	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-	return ret;
+	return run_command_sv_opt(&argv, RUN_GIT_CMD);
 }
 
 static int commit_staged_changes(struct repository *r,
-- 
2.38.0.1091.gf9d18265e59

Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> This series provides a more idiomatic set of run-command API helpers
> to match our current use-cases for run_command_v_opt(). See v1[1] for
> a more general overview.

Hmph...  I somehow thought that the concensus is rather try the
complete opposite approach shown by René's patch to lose the use of
run_command_v_opt() by replacing it with run_command(&args), with
args.v populated using strvec_pushl() and other strvec API
functions.

One of the reasons I would prefer to see us moving in that direction
was because the first iteration of this series was a good
demonstration of the relatively limited usefulness of _l_opt()
variant and also it seemed to be error prone to use it.

Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[Ævar Arnfjörð Bjarmason]

On Tue, Oct 18 2022, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> This series provides a more idiomatic set of run-command API helpers
>> to match our current use-cases for run_command_v_opt(). See v1[1] for
>> a more general overview.
>
> Hmph...  I somehow thought that the concensus is rather try the
> complete opposite approach shown by René's patch to lose the use of
> run_command_v_opt() by replacing it with run_command(&args), with
> args.v populated using strvec_pushl() and other strvec API
> functions.
>
> One of the reasons I would prefer to see us moving in that direction
> was because the first iteration of this series was a good
> demonstration of the relatively limited usefulness of _l_opt()
> variant and also it seemed to be error prone to use it.

I'm getting somewhat mixed messages. Jeff seemed to like the end-to-end
safety of run_command_l_opt() before I wrote it. I think the
run_command_l_opt() still really shines for the simple cases.

I don't see how *_l_opt() is particularly error prone, I just had a
stupid think-o in v1 of this, but that if/else if bug is something that
could have snuck in with run_command() given the same stupidity :)

I checked out René's [1] and diff'd it with mine, excluding various
parts that inflated the diff for no good explanatory purpose (e.g. the
API itself, or where I omitted some conversions).

I think the diffstat is a good argument for *some* version of my series,
but as e.g. the first hunk shows we could also just convert
e.g. run_diff() to run_command() instead of run_command_sv_opt().

I wonder if a run_command() that just had the prototype (struct
child_process *cmd, ...) might not be the best of both worlds (or a
run_commandl() alternative). I.e. to do away with the whole custom way
of specifying the flag(s), and just take the passed-in arguments and
append them to "&cmd.args".

That would give us run_command_l_opt() behavior, which is handy in some
cases, and gives us compile-time checks. We could BUG() out if
"cmd.args.nr > 0" if we use it, to ensure we use one or the other
(although a combination would be handy in some cases, so maybe not).

What do you all think?

It's also interesting to consider adding a --noop option to be supported
by parse-options.c. The reason we can't use a run_command_l_opt() in
some cases is because we're doing e.g.:

	if (progress)
		strvec_push(&args, "--progress");

We have a --no-progress, but in those cases the recipient at the end
often cares about a default of -1 for a bool variable, or similar. So if
we could do:

	run_command_l_opt(0, command,
		(progress ? "--progress" : "--noop"),
		...,
		NULL
	);

We could benefit from compile-time checks, and wouldn't have to allocate
a strvec just for building up the arguments in some cases. Just food for
thought...

1. https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/

 add-interactive.c        |   8 +--
 builtin/add.c            |  15 +++---
 builtin/am.c             |  12 +++--
 builtin/clone.c          |  44 +++++++++------
 builtin/difftool.c       |  24 +++++----
 builtin/fetch.c          |   9 ++--
 builtin/gc.c             |  74 ++++++++++++++++---------
 builtin/merge-index.c    |   4 +-
 builtin/pull.c           | 138 ++++++++++++++++++++++++-----------------------
 builtin/remote.c         |  37 +++++++------
 compat/mingw.c           |  11 ++--
 diff.c                   |   5 +-
 fsmonitor-ipc.c          |   4 +-
 ll-merge.c               |   5 +-
 merge.c                  |  17 +++---
 scalar.c                 |   9 ++--
 sequencer.c              |  23 ++++----
 t/helper/test-fake-ssh.c |   5 +-
 t/helper/test-trace2.c   |   4 +-
 tmp-objdir.h             |   6 +--
 20 files changed, 265 insertions(+), 189 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index 9c86f3b9532..ecc5ae1b249 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -997,17 +997,17 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,
 	count = list_and_choose(s, files, opts);
 	opts->flags = 0;
 	if (count > 0) {
-		struct strvec args = STRVEC_INIT;
+		struct child_process cmd = CHILD_PROCESS_INIT;
 
-		strvec_pushl(&args, "git", "diff", "-p", "--cached",
+		strvec_pushl(&cmd.args, "git", "diff", "-p", "--cached",
 			     oid_to_hex(!is_initial ? &oid :
 					s->r->hash_algo->empty_tree),
 			     "--", NULL);
 		for (i = 0; i < files->items.nr; i++)
 			if (files->selected[i])
-				strvec_push(&args,
+				strvec_push(&cmd.args,
 					    files->items.items[i].string);
-		res = run_command_sv_opt(&args, 0);
+		res = run_command(&cmd);
 	}
 
 	putchar('\n');
diff --git a/builtin/add.c b/builtin/add.c
index 7c783eebc0e..626c71ec6aa 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -241,7 +241,7 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 			const struct pathspec *pathspec)
 {
 	int i;
-	struct strvec argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int use_builtin_add_i =
 		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);
 
@@ -272,17 +272,18 @@ int run_add_interactive(const char *revision, const char *patch_mode,
 		return !!run_add_p(the_repository, mode, revision, pathspec);
 	}
 
-	strvec_push(&argv, "add--interactive");
+	strvec_push(&cmd.args, "add--interactive");
 	if (patch_mode)
-		strvec_push(&argv, patch_mode);
+		strvec_push(&cmd.args, patch_mode);
 	if (revision)
-		strvec_push(&argv, revision);
-	strvec_push(&argv, "--");
+		strvec_push(&cmd.args, revision);
+	strvec_push(&cmd.args, "--");
 	for (i = 0; i < pathspec->nr; i++)
 		/* pass original pathspec, to be re-parsed */
-		strvec_push(&argv, pathspec->items[i].original);
+		strvec_push(&cmd.args, pathspec->items[i].original);
 
-	return run_command_sv_opt(&argv, RUN_GIT_CMD);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }
 
 int interactive_add(const char **argv, const char *prefix, int patch)
diff --git a/builtin/am.c b/builtin/am.c
index 1d298a91306..20aea0d2487 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2186,10 +2186,14 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
 	const char *patch_path;
 	int len;
 
-	if (!is_null_oid(&state->orig_commit))
-		return run_command_l_opt(RUN_GIT_CMD, "show",
-					 oid_to_hex(&state->orig_commit),
-					 "--", NULL);
+	if (!is_null_oid(&state->orig_commit)) {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit),
+			     "--", NULL);
+		cmd.git_cmd = 1;
+		return run_command(&cmd);
+	}
 
 	switch (sub_mode) {
 	case SHOW_PATCH_RAW:
diff --git a/builtin/clone.c b/builtin/clone.c
index 8e43781e147..219cfbd7355 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -651,19 +651,23 @@ static void update_head(const struct ref *our, const struct ref *remote,
 
 static int git_sparse_checkout_init(const char *repo)
 {
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	int result = 0;
+	strvec_pushl(&cmd.args, "-C", repo, "sparse-checkout", "set", NULL);
+
 	/*
 	 * We must apply the setting in the current process
 	 * for the later checkout to use the sparse-checkout file.
 	 */
 	core_apply_sparse_checkout = 1;
 
-	if (run_command_l_opt(RUN_GIT_CMD, "-C", repo, "sparse-checkout",
-			      "set", NULL)) {
+	cmd.git_cmd = 1;
+	if (run_command(&cmd)) {
 		error(_("failed to initialize sparse-checkout"));
-		return 1;
+		result = 1;
 	}
 
-	return 0;
+	return result;
 }
 
 static int checkout(int submodule_progress, int filter_submodules)
@@ -727,36 +731,38 @@ static int checkout(int submodule_progress, int filter_submodules)
 			   oid_to_hex(&oid), "1", NULL);
 
 	if (!err && (option_recurse_submodules.nr > 0)) {
-		struct strvec args = STRVEC_INIT;
-		strvec_pushl(&args, "submodule", "update", "--require-init", "--recursive", NULL);
+		struct child_process cmd = CHILD_PROCESS_INIT;
+		strvec_pushl(&cmd.args, "submodule", "update", "--require-init",
+			     "--recursive", NULL);
 
 		if (option_shallow_submodules == 1)
-			strvec_push(&args, "--depth=1");
+			strvec_push(&cmd.args, "--depth=1");
 
 		if (max_jobs != -1)
-			strvec_pushf(&args, "--jobs=%d", max_jobs);
+			strvec_pushf(&cmd.args, "--jobs=%d", max_jobs);
 
 		if (submodule_progress)
-			strvec_push(&args, "--progress");
+			strvec_push(&cmd.args, "--progress");
 
 		if (option_verbosity < 0)
-			strvec_push(&args, "--quiet");
+			strvec_push(&cmd.args, "--quiet");
 
 		if (option_remote_submodules) {
-			strvec_push(&args, "--remote");
-			strvec_push(&args, "--no-fetch");
+			strvec_push(&cmd.args, "--remote");
+			strvec_push(&cmd.args, "--no-fetch");
 		}
 
 		if (filter_submodules && filter_options.choice)
-			strvec_pushf(&args, "--filter=%s",
+			strvec_pushf(&cmd.args, "--filter=%s",
 				     expand_list_objects_filter_spec(&filter_options));
 
 		if (option_single_branch >= 0)
-			strvec_push(&args, option_single_branch ?
+			strvec_push(&cmd.args, option_single_branch ?
 					       "--single-branch" :
 					       "--no-single-branch");
 
-		return run_command_sv_opt(&args, RUN_GIT_CMD);
+		cmd.git_cmd = 1;
+		err = run_command(&cmd);
 	}
 
 	return err;
@@ -860,8 +866,12 @@ static void dissociate_from_references(void)
 	char *alternates = git_pathdup("objects/info/alternates");
 
 	if (!access(alternates, F_OK)) {
-		if (run_command_l_opt(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,
-				      "repack", "-a", "-d", NULL))
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		strvec_pushl(&cmd.args, "repack", "-a", "-d", NULL);
+		cmd.git_cmd = 1;
+		cmd.no_stdin = 1;
+		if (run_command(&cmd))
 			die(_("cannot repack to clean up"));
 		if (unlink(alternates) && errno != ENOENT)
 			die_errno(_("cannot unlink temporary alternates file"));
diff --git a/builtin/difftool.c b/builtin/difftool.c
index ed211a87322..d6c262e15ff 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -44,8 +44,10 @@ static int difftool_config(const char *var, const char *value, void *cb)
 
 static int print_tool_help(void)
 {
-	return run_command_l_opt(RUN_GIT_CMD, "mergetool", "--tool-help=diff",
-				 NULL);
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	strvec_pushl(&cmd.args, "mergetool", "--tool-help=diff", NULL);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }
 
 static int parse_index_info(char *p, int *mode1, int *mode2,
@@ -360,8 +362,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct pair_entry *entry;
 	struct index_state wtindex;
 	struct checkout lstate, rstate;
-	int flags = RUN_GIT_CMD, err = 0;
-	const char *helper_command = "difftool--helper";
+	int err = 0;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct hashmap wt_modified, tmp_modified;
 	int indices_loaded = 0;
 
@@ -564,13 +566,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 
 	strbuf_setlen(&ldir, ldir_len);
 	strbuf_setlen(&rdir, rdir_len);
+
 	if (extcmd) {
-		helper_command = extcmd;
-		flags = 0;
-	} else
+		strvec_push(&cmd.args, extcmd);
+	} else {
+		strvec_push(&cmd.args, "difftool--helper");
+		cmd.git_cmd = 1;
 		setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
-	ret = run_command_l_opt(flags, helper_command, ldir.buf, rdir.buf,
-				NULL);
+	}
+	strvec_pushl(&cmd.args, ldir.buf, rdir.buf, NULL);
+
+	ret = run_command(&cmd);
 
 	/* TODO: audit for interaction with sparse-index. */
 	ensure_full_index(&wtindex);
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a0fca93bb6a..dd060dd65af 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1965,14 +1965,17 @@ static int fetch_multiple(struct string_list *list, int max_children)
 	} else
 		for (i = 0; i < list->nr; i++) {
 			const char *name = list->items[i].string;
-			strvec_push(&argv, name);
+			struct child_process cmd = CHILD_PROCESS_INIT;
+
+			strvec_pushv(&cmd.args, argv.v);
+			strvec_push(&cmd.args, name);
 			if (verbosity >= 0)
 				printf(_("Fetching %s\n"), name);
-			if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+			cmd.git_cmd = 1;
+			if (run_command(&cmd)) {
 				error(_("could not fetch %s"), name);
 				result = 1;
 			}
-			strvec_pop(&argv);
 		}
 
 	strvec_clear(&argv);
diff --git a/builtin/gc.c b/builtin/gc.c
index 8393e19b504..b47e53c2101 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -58,6 +58,8 @@ static unsigned long max_delta_cache_size = DEFAULT_DELTA_CACHE_SIZE;
 static struct strvec reflog = STRVEC_INIT;
 static struct strvec repack = STRVEC_INIT;
 static struct strvec prune = STRVEC_INIT;
+static struct strvec prune_worktrees = STRVEC_INIT;
+static struct strvec rerere = STRVEC_INIT;
 
 static struct tempfile *pidfile;
 static struct lock_file log_lock;
@@ -165,8 +167,11 @@ static void gc_config(void)
 struct maintenance_run_opts;
 static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)
 {
-	return run_command_l_opt(RUN_GIT_CMD, "pack-refs", "--all", "--prune",
-				 NULL);
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_pushl(&cmd.args, "pack-refs", "--all", "--prune", NULL);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }
 
 static int too_many_loose_objects(void)
@@ -518,6 +523,16 @@ static int report_last_gc_error(void)
 	return ret;
 }
 
+static void run_git_or_die(const char **argv)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_pushv(&cmd.args, argv);
+	cmd.git_cmd = 1;
+	if (run_command(&cmd))
+		die(FAILED_RUN, argv[0]);
+}
+
 static void gc_before_repack(void)
 {
 	/*
@@ -532,8 +547,8 @@ static void gc_before_repack(void)
 	if (pack_refs && maintenance_task_pack_refs(NULL))
 		die(FAILED_RUN, "pack-refs");
 
-	if (prune_reflogs && run_command_v_opt(reflog.v, RUN_GIT_CMD))
-		die(FAILED_RUN, reflog.v[0]);
+	if (prune_reflogs)
+		run_git_or_die(reflog.v);
 }
 
 int cmd_gc(int argc, const char **argv, const char *prefix)
@@ -571,6 +586,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	strvec_pushl(&reflog, "reflog", "expire", "--all", NULL);
 	strvec_pushl(&repack, "repack", "-d", "-l", NULL);
 	strvec_pushl(&prune, "prune", "--expire", NULL);
+	strvec_pushl(&prune_worktrees, "worktree", "prune", "--expire", NULL);
+	strvec_pushl(&rerere, "rerere", "gc", NULL);
 
 	/* default expiry time, overwritten in gc_config */
 	gc_config();
@@ -666,8 +683,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	gc_before_repack();
 
 	if (!repository_format_precious_objects) {
-		if (run_command_v_opt(repack.v,
-				      RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE))
+		struct child_process repack_cmd = CHILD_PROCESS_INIT;
+
+		strvec_pushv(&repack_cmd.args, repack.v);
+		repack_cmd.git_cmd = 1;
+		repack_cmd.close_object_store = 1;
+		if (run_command(&repack_cmd))
 			die(FAILED_RUN, repack.v[0]);
 
 		if (prune_expire) {
@@ -678,18 +699,16 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 			if (has_promisor_remote())
 				strvec_push(&prune,
 					    "--exclude-promisor-objects");
-			if (run_command_v_opt(prune.v, RUN_GIT_CMD))
-				die(FAILED_RUN, prune.v[0]);
+			run_git_or_die(prune.v);
 		}
 	}
 
-	if (prune_worktrees_expire &&
-	    run_command_l_opt(RUN_GIT_CMD, "worktree", "prune", "--expire",
-			      prune_worktrees_expire, NULL))
-		die(FAILED_RUN, "worktree");
+	if (prune_worktrees_expire) {
+		strvec_push(&prune_worktrees, prune_worktrees_expire);
+		run_git_or_die(prune_worktrees.v);
+	}
 
-	if (run_command_l_opt(RUN_GIT_CMD, "rerere", "gc", NULL))
-		die(FAILED_RUN, "rerere");
+	run_git_or_die(rerere.v);
 
 	report_garbage = report_pack_garbage;
 	reprepare_packed_git(the_repository);
@@ -1894,21 +1913,26 @@ static int is_schtasks_available(void)
 #endif
 }
 
-#define SCHTASKS_NAME_FMT "Git Maintenance (%s)"
+static char *schtasks_task_name(const char *frequency)
+{
+	struct strbuf label = STRBUF_INIT;
+	strbuf_addf(&label, "Git Maintenance (%s)", frequency);
+	return strbuf_detach(&label, NULL);
+}
 
 static int schtasks_remove_task(enum schedule_priority schedule)
 {
 	const char *cmd = "schtasks";
-	struct strvec args = STRVEC_INIT;
+	struct child_process child = CHILD_PROCESS_INIT;
 	const char *frequency = get_frequency(schedule);
+	char *name = schtasks_task_name(frequency);
 
 	get_schedule_cmd(&cmd, NULL);
-	strvec_split(&args, cmd);
-	strvec_pushl(&args, "/delete", "/tn", NULL);
-	strvec_pushf(&args, SCHTASKS_NAME_FMT, frequency);
-	strvec_pushl(&args, "/f", NULL);
+	strvec_split(&child.args, cmd);
+	strvec_pushl(&child.args, "/delete", "/tn", name, "/f", NULL);
+	free(name);
 
-	return run_command_sv_opt(&args, 0);
+	return run_command(&child);
 }
 
 static int schtasks_remove_tasks(void)
@@ -1926,6 +1950,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	const char *xml;
 	struct tempfile *tfile;
 	const char *frequency = get_frequency(schedule);
+	char *name = schtasks_task_name(frequency);
 	struct strbuf tfilename = STRBUF_INIT;
 
 	get_schedule_cmd(&cmd, NULL);
@@ -2018,10 +2043,8 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	      "</Task>\n";
 	fprintf(tfile->fp, xml, exec_path, exec_path, frequency);
 	strvec_split(&child.args, cmd);
-	strvec_pushl(&child.args, "/create", "/tn", NULL);
-	strvec_pushf(&child.args, SCHTASKS_NAME_FMT, frequency);
-	strvec_pushl(&child.args, "/f", "/xml",
-		     get_tempfile_path(tfile), NULL);
+	strvec_pushl(&child.args, "/create", "/tn", name, "/f", "/xml",
+				  get_tempfile_path(tfile), NULL);
 	close_tempfile_gently(tfile);
 
 	child.no_stdout = 1;
@@ -2032,6 +2055,7 @@ static int schtasks_schedule_task(const char *exec_path, enum schedule_priority
 	result = finish_command(&child);
 
 	delete_tempfile(&tfile);
+	free(name);
 	return result;
 }
 
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index c0383fe9df9..012f52bd007 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -12,6 +12,7 @@ static int merge_entry(int pos, const char *path)
 	const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
 	char hexbuf[4][GIT_MAX_HEXSZ + 1];
 	char ownbuf[4][60];
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
 	if (pos >= active_nr)
 		die("git merge-index: %s not in the cache", path);
@@ -31,7 +32,8 @@ static int merge_entry(int pos, const char *path)
 	if (!found)
 		die("git merge-index: %s not in the cache", path);
 
-	if (run_command_v_opt(arguments, 0)) {
+	strvec_pushv(&cmd.args, arguments);
+	if (run_command(&cmd)) {
 		if (one_shot)
 			err++;
 		else {
diff --git a/builtin/pull.c b/builtin/pull.c
index 2f36823c97e..b21edd767aa 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -515,73 +515,75 @@ static void parse_repo_refspecs(int argc, const char **argv, const char **repo,
  */
 static int run_fetch(const char *repo, const char **refspecs)
 {
-	struct strvec args = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
-	strvec_pushl(&args, "fetch", "--update-head-ok", NULL);
+	strvec_pushl(&cmd.args, "fetch", "--update-head-ok", NULL);
 
 	/* Shared options */
-	argv_push_verbosity(&args);
+	argv_push_verbosity(&cmd.args);
 	if (opt_progress)
-		strvec_push(&args, opt_progress);
+		strvec_push(&cmd.args, opt_progress);
 
 	/* Options passed to git-fetch */
 	if (opt_all)
-		strvec_push(&args, opt_all);
+		strvec_push(&cmd.args, opt_all);
 	if (opt_append)
-		strvec_push(&args, opt_append);
+		strvec_push(&cmd.args, opt_append);
 	if (opt_upload_pack)
-		strvec_push(&args, opt_upload_pack);
-	argv_push_force(&args);
+		strvec_push(&cmd.args, opt_upload_pack);
+	argv_push_force(&cmd.args);
 	if (opt_tags)
-		strvec_push(&args, opt_tags);
+		strvec_push(&cmd.args, opt_tags);
 	if (opt_prune)
-		strvec_push(&args, opt_prune);
+		strvec_push(&cmd.args, opt_prune);
 	if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
 		switch (recurse_submodules_cli) {
 		case RECURSE_SUBMODULES_ON:
-			strvec_push(&args, "--recurse-submodules=on");
+			strvec_push(&cmd.args, "--recurse-submodules=on");
 			break;
 		case RECURSE_SUBMODULES_OFF:
-			strvec_push(&args, "--recurse-submodules=no");
+			strvec_push(&cmd.args, "--recurse-submodules=no");
 			break;
 		case RECURSE_SUBMODULES_ON_DEMAND:
-			strvec_push(&args, "--recurse-submodules=on-demand");
+			strvec_push(&cmd.args, "--recurse-submodules=on-demand");
 			break;
 		default:
 			BUG("submodule recursion option not understood");
 		}
 	if (max_children)
-		strvec_push(&args, max_children);
+		strvec_push(&cmd.args, max_children);
 	if (opt_dry_run)
-		strvec_push(&args, "--dry-run");
+		strvec_push(&cmd.args, "--dry-run");
 	if (opt_keep)
-		strvec_push(&args, opt_keep);
+		strvec_push(&cmd.args, opt_keep);
 	if (opt_depth)
-		strvec_push(&args, opt_depth);
+		strvec_push(&cmd.args, opt_depth);
 	if (opt_unshallow)
-		strvec_push(&args, opt_unshallow);
+		strvec_push(&cmd.args, opt_unshallow);
 	if (opt_update_shallow)
-		strvec_push(&args, opt_update_shallow);
+		strvec_push(&cmd.args, opt_update_shallow);
 	if (opt_refmap)
-		strvec_push(&args, opt_refmap);
+		strvec_push(&cmd.args, opt_refmap);
 	if (opt_ipv4)
-		strvec_push(&args, opt_ipv4);
+		strvec_push(&cmd.args, opt_ipv4);
 	if (opt_ipv6)
-		strvec_push(&args, opt_ipv6);
+		strvec_push(&cmd.args, opt_ipv6);
 	if (opt_show_forced_updates > 0)
-		strvec_push(&args, "--show-forced-updates");
+		strvec_push(&cmd.args, "--show-forced-updates");
 	else if (opt_show_forced_updates == 0)
-		strvec_push(&args, "--no-show-forced-updates");
+		strvec_push(&cmd.args, "--no-show-forced-updates");
 	if (set_upstream)
-		strvec_push(&args, set_upstream);
-	strvec_pushv(&args, opt_fetch.v);
+		strvec_push(&cmd.args, set_upstream);
+	strvec_pushv(&cmd.args, opt_fetch.v);
 
 	if (repo) {
-		strvec_push(&args, repo);
-		strvec_pushv(&args, refspecs);
+		strvec_push(&cmd.args, repo);
+		strvec_pushv(&cmd.args, refspecs);
 	} else if (*refspecs)
 		BUG("refspecs without repo?");
-	return run_command_sv_opt(&args, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);
+	cmd.git_cmd = 1;
+	cmd.close_object_store = 1;
+	return run_command(&cmd);
 }
 
 /**
@@ -650,49 +652,50 @@ static int update_submodules(void)
  */
 static int run_merge(void)
 {
-	struct strvec args = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
-	strvec_pushl(&args, "merge", NULL);
+	strvec_pushl(&cmd.args, "merge", NULL);
 
 	/* Shared options */
-	argv_push_verbosity(&args);
+	argv_push_verbosity(&cmd.args);
 	if (opt_progress)
-		strvec_push(&args, opt_progress);
+		strvec_push(&cmd.args, opt_progress);
 
 	/* Options passed to git-merge */
 	if (opt_diffstat)
-		strvec_push(&args, opt_diffstat);
+		strvec_push(&cmd.args, opt_diffstat);
 	if (opt_log)
-		strvec_push(&args, opt_log);
+		strvec_push(&cmd.args, opt_log);
 	if (opt_signoff)
-		strvec_push(&args, opt_signoff);
+		strvec_push(&cmd.args, opt_signoff);
 	if (opt_squash)
-		strvec_push(&args, opt_squash);
+		strvec_push(&cmd.args, opt_squash);
 	if (opt_commit)
-		strvec_push(&args, opt_commit);
+		strvec_push(&cmd.args, opt_commit);
 	if (opt_edit)
-		strvec_push(&args, opt_edit);
+		strvec_push(&cmd.args, opt_edit);
 	if (cleanup_arg)
-		strvec_pushf(&args, "--cleanup=%s", cleanup_arg);
+		strvec_pushf(&cmd.args, "--cleanup=%s", cleanup_arg);
 	if (opt_ff)
-		strvec_push(&args, opt_ff);
+		strvec_push(&cmd.args, opt_ff);
 	if (opt_verify)
-		strvec_push(&args, opt_verify);
+		strvec_push(&cmd.args, opt_verify);
 	if (opt_verify_signatures)
-		strvec_push(&args, opt_verify_signatures);
-	strvec_pushv(&args, opt_strategies.v);
-	strvec_pushv(&args, opt_strategy_opts.v);
+		strvec_push(&cmd.args, opt_verify_signatures);
+	strvec_pushv(&cmd.args, opt_strategies.v);
+	strvec_pushv(&cmd.args, opt_strategy_opts.v);
 	if (opt_gpg_sign)
-		strvec_push(&args, opt_gpg_sign);
+		strvec_push(&cmd.args, opt_gpg_sign);
 	if (opt_autostash == 0)
-		strvec_push(&args, "--no-autostash");
+		strvec_push(&cmd.args, "--no-autostash");
 	else if (opt_autostash == 1)
-		strvec_push(&args, "--autostash");
+		strvec_push(&cmd.args, "--autostash");
 	if (opt_allow_unrelated_histories > 0)
-		strvec_push(&args, "--allow-unrelated-histories");
+		strvec_push(&cmd.args, "--allow-unrelated-histories");
 
-	strvec_push(&args, "FETCH_HEAD");
-	return run_command_sv_opt(&args, RUN_GIT_CMD);
+	strvec_push(&cmd.args, "FETCH_HEAD");
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }
 
 /**
@@ -873,40 +876,41 @@ static int get_rebase_newbase_and_upstream(struct object_id *newbase,
 static int run_rebase(const struct object_id *newbase,
 		const struct object_id *upstream)
 {
-	struct strvec args = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
-	strvec_push(&args, "rebase");
+	strvec_push(&cmd.args, "rebase");
 
 	/* Shared options */
-	argv_push_verbosity(&args);
+	argv_push_verbosity(&cmd.args);
 
 	/* Options passed to git-rebase */
 	if (opt_rebase == REBASE_MERGES)
-		strvec_push(&args, "--rebase-merges");
+		strvec_push(&cmd.args, "--rebase-merges");
 	else if (opt_rebase == REBASE_INTERACTIVE)
-		strvec_push(&args, "--interactive");
+		strvec_push(&cmd.args, "--interactive");
 	if (opt_diffstat)
-		strvec_push(&args, opt_diffstat);
-	strvec_pushv(&args, opt_strategies.v);
-	strvec_pushv(&args, opt_strategy_opts.v);
+		strvec_push(&cmd.args, opt_diffstat);
+	strvec_pushv(&cmd.args, opt_strategies.v);
+	strvec_pushv(&cmd.args, opt_strategy_opts.v);
 	if (opt_gpg_sign)
-		strvec_push(&args, opt_gpg_sign);
+		strvec_push(&cmd.args, opt_gpg_sign);
 	if (opt_signoff)
-		strvec_push(&args, opt_signoff);
+		strvec_push(&cmd.args, opt_signoff);
 	if (opt_autostash == 0)
-		strvec_push(&args, "--no-autostash");
+		strvec_push(&cmd.args, "--no-autostash");
 	else if (opt_autostash == 1)
-		strvec_push(&args, "--autostash");
+		strvec_push(&cmd.args, "--autostash");
 	if (opt_verify_signatures &&
 	    !strcmp(opt_verify_signatures, "--verify-signatures"))
 		warning(_("ignoring --verify-signatures for rebase"));
 
-	strvec_push(&args, "--onto");
-	strvec_push(&args, oid_to_hex(newbase));
+	strvec_push(&cmd.args, "--onto");
+	strvec_push(&cmd.args, oid_to_hex(newbase));
 
-	strvec_push(&args, oid_to_hex(upstream));
+	strvec_push(&cmd.args, oid_to_hex(upstream));
 
-	return run_command_sv_opt(&args, RUN_GIT_CMD);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }
 
 static int get_can_ff(struct object_id *orig_head,
diff --git a/builtin/remote.c b/builtin/remote.c
index 5d3534db19c..0cde2e244a4 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -92,13 +92,15 @@ static int verbose;
 
 static int fetch_remote(const char *name)
 {
-	const char *argv[] = { "fetch", name, NULL, NULL };
-	if (verbose) {
-		argv[1] = "-v";
-		argv[2] = name;
-	}
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_push(&cmd.args, "fetch");
+	if (verbose)
+		strvec_push(&cmd.args, "-v");
+	strvec_push(&cmd.args, name);
 	printf_ln(_("Updating %s"), name);
-	if (run_command_v_opt(argv, RUN_GIT_CMD))
+	cmd.git_cmd = 1;
+	if (run_command(&cmd))
 		return error(_("Could not fetch %s"), name);
 	return 0;
 }
@@ -1508,34 +1510,35 @@ static int update(int argc, const char **argv, const char *prefix)
 			 N_("prune remotes after fetching")),
 		OPT_END()
 	};
-	struct strvec fetch_argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int default_defined = 0;
 
 	argc = parse_options(argc, argv, prefix, options,
 			     builtin_remote_update_usage,
 			     PARSE_OPT_KEEP_ARGV0);
 
-	strvec_push(&fetch_argv, "fetch");
+	strvec_push(&cmd.args, "fetch");
 
 	if (prune != -1)
-		strvec_push(&fetch_argv, prune ? "--prune" : "--no-prune");
+		strvec_push(&cmd.args, prune ? "--prune" : "--no-prune");
 	if (verbose)
-		strvec_push(&fetch_argv, "-v");
-	strvec_push(&fetch_argv, "--multiple");
+		strvec_push(&cmd.args, "-v");
+	strvec_push(&cmd.args, "--multiple");
 	if (argc < 2)
-		strvec_push(&fetch_argv, "default");
+		strvec_push(&cmd.args, "default");
 	for (i = 1; i < argc; i++)
-		strvec_push(&fetch_argv, argv[i]);
+		strvec_push(&cmd.args, argv[i]);
 
-	if (strcmp(fetch_argv.v[fetch_argv.nr-1], "default") == 0) {
+	if (strcmp(cmd.args.v[cmd.args.nr-1], "default") == 0) {
 		git_config(get_remote_default, &default_defined);
 		if (!default_defined) {
-			strvec_pop(&fetch_argv);
-			strvec_push(&fetch_argv, "--all");
+			strvec_pop(&cmd.args);
+			strvec_push(&cmd.args, "--all");
 		}
 	}
 
-	return run_command_sv_opt(&fetch_argv, RUN_GIT_CMD);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }
 
 static int remove_all_fetch_refspecs(const char *key)
diff --git a/compat/mingw.c b/compat/mingw.c
index 4f5392c5796..d614f156df1 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -196,15 +196,20 @@ static int read_yes_no_answer(void)
 static int ask_yes_no_if_possible(const char *format, ...)
 {
 	char question[4096];
-	char *retry_hook;
+	const char *retry_hook;
 	va_list args;
 
 	va_start(args, format);
 	vsnprintf(question, sizeof(question), format, args);
 	va_end(args);
 
-	if ((retry_hook = mingw_getenv("GIT_ASK_YESNO")))
-		return !run_command_l_opt(0, retry_hook, question, NULL);
+	retry_hook = mingw_getenv("GIT_ASK_YESNO");
+	if (retry_hook) {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		strvec_pushl(&cmd.args, retry_hook, question, NULL);
+		return !run_command(&cmd);
+	}
 
 	if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
 		return 0;
diff --git a/diff.c b/diff.c
index 392530016fa..8451c230d9e 100644
--- a/diff.c
+++ b/diff.c
@@ -4278,8 +4278,8 @@ static void run_external_diff(const char *pgm,
 			      const char *xfrm_msg,
 			      struct diff_options *o)
 {
-	struct diff_queue_struct *q = &diff_queued_diff;
 	struct child_process cmd = CHILD_PROCESS_INIT;
+	struct diff_queue_struct *q = &diff_queued_diff;
 
 	strvec_push(&cmd.args, pgm);
 	strvec_push(&cmd.args, name);
@@ -4295,7 +4295,8 @@ static void run_external_diff(const char *pgm,
 		}
 	}
 
-	strvec_pushf(&cmd.env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter);
+	strvec_pushf(&cmd.env, "GIT_DIFF_PATH_COUNTER=%d",
+		     ++o->diff_path_counter);
 	strvec_pushf(&cmd.env, "GIT_DIFF_PATH_TOTAL=%d", q->nr);
 
 	diff_free_filespec_data(one);
diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index 19d772f0f3a..96d8d37c06d 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -56,11 +56,11 @@ static int spawn_daemon(void)
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 
+	strvec_pushl(&cmd.args, "fsmonitor--daemon", "start", NULL);
+
 	cmd.git_cmd = 1;
 	cmd.no_stdin = 1;
 	cmd.trace2_child_class = "fsmonitor";
-	strvec_pushl(&cmd.args, "fsmonitor--daemon", "start", NULL);
-
 	return run_command(&cmd);
 }
 
diff --git a/ll-merge.c b/ll-merge.c
index 740689b7bd6..b20491043e0 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -193,6 +193,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf_expand_dict_entry dict[6];
 	struct strbuf path_sq = STRBUF_INIT;
+	struct child_process child = CHILD_PROCESS_INIT;
 	int status, fd, i;
 	struct stat st;
 	enum ll_merge_result ret;
@@ -218,7 +219,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 
 	strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);
 
-	status = run_command_l_opt(RUN_USING_SHELL, cmd.buf, NULL);
+	strvec_push(&child.args, cmd.buf);
+	child.use_shell = 1;
+	status = run_command(&child);
 	fd = open(temp[1], O_RDONLY);
 	if (fd < 0)
 		goto bad;
diff --git a/merge.c b/merge.c
index 487debacecb..445b4f19aa8 100644
--- a/merge.c
+++ b/merge.c
@@ -19,21 +19,22 @@ int try_merge_command(struct repository *r,
 		      const char **xopts, struct commit_list *common,
 		      const char *head_arg, struct commit_list *remotes)
 {
-	struct strvec args = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int i, ret;
 	struct commit_list *j;
 
-	strvec_pushf(&args, "merge-%s", strategy);
+	strvec_pushf(&cmd.args, "merge-%s", strategy);
 	for (i = 0; i < xopts_nr; i++)
-		strvec_pushf(&args, "--%s", xopts[i]);
+		strvec_pushf(&cmd.args, "--%s", xopts[i]);
 	for (j = common; j; j = j->next)
-		strvec_push(&args, merge_argument(j->item));
-	strvec_push(&args, "--");
-	strvec_push(&args, head_arg);
+		strvec_push(&cmd.args, merge_argument(j->item));
+	strvec_push(&cmd.args, "--");
+	strvec_push(&cmd.args, head_arg);
 	for (j = remotes; j; j = j->next)
-		strvec_push(&args, merge_argument(j->item));
+		strvec_push(&cmd.args, merge_argument(j->item));
 
-	ret = run_command_sv_opt(&args, RUN_GIT_CMD);
+	cmd.git_cmd = 1;
+	ret = run_command(&cmd);
 
 	discard_index(r->index);
 	if (repo_read_index(r) < 0)
diff --git a/scalar.c b/scalar.c
index 3480bf73cbd..03f9e480dd6 100644
--- a/scalar.c
+++ b/scalar.c
@@ -69,17 +69,18 @@ static void setup_enlistment_directory(int argc, const char **argv,
 
 static int run_git(const char *arg, ...)
 {
-	struct strvec argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	va_list args;
 	const char *p;
 
 	va_start(args, arg);
-	strvec_push(&argv, arg);
+	strvec_push(&cmd.args, arg);
 	while ((p = va_arg(args, const char *)))
-		strvec_push(&argv, p);
+		strvec_push(&cmd.args, p);
 	va_end(args);
 
-	return run_command_sv_opt(&argv, RUN_GIT_CMD);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }
 
 struct scalar_config {
diff --git a/sequencer.c b/sequencer.c
index 7ee0e05512c..9b9d6a55828 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3183,14 +3183,15 @@ static int rollback_is_safe(void)
 
 static int reset_merge(const struct object_id *oid)
 {
-	struct strvec argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
-	strvec_pushl(&argv, "reset", "--merge", NULL);
+	strvec_pushl(&cmd.args, "reset", "--merge", NULL);
 
 	if (!is_null_oid(oid))
-		strvec_push(&argv, oid_to_hex(oid));
+		strvec_push(&cmd.args, oid_to_hex(oid));
 
-	return run_command_sv_opt(&argv, RUN_GIT_CMD);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }
 
 static int rollback_single_pick(struct repository *r)
@@ -3554,10 +3555,13 @@ static int error_failed_squash(struct repository *r,
 
 static int do_exec(struct repository *r, const char *command_line)
 {
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int dirty, status;
 
 	fprintf(stderr, _("Executing: %s\n"), command_line);
-	status = run_command_l_opt(RUN_USING_SHELL, command_line, NULL);
+	strvec_push(&cmd.args, command_line);
+	cmd.use_shell = 1;
+	status = run_command(&cmd);
 
 	/* force re-reading of the cache */
 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
@@ -4861,13 +4865,13 @@ static int pick_commits(struct repository *r,
 
 static int continue_single_pick(struct repository *r, struct replay_opts *opts)
 {
-	struct strvec argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
 	if (!refs_ref_exists(get_main_ref_store(r), "CHERRY_PICK_HEAD") &&
 	    !refs_ref_exists(get_main_ref_store(r), "REVERT_HEAD"))
 		return error(_("no cherry-pick or revert in progress"));
 
-	strvec_push(&argv, "commit");
+	strvec_push(&cmd.args, "commit");
 
 	/*
 	 * continue_single_pick() handles the case of recovering from a
@@ -4880,9 +4884,10 @@ static int continue_single_pick(struct repository *r, struct replay_opts *opts)
 		 * Include --cleanup=strip as well because we don't want the
 		 * "# Conflicts:" messages.
 		 */
-		strvec_pushl(&argv, "--no-edit", "--cleanup=strip", NULL);
+		strvec_pushl(&cmd.args, "--no-edit", "--cleanup=strip", NULL);
 
-	return run_command_sv_opt(&argv, RUN_GIT_CMD);
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }
 
 static int commit_staged_changes(struct repository *r,
diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c
index 7967478a40a..42185f3fc05 100644
--- a/t/helper/test-fake-ssh.c
+++ b/t/helper/test-fake-ssh.c
@@ -8,6 +8,7 @@ int cmd_main(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f;
 	int i;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 
 	/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */
 	if (!trash_directory)
@@ -24,5 +25,7 @@ int cmd_main(int argc, const char **argv)
 	/* Now, evaluate the *last* parameter */
 	if (argc < 2)
 		return 0;
-	return run_command_l_opt(RUN_USING_SHELL, argv[argc - 1], NULL);
+	strvec_push(&cmd.args, argv[argc - 1]);
+	cmd.use_shell = 1;
+	return run_command(&cmd);
 }
diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index a714130ece7..94fd8ccf513 100644
--- a/t/helper/test-trace2.c
+++ b/t/helper/test-trace2.c
@@ -132,6 +132,7 @@ static int ut_003error(int argc, const char **argv)
  */
 static int ut_004child(int argc, const char **argv)
 {
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int result;
 
 	/*
@@ -141,7 +142,8 @@ static int ut_004child(int argc, const char **argv)
 	if (!argc)
 		return 0;
 
-	result = run_command_v_opt(argv, 0);
+	strvec_pushv(&cmd.args, argv);
+	result = run_command(&cmd);
 	exit(result);
 }
 
diff --git a/tmp-objdir.h b/tmp-objdir.h
index c96aa77396d..615b1885733 100644
--- a/tmp-objdir.h
+++ b/tmp-objdir.h
@@ -10,11 +10,9 @@
  *
  * Example:
  *
- *	struct child_process cmd = CHILD_PROCESS_INIT;
- *	...
  *	struct tmp_objdir *t = tmp_objdir_create("incoming");
- *      strvec_pushl(&cmd.env, tmp_objdir_env(t));
- *	if (!run_command(&cmd) && !tmp_objdir_migrate(t))
+ *	strvec_pushv(&cmd.env, tmp_objdir_env(t));
+ *	if (!run_command(&cmd)) && !tmp_objdir_migrate(t))
  *		printf("success!\n");
  *	else
  *		die("failed...tmp_objdir will clean up for us");

Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[Jeff King]

On Tue, Oct 18, 2022 at 03:28:43PM +0200, Ævar Arnfjörð Bjarmason wrote:

> > Hmph...  I somehow thought that the concensus is rather try the
> > complete opposite approach shown by René's patch to lose the use of
> > run_command_v_opt() by replacing it with run_command(&args), with
> > args.v populated using strvec_pushl() and other strvec API
> > functions.
> >
> > One of the reasons I would prefer to see us moving in that direction
> > was because the first iteration of this series was a good
> > demonstration of the relatively limited usefulness of _l_opt()
> > variant and also it seemed to be error prone to use it.
> 
> I'm getting somewhat mixed messages. Jeff seemed to like the end-to-end
> safety of run_command_l_opt() before I wrote it. I think the
> run_command_l_opt() still really shines for the simple cases.

Sorry if I gave that impression. I like the safety of strvec_pushl(),
but I think using it with a "struct child_process" is just fine. It's
more flexible, and as René's patch showed, doesn't really make the
callers more complex (that definitely _wasn't_ the case long ago, when
most of these callers were written).

I do think Junio saying "consensus" may have been premature. I expressed
my opinion and he agreed, but I think that is as far as it got. :)

> I don't see how *_l_opt() is particularly error prone, I just had a
> stupid think-o in v1 of this, but that if/else if bug is something that
> could have snuck in with run_command() given the same stupidity :)

I don't think it's error-prone. It just seems like it complicates an API
for little gain, and causes us to have a lot of boilerplate mapping
RUN_* flags into cmd.* fields.

> I wonder if a run_command() that just had the prototype (struct
> child_process *cmd, ...) might not be the best of both worlds (or a
> run_commandl() alternative). I.e. to do away with the whole custom way
> of specifying the flag(s), and just take the passed-in arguments and
> append them to "&cmd.args".

That would work, but is it buying much? You still have to declare the
child_process at that point, which means you have:

  struct child_process cmd = CHILD_PROCESS_INIT;
  cmd.git_cmd = 1;
  run_command(&cmd, "log", "--", "HEAD", NULL);

instead of:

  struct child_process cmd = CHILD_PROCESS_INIT;
  cmd.git_cmd = 1;
  strvec_pushl(&cmd.args, "log", "--", "HEAD", NULL);
  run_command(&cmd);

Saving one line doesn't seem worth complicating the API to me. Plus
existing users have to say "run_command(&cmd, NULL)", or we need to
ignore varargs when "cmd.args.nr > 0" (which papers over errors).

And most of the time run_command() is inside an if() anyway. Just
style-wise, keeping the long command name on its own line is a
readability improvement (IMHO, anyway).

> It's also interesting to consider adding a --noop option to be supported
> by parse-options.c. The reason we can't use a run_command_l_opt() in
> some cases is because we're doing e.g.:
> 
> 	if (progress)
> 		strvec_push(&args, "--progress");
> 
> We have a --no-progress, but in those cases the recipient at the end
> often cares about a default of -1 for a bool variable, or similar. So if
> we could do:
> 
> 	run_command_l_opt(0, command,
> 		(progress ? "--progress" : "--noop"),
> 		...,
> 		NULL
> 	);

I dunno. That does not seem more readable to me, and would end up with
GIT_TRACE lines like:

  git foo --noop --noop --real-option --noop

> We could benefit from compile-time checks, and wouldn't have to allocate
> a strvec just for building up the arguments in some cases. Just food for
> thought...

Keep in mind we allocate a strvec either way. And IMHO seeing:

  if (foo)
          strvec_push(&cmd.args, "foo");

is the most readable form. Not to mention that it is more flexible. Many
cases use "pushf" for the same thing.

-Peff

Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I do think Junio saying "consensus" may have been premature. I expressed
> my opinion and he agreed, but I think that is as far as it got. :)

Maybe.  This is a tangent, but as far as I am concerned, when Réne
writes something that looks to me very straight-forward and correct,
and it passes your taste buds, then that is enough consensus to move
ahead.  As Linus often said and I concur, some people got good
taste, that is hard to quantify and probably hard to teach, and
there are a handful folks here with good taste.  And when two who
have demonstrated they are with good taste agrees, that is good
enough to me.

>> I don't see how *_l_opt() is particularly error prone, I just had a
>> stupid think-o in v1 of this, but that if/else if bug is something that
>> could have snuck in with run_command() given the same stupidity :)
>
> I don't think it's error-prone. It just seems like it complicates an API
> for little gain, and causes us to have a lot of boilerplate mapping
> RUN_* flags into cmd.* fields.

True. run_command() needs the RUN_* flags twiddling, too, so it is
not a point against _l_opt() variant.  What I see as its biggest
problem is it is a bit too rigid for many of the current callers
that use _v_opt() to replace them, and can easily tempt developers
to write almost-duplicate _l_opt() calls, leading to an easily
avoidable bug like we saw in the if/else if/else cascade.

Thanks.

Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[Jeff King]

On Wed, Oct 19, 2022 at 08:43:43AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I do think Junio saying "consensus" may have been premature. I expressed
> > my opinion and he agreed, but I think that is as far as it got. :)
> 
> Maybe.  This is a tangent, but as far as I am concerned, when Réne
> writes something that looks to me very straight-forward and correct,
> and it passes your taste buds, then that is enough consensus to move
> ahead.  As Linus often said and I concur, some people got good
> taste, that is hard to quantify and probably hard to teach, and
> there are a handful folks here with good taste.  And when two who
> have demonstrated they are with good taste agrees, that is good
> enough to me.

I pretty much agree with that worldview, but I try hard not to assume
"my taste is good" when going into a technical discussion. If only
because I've embarrassed myself a sufficient number of times. :)

> >> I don't see how *_l_opt() is particularly error prone, I just had a
> >> stupid think-o in v1 of this, but that if/else if bug is something that
> >> could have snuck in with run_command() given the same stupidity :)
> >
> > I don't think it's error-prone. It just seems like it complicates an API
> > for little gain, and causes us to have a lot of boilerplate mapping
> > RUN_* flags into cmd.* fields.
> 
> True. run_command() needs the RUN_* flags twiddling, too, so it is
> not a point against _l_opt() variant.

Did you mean run_command_v() here? If so, then yes, it requires the
flags. But if we are going to drop it in favor of just run_command(),
then those flags go away (and moving to the _l() variant is a step in
the opposite direction).

-Peff

Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> True. run_command() needs the RUN_* flags twiddling, too, so it is
>> not a point against _l_opt() variant.
>
> Did you mean run_command_v() here? If so, then yes, it requires the
> flags. But if we are going to drop it in favor of just run_command(),
> then those flags go away (and moving to the _l() variant is a step in
> the opposite direction).

Ah, but we'd still need to prepare individual bits in the child
structure (e.g. RUN_GIT_CMD vs .git_cmd) anyway.  Even though we may
not have to work with the flags, somehow we need to set it up.  So
it is not all that strong argument against the _l_opt().

The above assessment is primarily because I do not have too much
against RUN_GIT_CMD and friends, compared to setting the individual
members in the child_process struct.  Setting individual members in
the struct is more direct and it may be an improvement use
run_command() directly over using _v_opt() and _l_opt() variants.

Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[Jeff King]

On Wed, Oct 19, 2022 at 11:00:22AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> True. run_command() needs the RUN_* flags twiddling, too, so it is
> >> not a point against _l_opt() variant.
> >
> > Did you mean run_command_v() here? If so, then yes, it requires the
> > flags. But if we are going to drop it in favor of just run_command(),
> > then those flags go away (and moving to the _l() variant is a step in
> > the opposite direction).
> 
> Ah, but we'd still need to prepare individual bits in the child
> structure (e.g. RUN_GIT_CMD vs .git_cmd) anyway.  Even though we may
> not have to work with the flags, somehow we need to set it up.  So
> it is not all that strong argument against the _l_opt().

Right, to the callers it is no different; they must use the flags or the
struct fields. What I meant is that we do not need to support _both_.
I.e., the big mapping in run_command_v_opt_cd_env_tr2() goes away.

> The above assessment is primarily because I do not have too much
> against RUN_GIT_CMD and friends, compared to setting the individual
> members in the child_process struct.  Setting individual members in
> the struct is more direct and it may be an improvement use
> run_command() directly over using _v_opt() and _l_opt() variants.

Yeah. I do not find RUN_GIT_CMD all that bad myself; I was mainly
pointing out that we do not need to support both of them (and of the
two, the struct fields are by far the more flexible, so that's what we
should keep).

That said, I am not too sad to have both. I would not have bothered to
do the work to remove the _v() versions with flags. But since René
already did so...

-Peff

Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> That said, I am not too sad to have both. I would not have bothered to
> do the work to remove the _v() versions with flags. But since René
> already did so...

It makes two of us ;-)

Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[René Scharfe]

Am 19.10.22 um 21:41 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
>
>> That said, I am not too sad to have both. I would not have bothered to
>> do the work to remove the _v() versions with flags. But since René
>> already did so...
>
> It makes two of us ;-)

I wrote that patch out of curiosity and was pleased by the total line
count reduction, getting rid of the on-stack array construction sites
with their magic numbers, not having to manage strvecs explicitly
anymore and the callers staying readable.

One weak spot is the new helper builtin/gc.c::run_git_or_die() that I
added because it was easier than replacing all those strvecs that are
prepared before deciding whether their commands are even needed.

Stripping down the central API to a single shared object (a struct and
functions that get it passed) simplifies it for programmers.  It takes
the idea of d3b2159712 (run-command API: remove "argv" member, always
use "args", 2021-11-25) and c7c4bdeccf (run-command API: remove "env"
member, always use "env_array", 2021-11-25) to its logical conclusion
of going fully dynamic and using standard strvec functions everywhere.
Local shortcuts like builtin/gc.c::run_git_or_die() may still be
defensible.

But still: Is all of that code churn worth it?  Not sure.

René

Re: [PATCH v2 00/10] run-command API: add run_command_{l,sv}_opt()

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> I wrote that patch out of curiosity and was pleased by the total line
> count reduction, getting rid of the on-stack array construction sites
> with their magic numbers, not having to manage strvecs explicitly
> anymore and the callers staying readable.
> ...
> But still: Is all of that code churn worth it?  Not sure.

Compared to doing nothing?  The result did look easier to grok,
especially as we no longer need to worry about another way to do the
same thing (i.e. run_command() vs run_command_v_*() variants) and
can get rid of the flags constants that need to be kept in sync with
the members of the child_process struct.

Compared to adding the _l_opt() variant?  Surely, as that goes the
other direction to add more callers that use the flags constants,
without much practical gain (which was a bit sad, but we only found
that out after seeing the result).