[PATCH 0/8] run-command: remove run_command_v_*()

[PATCH 0/8] run-command: remove run_command_v_*()

[René Scharfe]

Replace the convenience functions run_command_v_opt() et. al. and use
struct child_process and run_command() directly instead, for an overall
code reduction and a simpler and more flexible API that allows creating
argument lists without magic numbers and reduced risk of memory leaks.

This is a broken-out and polished version of the original scratch at
https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/

Ævar Arnfjörð Bjarmason (1):
  merge: remove always-the-same "verbose" arguments

René Scharfe (7):
  bisect--helper: factor out do_bisect_run()
  use child_process members "args" and "env" directly
  use child_process member "args" instead of string array variable
  replace and remove run_command_v_opt_cd_env()
  replace and remove run_command_v_opt_tr2()
  replace and remove run_command_v_opt_cd_env_tr2()
  replace and remove run_command_v_opt()

 add-interactive.c        |   9 ++-
 bisect.c                 |  12 ++--
 builtin/add.c            |  19 +++--
 builtin/am.c             |  12 ++--
 builtin/bisect--helper.c |  68 +++++++++---------
 builtin/clone.c          |  41 ++++++-----
 builtin/difftool.c       |  24 ++++---
 builtin/fetch.c          |   9 ++-
 builtin/gc.c             |  55 ++++++++++-----
 builtin/merge-index.c    |   4 +-
 builtin/merge.c          |  53 ++++++--------
 builtin/pull.c           | 147 +++++++++++++++++++--------------------
 builtin/remote.c         |  40 +++++------
 compat/mingw.c           |  11 +--
 diff.c                   |  27 ++++---
 fsmonitor-ipc.c          |  10 ++-
 git.c                    |  15 ++--
 ll-merge.c               |   7 +-
 merge.c                  |  18 ++---
 run-command.c            |  35 ----------
 run-command.h            |  33 +--------
 scalar.c                 |  13 ++--
 sequencer.c              |  32 ++++-----
 shell.c                  |  17 +++--
 t/helper/test-fake-ssh.c |   7 +-
 t/helper/test-trace2.c   |   4 +-
 tmp-objdir.h             |   4 +-
 27 files changed, 344 insertions(+), 382 deletions(-)

--
2.38.1

[PATCH 1/8] merge: remove always-the-same "verbose" arguments

[René Scharfe]

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

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>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/merge.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 5900b81729..3bb49d805b 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.1

Re: [PATCH 1/8] merge: remove always-the-same "verbose" arguments

[Taylor Blau]

On Thu, Oct 27, 2022 at 06:35:02PM +0200, René Scharfe wrote:
> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> 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.

All looks reasonable here. Thanks for the detailed explanation of when
and why these changed and at which point they became redundant.

Thanks,
Taylor

[PATCH 2/8] bisect--helper: factor out do_bisect_run()

[René Scharfe]

Deduplicate the code for reporting and starting the bisect run command
by moving it to a short helper function.  Use a string array instead of
a strvec to prepare the arguments, for simplicity.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/bisect--helper.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 28ef7ec2a4..70d1e9d1ad 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -1142,8 +1142,14 @@ static int get_first_good(const char *refname UNUSED,
 	return 1;
 }

-static int verify_good(const struct bisect_terms *terms,
-		       const char **quoted_argv)
+static int do_bisect_run(const char *command)
+{
+	const char *argv[] = { command, NULL };
+	printf(_("running %s\n"), command);
+	return run_command_v_opt(argv, RUN_USING_SHELL);
+}
+
+static int verify_good(const struct bisect_terms *terms, const char *command)
 {
 	int rc;
 	enum bisect_error res;
@@ -1163,8 +1169,7 @@ static int verify_good(const struct bisect_terms *terms,
 	if (res != BISECT_OK)
 		return -1;

-	printf(_("running %s\n"), quoted_argv[0]);
-	rc = run_command_v_opt(quoted_argv, RUN_USING_SHELL);
+	rc = do_bisect_run(command);

 	res = bisect_checkout(&current_rev, no_checkout);
 	if (res != BISECT_OK)
@@ -1177,7 +1182,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 {
 	int res = BISECT_OK;
 	struct strbuf command = STRBUF_INIT;
-	struct strvec run_args = STRVEC_INIT;
 	const char *new_state;
 	int temporary_stdout_fd, saved_stdout;
 	int is_first_run = 1;
@@ -1192,11 +1196,8 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		return BISECT_FAILED;
 	}

-	strvec_push(&run_args, command.buf);
-
 	while (1) {
-		printf(_("running %s\n"), command.buf);
-		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
+		res = do_bisect_run(command.buf);

 		/*
 		 * Exit code 126 and 127 can either come from the shell
@@ -1206,7 +1207,7 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		 * missing or non-executable script.
 		 */
 		if (is_first_run && (res == 126 || res == 127)) {
-			int rc = verify_good(terms, run_args.v);
+			int rc = verify_good(terms, command.buf);
 			is_first_run = 0;
 			if (rc < 0) {
 				error(_("unable to verify '%s' on good"
@@ -1273,7 +1274,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 	}

 	strbuf_release(&command);
-	strvec_clear(&run_args);
 	return res;
 }

--
2.38.1

Re: [PATCH 2/8] bisect--helper: factor out do_bisect_run()

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 27 2022, René Scharfe wrote:

> Deduplicate the code for reporting and starting the bisect run command
> by moving it to a short helper function.  Use a string array instead of
> a strvec to prepare the arguments, for simplicity.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/bisect--helper.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 28ef7ec2a4..70d1e9d1ad 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -1142,8 +1142,14 @@ static int get_first_good(const char *refname UNUSED,
>  	return 1;
>  }
>
> -static int verify_good(const struct bisect_terms *terms,
> -		       const char **quoted_argv)
> +static int do_bisect_run(const char *command)
> +{
> +	const char *argv[] = { command, NULL };
> +	printf(_("running %s\n"), command);
> +	return run_command_v_opt(argv, RUN_USING_SHELL);
> +}
> +
> +static int verify_good(const struct bisect_terms *terms, const char *command)
>  {
>  	int rc;
>  	enum bisect_error res;
> @@ -1163,8 +1169,7 @@ static int verify_good(const struct bisect_terms *terms,
>  	if (res != BISECT_OK)
>  		return -1;
>
> -	printf(_("running %s\n"), quoted_argv[0]);
> -	rc = run_command_v_opt(quoted_argv, RUN_USING_SHELL);
> +	rc = do_bisect_run(command);
>
>  	res = bisect_checkout(&current_rev, no_checkout);
>  	if (res != BISECT_OK)
> [...]

Okey, so we end up with a helper just for the convenience of doing that
printf at the start, that's fine.

But given the line count of some of the other changes, and
e.g. including the free(), oid_to_hex_r() to oid_to_hex() etc. in later
commits I don't see why we can't just make it use run_command()
directly.

I.e. I think it makes sense as one commit, but the conversion is easy
enough, easier than looking at the same code again later in the
series...

Re: [PATCH 2/8] bisect--helper: factor out do_bisect_run()

[Taylor Blau]

On Fri, Oct 28, 2022 at 12:26:51AM +0200, Ævar Arnfjörð Bjarmason wrote:
> But given the line count of some of the other changes, and
> e.g. including the free(), oid_to_hex_r() to oid_to_hex() etc. in later
> commits I don't see why we can't just make it use run_command()
> directly.
>
> I.e. I think it makes sense as one commit, but the conversion is easy
> enough, easier than looking at the same code again later in the
> series...

Hmmph. I don't think that it matters much, either way. I was initially
confused not having read the whole series, but I see what you're saying
now.

We see the same part of the diff twice here: the post-image of this hunk
to use run_command_v_opt() becomes the pre-image of a hunk in one of the
later patches to change it to use run_command() directly.

Arguably the two could be combined into a single patch, yes. Though I
find the version as written here to be clearer, since it's a
straightforward code movement within bisect--helper.

Thanks,
Taylor

[PATCH 3/8] use child_process members "args" and "env" directly

[René Scharfe]

Build argument list and environment of child processes by using
struct child_process and populating its members "args" and "env"
directly instead of maintaining separate strvecs and letting
run_command_v_opt() and friends populate these members.  This is
simpler, shorter and slightly more efficient.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 add-interactive.c        |   9 ++-
 builtin/add.c            |  19 +++--
 builtin/bisect--helper.c |  35 +++++-----
 builtin/clone.c          |  33 ++++-----
 builtin/gc.c             |  14 ++--
 builtin/merge.c          |  10 +--
 builtin/pull.c           | 147 +++++++++++++++++++--------------------
 builtin/remote.c         |  26 ++++---
 diff.c                   |  27 ++++---
 git.c                    |  15 ++--
 merge.c                  |  18 ++---
 scalar.c                 |  13 ++--
 sequencer.c              |  25 +++----
 13 files changed, 185 insertions(+), 206 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index f071b2a1b4..ecc5ae1b24 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -997,18 +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_v_opt(args.v, 0);
-		strvec_clear(&args);
+		res = run_command(&cmd);
 	}

 	putchar('\n');
diff --git a/builtin/add.c b/builtin/add.c
index f84372964c..626c71ec6a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -240,8 +240,8 @@ 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;
-	struct strvec argv = STRVEC_INIT;
+	int i;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int use_builtin_add_i =
 		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);

@@ -272,19 +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);

-	status = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-	return status;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 int interactive_add(const char **argv, const char *prefix, int patch)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 70d1e9d1ad..5c63ba6994 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -220,18 +220,17 @@ static int bisect_reset(const char *commit)
 	}

 	if (!ref_exists("BISECT_HEAD")) {
-		struct strvec argv = STRVEC_INIT;
+		struct child_process cmd = CHILD_PROCESS_INIT;

-		strvec_pushl(&argv, "checkout", branch.buf, "--", NULL);
-		if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+		cmd.git_cmd = 1;
+		strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL);
+		if (run_command(&cmd)) {
 			error(_("could not check out original"
 				" HEAD '%s'. Try 'git bisect"
 				" reset <commit>'."), branch.buf);
 			strbuf_release(&branch);
-			strvec_clear(&argv);
 			return -1;
 		}
-		strvec_clear(&argv);
 	}

 	strbuf_release(&branch);
@@ -1098,40 +1097,38 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar

 static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)
 {
-	struct strvec args = STRVEC_INIT;
-	int flags = RUN_COMMAND_NO_STDIN, res = 0;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;

 	if (bisect_next_check(terms, NULL) != 0)
 		return BISECT_FAILED;

+	cmd.no_stdin = 1;
 	if (!argc) {
 		if ((getenv("DISPLAY") || getenv("SESSIONNAME") || getenv("MSYSTEM") ||
 		     getenv("SECURITYSESSIONID")) && exists_in_PATH("gitk")) {
-			strvec_push(&args, "gitk");
+			strvec_push(&cmd.args, "gitk");
 		} else {
-			strvec_push(&args, "log");
-			flags |= RUN_GIT_CMD;
+			strvec_push(&cmd.args, "log");
+			cmd.git_cmd = 1;
 		}
 	} else {
 		if (argv[0][0] == '-') {
-			strvec_push(&args, "log");
-			flags |= RUN_GIT_CMD;
+			strvec_push(&cmd.args, "log");
+			cmd.git_cmd = 1;
 		} else if (strcmp(argv[0], "tig") && !starts_with(argv[0], "git"))
-			flags |= RUN_GIT_CMD;
+			cmd.git_cmd = 1;

-		strvec_pushv(&args, argv);
+		strvec_pushv(&cmd.args, argv);
 	}

-	strvec_pushl(&args, "--bisect", "--", NULL);
+	strvec_pushl(&cmd.args, "--bisect", "--", NULL);

 	strbuf_read_file(&sb, git_path_bisect_names(), 0);
-	sq_dequote_to_strvec(sb.buf, &args);
+	sq_dequote_to_strvec(sb.buf, &cmd.args);
 	strbuf_release(&sb);

-	res = run_command_v_opt(args.v, flags);
-	strvec_clear(&args);
-	return res;
+	return run_command(&cmd);
 }

 static int get_first_good(const char *refname UNUSED,
diff --git a/builtin/clone.c b/builtin/clone.c
index 547d6464b3..56e7775dae 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -653,9 +653,9 @@ 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;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int result = 0;
-	strvec_pushl(&argv, "-C", repo, "sparse-checkout", "set", NULL);
+	strvec_pushl(&cmd.args, "-C", repo, "sparse-checkout", "set", NULL);

 	/*
 	 * We must apply the setting in the current process
@@ -663,12 +663,12 @@ static int git_sparse_checkout_init(const char *repo)
 	 */
 	core_apply_sparse_checkout = 1;

-	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+	cmd.git_cmd = 1;
+	if (run_command(&cmd)) {
 		error(_("failed to initialize sparse-checkout"));
 		result = 1;
 	}

-	strvec_clear(&argv);
 	return result;
 }

@@ -733,37 +733,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");

-		err = run_command_v_opt(args.v, RUN_GIT_CMD);
-		strvec_clear(&args);
+		cmd.git_cmd = 1;
+		err = run_command(&cmd);
 	}

 	return err;
diff --git a/builtin/gc.c b/builtin/gc.c
index 243ee85d28..87ad0077d8 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1910,20 +1910,16 @@ static char *schtasks_task_name(const char *frequency)
 static int schtasks_remove_task(enum schedule_priority schedule)
 {
 	const char *cmd = "schtasks";
-	int result;
-	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", name, "/f", NULL);
-
-	result = run_command_v_opt(args.v, 0);
-
-	strvec_clear(&args);
+	strvec_split(&child.args, cmd);
+	strvec_pushl(&child.args, "/delete", "/tn", name, "/f", NULL);
 	free(name);
-	return result;
+
+	return run_command(&child);
 }

 static int schtasks_remove_tasks(void)
diff --git a/builtin/merge.c b/builtin/merge.c
index 3bb49d805b..864808f51a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -380,22 +380,22 @@ static void reset_hard(const struct object_id *oid)
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	struct strvec args = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_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));
+	strvec_pushl(&cmd.args, "stash", "apply", "--index", "--quiet", NULL);
+	strvec_push(&cmd.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);
+	cmd.git_cmd = 1;
+	run_command(&cmd);

 refresh_cache:
 	if (discard_cache() < 0 || read_cache() < 0)
diff --git a/builtin/pull.c b/builtin/pull.c
index 403a24d7ca..b21edd767a 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -515,76 +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;
-	int ret;
+	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?");
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);
-	strvec_clear(&args);
-	return ret;
+	cmd.git_cmd = 1;
+	cmd.close_object_store = 1;
+	return run_command(&cmd);
 }

 /**
@@ -653,52 +652,50 @@ static int update_submodules(void)
  */
 static int run_merge(void)
 {
-	int ret;
-	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");
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
-	return ret;
+	strvec_push(&cmd.args, "FETCH_HEAD");
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 /**
@@ -879,43 +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)
 {
-	int ret;
-	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));

-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
-	return ret;
+	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 910f7b9316..11304c096a 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1508,37 +1508,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;
-	int retval;

 	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");
 		}
 	}

-	retval = run_command_v_opt(fetch_argv.v, RUN_GIT_CMD);
-	strvec_clear(&fetch_argv);
-	return retval;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 static int remove_all_fetch_refspecs(const char *key)
diff --git a/diff.c b/diff.c
index 648f6717a5..8451c230d9 100644
--- a/diff.c
+++ b/diff.c
@@ -4278,35 +4278,34 @@ 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 child_process cmd = CHILD_PROCESS_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;

-	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/git.c b/git.c
index ee7758dcb0..6662548986 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/merge.c b/merge.c
index 2382ff66d3..445b4f19aa 100644
--- a/merge.c
+++ b/merge.c
@@ -19,22 +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_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
+	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 6de9c0ee52..03f9e480dd 100644
--- a/scalar.c
+++ b/scalar.c
@@ -69,21 +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;
-	int res;

 	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);

-	res = run_command_v_opt(argv.v, RUN_GIT_CMD);
-
-	strvec_clear(&argv);
-	return res;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 struct scalar_config {
diff --git a/sequencer.c b/sequencer.c
index debb2ecbaf..8ab0225f0f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3183,18 +3183,15 @@ static int rollback_is_safe(void)

 static int reset_merge(const struct object_id *oid)
 {
-	int ret;
-	struct strvec argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	strvec_pushl(&argv, "reset", "--merge", NULL);
+	cmd.git_cmd = 1;
+	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));

-	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-
-	return ret;
+	return run_command(&cmd);
 }

 static int rollback_single_pick(struct repository *r)
@@ -4867,14 +4864,14 @@ 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;
+	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");
+	cmd.git_cmd = 1;
+	strvec_push(&cmd.args, "commit");

 	/*
 	 * continue_single_pick() handles the case of recovering from a
@@ -4887,11 +4884,9 @@ 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);

-	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-	return ret;
+	return run_command(&cmd);
 }

 static int commit_staged_changes(struct repository *r,
--
2.38.1

Re: [PATCH 3/8] use child_process members "args" and "env" directly

[Junio C Hamano]

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

> Build argument list and environment of child processes by using
> struct child_process and populating its members "args" and "env"
> directly instead of maintaining separate strvecs and letting
> run_command_v_opt() and friends populate these members.  This is
> simpler, shorter and slightly more efficient.

Nice.

Re: [PATCH 3/8] use child_process members "args" and "env" directly

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 27 2022, René Scharfe wrote:

> Build argument list and environment of child processes by using
> struct child_process and populating its members "args" and "env"
> directly instead of maintaining separate strvecs and letting
> run_command_v_opt() and friends populate these members.  This is
> simpler, shorter and slightly more efficient.

General comment: Here you make 2x of the flags in in run-command.h
orphans, we could just remove them here atomically. I.e. squash in part
of 8/8.

>  static int similarity_index(struct diff_filepair *p)
> diff --git a/git.c b/git.c
> index ee7758dcb0..6662548986 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");

^^ here, I didn't look carefully at the rest of the commits, but maybe
there's similar squashes elsewhere...

[PATCH 4/8] use child_process member "args" instead of string array variable

[René Scharfe]

Use run_command() with a struct child_process variable and populate its
"args" member directly instead of building a string array and passing it
to run_command_v_opt().  This avoids the use of magic index numbers.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 bisect.c                 | 12 ++++++------
 builtin/am.c             | 12 +++++-------
 builtin/difftool.c       | 17 +++++++++--------
 builtin/merge.c          | 32 ++++++++++++--------------------
 builtin/remote.c         | 14 ++++++++------
 compat/mingw.c           | 11 +++++++----
 ll-merge.c               |  7 ++++---
 sequencer.c              |  7 ++++---
 t/helper/test-fake-ssh.c |  7 ++++---
 9 files changed, 59 insertions(+), 60 deletions(-)

diff --git a/bisect.c b/bisect.c
index fd581b85a7..ec7487e683 100644
--- a/bisect.c
+++ b/bisect.c
@@ -22,8 +22,6 @@ static struct oid_array skipped_revs;

 static struct object_id *current_bad_oid;

-static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
-
 static const char *term_bad;
 static const char *term_good;

@@ -729,20 +727,22 @@ static int is_expected_rev(const struct object_id *oid)
 enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
 				  int no_checkout)
 {
-	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
 	struct commit *commit;
 	struct pretty_print_context pp = {0};
 	struct strbuf commit_msg = STRBUF_INIT;

-	oid_to_hex_r(bisect_rev_hex, bisect_rev);
 	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);

-	argv_checkout[2] = bisect_rev_hex;
 	if (no_checkout) {
 		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
 	} else {
-		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		cmd.git_cmd = 1;
+		strvec_pushl(&cmd.args, "checkout", "-q",
+			     oid_to_hex(bisect_rev), "--", NULL);
+		if (run_command(&cmd))
 			/*
 			 * Errors in `run_command()` itself, signaled by res < 0,
 			 * and errors in the child process, signaled by res > 0
diff --git a/builtin/am.c b/builtin/am.c
index 39fea24833..20aea0d248 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2187,14 +2187,12 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
 	int len;

 	if (!is_null_oid(&state->orig_commit)) {
-		const char *av[4] = { "show", NULL, "--", NULL };
-		char *new_oid_str;
-		int ret;
+		struct child_process cmd = CHILD_PROCESS_INIT;

-		av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));
-		ret = run_command_v_opt(av, RUN_GIT_CMD);
-		free(new_oid_str);
-		return ret;
+		strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit),
+			     "--", NULL);
+		cmd.git_cmd = 1;
+		return run_command(&cmd);
 	}

 	switch (sub_mode) {
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4b10ad1a36..22bcc3444b 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -360,8 +360,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct pair_entry *entry;
 	struct index_state wtindex;
 	struct checkout lstate, rstate;
-	int flags = RUN_GIT_CMD, err = 0;
-	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
+	int err = 0;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct hashmap wt_modified, tmp_modified;
 	int indices_loaded = 0;

@@ -563,16 +563,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	}

 	strbuf_setlen(&ldir, ldir_len);
-	helper_argv[1] = ldir.buf;
 	strbuf_setlen(&rdir, rdir_len);
-	helper_argv[2] = rdir.buf;

 	if (extcmd) {
-		helper_argv[0] = extcmd;
-		flags = 0;
-	} else
+		strvec_push(&cmd.args, extcmd);
+	} else {
+		strvec_push(&cmd.args, "difftool--helper");
+		cmd.git_cmd = 1;
 		setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
-	ret = run_command_v_opt(helper_argv, flags);
+	}
+	strvec_pushl(&cmd.args, ldir.buf, rdir.buf, NULL);
+	ret = run_command(&cmd);

 	/* TODO: audit for interaction with sparse-index. */
 	ensure_full_index(&wtindex);
diff --git a/builtin/merge.c b/builtin/merge.c
index 864808f51a..b3f75f55c8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -347,33 +347,25 @@ static int save_state(struct object_id *stash)

 static void read_empty(const struct object_id *oid)
 {
-	int i = 0;
-	const char *args[7];
-
-	args[i++] = "read-tree";
-	args[i++] = "-m";
-	args[i++] = "-u";
-	args[i++] = empty_tree_oid_hex();
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_pushl(&cmd.args, "read-tree", "-m", "-u", empty_tree_oid_hex(),
+		     oid_to_hex(oid), NULL);
+	cmd.git_cmd = 1;

-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	if (run_command(&cmd))
 		die(_("read-tree failed"));
 }

 static void reset_hard(const struct object_id *oid)
 {
-	int i = 0;
-	const char *args[6];
-
-	args[i++] = "read-tree";
-	args[i++] = "-v";
-	args[i++] = "--reset";
-	args[i++] = "-u";
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_pushl(&cmd.args, "read-tree", "-v", "--reset", "-u",
+		     oid_to_hex(oid), NULL);
+	cmd.git_cmd = 1;

-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	if (run_command(&cmd))
 		die(_("read-tree failed"));
 }

diff --git a/builtin/remote.c b/builtin/remote.c
index 11304c096a..12632676cd 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -92,13 +92,15 @@ static int verbose;

 static int fetch_remote(const char *name)
 {
-	const char *argv[] = { "fetch", name, NULL, NULL };
-	if (verbose) {
-		argv[1] = "-v";
-		argv[2] = name;
-	}
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_push(&cmd.args, "fetch");
+	if (verbose)
+		strvec_push(&cmd.args, "-v");
+	strvec_push(&cmd.args, name);
+	cmd.git_cmd = 1;
 	printf_ln(_("Updating %s"), name);
-	if (run_command_v_opt(argv, RUN_GIT_CMD))
+	if (run_command(&cmd))
 		return error(_("Could not fetch %s"), name);
 	return 0;
 }
diff --git a/compat/mingw.c b/compat/mingw.c
index 901375d584..d614f156df 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -196,16 +196,19 @@ static int read_yes_no_answer(void)
 static int ask_yes_no_if_possible(const char *format, ...)
 {
 	char question[4096];
-	const char *retry_hook[] = { NULL, NULL, NULL };
+	const char *retry_hook;
 	va_list args;

 	va_start(args, format);
 	vsnprintf(question, sizeof(question), format, args);
 	va_end(args);

-	if ((retry_hook[0] = mingw_getenv("GIT_ASK_YESNO"))) {
-		retry_hook[1] = question;
-		return !run_command_v_opt(retry_hook, 0);
+	retry_hook = mingw_getenv("GIT_ASK_YESNO");
+	if (retry_hook) {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		strvec_pushl(&cmd.args, retry_hook, question, NULL);
+		return !run_command(&cmd);
 	}

 	if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
diff --git a/ll-merge.c b/ll-merge.c
index 8955d7e1f6..d5f0c62bd8 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -193,7 +193,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf_expand_dict_entry dict[6];
 	struct strbuf path_sq = STRBUF_INIT;
-	const char *args[] = { NULL, NULL };
+	struct child_process child = CHILD_PROCESS_INIT;
 	int status, fd, i;
 	struct stat st;
 	enum ll_merge_result ret;
@@ -219,8 +219,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,

 	strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);

-	args[0] = cmd.buf;
-	status = run_command_v_opt(args, RUN_USING_SHELL);
+	child.use_shell = 1;
+	strvec_push(&child.args, cmd.buf);
+	status = run_command(&child);
 	fd = open(temp[1], O_RDONLY);
 	if (fd < 0)
 		goto bad;
diff --git a/sequencer.c b/sequencer.c
index 8ab0225f0f..643744fb9b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3555,12 +3555,13 @@ static int error_failed_squash(struct repository *r,

 static int do_exec(struct repository *r, const char *command_line)
 {
-	const char *child_argv[] = { NULL, NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int dirty, status;

 	fprintf(stderr, _("Executing: %s\n"), command_line);
-	child_argv[0] = command_line;
-	status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+	cmd.use_shell = 1;
+	strvec_push(&cmd.args, command_line);
+	status = run_command(&cmd);

 	/* force re-reading of the cache */
 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c
index 12beee99ad..2e576bcc11 100644
--- a/t/helper/test-fake-ssh.c
+++ b/t/helper/test-fake-ssh.c
@@ -8,7 +8,7 @@ int cmd_main(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f;
 	int i;
-	const char *child_argv[] = { NULL, NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;

 	/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */
 	if (!trash_directory)
@@ -25,6 +25,7 @@ int cmd_main(int argc, const char **argv)
 	/* Now, evaluate the *last* parameter */
 	if (argc < 2)
 		return 0;
-	child_argv[0] = argv[argc - 1];
-	return run_command_v_opt(child_argv, RUN_USING_SHELL);
+	cmd.use_shell = 1;
+	strvec_push(&cmd.args, argv[argc - 1]);
+	return run_command(&cmd);
 }
--
2.38.1

Re: [PATCH 4/8] use child_process member "args" instead of string array variable

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 27 2022, René Scharfe wrote:

> @@ -729,20 +727,22 @@ static int is_expected_rev(const struct object_id *oid)
>  enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
>  				  int no_checkout)
>  {
> -	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
>  	struct commit *commit;
>  	struct pretty_print_context pp = {0};
>  	struct strbuf commit_msg = STRBUF_INIT;
>
> -	oid_to_hex_r(bisect_rev_hex, bisect_rev);
>  	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
>
> -	argv_checkout[2] = bisect_rev_hex;
>  	if (no_checkout) {
>  		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
>  			   UPDATE_REFS_DIE_ON_ERR);
>  	} else {
> -		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
> +		struct child_process cmd = CHILD_PROCESS_INIT;
> +
> +		cmd.git_cmd = 1;
> +		strvec_pushl(&cmd.args, "checkout", "-q",
> +			     oid_to_hex(bisect_rev), "--", NULL);
> +		if (run_command(&cmd))
>  			/*
>  			 * Errors in `run_command()` itself, signaled by res < 0,
>  			 * and errors in the child process, signaled by res > 0

Perhaps I went overboard with it in my version, but it's probably worth
mentioning when converting some of these that the reason for the
pre-image of some is really not like the others.

Now that we're on C99 it perhaps make s no difference, but the pre-image
here is explicitly trying to avoid dynamic initializer elements, per
442c27dde78 (CodingGuidelines: mention dynamic C99 initializer elements,
2022-10-10).

Well, partially, some of it appears to just be based on a
misunderstanding of how our own APIs work, i.e. the use of
oid_to_hex_r() over oid_to_hex().

> diff --git a/builtin/am.c b/builtin/am.c
> index 39fea24833..20aea0d248 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2187,14 +2187,12 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
>  	int len;
>
>  	if (!is_null_oid(&state->orig_commit)) {
> -		const char *av[4] = { "show", NULL, "--", NULL };
> -		char *new_oid_str;
> -		int ret;
> +		struct child_process cmd = CHILD_PROCESS_INIT;
>
> -		av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));
> -		ret = run_command_v_opt(av, RUN_GIT_CMD);
> -		free(new_oid_str);
> -		return ret;
> +		strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit),
> +			     "--", NULL);
> +		cmd.git_cmd = 1;
> +		return run_command(&cmd);
>  	}

The same goes for this, FWIW I split this one out into its own commit (I
left the earlier one alone):
https://lore.kernel.org/git/patch-v2-04.10-5cfd6a94ce3-20221017T170316Z-avarab@gmail.com/;
It uses the same pattern

> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index 4b10ad1a36..22bcc3444b 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -360,8 +360,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>  	struct pair_entry *entry;
>  	struct index_state wtindex;
>  	struct checkout lstate, rstate;
> -	int flags = RUN_GIT_CMD, err = 0;
> -	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
> +	int err = 0;
> +	struct child_process cmd = CHILD_PROCESS_INIT;

In general, I like the disection of this series, but with this...

>  	struct hashmap wt_modified, tmp_modified;
>  	int indices_loaded = 0;
>
> @@ -563,16 +563,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>  	}
>
>  	strbuf_setlen(&ldir, ldir_len);
> -	helper_argv[1] = ldir.buf;
>  	strbuf_setlen(&rdir, rdir_len);
> -	helper_argv[2] = rdir.buf;
>
>  	if (extcmd) {
> -		helper_argv[0] = extcmd;
> -		flags = 0;
> -	} else
> +		strvec_push(&cmd.args, extcmd);
> +	} else {
> +		strvec_push(&cmd.args, "difftool--helper");
> +		cmd.git_cmd = 1;

...and the frequent occurance of just e.g. "cmd.git_cmd = 1" and nothing
else I'm wondering if we're not throwing the baby out with the bath
water in having no convenience wrappers or macros at all.

A lot of your 3-lines would be 1 lines if we just had e.g. (untested,
and could be a function not a macro, but you get the idea):

	#define run_command_git_simple(__VA_ARGS__) \
		struct child_process cmd = CHILD_PROCESS_INIT; \
		cmd.git_cmd = 1; \
		strvec_pushl(&cmd.args, __VA_ARGS__); \
		run_command(&cmd);

But maybe nobody except me thinks that's worthwhile...

>  static void read_empty(const struct object_id *oid)
>  {
> -	int i = 0;
> -	const char *args[7];
> -
> -	args[i++] = "read-tree";
> -	args[i++] = "-m";
> -	args[i++] = "-u";
> -	args[i++] = empty_tree_oid_hex();
> -	args[i++] = oid_to_hex(oid);
> -	args[i] = NULL;
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> +
> +	strvec_pushl(&cmd.args, "read-tree", "-m", "-u", empty_tree_oid_hex(),
> +		     oid_to_hex(oid), NULL);
> +	cmd.git_cmd = 1;
>
> -	if (run_command_v_opt(args, RUN_GIT_CMD))
> +	if (run_command(&cmd))
>  		die(_("read-tree failed"));
>  }
>
>  static void reset_hard(const struct object_id *oid)
>  {
> -	int i = 0;
> -	const char *args[6];
> -
> -	args[i++] = "read-tree";
> -	args[i++] = "-v";
> -	args[i++] = "--reset";
> -	args[i++] = "-u";
> -	args[i++] = oid_to_hex(oid);
> -	args[i] = NULL;
> +	struct child_process cmd = CHILD_PROCESS_INIT;
> +
> +	strvec_pushl(&cmd.args, "read-tree", "-v", "--reset", "-u",
> +		     oid_to_hex(oid), NULL);
> +	cmd.git_cmd = 1;
>
> -	if (run_command_v_opt(args, RUN_GIT_CMD))
> +	if (run_command(&cmd))
>  		die(_("read-tree failed"));
>  }

Two perfect examples, e.g. the former would just be:

	if (run_command_git_simple("read-tree", "-m", "-u", empty_tree_oid_hex(),
				   oid_to_hex(oid), NULL))
		die(...);

Re: [PATCH 4/8] use child_process member "args" instead of string array variable

[René Scharfe]

Am 27.10.22 um 23:09 schrieb Ævar Arnfjörð Bjarmason:
>
> On Thu, Oct 27 2022, René Scharfe wrote:
>
>> @@ -729,20 +727,22 @@ static int is_expected_rev(const struct object_id *oid)
>>  enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
>>  				  int no_checkout)
>>  {
>> -	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
>>  	struct commit *commit;
>>  	struct pretty_print_context pp = {0};
>>  	struct strbuf commit_msg = STRBUF_INIT;
>>
>> -	oid_to_hex_r(bisect_rev_hex, bisect_rev);
>>  	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
>>
>> -	argv_checkout[2] = bisect_rev_hex;
>>  	if (no_checkout) {
>>  		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
>>  			   UPDATE_REFS_DIE_ON_ERR);
>>  	} else {
>> -		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
>> +		struct child_process cmd = CHILD_PROCESS_INIT;
>> +
>> +		cmd.git_cmd = 1;
>> +		strvec_pushl(&cmd.args, "checkout", "-q",
>> +			     oid_to_hex(bisect_rev), "--", NULL);
>> +		if (run_command(&cmd))
>>  			/*
>>  			 * Errors in `run_command()` itself, signaled by res < 0,
>>  			 * and errors in the child process, signaled by res > 0
>
> Perhaps I went overboard with it in my version, but it's probably worth
> mentioning when converting some of these that the reason for the
> pre-image of some is really not like the others.
>
> Now that we're on C99 it perhaps make s no difference, but the pre-image
> here is explicitly trying to avoid dynamic initializer elements, per
> 442c27dde78 (CodingGuidelines: mention dynamic C99 initializer elements,
> 2022-10-10).

True, some cases could be converted to string array initializations,
which also would get rid of magic numbers.  This would make the final
patch to convert them to run_command() longer.

> Well, partially, some of it appears to just be based on a
> misunderstanding of how our own APIs work, i.e. the use of
> oid_to_hex_r() over oid_to_hex().
>
>> diff --git a/builtin/am.c b/builtin/am.c
>> index 39fea24833..20aea0d248 100644
>> --- a/builtin/am.c
>> +++ b/builtin/am.c
>> @@ -2187,14 +2187,12 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
>>  	int len;
>>
>>  	if (!is_null_oid(&state->orig_commit)) {
>> -		const char *av[4] = { "show", NULL, "--", NULL };
>> -		char *new_oid_str;
>> -		int ret;
>> +		struct child_process cmd = CHILD_PROCESS_INIT;
>>
>> -		av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));
>> -		ret = run_command_v_opt(av, RUN_GIT_CMD);
>> -		free(new_oid_str);
>> -		return ret;
>> +		strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit),
>> +			     "--", NULL);
>> +		cmd.git_cmd = 1;
>> +		return run_command(&cmd);
>>  	}
>
> The same goes for this, FWIW I split this one out into its own commit (I
> left the earlier one alone):
> https://lore.kernel.org/git/patch-v2-04.10-5cfd6a94ce3-20221017T170316Z-avarab@gmail.com/;
> It uses the same pattern

OK, I just chalked that up as "slightly odd" and bulldozed over them
without a second thought.  Hmm.

>
>> diff --git a/builtin/difftool.c b/builtin/difftool.c
>> index 4b10ad1a36..22bcc3444b 100644
>> --- a/builtin/difftool.c
>> +++ b/builtin/difftool.c
>> @@ -360,8 +360,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>>  	struct pair_entry *entry;
>>  	struct index_state wtindex;
>>  	struct checkout lstate, rstate;
>> -	int flags = RUN_GIT_CMD, err = 0;
>> -	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
>> +	int err = 0;
>> +	struct child_process cmd = CHILD_PROCESS_INIT;
>
> In general, I like the disection of this series, but with this...
>
>>  	struct hashmap wt_modified, tmp_modified;
>>  	int indices_loaded = 0;
>>
>> @@ -563,16 +563,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
>>  	}
>>
>>  	strbuf_setlen(&ldir, ldir_len);
>> -	helper_argv[1] = ldir.buf;
>>  	strbuf_setlen(&rdir, rdir_len);
>> -	helper_argv[2] = rdir.buf;
>>
>>  	if (extcmd) {
>> -		helper_argv[0] = extcmd;
>> -		flags = 0;
>> -	} else
>> +		strvec_push(&cmd.args, extcmd);
>> +	} else {
>> +		strvec_push(&cmd.args, "difftool--helper");
>> +		cmd.git_cmd = 1;
>
> ...and the frequent occurance of just e.g. "cmd.git_cmd = 1" and nothing
> else I'm wondering if we're not throwing the baby out with the bath
> water in having no convenience wrappers or macros at all.
>
> A lot of your 3-lines would be 1 lines if we just had e.g. (untested,
> and could be a function not a macro, but you get the idea):
>
> 	#define run_command_git_simple(__VA_ARGS__) \
> 		struct child_process cmd = CHILD_PROCESS_INIT; \
> 		cmd.git_cmd = 1; \
> 		strvec_pushl(&cmd.args, __VA_ARGS__); \
> 		run_command(&cmd);
>
> But maybe nobody except me thinks that's worthwhile...

I have similar temptations; you could see that in my scratch patch
https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/
which added run_git_or_die() in builtin/gc.c.  Why, oh why?  Perhaps
because taking a blank form (CHILD_PROCESS_INIT), ticking boxes
(.git_cmd = 1), filling out text fields (strvec_push(...)) and
submitting it (run_command()) feels tedious and bureaucratic, Java-esque
even.  And some patterns appear again and again.

How bad is that?  Is it bad at all?  I think overall we should try to
reduce the number of external calls and make those we have to do
self-documenting and leak-free.  A bit of tedium is OK; this API should
be used rarely and sparingly.  Still I get the urge to search for
patterns and define shortcuts when I see all those similar calls..

run_command_git_simple as defined above wouldn't compile, but I get it.
Reducing the number of lines feels good, but it also makes the code less
flexible -- adding a conditional parameter requires converting back to
run_command().

>
>>  static void read_empty(const struct object_id *oid)
>>  {
>> -	int i = 0;
>> -	const char *args[7];
>> -
>> -	args[i++] = "read-tree";
>> -	args[i++] = "-m";
>> -	args[i++] = "-u";
>> -	args[i++] = empty_tree_oid_hex();
>> -	args[i++] = oid_to_hex(oid);
>> -	args[i] = NULL;
>> +	struct child_process cmd = CHILD_PROCESS_INIT;
>> +
>> +	strvec_pushl(&cmd.args, "read-tree", "-m", "-u", empty_tree_oid_hex(),
>> +		     oid_to_hex(oid), NULL);
>> +	cmd.git_cmd = 1;
>>
>> -	if (run_command_v_opt(args, RUN_GIT_CMD))
>> +	if (run_command(&cmd))
>>  		die(_("read-tree failed"));
>>  }
>>
>>  static void reset_hard(const struct object_id *oid)
>>  {
>> -	int i = 0;
>> -	const char *args[6];
>> -
>> -	args[i++] = "read-tree";
>> -	args[i++] = "-v";
>> -	args[i++] = "--reset";
>> -	args[i++] = "-u";
>> -	args[i++] = oid_to_hex(oid);
>> -	args[i] = NULL;
>> +	struct child_process cmd = CHILD_PROCESS_INIT;
>> +
>> +	strvec_pushl(&cmd.args, "read-tree", "-v", "--reset", "-u",
>> +		     oid_to_hex(oid), NULL);
>> +	cmd.git_cmd = 1;
>>
>> -	if (run_command_v_opt(args, RUN_GIT_CMD))
>> +	if (run_command(&cmd))
>>  		die(_("read-tree failed"));
>>  }
>
> Two perfect examples, e.g. the former would just be:
>
> 	if (run_command_git_simple("read-tree", "-m", "-u", empty_tree_oid_hex(),
> 				   oid_to_hex(oid), NULL))
> 		die(...);

Re: [PATCH 4/8] use child_process member "args" instead of string array variable

[Taylor Blau]

On Fri, Oct 28, 2022 at 04:23:31PM +0200, René Scharfe wrote:
> > A lot of your 3-lines would be 1 lines if we just had e.g. (untested,
> > and could be a function not a macro, but you get the idea):
> >
> > 	#define run_command_git_simple(__VA_ARGS__) \
> > 		struct child_process cmd = CHILD_PROCESS_INIT; \
> > 		cmd.git_cmd = 1; \
> > 		strvec_pushl(&cmd.args, __VA_ARGS__); \
> > 		run_command(&cmd);
> >
> > But maybe nobody except me thinks that's worthwhile...
>
> I have similar temptations; you could see that in my scratch patch
> https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/
> which added run_git_or_die() in builtin/gc.c.  Why, oh why?  Perhaps
> because taking a blank form (CHILD_PROCESS_INIT), ticking boxes
> (.git_cmd = 1), filling out text fields (strvec_push(...)) and
> submitting it (run_command()) feels tedious and bureaucratic, Java-esque
> even.  And some patterns appear again and again.
>
> How bad is that?  Is it bad at all?  I think overall we should try to
> reduce the number of external calls and make those we have to do
> self-documenting and leak-free.  A bit of tedium is OK; this API should
> be used rarely and sparingly.  Still I get the urge to search for
> patterns and define shortcuts when I see all those similar calls..
>
> run_command_git_simple as defined above wouldn't compile, but I get it.
> Reducing the number of lines feels good, but it also makes the code less
> flexible -- adding a conditional parameter requires converting back to
> run_command().

For what it's worth, I agree. I don't think there are or should be any
hard and fast rules about when extracting a pattern like this into a
function or macro is right. But here it feels wrong and
counterproductive to the goal of this series.

My main gripe with it is that it seems to be overfit to these small
handful of calls, and that changing it in the future would require us to
go and update existing callers to accommodate new functionality in other
callers.

So I'd prefer to see it left alone.

Thanks,
Taylor

Re: [PATCH 4/8] use child_process member "args" instead of string array variable

[Taylor Blau]

On Thu, Oct 27, 2022 at 06:37:52PM +0200, René Scharfe wrote:
> @@ -729,20 +727,22 @@ static int is_expected_rev(const struct object_id *oid)
>  enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
>  				  int no_checkout)
>  {
> -	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
>  	struct commit *commit;
>  	struct pretty_print_context pp = {0};
>  	struct strbuf commit_msg = STRBUF_INIT;
>
> -	oid_to_hex_r(bisect_rev_hex, bisect_rev);
>  	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
>
> -	argv_checkout[2] = bisect_rev_hex;
>  	if (no_checkout) {
>  		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
>  			   UPDATE_REFS_DIE_ON_ERR);
>  	} else {
> -		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
> +		struct child_process cmd = CHILD_PROCESS_INIT;
> +
> +		cmd.git_cmd = 1;
> +		strvec_pushl(&cmd.args, "checkout", "-q",
> +			     oid_to_hex(bisect_rev), "--", NULL);

I was wondering if this part of the conversion was right, since
oid_to_hex() uses a ring of output buffers (see hash_to_hex_algop()).
But we do call xstrdup() on the argument from strvec_push(), so we are
OK here.

Thanks,
Taylor

[PATCH 5/8] replace and remove run_command_v_opt_cd_env()

[René Scharfe]

run_command_v_opt_cd_env() is only used in an example in a comment.  Use
the struct child_process member "env" and run_command() directly instead
and then remove the unused convenience function.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 run-command.c | 7 +------
 run-command.h | 4 +---
 tmp-objdir.h  | 4 ++--
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/run-command.c b/run-command.c
index 5ec3a46dcc..1c9ed510f8 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1006,7 +1006,7 @@ int run_command(struct child_process *cmd)

 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)
@@ -1014,11 +1014,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 0e85e5846a..a5e210fd1a 100644
--- a/run-command.h
+++ b/run-command.h
@@ -151,8 +151,7 @@ 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_v_opt, child_process_clear do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
  *   is printed.
@@ -250,7 +249,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 76efc7edee..615b188573 100644
--- a/tmp-objdir.h
+++ b/tmp-objdir.h
@@ -11,8 +11,8 @@
  * Example:
  *
  *	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_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");
--
2.38.1

Re: [PATCH 5/8] replace and remove run_command_v_opt_cd_env()

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 27 2022, René Scharfe wrote:

> run_command_v_opt_cd_env() is only used in an example in a comment.

Is it? I could have sworn that....

....ah, it is used be before this series, but whereas I arranged it as:

 1. Remove run_command_v_opt_cd_env() and its last user:
    https://lore.kernel.org/git/patch-v2-07.10-3b3d3777232-20221017T170316Z-avarab@gmail.com/
 2. Remove run_command_v_opt_tr2() and its last user:
    https://lore.kernel.org/git/patch-v2-08.10-4f1a051823f-20221017T170316Z-avarab@gmail.com/

You are:

 1. In your 3/8 you rewrite a bunch of callers, and one of those is the
    odd one out in 3/8 using run_command_v_opt_cd_env().
 2. There's an in-between unrelated cleanup in 4/8
 3. This 5/8 commit, removing the now-stale run_command_v_opt_cd_env().
 4. A nicely atomic 6/8 removing run_command_v_opt_tr2() and its last caller
 5. Your 7/8 has the run_command_v_opt_cd_env_tr2(), but unlike this
    commit you didn't bundle up the "Use [...it...] directly" like here.

Anyway, I find the end state to be mostly OK, but FWIW I wouldn't mind
if this were a bit less confusing.

You seem to have ended up with this because of grouping the '"args" and
"env" directly' callers together.

FWIW I'd be fine with just squashing most of this together, it's already
a ~200 line commit, the commit message could just call out these
exceptions.

*Or* do it more incrementally, but then the choice of not doing the last
callers of the odd ones out atomically seems a bit weird....

Anyway, if you want to just keep it as it is I'm also fine with it. I
mainly wrote the above while narrating the state of this to myself, to
see if there were any lurking issues...

> Use
> the struct child_process member "env" and run_command() directly instead
> and then remove the unused convenience function.
>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  run-command.c | 7 +------
>  run-command.h | 4 +---
>  tmp-objdir.h  | 4 ++--
>  3 files changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 5ec3a46dcc..1c9ed510f8 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -1006,7 +1006,7 @@ int run_command(struct child_process *cmd)
>
>  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)
> @@ -1014,11 +1014,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 0e85e5846a..a5e210fd1a 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -151,8 +151,7 @@ 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_v_opt, child_process_clear do the following:
>   *
>   * - If a system call failed, errno is set and -1 is returned. A diagnostic
>   *   is printed.
> @@ -250,7 +249,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 76efc7edee..615b188573 100644
> --- a/tmp-objdir.h
> +++ b/tmp-objdir.h
> @@ -11,8 +11,8 @@
>   * Example:
>   *
>   *	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_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 5/8] replace and remove run_command_v_opt_cd_env()

[Taylor Blau]

On Thu, Oct 27, 2022 at 06:38:41PM +0200, René Scharfe wrote:
> run_command_v_opt_cd_env() is only used in an example in a comment.  Use
> the struct child_process member "env" and run_command() directly instead
> and then remove the unused convenience function.

Nice, this is a very satisfying clean-up.

Thanks,
Taylor

[PATCH 6/8] replace and remove run_command_v_opt_tr2()

[René Scharfe]

The convenience function run_command_v_opt_tr2() is only used by a
single caller.  Use struct child_process and run_command() directly
instead and remove the underused function.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 fsmonitor-ipc.c | 10 +++++++---
 run-command.c   |  5 -----
 run-command.h   |  1 -
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index c0f42301c8..19d772f0f3 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -54,10 +54,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/run-command.c b/run-command.c
index 1c9ed510f8..32fa4b0ed6 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1009,11 +1009,6 @@ 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)
 {
diff --git a/run-command.h b/run-command.h
index a5e210fd1a..482da7f60c 100644
--- a/run-command.h
+++ b/run-command.h
@@ -244,7 +244,6 @@ int run_auto_maintenance(int quiet);
  * 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".
--
2.38.1

[PATCH 7/8] replace and remove run_command_v_opt_cd_env_tr2()

[René Scharfe]

The convenience function run_command_v_opt_cd_env_tr2() has no external
callers left.  Inline it and remove it from the API.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 run-command.c | 10 ----------
 run-command.h | 10 +---------
 2 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/run-command.c b/run-command.c
index 32fa4b0ed6..923bad37fe 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1005,12 +1005,6 @@ int run_command(struct child_process *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_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);
@@ -1022,10 +1016,6 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 	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);
 }

diff --git a/run-command.h b/run-command.h
index 482da7f60c..04bd07dc7a 100644
--- a/run-command.h
+++ b/run-command.h
@@ -233,23 +233,15 @@ int run_auto_maintenance(int quiet);
 #define RUN_CLOSE_OBJECT_STORE		(1<<7)

 /**
- * Convenience functions that encapsulate a sequence of
+ * Convenience function 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 argument dir corresponds the member .dir. The argument env
- * corresponds to the member .env.
  */
 int run_command_v_opt(const char **argv, int opt);
-/*
- * 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);

 /**
  * Execute the given command, sending "in" to its stdin, and capturing its
--
2.38.1

Re: [PATCH 7/8] replace and remove run_command_v_opt_cd_env_tr2()

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 27 2022, René Scharfe wrote:

> diff --git a/run-command.h b/run-command.h
> index 482da7f60c..04bd07dc7a 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -233,23 +233,15 @@ int run_auto_maintenance(int quiet);
>  #define RUN_CLOSE_OBJECT_STORE		(1<<7)
>
>  /**
> - * Convenience functions that encapsulate a sequence of
> + * Convenience function that encapsulate a sequence of

Maybe we shouldn't bother, because we remove this altogether in 8/8, but
this has a grammar error, should be:

	Convenience function that encapsulates a sequence of

Or:

	A convenience function that encapsulates a sequence of

Not:

	Convenience function that encapsulate a sequence of

If you're re-rolling I think just dropping this hunk is better.

It can just go away entirely in 8/8. The pre-image had bad grammar
anyway, so not worth fixing just to remove it later.

Re: [PATCH 7/8] replace and remove run_command_v_opt_cd_env_tr2()

[René Scharfe]

Am 28.10.22 um 00:46 schrieb Ævar Arnfjörð Bjarmason:
>
> On Thu, Oct 27 2022, René Scharfe wrote:
>
>> diff --git a/run-command.h b/run-command.h
>> index 482da7f60c..04bd07dc7a 100644
>> --- a/run-command.h
>> +++ b/run-command.h
>> @@ -233,23 +233,15 @@ int run_auto_maintenance(int quiet);
>>  #define RUN_CLOSE_OBJECT_STORE		(1<<7)
>>
>>  /**
>> - * Convenience functions that encapsulate a sequence of
>> + * Convenience function that encapsulate a sequence of
>
> Maybe we shouldn't bother, because we remove this altogether in 8/8, but
> this has a grammar error, should be:
>
> 	Convenience function that encapsulates a sequence of
>
> Or:
>
> 	A convenience function that encapsulates a sequence of
>
> Not:
>
> 	Convenience function that encapsulate a sequence of

Ah, good catch.

>
> If you're re-rolling I think just dropping this hunk is better.
>
> It can just go away entirely in 8/8. The pre-image had bad grammar
> anyway, so not worth fixing just to remove it later.

[PATCH 8/8] replace and remove run_command_v_opt()

[René Scharfe]

Replace the remaining calls of run_command_v_opt() with run_command()
calls and explict struct child_process variables.  This is more verbose,
but not by much overall.  The code becomes more flexible, e.g. it's easy
to extend to conditionally add a new argument.

Then remove the now unused function and its own flag names, simplifying
the run-command API.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/bisect--helper.c | 15 ++++++++++-----
 builtin/clone.c          |  8 ++++++--
 builtin/difftool.c       |  7 +++++--
 builtin/fetch.c          |  9 ++++++---
 builtin/gc.c             | 41 +++++++++++++++++++++++++++++++---------
 builtin/merge-index.c    |  4 +++-
 run-command.c            | 15 ---------------
 run-command.h            | 22 +--------------------
 shell.c                  | 17 ++++++++++++-----
 t/helper/test-trace2.c   |  4 +++-
 10 files changed, 78 insertions(+), 64 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 5c63ba6994..1d2ce8a0e1 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -764,10 +764,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
 		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
 		strbuf_trim(&start_head);
 		if (!no_checkout) {
-			const char *argv[] = { "checkout", start_head.buf,
-					       "--", NULL };
+			struct child_process cmd = CHILD_PROCESS_INIT;

-			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
+			cmd.git_cmd = 1;
+			strvec_pushl(&cmd.args, "checkout", start_head.buf,
+				     "--", NULL);
+			if (run_command(&cmd)) {
 				res = error(_("checking out '%s' failed."
 						 " Try 'git bisect start "
 						 "<valid-branch>'."),
@@ -1141,9 +1143,12 @@ static int get_first_good(const char *refname UNUSED,

 static int do_bisect_run(const char *command)
 {
-	const char *argv[] = { command, NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
 	printf(_("running %s\n"), command);
-	return run_command_v_opt(argv, RUN_USING_SHELL);
+	cmd.use_shell = 1;
+	strvec_push(&cmd.args, command);
+	return run_command(&cmd);
 }

 static int verify_good(const struct bisect_terms *terms, const char *command)
diff --git a/builtin/clone.c b/builtin/clone.c
index 56e7775dae..0e4348686b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -865,11 +865,15 @@ 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))
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		cmd.git_cmd = 1;
+		cmd.no_stdin = 1;
+		strvec_pushl(&cmd.args, "repack", "-a", "-d", NULL);
+		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 22bcc3444b..d7f08c8a7f 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -44,8 +44,11 @@ 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);
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	cmd.git_cmd = 1;
+	strvec_pushl(&cmd.args, "mergetool", "--tool-help=diff", NULL);
+	return run_command(&cmd);
 }

 static int parse_index_info(char *p, int *mode1, int *mode2,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index a0fca93bb6..dd060dd65a 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 87ad0077d8..962bab61f9 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -167,9 +167,11 @@ 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 };
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	return run_command_v_opt(argv, RUN_GIT_CMD);
+	cmd.git_cmd = 1;
+	strvec_pushl(&cmd.args, "pack-refs", "--all", "--prune", NULL);
+	return run_command(&cmd);
 }

 static int too_many_loose_objects(void)
@@ -535,8 +537,14 @@ 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) {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		cmd.git_cmd = 1;
+		strvec_pushv(&cmd.args, reflog.v);
+		if (run_command(&cmd))
+			die(FAILED_RUN, reflog.v[0]);
+	}
 }

 int cmd_gc(int argc, const char **argv, const char *prefix)
@@ -550,6 +558,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	int daemonized = 0;
 	int keep_largest_pack = -1;
 	timestamp_t dummy;
+	struct child_process rerere_cmd = CHILD_PROCESS_INIT;

 	struct option builtin_gc_options[] = {
 		OPT__QUIET(&quiet, N_("suppress progress reporting")),
@@ -671,11 +680,17 @@ 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;
+
+		repack_cmd.git_cmd = 1;
+		repack_cmd.close_object_store = 1;
+		strvec_pushv(&repack_cmd.args, repack.v);
+		if (run_command(&repack_cmd))
 			die(FAILED_RUN, repack.v[0]);

 		if (prune_expire) {
+			struct child_process prune_cmd = CHILD_PROCESS_INIT;
+
 			/* run `git prune` even if using cruft packs */
 			strvec_push(&prune, prune_expire);
 			if (quiet)
@@ -683,18 +698,26 @@ 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))
+			prune_cmd.git_cmd = 1;
+			strvec_pushv(&prune_cmd.args, prune.v);
+			if (run_command(&prune_cmd))
 				die(FAILED_RUN, prune.v[0]);
 		}
 	}

 	if (prune_worktrees_expire) {
+		struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT;
+
 		strvec_push(&prune_worktrees, prune_worktrees_expire);
-		if (run_command_v_opt(prune_worktrees.v, RUN_GIT_CMD))
+		prune_worktrees_cmd.git_cmd = 1;
+		strvec_pushv(&prune_worktrees_cmd.args, prune_worktrees.v);
+		if (run_command(&prune_worktrees_cmd))
 			die(FAILED_RUN, prune_worktrees.v[0]);
 	}

-	if (run_command_v_opt(rerere.v, RUN_GIT_CMD))
+	rerere_cmd.git_cmd = 1;
+	strvec_pushv(&rerere_cmd.args, rerere.v);
+	if (run_command(&rerere_cmd))
 		die(FAILED_RUN, rerere.v[0]);

 	report_garbage = report_pack_garbage;
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index c0383fe9df..012f52bd00 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/run-command.c b/run-command.c
index 923bad37fe..23e100dffc 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1004,21 +1004,6 @@ int run_command(struct child_process *cmd)
 	return finish_command(cmd);
 }

-int run_command_v_opt(const char **argv, int opt)
-{
-	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;
-	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 04bd07dc7a..fe2717ad11 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_v_opt, child_process_clear do the following:
+ * run_command, child_process_clear do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
  *   is printed.
@@ -223,26 +223,6 @@ int run_command(struct child_process *);
  */
 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)
-
-/**
- * Convenience function 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`.
- */
-int run_command_v_opt(const char **argv, 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/shell.c b/shell.c
index 7ff4109db7..af0d7c734f 100644
--- a/shell.c
+++ b/shell.c
@@ -52,21 +52,24 @@ static void cd_to_homedir(void)
 static void run_shell(void)
 {
 	int done = 0;
-	static const char *help_argv[] = { HELP_COMMAND, NULL };
+	struct child_process help_cmd = CHILD_PROCESS_INIT;

 	if (!access(NOLOGIN_COMMAND, F_OK)) {
 		/* Interactive login disabled. */
-		const char *argv[] = { NOLOGIN_COMMAND, NULL };
+		struct child_process nologin_cmd = CHILD_PROCESS_INIT;
 		int status;

-		status = run_command_v_opt(argv, 0);
+		strvec_push(&nologin_cmd.args, NOLOGIN_COMMAND);
+		status = run_command(&nologin_cmd);
 		if (status < 0)
 			exit(127);
 		exit(status);
 	}

 	/* Print help if enabled */
-	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+	help_cmd.silent_exec_failure = 1;
+	strvec_push(&help_cmd.args, HELP_COMMAND);
+	run_command(&help_cmd);

 	do {
 		const char *prog;
@@ -125,9 +128,13 @@ static void run_shell(void)
 			   !strcmp(prog, "exit") || !strcmp(prog, "bye")) {
 			done = 1;
 		} else if (is_valid_cmd_name(prog)) {
+			struct child_process cmd = CHILD_PROCESS_INIT;
+
 			full_cmd = make_cmd(prog);
 			argv[0] = full_cmd;
-			code = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);
+			cmd.silent_exec_failure = 1;
+			strvec_pushv(&cmd.args, argv);
+			code = run_command(&cmd);
 			if (code == -1 && errno == ENOENT) {
 				fprintf(stderr, "unrecognized command '%s'\n", prog);
 			}
diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index a714130ece..94fd8ccf51 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);
 }

--
2.38.1

Re: [PATCH 8/8] replace and remove run_command_v_opt()

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 27 2022, René Scharfe wrote:

>  #ifndef NO_PTHREADS
>  static pthread_t main_thread;
>  static int main_thread_set;
> diff --git a/run-command.h b/run-command.h
> index 04bd07dc7a..fe2717ad11 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_v_opt, child_process_clear do the following:
> + * run_command, child_process_clear do the following:
>   *
>   * - If a system call failed, errno is set and -1 is returned. A diagnostic
>   *   is printed.

A pre-existing issue mostly, but maybe worth cleaning up while we're
fixing these docs in general.

This summary is incorrect, because the first bullet point is claiming
that these "return -1", but 2 functions on this list return void.

It looks to me if we just remove child_process_{init,clear} altgother
from this it'll be correct. They have their own docs below, and this is
really describing how this now-simpler API works vis-a-vis the
start/run/finish functions.

Re: [PATCH 8/8] replace and remove run_command_v_opt()

[René Scharfe]

Am 28.10.22 um 00:41 schrieb Ævar Arnfjörð Bjarmason:
>
> On Thu, Oct 27 2022, René Scharfe wrote:
>
>>  #ifndef NO_PTHREADS
>>  static pthread_t main_thread;
>>  static int main_thread_set;
>> diff --git a/run-command.h b/run-command.h
>> index 04bd07dc7a..fe2717ad11 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_v_opt, child_process_clear do the following:
>> + * run_command, child_process_clear do the following:
>>   *
>>   * - If a system call failed, errno is set and -1 is returned. A diagnostic
>>   *   is printed.
>
> A pre-existing issue mostly, but maybe worth cleaning up while we're
> fixing these docs in general.
>
> This summary is incorrect, because the first bullet point is claiming
> that these "return -1", but 2 functions on this list return void.
>
> It looks to me if we just remove child_process_{init,clear} altgother
> from this it'll be correct. They have their own docs below, and this is
> really describing how this now-simpler API works vis-a-vis the
> start/run/finish functions.

True, and I added that mistake a while ago.  Oops.

--- >8 ---
Subject: [PATCH 9/8] run-command: fix return value comment

483bbd4e4c (run-command: introduce child_process_init(), 2014-08-19) and
2d71608ec0 (run-command: factor out child_process_clear(), 2015-10-24)
added help texts about child_process_init() and child_process_clear()
without updating the immediately following documentation of return codes
that only applied to the preexisting functions.

4c4066d95d (run-command: move doc to run-command.h, 2019-11-17) started
to list the functions explicitly that this paragraph applies to, but
still wrongly included child_process_init() and child_process_clear().
Remove their names from that list.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 run-command.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/run-command.h b/run-command.h
index fe2717ad11..7ed2c04427 100644
--- a/run-command.h
+++ b/run-command.h
@@ -150,8 +150,7 @@ struct child_process {
 }

 /**
- * The functions: child_process_init, start_command, finish_command,
- * run_command, child_process_clear do the following:
+ * The functions start_command, finish_command, run_command do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
  *   is printed.
--
2.38.1

Re: [PATCH 0/8] run-command: remove run_command_v_*()

[Jeff King]

On Thu, Oct 27, 2022 at 06:30:36PM +0200, René Scharfe wrote:

> Replace the convenience functions run_command_v_opt() et. al. and use
> struct child_process and run_command() directly instead, for an overall
> code reduction and a simpler and more flexible API that allows creating
> argument lists without magic numbers and reduced risk of memory leaks.
> 
> This is a broken-out and polished version of the original scratch at
> https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/

I read through this and it all looks fine to me. I was a bit puzzled at
the layout of your series at first. In particular, the difference
between cases in patch 4 versus the later ones.

I think it is that in patch 4, these are all unambiguously positive
because we are getting rid of magic numbers (or magically-sized arrays).
Whereas in patches 5-8, there's nothing inherently wrong with the
call-sites; but as we get rid of the API wrappers, we convert them. So
they are collateral damage, so to speak, from the simplification of the
API.

That makes sense to me, though I could point out that most of the sites
cleaned up in patch 4 _could_ be converted to look like the ones that
are converted in 5-8. Obviously that doesn't make sense to do, knowing
that 5-8 are coming. But if the point in splitting it this way is to
show that we could stop at patch 4, cleaning up call sites but not
shrinking the run-command API, then I just want to point out that there
is another way to do those cleanups. :)

All that said, you will be unsurprised to hear that I am on board with
the direction of 5-8, so this all looks good to me. I'm just laying out
my thought process while reviewing.

I also think it would have been fine to just drop the whole API in one
patch (i.e., squashing 5-8), which makes the intermediate diffs a bit
shorter. But I do not mind at all having the cases broken out.

Thanks for cleaning this up.

-Peff

Re: [PATCH 0/8] run-command: remove run_command_v_*()

[René Scharfe]

Am 27.10.22 um 22:11 schrieb Jeff King:
> On Thu, Oct 27, 2022 at 06:30:36PM +0200, René Scharfe wrote:
>
>> Replace the convenience functions run_command_v_opt() et. al. and use
>> struct child_process and run_command() directly instead, for an overall
>> code reduction and a simpler and more flexible API that allows creating
>> argument lists without magic numbers and reduced risk of memory leaks.
>>
>> This is a broken-out and polished version of the original scratch at
>> https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/
>
> I read through this and it all looks fine to me. I was a bit puzzled at
> the layout of your series at first. In particular, the difference
> between cases in patch 4 versus the later ones.
>
> I think it is that in patch 4, these are all unambiguously positive
> because we are getting rid of magic numbers (or magically-sized arrays).
> Whereas in patches 5-8, there's nothing inherently wrong with the
> call-sites; but as we get rid of the API wrappers, we convert them. So
> they are collateral damage, so to speak, from the simplification of the
> API.
>
> That makes sense to me, though I could point out that most of the sites
> cleaned up in patch 4 _could_ be converted to look like the ones that
> are converted in 5-8. Obviously that doesn't make sense to do, knowing
> that 5-8 are coming. But if the point in splitting it this way is to
> show that we could stop at patch 4, cleaning up call sites but not
> shrinking the run-command API, then I just want to point out that there
> is another way to do those cleanups. :)

Yes, almost, except that I think 5-7 are doing necessary pruning and 8
requires a small leap of faith in the value of simplicity.  And I wanted
to shrink down and simplify that last patch.

René

Re: [PATCH 0/8] run-command: remove run_command_v_*()

[Ævar Arnfjörð Bjarmason]

On Thu, Oct 27 2022, René Scharfe wrote:

> Replace the convenience functions run_command_v_opt() et. al. and use
> struct child_process and run_command() directly instead, for an overall
> code reduction and a simpler and more flexible API that allows creating
> argument lists without magic numbers and reduced risk of memory leaks.
>
> This is a broken-out and polished version of the original scratch at
> https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/
>
> Ævar Arnfjörð Bjarmason (1):
>   merge: remove always-the-same "verbose" arguments
>
> René Scharfe (7):
>   bisect--helper: factor out do_bisect_run()
>   use child_process members "args" and "env" directly
>   use child_process member "args" instead of string array variable
>   replace and remove run_command_v_opt_cd_env()
>   replace and remove run_command_v_opt_tr2()
>   replace and remove run_command_v_opt_cd_env_tr2()
>   replace and remove run_command_v_opt()

Even though I had a an earlier alternate series series[1] for this I'd
be happy to see this version go in. I left some comments here and there,
but with/without a re-roll am happy to give this:

	Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

I think I would have just gone for this in the first place, but thought
that people loved the convenience functions too much. It can be hard to
judge sentiments in advance :)

One reason I hadn't re-submitted something is that there were
outstanding new conflicts with "seen", and I see from applying this
topic & merging it that it conflicts somewhat heavily. Junio seems to be
on-board with this though, so maybe he won't mind.

I didn't see any glaring instances where this made things worse, so
maybe we didn't need these convenience wrappers in the first place.

But from the earlier discussion it does seema bit like we tossed the
very notion out because some didn't like the duplicating of struct
members with the flags (which I also doen't like).

So I came up with the below experiment on top, it's not an attempt to
convert all callers, just a demo.

Maybe you think some ideas here are worth using, I probably won't pursue
it (but maybe as ideas for some other future API).

It's a combination of stuff, some of which you might like, some not,
namely:

- Have the API work the same way, but just have a run_commandl(&opt,
  ...) in addition to the run_command(). It's just a trivial wrapper to
  push stuff into &cmd.args for you.

- I saw a few callers that could have perhaps used a similarly trivial
  run_commandv(), but that doesn't benefit from LAST_ARG_MUST_BE_NULL,
  so I didn't bother.

- I wish C had a nicer syntax for not just declaring but squashing
  together compile_time bracketed lists (think set operations). But the
  below "CHILD_PROCESS_INIT_LIST" is a pretty good poor man's version.

  I see gcc/clang nicely give us safety rails for that with
  "-Woverride-init", for this sort of "opts struct with internal stuff,
  but also user options" I think it works out very nicely.

- We have quite a few callers that want "on error, die", so maybe we
  should have something like that "on_error" sooner than later.

On clever (but perhaps overly clever) thing I didn't use, but played
with recently in another context, is that now with C99 you can also do:

	int run_commandl(struct child_process *cmd, ...);
	#define run_command(...) run_command_1(__VA_ARGS__, NULL)

I.e. make the API itself support all of:

	run_command(&cmd);
	run_command(&cmd, "reboot");
	run_command(&cmd, "reboot", NULL);

I haven't made up my mind on whether that's just overly clever, or
outright insane :)

diff --git a/bisect.c b/bisect.c
index ec7487e6836..ef4f80650f7 100644
--- a/bisect.c
+++ b/bisect.c
@@ -740,9 +740,8 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
 		struct child_process cmd = CHILD_PROCESS_INIT;
 
 		cmd.git_cmd = 1;
-		strvec_pushl(&cmd.args, "checkout", "-q",
-			     oid_to_hex(bisect_rev), "--", NULL);
-		if (run_command(&cmd))
+		if (run_commandl(&cmd, "checkout", "-q",
+				 oid_to_hex(bisect_rev), "--", NULL))
 			/*
 			 * Errors in `run_command()` itself, signaled by res < 0,
 			 * and errors in the child process, signaled by res > 0
diff --git a/builtin/am.c b/builtin/am.c
index 20aea0d2487..3b7df32ce22 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2189,10 +2189,9 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
 	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);
+		return run_commandl(&cmd, "show", oid_to_hex(&state->orig_commit),
+				    "--", NULL);
 	}
 
 	switch (sub_mode) {
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1d2ce8a0e12..087d21c614a 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -764,12 +764,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
 		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
 		strbuf_trim(&start_head);
 		if (!no_checkout) {
-			struct child_process cmd = CHILD_PROCESS_INIT;
-
-			cmd.git_cmd = 1;
-			strvec_pushl(&cmd.args, "checkout", start_head.buf,
-				     "--", NULL);
-			if (run_command(&cmd)) {
+			struct child_process cmd = {
+				CHILD_PROCESS_INIT_LIST,
+				.git_cmd = 1,
+			};
+			if (run_commandl(&cmd, "checkout", start_head.buf,
+					 "--", NULL)) {
 				res = error(_("checking out '%s' failed."
 						 " Try 'git bisect start "
 						 "<valid-branch>'."),
@@ -1147,8 +1147,7 @@ static int do_bisect_run(const char *command)
 
 	printf(_("running %s\n"), command);
 	cmd.use_shell = 1;
-	strvec_push(&cmd.args, command);
-	return run_command(&cmd);
+	return run_commandl(&cmd, command, NULL);
 }
 
 static int verify_good(const struct bisect_terms *terms, const char *command)
diff --git a/builtin/clone.c b/builtin/clone.c
index 0e4348686b6..80d09e0fbf1 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -655,7 +655,6 @@ 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
@@ -664,7 +663,7 @@ static int git_sparse_checkout_init(const char *repo)
 	core_apply_sparse_checkout = 1;
 
 	cmd.git_cmd = 1;
-	if (run_command(&cmd)) {
+	if (run_commandl(&cmd, "-C", repo, "sparse-checkout", "set", NULL)) {
 		error(_("failed to initialize sparse-checkout"));
 		result = 1;
 	}
@@ -868,13 +867,14 @@ static void dissociate_from_references(void)
 	char *alternates = git_pathdup("objects/info/alternates");
 
 	if (!access(alternates, F_OK)) {
-		struct child_process cmd = CHILD_PROCESS_INIT;
-
-		cmd.git_cmd = 1;
-		cmd.no_stdin = 1;
-		strvec_pushl(&cmd.args, "repack", "-a", "-d", NULL);
-		if (run_command(&cmd))
-			die(_("cannot repack to clean up"));
+		struct child_process cmd = {
+			CHILD_PROCESS_INIT_LIST,
+			.git_cmd = 1,
+			.no_stdin = 1,
+			.on_error = CHILD_PROCESS_ON_ERROR_DIE,
+		};
+
+		run_commandl(&cmd, "repack", "-a", "-d", NULL);
 		if (unlink(alternates) && errno != ENOENT)
 			die_errno(_("cannot unlink temporary alternates file"));
 	}
diff --git a/builtin/difftool.c b/builtin/difftool.c
index d7f08c8a7fa..b4165b5a8ae 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -44,11 +44,12 @@ static int difftool_config(const char *var, const char *value, void *cb)
 
 static int print_tool_help(void)
 {
-	struct child_process cmd = CHILD_PROCESS_INIT;
-
-	cmd.git_cmd = 1;
-	strvec_pushl(&cmd.args, "mergetool", "--tool-help=diff", NULL);
-	return run_command(&cmd);
+	struct child_process cmd = {
+		CHILD_PROCESS_INIT_LIST,
+		.git_cmd = 1,
+	};
+		
+	return run_commandl(&cmd, "mergetool", "--tool-help=diff", NULL);
 }
 
 static int parse_index_info(char *p, int *mode1, int *mode2,
diff --git a/git.c b/git.c
index 6662548986f..93179f44f78 100644
--- a/git.c
+++ b/git.c
@@ -724,7 +724,13 @@ static void handle_builtin(int argc, const char **argv)
 
 static void execv_dashed_external(const char **argv)
 {
-	struct child_process cmd = CHILD_PROCESS_INIT;
+	struct child_process cmd = {
+		CHILD_PROCESS_INIT_LIST,
+		.clean_on_exit = 1,
+		.wait_after_clean = 1,
+		.silent_exec_failure = 1,
+		.trace2_child_class = "dashed",
+	};
 	int status;
 
 	if (get_super_prefix())
@@ -736,10 +742,6 @@ static void execv_dashed_external(const char **argv)
 
 	strvec_pushf(&cmd.args, "git-%s", argv[0]);
 	strvec_pushv(&cmd.args, argv + 1);
-	cmd.clean_on_exit = 1;
-	cmd.wait_after_clean = 1;
-	cmd.silent_exec_failure = 1;
-	cmd.trace2_child_class = "dashed";
 
 	trace2_cmd_name("_run_dashed_");
 
diff --git a/run-command.c b/run-command.c
index 23e100dffc4..4b20aa1b577 100644
--- a/run-command.c
+++ b/run-command.c
@@ -993,15 +993,45 @@ int finish_command_in_signal(struct child_process *cmd)
 
 int run_command(struct child_process *cmd)
 {
-	int code;
+	int starting = 1;
+	int code = 0;
 
 	if (cmd->out < 0 || cmd->err < 0)
 		BUG("run_command with a pipe can cause deadlock");
 
 	code = start_command(cmd);
 	if (code)
+		goto error;
+	starting = 0;
+	code = finish_command(cmd);
+	if (!code)
+		return 0;
+error:
+	switch (cmd->on_error) {
+	case CHILD_PROCESS_ON_ERROR_DIE:
+		die(starting ?
+		    _("start_command() for '%s' failed (error code %d)") :
+		    _("'%s': got non-zero exit code '%d'"),
+		    cmd->args.v[0], code);
+		break;
+	case CHILD_PROCESS_ON_ERROR_RETURN:
 		return code;
-	return finish_command(cmd);
+	default:
+		BUG("unreachable");
+	}
+}
+
+int run_commandl(struct child_process *cmd, ...)
+{
+	va_list ap;
+	const char *arg;
+
+	va_start(ap, cmd);
+	while ((arg = va_arg(ap, const char *)))
+		strvec_push(&cmd->args, arg);
+	va_end(ap);
+
+	return run_command(cmd);
 }
 
 #ifndef NO_PTHREADS
diff --git a/run-command.h b/run-command.h
index fe2717ad11e..71e390350ed 100644
--- a/run-command.h
+++ b/run-command.h
@@ -15,7 +15,22 @@
  * produces in the caller in order to process it.
  */
 
+enum child_process_on_error {
+	/**
+	 * Return a status code from run_command(). Set to 0 so that
+	 * it'll be { 0 } init'd. If it's specified in a
+	 * CHILD_PROCESS_INIT_LIST initialization we don't want a
+	 * "-Woverride-init" warning.
+	 */
+	CHILD_PROCESS_ON_ERROR_RETURN = 0,
 
+	/**
+	 * die() with some sensible message if run_command() would
+	 * have returned a non-zero exit code.
+	 */
+	CHILD_PROCESS_ON_ERROR_DIE,
+};
+	
 /**
  * This describes the arguments, redirections, and environment of a
  * command to run in a sub-process.
@@ -42,6 +57,10 @@
  *		redirected.
  */
 struct child_process {
+	/**
+	 * Error behavior, see "enum child_process_on_error" above.
+	 */
+	enum child_process_on_error on_error;
 
 	/**
 	 * The .args is a `struct strvec', use that API to manipulate
@@ -144,10 +163,11 @@ struct child_process {
 	void (*clean_on_exit_handler)(struct child_process *process);
 };
 
-#define CHILD_PROCESS_INIT { \
+#define CHILD_PROCESS_INIT_LIST \
+	/* .on_error = CHILD_PROCESS_ON_ERROR_RETURN */ \
 	.args = STRVEC_INIT, \
-	.env = STRVEC_INIT, \
-}
+	.env = STRVEC_INIT
+#define CHILD_PROCESS_INIT { CHILD_PROCESS_INIT_LIST }
 
 /**
  * The functions: child_process_init, start_command, finish_command,
@@ -218,6 +238,14 @@ int finish_command_in_signal(struct child_process *);
  */
 int run_command(struct child_process *);
 
+/**
+ * Like run_command() but takes a variable number of arguments, which
+ * will be appended with the equivalent of strvec_pushl(&cmd.args,
+ * ...) before invoking run_command().
+ */
+LAST_ARG_MUST_BE_NULL
+int run_commandl(struct child_process *cmd, ...);
+
 /*
  * Trigger an auto-gc
  */

Re: [PATCH 0/8] run-command: remove run_command_v_*()

[René Scharfe]

Am 27.10.22 um 23:46 schrieb Ævar Arnfjörð Bjarmason:
>
> On Thu, Oct 27 2022, René Scharfe wrote:
>
>> Replace the convenience functions run_command_v_opt() et. al. and use
>> struct child_process and run_command() directly instead, for an overall
>> code reduction and a simpler and more flexible API that allows creating
>> argument lists without magic numbers and reduced risk of memory leaks.
>>
>> This is a broken-out and polished version of the original scratch at
>> https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/
>>
>> Ævar Arnfjörð Bjarmason (1):
>>   merge: remove always-the-same "verbose" arguments
>>
>> René Scharfe (7):
>>   bisect--helper: factor out do_bisect_run()
>>   use child_process members "args" and "env" directly
>>   use child_process member "args" instead of string array variable
>>   replace and remove run_command_v_opt_cd_env()
>>   replace and remove run_command_v_opt_tr2()
>>   replace and remove run_command_v_opt_cd_env_tr2()
>>   replace and remove run_command_v_opt()
>
> Even though I had a an earlier alternate series series[1] for this I'd
> be happy to see this version go in. I left some comments here and there,
> but with/without a re-roll am happy to give this:
>
> 	Reviewed-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>
> I think I would have just gone for this in the first place, but thought
> that people loved the convenience functions too much. It can be hard to
> judge sentiments in advance :)
>
> One reason I hadn't re-submitted something is that there were
> outstanding new conflicts with "seen", and I see from applying this
> topic & merging it that it conflicts somewhat heavily. Junio seems to be
> on-board with this though, so maybe he won't mind.
>
> I didn't see any glaring instances where this made things worse, so
> maybe we didn't need these convenience wrappers in the first place.

Right, and I think this is important.

>
> But from the earlier discussion it does seema bit like we tossed the
> very notion out because some didn't like the duplicating of struct
> members with the flags (which I also doen't like).
>
> So I came up with the below experiment on top, it's not an attempt to
> convert all callers, just a demo.
>
> Maybe you think some ideas here are worth using, I probably won't pursue
> it (but maybe as ideas for some other future API).
>
> It's a combination of stuff, some of which you might like, some not,
> namely:
>
> - Have the API work the same way, but just have a run_commandl(&opt,
>   ...) in addition to the run_command(). It's just a trivial wrapper to
>   push stuff into &cmd.args for you.
>
> - I saw a few callers that could have perhaps used a similarly trivial
>   run_commandv(), but that doesn't benefit from LAST_ARG_MUST_BE_NULL,
>   so I didn't bother.

I thought about that as well, but at least for me is probably a just a
case of loss aversion, which I have aplenty.  run_command_v_opt() alone
isn't bad and patch 8 on its own probably wouldn't fly, so I mentally
cling to it.  But without it the API is untangled and simpler.  Only a
single way to specify flags and arguments, no shortcuts for special
combinations.  Overall easier to use once we forget the old ways.

>
> - I wish C had a nicer syntax for not just declaring but squashing
>   together compile_time bracketed lists (think set operations). But the
>   below "CHILD_PROCESS_INIT_LIST" is a pretty good poor man's version.
>
>   I see gcc/clang nicely give us safety rails for that with
>   "-Woverride-init", for this sort of "opts struct with internal stuff,
>   but also user options" I think it works out very nicely.
>

That's a nice and simple macro.  I played with a gross variant à la

  #define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ }

which would allow e.g.

  struct child_process cmd = CHILD_PROCESS_INIT_EX(.git_cmd = 1);

Yours is better, but they share the downside of not actually saving any
lines of code..

> - We have quite a few callers that want "on error, die", so maybe we
>   should have something like that "on_error" sooner than later.

We could add a die_on_error bit for that, or start_command_or_die() and
run_command_or_die() variants (there I go again, multiplying APIs..).
They could report the failed command, which a caller can't do because
the internal strvec is already cleared once it learns of the failure.

>
> On clever (but perhaps overly clever) thing I didn't use, but played
> with recently in another context, is that now with C99 you can also do:
>
> 	int run_commandl(struct child_process *cmd, ...);
> 	#define run_command(...) run_command_1(__VA_ARGS__, NULL)
>
> I.e. make the API itself support all of:
>
> 	run_command(&cmd);
> 	run_command(&cmd, "reboot");
> 	run_command(&cmd, "reboot", NULL);
>
> I haven't made up my mind on whether that's just overly clever, or
> outright insane :)

Neither, I'd say.  By combining two operations it's less flexible than
a pure run method plus direct access to a strvec member.  It's a
shortcut that may save a line but requires more effort to extend its
callers, e.g. to conditionally add a new argument.

>
> diff --git a/bisect.c b/bisect.c
> index ec7487e6836..ef4f80650f7 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -740,9 +740,8 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
>  		struct child_process cmd = CHILD_PROCESS_INIT;
>
>  		cmd.git_cmd = 1;
> -		strvec_pushl(&cmd.args, "checkout", "-q",
> -			     oid_to_hex(bisect_rev), "--", NULL);
> -		if (run_command(&cmd))
> +		if (run_commandl(&cmd, "checkout", "-q",
> +				 oid_to_hex(bisect_rev), "--", NULL))
>  			/*
>  			 * Errors in `run_command()` itself, signaled by res < 0,
>  			 * and errors in the child process, signaled by res > 0
> diff --git a/builtin/am.c b/builtin/am.c
> index 20aea0d2487..3b7df32ce22 100644
> --- a/builtin/am.c
> +++ b/builtin/am.c
> @@ -2189,10 +2189,9 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
>  	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);
> +		return run_commandl(&cmd, "show", oid_to_hex(&state->orig_commit),
> +				    "--", NULL);
>  	}
>
>  	switch (sub_mode) {
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1d2ce8a0e12..087d21c614a 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -764,12 +764,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
>  		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
>  		strbuf_trim(&start_head);
>  		if (!no_checkout) {
> -			struct child_process cmd = CHILD_PROCESS_INIT;
> -
> -			cmd.git_cmd = 1;
> -			strvec_pushl(&cmd.args, "checkout", start_head.buf,
> -				     "--", NULL);
> -			if (run_command(&cmd)) {
> +			struct child_process cmd = {
> +				CHILD_PROCESS_INIT_LIST,
> +				.git_cmd = 1,
> +			};
> +			if (run_commandl(&cmd, "checkout", start_head.buf,
> +					 "--", NULL)) {
>  				res = error(_("checking out '%s' failed."
>  						 " Try 'git bisect start "
>  						 "<valid-branch>'."),
> @@ -1147,8 +1147,7 @@ static int do_bisect_run(const char *command)
>
>  	printf(_("running %s\n"), command);
>  	cmd.use_shell = 1;
> -	strvec_push(&cmd.args, command);
> -	return run_command(&cmd);
> +	return run_commandl(&cmd, command, NULL);
>  }
>
>  static int verify_good(const struct bisect_terms *terms, const char *command)
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 0e4348686b6..80d09e0fbf1 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -655,7 +655,6 @@ 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
> @@ -664,7 +663,7 @@ static int git_sparse_checkout_init(const char *repo)
>  	core_apply_sparse_checkout = 1;
>
>  	cmd.git_cmd = 1;
> -	if (run_command(&cmd)) {
> +	if (run_commandl(&cmd, "-C", repo, "sparse-checkout", "set", NULL)) {
>  		error(_("failed to initialize sparse-checkout"));
>  		result = 1;
>  	}
> @@ -868,13 +867,14 @@ static void dissociate_from_references(void)
>  	char *alternates = git_pathdup("objects/info/alternates");
>
>  	if (!access(alternates, F_OK)) {
> -		struct child_process cmd = CHILD_PROCESS_INIT;
> -
> -		cmd.git_cmd = 1;
> -		cmd.no_stdin = 1;
> -		strvec_pushl(&cmd.args, "repack", "-a", "-d", NULL);
> -		if (run_command(&cmd))
> -			die(_("cannot repack to clean up"));
> +		struct child_process cmd = {
> +			CHILD_PROCESS_INIT_LIST,
> +			.git_cmd = 1,
> +			.no_stdin = 1,
> +			.on_error = CHILD_PROCESS_ON_ERROR_DIE,
> +		};
> +
> +		run_commandl(&cmd, "repack", "-a", "-d", NULL);
>  		if (unlink(alternates) && errno != ENOENT)
>  			die_errno(_("cannot unlink temporary alternates file"));
>  	}
> diff --git a/builtin/difftool.c b/builtin/difftool.c
> index d7f08c8a7fa..b4165b5a8ae 100644
> --- a/builtin/difftool.c
> +++ b/builtin/difftool.c
> @@ -44,11 +44,12 @@ static int difftool_config(const char *var, const char *value, void *cb)
>
>  static int print_tool_help(void)
>  {
> -	struct child_process cmd = CHILD_PROCESS_INIT;
> -
> -	cmd.git_cmd = 1;
> -	strvec_pushl(&cmd.args, "mergetool", "--tool-help=diff", NULL);
> -	return run_command(&cmd);
> +	struct child_process cmd = {
> +		CHILD_PROCESS_INIT_LIST,
> +		.git_cmd = 1,
> +	};
> +
> +	return run_commandl(&cmd, "mergetool", "--tool-help=diff", NULL);
>  }
>
>  static int parse_index_info(char *p, int *mode1, int *mode2,
> diff --git a/git.c b/git.c
> index 6662548986f..93179f44f78 100644
> --- a/git.c
> +++ b/git.c
> @@ -724,7 +724,13 @@ static void handle_builtin(int argc, const char **argv)
>
>  static void execv_dashed_external(const char **argv)
>  {
> -	struct child_process cmd = CHILD_PROCESS_INIT;
> +	struct child_process cmd = {
> +		CHILD_PROCESS_INIT_LIST,
> +		.clean_on_exit = 1,
> +		.wait_after_clean = 1,
> +		.silent_exec_failure = 1,
> +		.trace2_child_class = "dashed",
> +	};
>  	int status;
>
>  	if (get_super_prefix())
> @@ -736,10 +742,6 @@ static void execv_dashed_external(const char **argv)
>
>  	strvec_pushf(&cmd.args, "git-%s", argv[0]);
>  	strvec_pushv(&cmd.args, argv + 1);
> -	cmd.clean_on_exit = 1;
> -	cmd.wait_after_clean = 1;
> -	cmd.silent_exec_failure = 1;
> -	cmd.trace2_child_class = "dashed";
>
>  	trace2_cmd_name("_run_dashed_");
>
> diff --git a/run-command.c b/run-command.c
> index 23e100dffc4..4b20aa1b577 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -993,15 +993,45 @@ int finish_command_in_signal(struct child_process *cmd)
>
>  int run_command(struct child_process *cmd)
>  {
> -	int code;
> +	int starting = 1;
> +	int code = 0;
>
>  	if (cmd->out < 0 || cmd->err < 0)
>  		BUG("run_command with a pipe can cause deadlock");
>
>  	code = start_command(cmd);
>  	if (code)
> +		goto error;
> +	starting = 0;
> +	code = finish_command(cmd);
> +	if (!code)
> +		return 0;
> +error:
> +	switch (cmd->on_error) {
> +	case CHILD_PROCESS_ON_ERROR_DIE:
> +		die(starting ?
> +		    _("start_command() for '%s' failed (error code %d)") :
> +		    _("'%s': got non-zero exit code '%d'"),
> +		    cmd->args.v[0], code);
> +		break;
> +	case CHILD_PROCESS_ON_ERROR_RETURN:
>  		return code;
> -	return finish_command(cmd);
> +	default:
> +		BUG("unreachable");
> +	}
> +}
> +
> +int run_commandl(struct child_process *cmd, ...)
> +{
> +	va_list ap;
> +	const char *arg;
> +
> +	va_start(ap, cmd);
> +	while ((arg = va_arg(ap, const char *)))
> +		strvec_push(&cmd->args, arg);
> +	va_end(ap);
> +
> +	return run_command(cmd);
>  }
>
>  #ifndef NO_PTHREADS
> diff --git a/run-command.h b/run-command.h
> index fe2717ad11e..71e390350ed 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -15,7 +15,22 @@
>   * produces in the caller in order to process it.
>   */
>
> +enum child_process_on_error {
> +	/**
> +	 * Return a status code from run_command(). Set to 0 so that
> +	 * it'll be { 0 } init'd. If it's specified in a
> +	 * CHILD_PROCESS_INIT_LIST initialization we don't want a
> +	 * "-Woverride-init" warning.
> +	 */
> +	CHILD_PROCESS_ON_ERROR_RETURN = 0,
>
> +	/**
> +	 * die() with some sensible message if run_command() would
> +	 * have returned a non-zero exit code.
> +	 */
> +	CHILD_PROCESS_ON_ERROR_DIE,
> +};
> +
>  /**
>   * This describes the arguments, redirections, and environment of a
>   * command to run in a sub-process.
> @@ -42,6 +57,10 @@
>   *		redirected.
>   */
>  struct child_process {
> +	/**
> +	 * Error behavior, see "enum child_process_on_error" above.
> +	 */
> +	enum child_process_on_error on_error;
>
>  	/**
>  	 * The .args is a `struct strvec', use that API to manipulate
> @@ -144,10 +163,11 @@ struct child_process {
>  	void (*clean_on_exit_handler)(struct child_process *process);
>  };
>
> -#define CHILD_PROCESS_INIT { \
> +#define CHILD_PROCESS_INIT_LIST \
> +	/* .on_error = CHILD_PROCESS_ON_ERROR_RETURN */ \
>  	.args = STRVEC_INIT, \
> -	.env = STRVEC_INIT, \
> -}
> +	.env = STRVEC_INIT
> +#define CHILD_PROCESS_INIT { CHILD_PROCESS_INIT_LIST }
>
>  /**
>   * The functions: child_process_init, start_command, finish_command,
> @@ -218,6 +238,14 @@ int finish_command_in_signal(struct child_process *);
>   */
>  int run_command(struct child_process *);
>
> +/**
> + * Like run_command() but takes a variable number of arguments, which
> + * will be appended with the equivalent of strvec_pushl(&cmd.args,
> + * ...) before invoking run_command().
> + */
> +LAST_ARG_MUST_BE_NULL
> +int run_commandl(struct child_process *cmd, ...);
> +
>  /*
>   * Trigger an auto-gc
>   */

Re: [PATCH 0/8] run-command: remove run_command_v_*()

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 28 2022, René Scharfe wrote:

> Am 27.10.22 um 23:46 schrieb Ævar Arnfjörð Bjarmason:
>>
>> - I wish C had a nicer syntax for not just declaring but squashing
>>   together compile_time bracketed lists (think set operations). But the
>>   below "CHILD_PROCESS_INIT_LIST" is a pretty good poor man's version.
>>
>>   I see gcc/clang nicely give us safety rails for that with
>>   "-Woverride-init", for this sort of "opts struct with internal stuff,
>>   but also user options" I think it works out very nicely.
>>
>
> That's a nice and simple macro.  I played with a gross variant à la
>
>   #define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ }
>
> which would allow e.g.
>
>   struct child_process cmd = CHILD_PROCESS_INIT_EX(.git_cmd = 1);
>
> Yours is better, 

I actually think yours is better, anyway...

> but they share the downside of not actually saving any lines of code..

To me it's not about saving code, but that it's immediately obvious when
reading the code that this set of options can be determined and set at
function or scope entry.

We tend to otherwise have creep where the decl and option init drifts
apart over time, and with complex init's you might stare at it for 30s,
before realizing that between the decl and fully init ing it often 50
lines later nothing actually changed vis-a-vis the state, we could have
just done it earlier.

I think that's worth it in general, whether it's worth the churn in this
case...

>> - We have quite a few callers that want "on error, die", so maybe we
>>   should have something like that "on_error" sooner than later.
>
> We could add a die_on_error bit for that, or start_command_or_die() and
> run_command_or_die() variants (there I go again, multiplying APIs..).
> They could report the failed command, which a caller can't do because
> the internal strvec is already cleared once it learns of the failure.

*nod*

Re: [PATCH 0/8] run-command: remove run_command_v_*()

[René Scharfe]

Am 28.10.22 um 18:11 schrieb Ævar Arnfjörð Bjarmason:
>
> On Fri, Oct 28 2022, René Scharfe wrote:
>
>> Am 27.10.22 um 23:46 schrieb Ævar Arnfjörð Bjarmason:
>>>
>>> - I wish C had a nicer syntax for not just declaring but squashing
>>>   together compile_time bracketed lists (think set operations). But the
>>>   below "CHILD_PROCESS_INIT_LIST" is a pretty good poor man's version.
>>>
>>>   I see gcc/clang nicely give us safety rails for that with
>>>   "-Woverride-init", for this sort of "opts struct with internal stuff,
>>>   but also user options" I think it works out very nicely.
>>>
>>
>> That's a nice and simple macro.  I played with a gross variant à la
>>
>>   #define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ }
>>
>> which would allow e.g.
>>
>>   struct child_process cmd = CHILD_PROCESS_INIT_EX(.git_cmd = 1);
>>
>> Yours is better,
>
> I actually think yours is better, anyway...
>
>> but they share the downside of not actually saving any lines of code..
>
> To me it's not about saving code, but that it's immediately obvious when
> reading the code that this set of options can be determined and set at
> function or scope entry.
>
> We tend to otherwise have creep where the decl and option init drifts
> apart over time, and with complex init's you might stare at it for 30s,
> before realizing that between the decl and fully init ing it often 50
> lines later nothing actually changed vis-a-vis the state, we could have
> just done it earlier.

Hmm, we could do that by collecting the flag setting parts at the top,
without the need for a new macro.  Or at the bottom, before the
run_command() call.

> I think that's worth it in general, whether it's worth the churn in this
> case...
>
>>> - We have quite a few callers that want "on error, die", so maybe we
>>>   should have something like that "on_error" sooner than later.
>>
>> We could add a die_on_error bit for that, or start_command_or_die() and
>> run_command_or_die() variants (there I go again, multiplying APIs..).
>> They could report the failed command, which a caller can't do because
>> the internal strvec is already cleared once it learns of the failure.
>
> *nod*

Wait a second: Does it even make sense to mention the command in a die()
message after start_command() failed?  Unless .silent_exec_failure is
set, start_command() already reports it.  E.g. archive-tar.c has:

	if (start_command(&filter) < 0)
		die_errno(_("unable to start '%s' filter"), cmd.buf);

... and the result looks like this upon failure:

   $ git -c tar.tgz.command=nonsense archive --format=tgz HEAD
   error: cannot run nonsense: No such file or directory
   fatal: unable to start 'nonsense' filter: No such file or directory

The second message is mostly redundant, but it mentions that the failed
command was a filter.  Probably .silent_exec_failure should be set here,
then the die() message is no longer redundant.  This requires args[0] to
be stored outside of struct child_process, though, which is already done
here, but may be a bit tedious in other cases.

So for start_command(), would it be a generally useful to support a
scenario where upon failure

- the program terminates,
- but before that prints a single message,
- which includes the command that could not be started
- and some kind of hint why we tried to start it?

For run_command() we'd need to distinguish between not being able to
run the command and getting an error code from it after a successful
start.

René

Re: [PATCH 0/8] run-command: remove run_command_v_*()

[Ævar Arnfjörð Bjarmason]

On Fri, Oct 28 2022, René Scharfe wrote:

> Am 28.10.22 um 18:11 schrieb Ævar Arnfjörð Bjarmason:
>>
>> On Fri, Oct 28 2022, René Scharfe wrote:
>>
>>> Am 27.10.22 um 23:46 schrieb Ævar Arnfjörð Bjarmason:
>>>>
>>>> - I wish C had a nicer syntax for not just declaring but squashing
>>>>   together compile_time bracketed lists (think set operations). But the
>>>>   below "CHILD_PROCESS_INIT_LIST" is a pretty good poor man's version.
>>>>
>>>>   I see gcc/clang nicely give us safety rails for that with
>>>>   "-Woverride-init", for this sort of "opts struct with internal stuff,
>>>>   but also user options" I think it works out very nicely.
>>>>
>>>
>>> That's a nice and simple macro.  I played with a gross variant à la
>>>
>>>   #define CHILD_PROCESS_INIT_EX(...) { .args = STRVEC_INIT, __VA_ARGS__ }
>>>
>>> which would allow e.g.
>>>
>>>   struct child_process cmd = CHILD_PROCESS_INIT_EX(.git_cmd = 1);
>>>
>>> Yours is better,
>>
>> I actually think yours is better, anyway...
>>
>>> but they share the downside of not actually saving any lines of code..
>>
>> To me it's not about saving code, but that it's immediately obvious when
>> reading the code that this set of options can be determined and set at
>> function or scope entry.
>>
>> We tend to otherwise have creep where the decl and option init drifts
>> apart over time, and with complex init's you might stare at it for 30s,
>> before realizing that between the decl and fully init ing it often 50
>> lines later nothing actually changed vis-a-vis the state, we could have
>> just done it earlier.
>
> Hmm, we could do that by collecting the flag setting parts at the top,
> without the need for a new macro.

You mean just to pinky promise to always try to set the flags right
after we declare variables. Yes, we can try to aim for that, but
sometimes you need to declare quite a few of them (e.g. that difftools
caller), so not having distance between the decl & setting can help.
>> I think that's worth it in general, whether it's worth the churn in this
>> case...
>>
>>>> - We have quite a few callers that want "on error, die", so maybe we
>>>>   should have something like that "on_error" sooner than later.
>>>
>>> We could add a die_on_error bit for that, or start_command_or_die() and
>>> run_command_or_die() variants (there I go again, multiplying APIs..).
>>> They could report the failed command, which a caller can't do because
>>> the internal strvec is already cleared once it learns of the failure.
>>
>> *nod*
>
> Wait a second: Does it even make sense to mention the command in a die()
> message after start_command() failed?  Unless .silent_exec_failure is
> set, start_command() already reports it.  E.g. archive-tar.c has:
>
> 	if (start_command(&filter) < 0)
> 		die_errno(_("unable to start '%s' filter"), cmd.buf);
>
> ... and the result looks like this upon failure:
>
>    $ git -c tar.tgz.command=nonsense archive --format=tgz HEAD
>    error: cannot run nonsense: No such file or directory
>    fatal: unable to start 'nonsense' filter: No such file or directory
>
> The second message is mostly redundant, but it mentions that the failed
> command was a filter.  Probably .silent_exec_failure should be set here,
> then the die() message is no longer redundant.  This requires args[0] to
> be stored outside of struct child_process, though, which is already done
> here, but may be a bit tedious in other cases.

Yes, maybe these messages aren't all that useful. But note that that
"error" is emittted by thy "#ifndef ...WINDOWS..." part of the code. See
also the recent 255a6f91ae4 (t1800: correct test to handle Cygwin,
2022-09-15).

But I suspect some of the "die" messaging is just boilerplate/legacy.

> So for start_command(), would it be a generally useful to support a
> scenario where upon failure
>
> - the program terminates,
> - but before that prints a single message,
> - which includes the command that could not be started

Isn't this just the "cannot spawn"/"cannot fork" etc. messaging from
start_command() now?

> - and some kind of hint why we tried to start it?
>
> For run_command() we'd need to distinguish between not being able to
> run the command and getting an error code from it after a successful
> start.

*nod*, it would be nice if that messaging was portable. Part of that
just seems to be that we need to do the same (or share) the whole "set
die message" part we do on *nix.

Some of the reasons a command fails aren't portable, but we should be
able to portably emit e.g. "can't execute this command because it
doesn't exist" etc.

Re: [PATCH 0/8] run-command: remove run_command_v_*()

[René Scharfe]

Am 29.10.2022 um 04:17 schrieb Ævar Arnfjörð Bjarmason:
>
> On Fri, Oct 28 2022, René Scharfe wrote:
>
>> Wait a second: Does it even make sense to mention the command in a die()
>> message after start_command() failed?  Unless .silent_exec_failure is
>> set, start_command() already reports it.  E.g. archive-tar.c has:
>>
>> 	if (start_command(&filter) < 0)
>> 		die_errno(_("unable to start '%s' filter"), cmd.buf);
>>
>> ... and the result looks like this upon failure:
>>
>>    $ git -c tar.tgz.command=nonsense archive --format=tgz HEAD
>>    error: cannot run nonsense: No such file or directory
>>    fatal: unable to start 'nonsense' filter: No such file or directory
>>
>> The second message is mostly redundant, but it mentions that the failed
>> command was a filter.  Probably .silent_exec_failure should be set here,
>> then the die() message is no longer redundant.  This requires args[0] to
>> be stored outside of struct child_process, though, which is already done
>> here, but may be a bit tedious in other cases.
>
> Yes, maybe these messages aren't all that useful. But note that that
> "error" is emittted by thy "#ifndef ...WINDOWS..." part of the code. See
> also the recent 255a6f91ae4 (t1800: correct test to handle Cygwin,
> 2022-09-15).

On Windows I get:

  $ git -c tar.tgz.command=nonsense archive --format=tgz HEAD
  error: cannot spawn nonsense: No such file or directory
  fatal: unable to start 'nonsense' filter: No such file or directory

>> So for start_command(), would it be a generally useful to support a
>> scenario where upon failure
>>
>> - the program terminates,
>> - but before that prints a single message,
>> - which includes the command that could not be started
>
> Isn't this just the "cannot spawn"/"cannot fork" etc. messaging from
> start_command() now?

Sure, a subset is easy, but do we need all four points?

>> - and some kind of hint why we tried to start it?

I suspect .silent_exec_failure suffices for most cases.  Testing calls
of commands that usually exist is a bit hard without source code
changes, though.  Then again: Beatifying messages that will almost
never be seen isn't probably worth it anyway.

But I'll send a patch for the archive case above.

René

Re: [PATCH 0/8] run-command: remove run_command_v_*()

[Taylor Blau]

On Thu, Oct 27, 2022 at 06:30:36PM +0200, René Scharfe wrote:
> Replace the convenience functions run_command_v_opt() et. al. and use
> struct child_process and run_command() directly instead, for an overall
> code reduction and a simpler and more flexible API that allows creating
> argument lists without magic numbers and reduced risk of memory leaks.
>
> This is a broken-out and polished version of the original scratch at
> https://lore.kernel.org/git/9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de/

Thanks, this round is looking pretty good to me. It sounds like there
were a couple of small comments for which a reroll would be appreciated.

Assuming that that all looks good, I can queue the rerolled version.
Thanks.

Thanks,
Taylor

[PATCH v2 0/12] run-command: remove run_command_v_*()

[René Scharfe]

Replace the convenience functions run_command_v_opt() et. al. and use
struct child_process and run_command() directly instead, for an overall
code reduction and a simpler and more flexible API that allows creating
argument lists without magic numbers and reduced risk of memory leaks.

Changes since v1:
- Do the return value fix earlier; it was only an afterthought before.
  Keep the colon (no "while at it, ...").
- Break out the xstrdup(), oid_to_hex_r() and C99 cleanups.
- Convert tricky string arrays before strvecs because Ævar didn't like
  the opposite order.
- Extend the example code in tmp-objdir.h so it still only requires
  "cmd".

Ævar Arnfjörð Bjarmason (1):
  merge: remove always-the-same "verbose" arguments

René Scharfe (11):
  run-command: fix return value comment
  am: simplify building "show" argument list
  bisect: simplify building "checkout" argument list
  bisect--helper: factor out do_bisect_run()
  sequencer: simplify building argument list in do_exec()
  use child_process member "args" instead of string array variable
  use child_process members "args" and "env" directly
  replace and remove run_command_v_opt_cd_env()
  replace and remove run_command_v_opt_tr2()
  replace and remove run_command_v_opt_cd_env_tr2()
  replace and remove run_command_v_opt()

 add-interactive.c        |   9 ++-
 bisect.c                 |  12 ++--
 builtin/add.c            |  19 +++--
 builtin/am.c             |  12 ++--
 builtin/bisect--helper.c |  68 +++++++++---------
 builtin/clone.c          |  41 ++++++-----
 builtin/difftool.c       |  24 ++++---
 builtin/fetch.c          |   9 ++-
 builtin/gc.c             |  55 ++++++++++-----
 builtin/merge-index.c    |   4 +-
 builtin/merge.c          |  53 ++++++--------
 builtin/pull.c           | 147 +++++++++++++++++++--------------------
 builtin/remote.c         |  40 +++++------
 compat/mingw.c           |  11 +--
 diff.c                   |  27 ++++---
 fsmonitor-ipc.c          |  10 ++-
 git.c                    |  15 ++--
 ll-merge.c               |   7 +-
 merge.c                  |  18 ++---
 run-command.c            |  35 ----------
 run-command.h            |  34 +--------
 scalar.c                 |  13 ++--
 sequencer.c              |  32 ++++-----
 shell.c                  |  17 +++--
 t/helper/test-fake-ssh.c |   7 +-
 t/helper/test-trace2.c   |   4 +-
 tmp-objdir.h             |   6 +-
 27 files changed, 346 insertions(+), 383 deletions(-)

Range-Diff gegen v1:
 1:  c0b2b88500 =  1:  113a9e0a81 merge: remove always-the-same "verbose" arguments
 -:  ---------- >  2:  d10e70460b run-command: fix return value comment
 -:  ---------- >  3:  c8cd913e39 am: simplify building "show" argument list
 -:  ---------- >  4:  4bb142e4a9 bisect: simplify building "checkout" argument list
 2:  387db545d1 =  5:  5fcbe94eb4 bisect--helper: factor out do_bisect_run()
 -:  ---------- >  6:  962403cf22 sequencer: simplify building argument list in do_exec()
 4:  348bc6d32c !  7:  f1689abe85 use child_process member "args" instead of string array variable
    @@ Commit message

         Use run_command() with a struct child_process variable and populate its
         "args" member directly instead of building a string array and passing it
    -    to run_command_v_opt().  This avoids the use of magic index numbers.
    -
    - ## bisect.c ##
    -@@ bisect.c: static struct oid_array skipped_revs;
    -
    - static struct object_id *current_bad_oid;
    -
    --static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
    --
    - static const char *term_bad;
    - static const char *term_good;
    -
    -@@ bisect.c: static int is_expected_rev(const struct object_id *oid)
    - enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
    - 				  int no_checkout)
    - {
    --	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
    - 	struct commit *commit;
    - 	struct pretty_print_context pp = {0};
    - 	struct strbuf commit_msg = STRBUF_INIT;
    -
    --	oid_to_hex_r(bisect_rev_hex, bisect_rev);
    - 	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
    -
    --	argv_checkout[2] = bisect_rev_hex;
    - 	if (no_checkout) {
    - 		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
    - 			   UPDATE_REFS_DIE_ON_ERR);
    - 	} else {
    --		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
    -+		struct child_process cmd = CHILD_PROCESS_INIT;
    -+
    -+		cmd.git_cmd = 1;
    -+		strvec_pushl(&cmd.args, "checkout", "-q",
    -+			     oid_to_hex(bisect_rev), "--", NULL);
    -+		if (run_command(&cmd))
    - 			/*
    - 			 * Errors in `run_command()` itself, signaled by res < 0,
    - 			 * and errors in the child process, signaled by res > 0
    -
    - ## builtin/am.c ##
    -@@ builtin/am.c: static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
    - 	int len;
    -
    - 	if (!is_null_oid(&state->orig_commit)) {
    --		const char *av[4] = { "show", NULL, "--", NULL };
    --		char *new_oid_str;
    --		int ret;
    -+		struct child_process cmd = CHILD_PROCESS_INIT;
    -
    --		av[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));
    --		ret = run_command_v_opt(av, RUN_GIT_CMD);
    --		free(new_oid_str);
    --		return ret;
    -+		strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit),
    -+			     "--", NULL);
    -+		cmd.git_cmd = 1;
    -+		return run_command(&cmd);
    - 	}
    -
    - 	switch (sub_mode) {
    +    to run_command_v_opt().  This avoids the use of magic index numbers and
    +    makes simplifies the possible addition of more arguments in the future.

      ## builtin/difftool.c ##
     @@ builtin/difftool.c: static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
    @@ ll-merge.c: static enum ll_merge_result ll_ext_merge(const struct ll_merge_drive
      	if (fd < 0)
      		goto bad;

    - ## sequencer.c ##
    -@@ sequencer.c: static int error_failed_squash(struct repository *r,
    -
    - static int do_exec(struct repository *r, const char *command_line)
    - {
    --	const char *child_argv[] = { NULL, NULL };
    -+	struct child_process cmd = CHILD_PROCESS_INIT;
    - 	int dirty, status;
    -
    - 	fprintf(stderr, _("Executing: %s\n"), command_line);
    --	child_argv[0] = command_line;
    --	status = run_command_v_opt(child_argv, RUN_USING_SHELL);
    -+	cmd.use_shell = 1;
    -+	strvec_push(&cmd.args, command_line);
    -+	status = run_command(&cmd);
    -
    - 	/* force re-reading of the cache */
    - 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
    -
      ## t/helper/test-fake-ssh.c ##
     @@ t/helper/test-fake-ssh.c: int cmd_main(int argc, const char **argv)
      	struct strbuf buf = STRBUF_INIT;
 3:  7745e16df4 =  8:  a467a4ecb5 use child_process members "args" and "env" directly
 5:  eeaa6aa86d !  9:  dc27358775 replace and remove run_command_v_opt_cd_env()
    @@ run-command.h
     @@ run-command.h: 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_v_opt, child_process_clear do the following:
    +  * The functions: start_command, finish_command, run_command,
    +- * run_command_v_opt, run_command_v_opt_cd_env do the following:
    ++ * run_command_v_opt do the following:
       *
       * - If a system call failed, errno is set and -1 is returned. A diagnostic
       *   is printed.
    @@ run-command.h: int run_command_v_opt_tr2(const char **argv, int opt, const char

      ## tmp-objdir.h ##
     @@
    +  *
       * Example:
       *
    ++ *	struct child_process child = 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_pushv(&cmd.env, tmp_objdir_env(t));
    -+ *	if (!run_command(&cmd)) && !tmp_objdir_migrate(t))
    ++ *	strvec_push(&child.args, cmd);
    ++ *	strvec_pushv(&child.env, tmp_objdir_env(t));
    ++ *	if (!run_command(&child)) && !tmp_objdir_migrate(t))
       *		printf("success!\n");
       *	else
       *		die("failed...tmp_objdir will clean up for us");
 6:  95b5dcbb66 = 10:  dcd65580c6 replace and remove run_command_v_opt_tr2()
 7:  5e111ab053 ! 11:  389ec8ef54 replace and remove run_command_v_opt_cd_env_tr2()
    @@ run-command.h: int run_auto_maintenance(int quiet);

      /**
     - * Convenience functions that encapsulate a sequence of
    -+ * Convenience function that encapsulate a sequence of
    ++ * Convenience function that encapsulates 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`,
 8:  f1f438d724 ! 12:  a6e7135e31 replace and remove run_command_v_opt()
    @@ Commit message

         Suggested-by: Jeff King <peff@peff.net>

    + ## bisect.c ##
    +@@ bisect.c: enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
    + 		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
    + 			   UPDATE_REFS_DIE_ON_ERR);
    + 	} else {
    +-		const char *argv_checkout[] = {
    +-			"checkout", "-q", oid_to_hex(bisect_rev), "--", NULL
    +-		};
    ++		struct child_process cmd = CHILD_PROCESS_INIT;
    +
    +-		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
    ++		cmd.git_cmd = 1;
    ++		strvec_pushl(&cmd.args, "checkout", "-q",
    ++			     oid_to_hex(bisect_rev), "--", NULL);
    ++		if (run_command(&cmd))
    + 			/*
    + 			 * Errors in `run_command()` itself, signaled by res < 0,
    + 			 * and errors in the child process, signaled by res > 0
    +
    + ## builtin/am.c ##
    +@@ builtin/am.c: static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
    + 	int len;
    +
    + 	if (!is_null_oid(&state->orig_commit)) {
    +-		const char *av[] = {
    +-			"show", oid_to_hex(&state->orig_commit), "--", NULL
    +-		};
    ++		struct child_process cmd = CHILD_PROCESS_INIT;
    +
    +-		return run_command_v_opt(av, RUN_GIT_CMD);
    ++		strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit),
    ++			     "--", NULL);
    ++		cmd.git_cmd = 1;
    ++		return run_command(&cmd);
    + 	}
    +
    + 	switch (sub_mode) {
    +
      ## builtin/bisect--helper.c ##
     @@ builtin/bisect--helper.c: static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
      		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
    @@ run-command.c: int run_command(struct child_process *cmd)

      ## run-command.h ##
     @@ run-command.h: struct child_process {
    + }

      /**
    -  * The functions: child_process_init, start_command, finish_command,
    -- * run_command, run_command_v_opt, child_process_clear do the following:
    -+ * run_command, child_process_clear do the following:
    +- * The functions: start_command, finish_command, run_command,
    +- * run_command_v_opt do the following:
    ++ * The functions: start_command, finish_command, run_command do the following:
       *
       * - If a system call failed, errno is set and -1 is returned. A diagnostic
       *   is printed.
    @@ run-command.h: int run_command(struct child_process *);
     -#define RUN_CLOSE_OBJECT_STORE		(1<<7)
     -
     -/**
    -- * Convenience function that encapsulate a sequence of
    +- * Convenience function that encapsulates 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.h: int run_command(struct child_process *);
       * 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

    + ## sequencer.c ##
    +@@ sequencer.c: static int error_failed_squash(struct repository *r,
    +
    + static int do_exec(struct repository *r, const char *command_line)
    + {
    +-	const char *child_argv[] = { command_line, NULL };
    ++	struct child_process cmd = CHILD_PROCESS_INIT;
    + 	int dirty, status;
    +
    + 	fprintf(stderr, _("Executing: %s\n"), command_line);
    +-	status = run_command_v_opt(child_argv, RUN_USING_SHELL);
    ++	cmd.use_shell = 1;
    ++	strvec_push(&cmd.args, command_line);
    ++	status = run_command(&cmd);
    +
    + 	/* force re-reading of the cache */
    + 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
    +
      ## shell.c ##
     @@ shell.c: static void cd_to_homedir(void)
      static void run_shell(void)
 9:  59677c9598 <  -:  ---------- run-command: fix return value comment
--
2.38.1

[PATCH v2 01/12] merge: remove always-the-same "verbose" arguments

[René Scharfe]

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

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>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/merge.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 5900b81729..3bb49d805b 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.1

[PATCH v2 02/12] run-command: fix return value comment

[René Scharfe]

483bbd4e4c (run-command: introduce child_process_init(), 2014-08-19) and
2d71608ec0 (run-command: factor out child_process_clear(), 2015-10-24)
added help texts about child_process_init() and child_process_clear()
without updating the immediately following documentation of return codes
that only applied to the preexisting functions.

4c4066d95d (run-command: move doc to run-command.h, 2019-11-17) started
to list the functions explicitly that this paragraph applies to, but
still wrongly included child_process_init() and child_process_clear().
Remove their names from that list.

Suggested-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 run-command.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/run-command.h b/run-command.h
index e3e1ea01ad..820fc25b02 100644
--- a/run-command.h
+++ b/run-command.h
@@ -150,9 +150,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:
+ * The functions: start_command, finish_command, run_command,
+ * run_command_v_opt, run_command_v_opt_cd_env do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
  *   is printed.
--
2.38.1

[PATCH v2 03/12] am: simplify building "show" argument list

[René Scharfe]

Build the string array av during initialization, without any magic
numbers or heap allocations.  Not duplicating the result of oid_to_hex()
is safe because run_command_v_opt() duplicates all arguments already.
(It would even be safe if it didn't, but that's a different story.)

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/am.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 39fea24833..5781e7a95e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2187,14 +2187,11 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
 	int len;

 	if (!is_null_oid(&state->orig_commit)) {
-		const char *av[4] = { "show", NULL, "--", NULL };
-		char *new_oid_str;
-		int ret;
+		const char *av[] = {
+			"show", oid_to_hex(&state->orig_commit), "--", NULL
+		};

-		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;
+		return run_command_v_opt(av, RUN_GIT_CMD);
 	}

 	switch (sub_mode) {
--
2.38.1

[PATCH v2 04/12] bisect: simplify building "checkout" argument list

[René Scharfe]

Reduce the scope of argv_checkout, which allows to fully build it during
initialization.  Use oid_to_hex() instead of oid_to_hex_r(), because
that's simpler and using the static buffer of the former is just as safe
as the old static argv_checkout.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 bisect.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/bisect.c b/bisect.c
index fd581b85a7..090aa5c4b4 100644
--- a/bisect.c
+++ b/bisect.c
@@ -22,8 +22,6 @@ static struct oid_array skipped_revs;

 static struct object_id *current_bad_oid;

-static const char *argv_checkout[] = {"checkout", "-q", NULL, "--", NULL};
-
 static const char *term_bad;
 static const char *term_good;

@@ -729,19 +727,20 @@ static int is_expected_rev(const struct object_id *oid)
 enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
 				  int no_checkout)
 {
-	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
 	struct commit *commit;
 	struct pretty_print_context pp = {0};
 	struct strbuf commit_msg = STRBUF_INIT;

-	oid_to_hex_r(bisect_rev_hex, bisect_rev);
 	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);

-	argv_checkout[2] = bisect_rev_hex;
 	if (no_checkout) {
 		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
 	} else {
+		const char *argv_checkout[] = {
+			"checkout", "-q", oid_to_hex(bisect_rev), "--", NULL
+		};
+
 		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
 			/*
 			 * Errors in `run_command()` itself, signaled by res < 0,
--
2.38.1

[PATCH v2 05/12] bisect--helper: factor out do_bisect_run()

[René Scharfe]

Deduplicate the code for reporting and starting the bisect run command
by moving it to a short helper function.  Use a string array instead of
a strvec to prepare the arguments, for simplicity.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/bisect--helper.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 28ef7ec2a4..70d1e9d1ad 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -1142,8 +1142,14 @@ static int get_first_good(const char *refname UNUSED,
 	return 1;
 }

-static int verify_good(const struct bisect_terms *terms,
-		       const char **quoted_argv)
+static int do_bisect_run(const char *command)
+{
+	const char *argv[] = { command, NULL };
+	printf(_("running %s\n"), command);
+	return run_command_v_opt(argv, RUN_USING_SHELL);
+}
+
+static int verify_good(const struct bisect_terms *terms, const char *command)
 {
 	int rc;
 	enum bisect_error res;
@@ -1163,8 +1169,7 @@ static int verify_good(const struct bisect_terms *terms,
 	if (res != BISECT_OK)
 		return -1;

-	printf(_("running %s\n"), quoted_argv[0]);
-	rc = run_command_v_opt(quoted_argv, RUN_USING_SHELL);
+	rc = do_bisect_run(command);

 	res = bisect_checkout(&current_rev, no_checkout);
 	if (res != BISECT_OK)
@@ -1177,7 +1182,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 {
 	int res = BISECT_OK;
 	struct strbuf command = STRBUF_INIT;
-	struct strvec run_args = STRVEC_INIT;
 	const char *new_state;
 	int temporary_stdout_fd, saved_stdout;
 	int is_first_run = 1;
@@ -1192,11 +1196,8 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		return BISECT_FAILED;
 	}

-	strvec_push(&run_args, command.buf);
-
 	while (1) {
-		printf(_("running %s\n"), command.buf);
-		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
+		res = do_bisect_run(command.buf);

 		/*
 		 * Exit code 126 and 127 can either come from the shell
@@ -1206,7 +1207,7 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 		 * missing or non-executable script.
 		 */
 		if (is_first_run && (res == 126 || res == 127)) {
-			int rc = verify_good(terms, run_args.v);
+			int rc = verify_good(terms, command.buf);
 			is_first_run = 0;
 			if (rc < 0) {
 				error(_("unable to verify '%s' on good"
@@ -1273,7 +1274,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 	}

 	strbuf_release(&command);
-	strvec_clear(&run_args);
 	return res;
 }

--
2.38.1

[PATCH v2 06/12] sequencer: simplify building argument list in do_exec()

[René Scharfe]

Build child_argv during initialization, taking advantage of the C99
support for initialization expressions that are not compile time
constants.  This avoids the use of a magic index constant and is shorter
and simpler.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 sequencer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index debb2ecbaf..66eedd2c76 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3558,11 +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 };
+	const char *child_argv[] = { command_line, 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);

 	/* force re-reading of the cache */
--
2.38.1

[PATCH v2 07/12] use child_process member "args" instead of string array variable

[René Scharfe]

Use run_command() with a struct child_process variable and populate its
"args" member directly instead of building a string array and passing it
to run_command_v_opt().  This avoids the use of magic index numbers and
makes simplifies the possible addition of more arguments in the future.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 builtin/difftool.c       | 17 +++++++++--------
 builtin/merge.c          | 32 ++++++++++++--------------------
 builtin/remote.c         | 14 ++++++++------
 compat/mingw.c           | 11 +++++++----
 ll-merge.c               |  7 ++++---
 t/helper/test-fake-ssh.c |  7 ++++---
 6 files changed, 44 insertions(+), 44 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4b10ad1a36..22bcc3444b 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -360,8 +360,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	struct pair_entry *entry;
 	struct index_state wtindex;
 	struct checkout lstate, rstate;
-	int flags = RUN_GIT_CMD, err = 0;
-	const char *helper_argv[] = { "difftool--helper", NULL, NULL, NULL };
+	int err = 0;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct hashmap wt_modified, tmp_modified;
 	int indices_loaded = 0;

@@ -563,16 +563,17 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,
 	}

 	strbuf_setlen(&ldir, ldir_len);
-	helper_argv[1] = ldir.buf;
 	strbuf_setlen(&rdir, rdir_len);
-	helper_argv[2] = rdir.buf;

 	if (extcmd) {
-		helper_argv[0] = extcmd;
-		flags = 0;
-	} else
+		strvec_push(&cmd.args, extcmd);
+	} else {
+		strvec_push(&cmd.args, "difftool--helper");
+		cmd.git_cmd = 1;
 		setenv("GIT_DIFFTOOL_DIRDIFF", "true", 1);
-	ret = run_command_v_opt(helper_argv, flags);
+	}
+	strvec_pushl(&cmd.args, ldir.buf, rdir.buf, NULL);
+	ret = run_command(&cmd);

 	/* TODO: audit for interaction with sparse-index. */
 	ensure_full_index(&wtindex);
diff --git a/builtin/merge.c b/builtin/merge.c
index 3bb49d805b..3481577145 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -347,33 +347,25 @@ static int save_state(struct object_id *stash)

 static void read_empty(const struct object_id *oid)
 {
-	int i = 0;
-	const char *args[7];
-
-	args[i++] = "read-tree";
-	args[i++] = "-m";
-	args[i++] = "-u";
-	args[i++] = empty_tree_oid_hex();
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_pushl(&cmd.args, "read-tree", "-m", "-u", empty_tree_oid_hex(),
+		     oid_to_hex(oid), NULL);
+	cmd.git_cmd = 1;

-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	if (run_command(&cmd))
 		die(_("read-tree failed"));
 }

 static void reset_hard(const struct object_id *oid)
 {
-	int i = 0;
-	const char *args[6];
-
-	args[i++] = "read-tree";
-	args[i++] = "-v";
-	args[i++] = "--reset";
-	args[i++] = "-u";
-	args[i++] = oid_to_hex(oid);
-	args[i] = NULL;
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_pushl(&cmd.args, "read-tree", "-v", "--reset", "-u",
+		     oid_to_hex(oid), NULL);
+	cmd.git_cmd = 1;

-	if (run_command_v_opt(args, RUN_GIT_CMD))
+	if (run_command(&cmd))
 		die(_("read-tree failed"));
 }

diff --git a/builtin/remote.c b/builtin/remote.c
index 93285fc06e..f0c11fa301 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -92,13 +92,15 @@ static int verbose;

 static int fetch_remote(const char *name)
 {
-	const char *argv[] = { "fetch", name, NULL, NULL };
-	if (verbose) {
-		argv[1] = "-v";
-		argv[2] = name;
-	}
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	strvec_push(&cmd.args, "fetch");
+	if (verbose)
+		strvec_push(&cmd.args, "-v");
+	strvec_push(&cmd.args, name);
+	cmd.git_cmd = 1;
 	printf_ln(_("Updating %s"), name);
-	if (run_command_v_opt(argv, RUN_GIT_CMD))
+	if (run_command(&cmd))
 		return error(_("Could not fetch %s"), name);
 	return 0;
 }
diff --git a/compat/mingw.c b/compat/mingw.c
index 901375d584..d614f156df 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -196,16 +196,19 @@ static int read_yes_no_answer(void)
 static int ask_yes_no_if_possible(const char *format, ...)
 {
 	char question[4096];
-	const char *retry_hook[] = { NULL, NULL, NULL };
+	const char *retry_hook;
 	va_list args;

 	va_start(args, format);
 	vsnprintf(question, sizeof(question), format, args);
 	va_end(args);

-	if ((retry_hook[0] = mingw_getenv("GIT_ASK_YESNO"))) {
-		retry_hook[1] = question;
-		return !run_command_v_opt(retry_hook, 0);
+	retry_hook = mingw_getenv("GIT_ASK_YESNO");
+	if (retry_hook) {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		strvec_pushl(&cmd.args, retry_hook, question, NULL);
+		return !run_command(&cmd);
 	}

 	if (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))
diff --git a/ll-merge.c b/ll-merge.c
index a8e2db9336..22a603e8af 100644
--- a/ll-merge.c
+++ b/ll-merge.c
@@ -193,7 +193,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,
 	struct strbuf cmd = STRBUF_INIT;
 	struct strbuf_expand_dict_entry dict[6];
 	struct strbuf path_sq = STRBUF_INIT;
-	const char *args[] = { NULL, NULL };
+	struct child_process child = CHILD_PROCESS_INIT;
 	int status, fd, i;
 	struct stat st;
 	enum ll_merge_result ret;
@@ -219,8 +219,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,

 	strbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);

-	args[0] = cmd.buf;
-	status = run_command_v_opt(args, RUN_USING_SHELL);
+	child.use_shell = 1;
+	strvec_push(&child.args, cmd.buf);
+	status = run_command(&child);
 	fd = open(temp[1], O_RDONLY);
 	if (fd < 0)
 		goto bad;
diff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c
index 12beee99ad..2e576bcc11 100644
--- a/t/helper/test-fake-ssh.c
+++ b/t/helper/test-fake-ssh.c
@@ -8,7 +8,7 @@ int cmd_main(int argc, const char **argv)
 	struct strbuf buf = STRBUF_INIT;
 	FILE *f;
 	int i;
-	const char *child_argv[] = { NULL, NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;

 	/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */
 	if (!trash_directory)
@@ -25,6 +25,7 @@ int cmd_main(int argc, const char **argv)
 	/* Now, evaluate the *last* parameter */
 	if (argc < 2)
 		return 0;
-	child_argv[0] = argv[argc - 1];
-	return run_command_v_opt(child_argv, RUN_USING_SHELL);
+	cmd.use_shell = 1;
+	strvec_push(&cmd.args, argv[argc - 1]);
+	return run_command(&cmd);
 }
--
2.38.1

[PATCH v2 08/12] use child_process members "args" and "env" directly

[René Scharfe]

Build argument list and environment of child processes by using
struct child_process and populating its members "args" and "env"
directly instead of maintaining separate strvecs and letting
run_command_v_opt() and friends populate these members.  This is
simpler, shorter and slightly more efficient.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 add-interactive.c        |   9 ++-
 builtin/add.c            |  19 +++--
 builtin/bisect--helper.c |  35 +++++-----
 builtin/clone.c          |  33 ++++-----
 builtin/gc.c             |  14 ++--
 builtin/merge.c          |  10 +--
 builtin/pull.c           | 147 +++++++++++++++++++--------------------
 builtin/remote.c         |  26 ++++---
 diff.c                   |  27 ++++---
 git.c                    |  15 ++--
 merge.c                  |  18 ++---
 scalar.c                 |  13 ++--
 sequencer.c              |  25 +++----
 13 files changed, 185 insertions(+), 206 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index f071b2a1b4..ecc5ae1b24 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -997,18 +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_v_opt(args.v, 0);
-		strvec_clear(&args);
+		res = run_command(&cmd);
 	}

 	putchar('\n');
diff --git a/builtin/add.c b/builtin/add.c
index f84372964c..626c71ec6a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -240,8 +240,8 @@ 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;
-	struct strvec argv = STRVEC_INIT;
+	int i;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int use_builtin_add_i =
 		git_env_bool("GIT_TEST_ADD_I_USE_BUILTIN", -1);

@@ -272,19 +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);

-	status = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-	return status;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 int interactive_add(const char **argv, const char *prefix, int patch)
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 70d1e9d1ad..5c63ba6994 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -220,18 +220,17 @@ static int bisect_reset(const char *commit)
 	}

 	if (!ref_exists("BISECT_HEAD")) {
-		struct strvec argv = STRVEC_INIT;
+		struct child_process cmd = CHILD_PROCESS_INIT;

-		strvec_pushl(&argv, "checkout", branch.buf, "--", NULL);
-		if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+		cmd.git_cmd = 1;
+		strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL);
+		if (run_command(&cmd)) {
 			error(_("could not check out original"
 				" HEAD '%s'. Try 'git bisect"
 				" reset <commit>'."), branch.buf);
 			strbuf_release(&branch);
-			strvec_clear(&argv);
 			return -1;
 		}
-		strvec_clear(&argv);
 	}

 	strbuf_release(&branch);
@@ -1098,40 +1097,38 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar

 static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)
 {
-	struct strvec args = STRVEC_INIT;
-	int flags = RUN_COMMAND_NO_STDIN, res = 0;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct strbuf sb = STRBUF_INIT;

 	if (bisect_next_check(terms, NULL) != 0)
 		return BISECT_FAILED;

+	cmd.no_stdin = 1;
 	if (!argc) {
 		if ((getenv("DISPLAY") || getenv("SESSIONNAME") || getenv("MSYSTEM") ||
 		     getenv("SECURITYSESSIONID")) && exists_in_PATH("gitk")) {
-			strvec_push(&args, "gitk");
+			strvec_push(&cmd.args, "gitk");
 		} else {
-			strvec_push(&args, "log");
-			flags |= RUN_GIT_CMD;
+			strvec_push(&cmd.args, "log");
+			cmd.git_cmd = 1;
 		}
 	} else {
 		if (argv[0][0] == '-') {
-			strvec_push(&args, "log");
-			flags |= RUN_GIT_CMD;
+			strvec_push(&cmd.args, "log");
+			cmd.git_cmd = 1;
 		} else if (strcmp(argv[0], "tig") && !starts_with(argv[0], "git"))
-			flags |= RUN_GIT_CMD;
+			cmd.git_cmd = 1;

-		strvec_pushv(&args, argv);
+		strvec_pushv(&cmd.args, argv);
 	}

-	strvec_pushl(&args, "--bisect", "--", NULL);
+	strvec_pushl(&cmd.args, "--bisect", "--", NULL);

 	strbuf_read_file(&sb, git_path_bisect_names(), 0);
-	sq_dequote_to_strvec(sb.buf, &args);
+	sq_dequote_to_strvec(sb.buf, &cmd.args);
 	strbuf_release(&sb);

-	res = run_command_v_opt(args.v, flags);
-	strvec_clear(&args);
-	return res;
+	return run_command(&cmd);
 }

 static int get_first_good(const char *refname UNUSED,
diff --git a/builtin/clone.c b/builtin/clone.c
index 547d6464b3..56e7775dae 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -653,9 +653,9 @@ 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;
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int result = 0;
-	strvec_pushl(&argv, "-C", repo, "sparse-checkout", "set", NULL);
+	strvec_pushl(&cmd.args, "-C", repo, "sparse-checkout", "set", NULL);

 	/*
 	 * We must apply the setting in the current process
@@ -663,12 +663,12 @@ static int git_sparse_checkout_init(const char *repo)
 	 */
 	core_apply_sparse_checkout = 1;

-	if (run_command_v_opt(argv.v, RUN_GIT_CMD)) {
+	cmd.git_cmd = 1;
+	if (run_command(&cmd)) {
 		error(_("failed to initialize sparse-checkout"));
 		result = 1;
 	}

-	strvec_clear(&argv);
 	return result;
 }

@@ -733,37 +733,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");

-		err = run_command_v_opt(args.v, RUN_GIT_CMD);
-		strvec_clear(&args);
+		cmd.git_cmd = 1;
+		err = run_command(&cmd);
 	}

 	return err;
diff --git a/builtin/gc.c b/builtin/gc.c
index 24ea85c7af..a185bec813 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -1910,20 +1910,16 @@ static char *schtasks_task_name(const char *frequency)
 static int schtasks_remove_task(enum schedule_priority schedule)
 {
 	const char *cmd = "schtasks";
-	int result;
-	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", name, "/f", NULL);
-
-	result = run_command_v_opt(args.v, 0);
-
-	strvec_clear(&args);
+	strvec_split(&child.args, cmd);
+	strvec_pushl(&child.args, "/delete", "/tn", name, "/f", NULL);
 	free(name);
-	return result;
+
+	return run_command(&child);
 }

 static int schtasks_remove_tasks(void)
diff --git a/builtin/merge.c b/builtin/merge.c
index 3481577145..b3f75f55c8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -372,22 +372,22 @@ static void reset_hard(const struct object_id *oid)
 static void restore_state(const struct object_id *head,
 			  const struct object_id *stash)
 {
-	struct strvec args = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_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));
+	strvec_pushl(&cmd.args, "stash", "apply", "--index", "--quiet", NULL);
+	strvec_push(&cmd.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);
+	cmd.git_cmd = 1;
+	run_command(&cmd);

 refresh_cache:
 	if (discard_cache() < 0 || read_cache() < 0)
diff --git a/builtin/pull.c b/builtin/pull.c
index 403a24d7ca..b21edd767a 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -515,76 +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;
-	int ret;
+	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?");
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);
-	strvec_clear(&args);
-	return ret;
+	cmd.git_cmd = 1;
+	cmd.close_object_store = 1;
+	return run_command(&cmd);
 }

 /**
@@ -653,52 +652,50 @@ static int update_submodules(void)
  */
 static int run_merge(void)
 {
-	int ret;
-	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");
-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
-	return ret;
+	strvec_push(&cmd.args, "FETCH_HEAD");
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 /**
@@ -879,43 +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)
 {
-	int ret;
-	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));

-	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
-	return ret;
+	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 f0c11fa301..729f6f3643 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -1510,37 +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;
-	int retval;

 	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");
 		}
 	}

-	retval = run_command_v_opt(fetch_argv.v, RUN_GIT_CMD);
-	strvec_clear(&fetch_argv);
-	return retval;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 static int remove_all_fetch_refspecs(const char *key)
diff --git a/diff.c b/diff.c
index 285d6e2d57..2f1bb18772 100644
--- a/diff.c
+++ b/diff.c
@@ -4301,35 +4301,34 @@ 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 child_process cmd = CHILD_PROCESS_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;

-	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/git.c b/git.c
index ee7758dcb0..6662548986 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/merge.c b/merge.c
index 2382ff66d3..445b4f19aa 100644
--- a/merge.c
+++ b/merge.c
@@ -19,22 +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_v_opt(args.v, RUN_GIT_CMD);
-	strvec_clear(&args);
+	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 6de9c0ee52..03f9e480dd 100644
--- a/scalar.c
+++ b/scalar.c
@@ -69,21 +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;
-	int res;

 	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);

-	res = run_command_v_opt(argv.v, RUN_GIT_CMD);
-
-	strvec_clear(&argv);
-	return res;
+	cmd.git_cmd = 1;
+	return run_command(&cmd);
 }

 struct scalar_config {
diff --git a/sequencer.c b/sequencer.c
index 66eedd2c76..31e24f38f8 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3183,18 +3183,15 @@ static int rollback_is_safe(void)

 static int reset_merge(const struct object_id *oid)
 {
-	int ret;
-	struct strvec argv = STRVEC_INIT;
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	strvec_pushl(&argv, "reset", "--merge", NULL);
+	cmd.git_cmd = 1;
+	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));

-	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-
-	return ret;
+	return run_command(&cmd);
 }

 static int rollback_single_pick(struct repository *r)
@@ -4866,14 +4863,14 @@ 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;
+	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");
+	cmd.git_cmd = 1;
+	strvec_push(&cmd.args, "commit");

 	/*
 	 * continue_single_pick() handles the case of recovering from a
@@ -4886,11 +4883,9 @@ 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);

-	ret = run_command_v_opt(argv.v, RUN_GIT_CMD);
-	strvec_clear(&argv);
-	return ret;
+	return run_command(&cmd);
 }

 static int commit_staged_changes(struct repository *r,
--
2.38.1

[PATCH v2 09/12] replace and remove run_command_v_opt_cd_env()

[René Scharfe]

run_command_v_opt_cd_env() is only used in an example in a comment.  Use
the struct child_process member "env" and run_command() directly instead
and then remove the unused convenience function.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 run-command.c | 7 +------
 run-command.h | 3 +--
 tmp-objdir.h  | 6 ++++--
 3 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/run-command.c b/run-command.c
index c772acd743..954257abb5 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1006,7 +1006,7 @@ int run_command(struct child_process *cmd)

 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)
@@ -1014,11 +1014,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 820fc25b02..05c02485df 100644
--- a/run-command.h
+++ b/run-command.h
@@ -151,7 +151,7 @@ struct child_process {

 /**
  * The functions: start_command, finish_command, run_command,
- * run_command_v_opt, run_command_v_opt_cd_env do the following:
+ * run_command_v_opt do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
  *   is printed.
@@ -249,7 +249,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 76efc7edee..237d96b660 100644
--- a/tmp-objdir.h
+++ b/tmp-objdir.h
@@ -10,9 +10,11 @@
  *
  * Example:
  *
+ *	struct child_process child = 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_push(&child.args, cmd);
+ *	strvec_pushv(&child.env, tmp_objdir_env(t));
+ *	if (!run_command(&child)) && !tmp_objdir_migrate(t))
  *		printf("success!\n");
  *	else
  *		die("failed...tmp_objdir will clean up for us");
--
2.38.1

[PATCH v2 10/12] replace and remove run_command_v_opt_tr2()

[René Scharfe]

The convenience function run_command_v_opt_tr2() is only used by a
single caller.  Use struct child_process and run_command() directly
instead and remove the underused function.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 fsmonitor-ipc.c | 10 +++++++---
 run-command.c   |  5 -----
 run-command.h   |  1 -
 3 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
index c0f42301c8..19d772f0f3 100644
--- a/fsmonitor-ipc.c
+++ b/fsmonitor-ipc.c
@@ -54,10 +54,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/run-command.c b/run-command.c
index 954257abb5..3a8d578f72 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1009,11 +1009,6 @@ 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)
 {
diff --git a/run-command.h b/run-command.h
index 05c02485df..ac67791c81 100644
--- a/run-command.h
+++ b/run-command.h
@@ -244,7 +244,6 @@ int run_auto_maintenance(int quiet);
  * 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".
--
2.38.1

[PATCH v2 11/12] replace and remove run_command_v_opt_cd_env_tr2()

[René Scharfe]

The convenience function run_command_v_opt_cd_env_tr2() has no external
callers left.  Inline it and remove it from the API.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 run-command.c | 10 ----------
 run-command.h | 10 +---------
 2 files changed, 1 insertion(+), 19 deletions(-)

diff --git a/run-command.c b/run-command.c
index 3a8d578f72..f8f810d755 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1005,12 +1005,6 @@ int run_command(struct child_process *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_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);
@@ -1022,10 +1016,6 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 	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);
 }

diff --git a/run-command.h b/run-command.h
index ac67791c81..4391696bf0 100644
--- a/run-command.h
+++ b/run-command.h
@@ -233,23 +233,15 @@ int run_auto_maintenance(int quiet);
 #define RUN_CLOSE_OBJECT_STORE		(1<<7)

 /**
- * Convenience functions that encapsulate a sequence of
+ * Convenience function that encapsulates 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 argument dir corresponds the member .dir. The argument env
- * corresponds to the member .env.
  */
 int run_command_v_opt(const char **argv, int opt);
-/*
- * 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);

 /**
  * Execute the given command, sending "in" to its stdin, and capturing its
--
2.38.1

[PATCH v2 12/12] replace and remove run_command_v_opt()

[René Scharfe]

Replace the remaining calls of run_command_v_opt() with run_command()
calls and explict struct child_process variables.  This is more verbose,
but not by much overall.  The code becomes more flexible, e.g. it's easy
to extend to conditionally add a new argument.

Then remove the now unused function and its own flag names, simplifying
the run-command API.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 bisect.c                 |  9 +++++----
 builtin/am.c             |  9 +++++----
 builtin/bisect--helper.c | 15 ++++++++++-----
 builtin/clone.c          |  8 ++++++--
 builtin/difftool.c       |  7 +++++--
 builtin/fetch.c          |  9 ++++++---
 builtin/gc.c             | 41 +++++++++++++++++++++++++++++++---------
 builtin/merge-index.c    |  4 +++-
 run-command.c            | 15 ---------------
 run-command.h            | 23 +---------------------
 sequencer.c              |  6 ++++--
 shell.c                  | 17 ++++++++++++-----
 t/helper/test-trace2.c   |  4 +++-
 13 files changed, 92 insertions(+), 75 deletions(-)

diff --git a/bisect.c b/bisect.c
index 090aa5c4b4..ec7487e683 100644
--- a/bisect.c
+++ b/bisect.c
@@ -737,11 +737,12 @@ enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
 		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
 	} else {
-		const char *argv_checkout[] = {
-			"checkout", "-q", oid_to_hex(bisect_rev), "--", NULL
-		};
+		struct child_process cmd = CHILD_PROCESS_INIT;

-		if (run_command_v_opt(argv_checkout, RUN_GIT_CMD))
+		cmd.git_cmd = 1;
+		strvec_pushl(&cmd.args, "checkout", "-q",
+			     oid_to_hex(bisect_rev), "--", NULL);
+		if (run_command(&cmd))
 			/*
 			 * Errors in `run_command()` itself, signaled by res < 0,
 			 * and errors in the child process, signaled by res > 0
diff --git a/builtin/am.c b/builtin/am.c
index 5781e7a95e..20aea0d248 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2187,11 +2187,12 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)
 	int len;

 	if (!is_null_oid(&state->orig_commit)) {
-		const char *av[] = {
-			"show", oid_to_hex(&state->orig_commit), "--", NULL
-		};
+		struct child_process cmd = CHILD_PROCESS_INIT;

-		return run_command_v_opt(av, RUN_GIT_CMD);
+		strvec_pushl(&cmd.args, "show", oid_to_hex(&state->orig_commit),
+			     "--", NULL);
+		cmd.git_cmd = 1;
+		return run_command(&cmd);
 	}

 	switch (sub_mode) {
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 5c63ba6994..1d2ce8a0e1 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -764,10 +764,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
 		strbuf_read_file(&start_head, git_path_bisect_start(), 0);
 		strbuf_trim(&start_head);
 		if (!no_checkout) {
-			const char *argv[] = { "checkout", start_head.buf,
-					       "--", NULL };
+			struct child_process cmd = CHILD_PROCESS_INIT;

-			if (run_command_v_opt(argv, RUN_GIT_CMD)) {
+			cmd.git_cmd = 1;
+			strvec_pushl(&cmd.args, "checkout", start_head.buf,
+				     "--", NULL);
+			if (run_command(&cmd)) {
 				res = error(_("checking out '%s' failed."
 						 " Try 'git bisect start "
 						 "<valid-branch>'."),
@@ -1141,9 +1143,12 @@ static int get_first_good(const char *refname UNUSED,

 static int do_bisect_run(const char *command)
 {
-	const char *argv[] = { command, NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
 	printf(_("running %s\n"), command);
-	return run_command_v_opt(argv, RUN_USING_SHELL);
+	cmd.use_shell = 1;
+	strvec_push(&cmd.args, command);
+	return run_command(&cmd);
 }

 static int verify_good(const struct bisect_terms *terms, const char *command)
diff --git a/builtin/clone.c b/builtin/clone.c
index 56e7775dae..0e4348686b 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -865,11 +865,15 @@ 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))
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		cmd.git_cmd = 1;
+		cmd.no_stdin = 1;
+		strvec_pushl(&cmd.args, "repack", "-a", "-d", NULL);
+		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 22bcc3444b..d7f08c8a7f 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -44,8 +44,11 @@ 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);
+	struct child_process cmd = CHILD_PROCESS_INIT;
+
+	cmd.git_cmd = 1;
+	strvec_pushl(&cmd.args, "mergetool", "--tool-help=diff", NULL);
+	return run_command(&cmd);
 }

 static int parse_index_info(char *p, int *mode1, int *mode2,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b06e454cbd..7378cafeec 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1972,14 +1972,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 a185bec813..6504c0b729 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -167,9 +167,11 @@ 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 };
+	struct child_process cmd = CHILD_PROCESS_INIT;

-	return run_command_v_opt(argv, RUN_GIT_CMD);
+	cmd.git_cmd = 1;
+	strvec_pushl(&cmd.args, "pack-refs", "--all", "--prune", NULL);
+	return run_command(&cmd);
 }

 static int too_many_loose_objects(void)
@@ -535,8 +537,14 @@ 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) {
+		struct child_process cmd = CHILD_PROCESS_INIT;
+
+		cmd.git_cmd = 1;
+		strvec_pushv(&cmd.args, reflog.v);
+		if (run_command(&cmd))
+			die(FAILED_RUN, reflog.v[0]);
+	}
 }

 int cmd_gc(int argc, const char **argv, const char *prefix)
@@ -550,6 +558,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 	int daemonized = 0;
 	int keep_largest_pack = -1;
 	timestamp_t dummy;
+	struct child_process rerere_cmd = CHILD_PROCESS_INIT;

 	struct option builtin_gc_options[] = {
 		OPT__QUIET(&quiet, N_("suppress progress reporting")),
@@ -671,11 +680,17 @@ 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;
+
+		repack_cmd.git_cmd = 1;
+		repack_cmd.close_object_store = 1;
+		strvec_pushv(&repack_cmd.args, repack.v);
+		if (run_command(&repack_cmd))
 			die(FAILED_RUN, repack.v[0]);

 		if (prune_expire) {
+			struct child_process prune_cmd = CHILD_PROCESS_INIT;
+
 			/* run `git prune` even if using cruft packs */
 			strvec_push(&prune, prune_expire);
 			if (quiet)
@@ -683,18 +698,26 @@ 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))
+			prune_cmd.git_cmd = 1;
+			strvec_pushv(&prune_cmd.args, prune.v);
+			if (run_command(&prune_cmd))
 				die(FAILED_RUN, prune.v[0]);
 		}
 	}

 	if (prune_worktrees_expire) {
+		struct child_process prune_worktrees_cmd = CHILD_PROCESS_INIT;
+
 		strvec_push(&prune_worktrees, prune_worktrees_expire);
-		if (run_command_v_opt(prune_worktrees.v, RUN_GIT_CMD))
+		prune_worktrees_cmd.git_cmd = 1;
+		strvec_pushv(&prune_worktrees_cmd.args, prune_worktrees.v);
+		if (run_command(&prune_worktrees_cmd))
 			die(FAILED_RUN, prune_worktrees.v[0]);
 	}

-	if (run_command_v_opt(rerere.v, RUN_GIT_CMD))
+	rerere_cmd.git_cmd = 1;
+	strvec_pushv(&rerere_cmd.args, rerere.v);
+	if (run_command(&rerere_cmd))
 		die(FAILED_RUN, rerere.v[0]);

 	report_garbage = report_pack_garbage;
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index c0383fe9df..012f52bd00 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/run-command.c b/run-command.c
index f8f810d755..48b9ba6d6f 100644
--- a/run-command.c
+++ b/run-command.c
@@ -1004,21 +1004,6 @@ int run_command(struct child_process *cmd)
 	return finish_command(cmd);
 }

-int run_command_v_opt(const char **argv, int opt)
-{
-	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;
-	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 4391696bf0..072db56a4d 100644
--- a/run-command.h
+++ b/run-command.h
@@ -150,8 +150,7 @@ struct child_process {
 }

 /**
- * The functions: start_command, finish_command, run_command,
- * run_command_v_opt do the following:
+ * The functions: start_command, finish_command, run_command do the following:
  *
  * - If a system call failed, errno is set and -1 is returned. A diagnostic
  *   is printed.
@@ -223,26 +222,6 @@ int run_command(struct child_process *);
  */
 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)
-
-/**
- * Convenience function that encapsulates 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`.
- */
-int run_command_v_opt(const char **argv, 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 31e24f38f8..643744fb9b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3555,11 +3555,13 @@ static int error_failed_squash(struct repository *r,

 static int do_exec(struct repository *r, const char *command_line)
 {
-	const char *child_argv[] = { command_line, NULL };
+	struct child_process cmd = CHILD_PROCESS_INIT;
 	int dirty, status;

 	fprintf(stderr, _("Executing: %s\n"), command_line);
-	status = run_command_v_opt(child_argv, RUN_USING_SHELL);
+	cmd.use_shell = 1;
+	strvec_push(&cmd.args, command_line);
+	status = run_command(&cmd);

 	/* force re-reading of the cache */
 	if (discard_index(r->index) < 0 || repo_read_index(r) < 0)
diff --git a/shell.c b/shell.c
index 7ff4109db7..af0d7c734f 100644
--- a/shell.c
+++ b/shell.c
@@ -52,21 +52,24 @@ static void cd_to_homedir(void)
 static void run_shell(void)
 {
 	int done = 0;
-	static const char *help_argv[] = { HELP_COMMAND, NULL };
+	struct child_process help_cmd = CHILD_PROCESS_INIT;

 	if (!access(NOLOGIN_COMMAND, F_OK)) {
 		/* Interactive login disabled. */
-		const char *argv[] = { NOLOGIN_COMMAND, NULL };
+		struct child_process nologin_cmd = CHILD_PROCESS_INIT;
 		int status;

-		status = run_command_v_opt(argv, 0);
+		strvec_push(&nologin_cmd.args, NOLOGIN_COMMAND);
+		status = run_command(&nologin_cmd);
 		if (status < 0)
 			exit(127);
 		exit(status);
 	}

 	/* Print help if enabled */
-	run_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);
+	help_cmd.silent_exec_failure = 1;
+	strvec_push(&help_cmd.args, HELP_COMMAND);
+	run_command(&help_cmd);

 	do {
 		const char *prog;
@@ -125,9 +128,13 @@ static void run_shell(void)
 			   !strcmp(prog, "exit") || !strcmp(prog, "bye")) {
 			done = 1;
 		} else if (is_valid_cmd_name(prog)) {
+			struct child_process cmd = CHILD_PROCESS_INIT;
+
 			full_cmd = make_cmd(prog);
 			argv[0] = full_cmd;
-			code = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);
+			cmd.silent_exec_failure = 1;
+			strvec_pushv(&cmd.args, argv);
+			code = run_command(&cmd);
 			if (code == -1 && errno == ENOENT) {
 				fprintf(stderr, "unrecognized command '%s'\n", prog);
 			}
diff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c
index a714130ece..94fd8ccf51 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);
 }

--
2.38.1

Re: [PATCH v2 0/12] run-command: remove run_command_v_*()

[René Scharfe]

Am 30.10.22 um 12:40 schrieb René Scharfe:
> Replace the convenience functions run_command_v_opt() et. al. and use
> struct child_process and run_command() directly instead, for an overall
> code reduction and a simpler and more flexible API that allows creating
> argument lists without magic numbers and reduced risk of memory leaks.
>
> Changes since v1:
> - Do the return value fix earlier; it was only an afterthought before.
>   Keep the colon (no "while at it, ...").
> - Break out the xstrdup(), oid_to_hex_r() and C99 cleanups.
> - Convert tricky string arrays before strvecs because Ævar didn't like
>   the opposite order.
> - Extend the example code in tmp-objdir.h so it still only requires
>   "cmd".

Forgot one:
- Fix grammar error in run-command.h added by the series in a comment
  that goes away at the end.

René

Re: [PATCH v2 0/12] run-command: remove run_command_v_*()

[Taylor Blau]

On Sun, Oct 30, 2022 at 12:58:13PM +0100, René Scharfe wrote:
> > Changes since v1:
> > - Do the return value fix earlier; it was only an afterthought before.
> >   Keep the colon (no "while at it, ...").
> > - Break out the xstrdup(), oid_to_hex_r() and C99 cleanups.
> > - Convert tricky string arrays before strvecs because Ævar didn't like
> >   the opposite order.
> > - Extend the example code in tmp-objdir.h so it still only requires
> >   "cmd".
>
> Forgot one:
> - Fix grammar error in run-command.h added by the series in a comment
>   that goes away at the end.
>
> René

Thanks, replaced. This round is looking good to me, though let's hear
from ÆVar before we start merging this down.

Thanks,
Taylor

Re: [PATCH v2 0/12] run-command: remove run_command_v_*()

[Ævar Arnfjörð Bjarmason]

On Sun, Oct 30 2022, Taylor Blau wrote:

> On Sun, Oct 30, 2022 at 12:58:13PM +0100, René Scharfe wrote:
>> > Changes since v1:
>> > - Do the return value fix earlier; it was only an afterthought before.
>> >   Keep the colon (no "while at it, ...").
>> > - Break out the xstrdup(), oid_to_hex_r() and C99 cleanups.
>> > - Convert tricky string arrays before strvecs because Ævar didn't like
>> >   the opposite order.
>> > - Extend the example code in tmp-objdir.h so it still only requires
>> >   "cmd".
>>
>> Forgot one:
>> - Fix grammar error in run-command.h added by the series in a comment
>>   that goes away at the end.
>>
>> René
>
> Thanks, replaced. This round is looking good to me, though let's hear
> from ÆVar before we start merging this down.

This LGTM. I don't think any of these are worth a re-roll, but just to
clarify in reply to the comment in the v2 CL:

 * I thought the C99-specific stuff, free() etc. might be worth
   splitting out[1], but not to the extent of churning through first
   moving the existing API use around, and then converting it to
   run_command().

   I.e. v1 had e.g. the am.c change as part of the large 3/8. Here
   there's 3/12, and we then re-visit that same caller in 12/12.

   I think that part of the v2 was better in v1. I.e. maybe split them
   out, but to the extent that it needs special explanation we could
   have just done it in one go with some explanation..

 * I suggested just squashing what's now the v2's 08-11/12 into the
   respective commits that represented the last API user, or if they're
   worth splitting out do split-out with that last API user converted
   (and also removing the run-commit.h flags that went unused then, if
   applicable).

Anyway, LGTM, and the stuff that was odd in the end state (e.g. 02/12)
is now fixed. Thanks both!

https://lore.kernel.org/git/Y11%2F2hyApN5NLruq@nand.local/T/#md2b7af02b1af34fa118779d3e1f4444604f95cb9