git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH v3 9/9] run-command API: remove "env" member, always use "env_array"

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Nov 25, 2021, 22:52 UTC
Message-ID
<patch-v3-9.9-4e4db79ea53-20211125T224833Z-avarab@gmail.com>
In-Reply-To
<cover-v3-0.9-00000000000-20211125T224833Z-avarab@gmail.com>

Remove the "env" member from "struct child_process" in favor of always using the "env_array". As with the preceding removal of "argv" in favor of "args" this gets rid of current and future oddities around memory management at the API boundary (see the amended API docs).

For some of the conversions we can replace patterns like:
    child.env = env->v;
With:
    strvec_pushv(&child.env_array, env->v);

But for others we need to guard the strvec_pushv() with a NULL check, since we're not passing in the "v" member of a "struct strvec", e.g. in the case of tmp_objdir_env()'s return value.

Ideally we'd rename the "env_array" member to simply "env" as a follow-up, since it and "args" are now inconsistent in not having an "_array" suffix, and seemingly without any good reason, unless we look at the history of how they came to be.

But as we've currently got 122 in-tree hits for a "git grep env_array" let's leave that for now (and possibly forever). Doing that rename would be too disruptive.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/receive-pack.c | 11 ++++++-----
 builtin/worktree.c     |  6 +++---
 connected.c            |  3 ++-
 editor.c               |  4 +++-
 object-file.c          |  2 +-
 run-command.c          | 20 +++++++-------------
 run-command.h          | 34 +++++++++++++++++-----------------
 trailer.c              |  2 +-
 8 files changed, 40 insertions(+), 42 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 48c99c8ee45..3979752ceca 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1367,7 +1367,7 @@ static const char *push_to_deploy(unsigned char *sha1,
 
 	strvec_pushl(&child.args, "update-index", "-q", "--ignore-submodules",
 		     "--refresh", NULL);
-	child.env = env->v;
+	strvec_pushv(&child.env_array, env->v);
 	child.dir = work_tree;
 	child.no_stdin = 1;
 	child.stdout_to_stderr = 1;
@@ -1379,7 +1379,7 @@ static const char *push_to_deploy(unsigned char *sha1,
 	child_process_init(&child);
 	strvec_pushl(&child.args, "diff-files", "--quiet",
 		     "--ignore-submodules", "--", NULL);
-	child.env = env->v;
+	strvec_pushv(&child.env_array, env->v);
 	child.dir = work_tree;
 	child.no_stdin = 1;
 	child.stdout_to_stderr = 1;
@@ -1393,7 +1393,7 @@ static const char *push_to_deploy(unsigned char *sha1,
 		     /* diff-index with either HEAD or an empty tree */
 		     head_has_history() ? "HEAD" : empty_tree_oid_hex(),
 		     "--", NULL);
-	child.env = env->v;
+	strvec_pushv(&child.env_array, env->v);
 	child.no_stdin = 1;
 	child.no_stdout = 1;
 	child.stdout_to_stderr = 0;
@@ -1404,7 +1404,7 @@ static const char *push_to_deploy(unsigned char *sha1,
 	child_process_init(&child);
 	strvec_pushl(&child.args, "read-tree", "-u", "-m", hash_to_hex(sha1),
 		     NULL);
-	child.env = env->v;
+	strvec_pushv(&child.env_array, env->v);
 	child.dir = work_tree;
 	child.no_stdin = 1;
 	child.no_stdout = 1;
@@ -2202,7 +2202,8 @@ static const char *unpack(int err_fd, struct shallow_info *si)
 			close(err_fd);
 		return "unable to create temporary object directory";
 	}
-	child.env = tmp_objdir_env(tmp_objdir);
+	if (tmp_objdir)
+		strvec_pushv(&child.env_array, tmp_objdir_env(tmp_objdir));
 
 	/*
 	 * Normally we just pass the tmp_objdir environment to the child
diff --git a/builtin/worktree.c b/builtin/worktree.c
index 9edd3e2829b..962d71cf987 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -349,7 +349,7 @@ static int add_worktree(const char *path, const char *refname,
 			strvec_push(&cp.args, "--quiet");
 	}
 
-	cp.env = child_env.v;
+	strvec_pushv(&cp.env_array, child_env.v);
 	ret = run_command(&cp);
 	if (ret)
 		goto done;
@@ -360,7 +360,7 @@ static int add_worktree(const char *path, const char *refname,
 		strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
 		if (opts->quiet)
 			strvec_push(&cp.args, "--quiet");
-		cp.env = child_env.v;
+		strvec_pushv(&cp.env_array, child_env.v);
 		ret = run_command(&cp);
 		if (ret)
 			goto done;
@@ -389,7 +389,7 @@ static int add_worktree(const char *path, const char *refname,
 			cp.no_stdin = 1;
 			cp.stdout_to_stderr = 1;
 			cp.dir = path;
-			cp.env = env;
+			strvec_pushv(&cp.env_array, env);
 			cp.trace2_hook_name = "post-checkout";
 			strvec_pushl(&cp.args, absolute_path(hook),
 				     oid_to_hex(null_oid()),
diff --git a/connected.c b/connected.c
index 35bd4a26382..ed3025e7a2a 100644
--- a/connected.c
+++ b/connected.c
@@ -109,7 +109,8 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 			     _("Checking connectivity"));
 
 	rev_list.git_cmd = 1;
-	rev_list.env = opt->env;
+	if (opt->env)
+		strvec_pushv(&rev_list.env_array, opt->env);
 	rev_list.in = -1;
 	rev_list.no_stdout = 1;
 	if (opt->err_fd)
diff --git a/editor.c b/editor.c
index d92a8d9ab5b..8b9648281d7 100644
--- a/editor.c
+++ b/editor.c
@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "config.h"
 #include "strbuf.h"
+#include "strvec.h"
 #include "run-command.h"
 #include "sigchain.h"
 
@@ -78,7 +79,8 @@ static int launch_specified_editor(const char *editor, const char *path,
 		strbuf_realpath(&realpath, path, 1);
 
 		strvec_pushl(&p.args, editor, realpath.buf, NULL);
-		p.env = env;
+		if (env)
+			strvec_pushv(&p.env_array, (const char **)env);
 		p.use_shell = 1;
 		p.trace2_child_class = "editor";
 		if (start_command(&p) < 0) {
diff --git a/object-file.c b/object-file.c
index c3d866a287e..2fc1bed417c 100644
--- a/object-file.c
+++ b/object-file.c
@@ -797,7 +797,7 @@ static void fill_alternate_refs_command(struct child_process *cmd,
 		}
 	}
 
-	cmd->env = local_repo_env;
+	strvec_pushv(&cmd->env_array, (const char **)local_repo_env);
 	cmd->out = -1;
 }
 
diff --git a/run-command.c b/run-command.c
index 99dc93e7300..ebade086700 100644
--- a/run-command.c
+++ b/run-command.c
@@ -655,12 +655,7 @@ static void trace_run_command(const struct child_process *cp)
 		sq_quote_buf_pretty(&buf, cp->dir);
 		strbuf_addch(&buf, ';');
 	}
-	/*
-	 * The caller is responsible for initializing cp->env from
-	 * cp->env_array if needed. We only check one place.
-	 */
-	if (cp->env)
-		trace_add_env(&buf, cp->env);
+	trace_add_env(&buf, cp->env_array.v);
 	if (cp->git_cmd)
 		strbuf_addstr(&buf, " git");
 	sq_quote_argv_pretty(&buf, cp->args.v);
@@ -676,9 +671,6 @@ int start_command(struct child_process *cmd)
 	int failed_errno;
 	char *str;
 
-	if (!cmd->env)
-		cmd->env = cmd->env_array.v;
-
 	/*
 	 * In case of errors we must keep the promise to close FDs
 	 * that have been passed in via ->in and ->out.
@@ -768,7 +760,7 @@ int start_command(struct child_process *cmd)
 		set_cloexec(null_fd);
 	}
 
-	childenv = prep_childenv(cmd->env);
+	childenv = prep_childenv(cmd->env_array.v);
 	atfork_prepare(&as);
 
 	/*
@@ -931,7 +923,7 @@ int start_command(struct child_process *cmd)
 	else if (cmd->use_shell)
 		cmd->args.v = prepare_shell_cmd(&nargv, sargv);
 
-	cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v, (char**) cmd->env,
+	cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v, (char**) cmd->env_array.v,
 			cmd->dir, fhin, fhout, fherr);
 	failed_errno = errno;
 	if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))
@@ -1047,7 +1039,8 @@ int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,
 	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;
-	cmd.env = env;
+	if (env)
+		strvec_pushv(&cmd.env_array, (const char **)env);
 	cmd.trace2_child_class = tr2_class;
 	return run_command(&cmd);
 }
@@ -1333,7 +1326,8 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)
 	strvec_push(&hook.args, p);
 	while ((p = va_arg(args, const char *)))
 		strvec_push(&hook.args, p);
-	hook.env = env;
+	if (env)
+		strvec_pushv(&hook.env_array, (const char **)env);
 	hook.no_stdin = 1;
 	hook.stdout_to_stderr = 1;
 	hook.trace2_hook_name = name;
diff --git a/run-command.h b/run-command.h
index c0d1210cc63..2be5f5d6422 100644
--- a/run-command.h
+++ b/run-command.h
@@ -56,6 +56,23 @@ struct child_process {
 	 * `finish_command` (or during `start_command` when it is unsuccessful).
 	 */
 	struct strvec args;
+
+	/**
+	 * Like .args the .env_array is a `struct strvec'.
+	 *
+	 * To modify the environment of the sub-process, specify an array of
+	 * environment settings. Each string in the array manipulates the
+	 * environment.
+	 *
+	 * - If the string is of the form "VAR=value", i.e. it contains '='
+	 *   the variable is added to the child process's environment.
+	 *
+	 * - If the string does not contain '=', it names an environment
+	 *   variable that will be removed from the child process's environment.
+	 *
+	 * The memory in .env_array will be cleaned up automatically during
+	 * `finish_command` (or during `start_command` when it is unsuccessful).
+	 */
 	struct strvec env_array;
 	pid_t pid;
 
@@ -92,23 +109,6 @@ struct child_process {
 	 */
 	const char *dir;
 
-	/**
-	 * To modify the environment of the sub-process, specify an array of
-	 * string pointers (NULL terminated) in .env:
-	 *
-	 * - If the string is of the form "VAR=value", i.e. it contains '='
-	 *   the variable is added to the child process's environment.
-	 *
-	 * - If the string does not contain '=', it names an environment
-	 *   variable that will be removed from the child process's environment.
-	 *
-	 * If the .env member is NULL, `start_command` will point it at the
-	 * .env_array `strvec` (so you may use one or the other, but not both).
-	 * The memory in .env_array will be cleaned up automatically during
-	 * `finish_command` (or during `start_command` when it is unsuccessful).
-	 */
-	const char *const *env;
-
 	unsigned no_stdin:1;
 	unsigned no_stdout:1;
 	unsigned no_stderr:1;
diff --git a/trailer.c b/trailer.c
index 7c7cb61a945..1b12f77d945 100644
--- a/trailer.c
+++ b/trailer.c
@@ -236,7 +236,7 @@ static char *apply_command(struct conf_info *conf, const char *arg)
 			strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);
 		strvec_push(&cp.args, cmd.buf);
 	}
-	cp.env = local_repo_env;
+	strvec_pushv(&cp.env_array, (const char **)local_repo_env);
 	cp.no_stdin = 1;
 	cp.use_shell = 1;
 

-- 
2.34.1.838.g779e9098efb
Previous: Ævar Arnfjörð BjarmasonNext: Enzo Matsumiya
Message 57 of 71 in “pager: fix crash when pager program doesn't exist”
  1. pager: fix crash when pager program doesn't existEnzo Matsumiya, Nov 20, 2021
  2. Jeff KingNov 21, 2021
  3. Junio C HamanoNov 22, 2021
  4. Jeff KingNov 22, 2021
  5. Enzo MatsumiyaNov 22, 2021
  6. Junio C HamanoNov 22, 2021
  7. Enzo MatsumiyaNov 23, 2021
  8. Ævar Arnfjörð BjarmasonNov 24, 2021
  9. Jeff KingNov 24, 2021
  10. 0/5 run-command API: get rid of "argv"Ævar Arnfjörð Bjarmason, Nov 22, 2021
  11. 1/5 archive-tar: use our own cmd.buf in error messageÆvar Arnfjörð Bjarmason, Nov 22, 2021
  12. Junio C HamanoNov 22, 2021
  13. 2/5 upload-archive: use regular "struct child_process" patternÆvar Arnfjörð Bjarmason, Nov 22, 2021
  14. Jeff KingNov 22, 2021
  15. Ævar Arnfjörð BjarmasonNov 22, 2021
  16. Jeff KingNov 22, 2021
  17. Ævar Arnfjörð BjarmasonNov 22, 2021
  18. 3/5 run-command API users: use strvec_pushv(), not argv assignmentÆvar Arnfjörð Bjarmason, Nov 22, 2021
  19. Junio C HamanoNov 22, 2021
  20. Ævar Arnfjörð BjarmasonNov 22, 2021
  21. 5/5 run-command API: remove "argv" member, always use "args"Ævar Arnfjörð Bjarmason, Nov 22, 2021
  22. Jeff KingNov 22, 2021
  23. Ævar Arnfjörð BjarmasonNov 22, 2021
  24. Jeff KingNov 22, 2021
  25. 4/5 run-command API users: use strvec_pushl(), not argv constructionÆvar Arnfjörð Bjarmason, Nov 22, 2021
  26. Jeff KingNov 22, 2021
  27. Junio C HamanoNov 22, 2021
  28. Ævar Arnfjörð BjarmasonNov 22, 2021
  29. Jeff KingNov 22, 2021
  30. Ævar Arnfjörð BjarmasonNov 22, 2021
  31. 0/9 run-command API: get rid of "argv" and "env"Ævar Arnfjörð Bjarmason, Nov 23, 2021
  32. 1/9 worktree: remove redundant NULL-ing of "cp.argvÆvar Arnfjörð Bjarmason, Nov 23, 2021
  33. Eric SunshineNov 23, 2021
  34. Junio C HamanoNov 24, 2021
  35. Eric SunshineNov 24, 2021
  36. Eric SunshineNov 24, 2021
  37. Eric SunshineNov 24, 2021
  38. 2/9 upload-archive: use regular "struct child_process" patternÆvar Arnfjörð Bjarmason, Nov 23, 2021
  39. 3/9 run-command API users: use strvec_pushv(), not argv assignmentÆvar Arnfjörð Bjarmason, Nov 23, 2021
  40. 4/9 run-command tests: use strvec_pushv(), not argv assignmentÆvar Arnfjörð Bjarmason, Nov 23, 2021
  41. Eric SunshineNov 24, 2021
  42. 5/9 run-command API users: use strvec_pushl(), not argv constructionÆvar Arnfjörð Bjarmason, Nov 23, 2021
  43. 6/9 run-command API users: use strvec_push(), not argv constructionÆvar Arnfjörð Bjarmason, Nov 23, 2021
  44. 7/9 run-command API: remove "argv" member, always use "args"Ævar Arnfjörð Bjarmason, Nov 23, 2021
  45. 8/9 difftool: use "env_array" to simplify memory managementÆvar Arnfjörð Bjarmason, Nov 23, 2021
  46. 9/9 run-command API: remove "env" member, always use "env_array"Ævar Arnfjörð Bjarmason, Nov 23, 2021
  47. 0/9 run-command API: get rid of "argv" and "env"Ævar Arnfjörð Bjarmason, Nov 25, 2021
  48. 1/9 worktree: stop being overly intimate with run_command() internalsÆvar Arnfjörð Bjarmason, Nov 25, 2021
  49. Eric SunshineNov 26, 2021
  50. 3/9 run-command API users: use strvec_pushv(), not argv assignmentÆvar Arnfjörð Bjarmason, Nov 25, 2021
  51. 2/9 upload-archive: use regular "struct child_process" patternÆvar Arnfjörð Bjarmason, Nov 25, 2021
  52. 4/9 run-command tests: use strvec_pushv(), not argv assignmentÆvar Arnfjörð Bjarmason, Nov 25, 2021
  53. 6/9 run-command API users: use strvec_push(), not argv constructionÆvar Arnfjörð Bjarmason, Nov 25, 2021
  54. 5/9 run-command API users: use strvec_pushl(), not argv constructionÆvar Arnfjörð Bjarmason, Nov 25, 2021
  55. 7/9 run-command API: remove "argv" member, always use "args"Ævar Arnfjörð Bjarmason, Nov 25, 2021
  56. 8/9 difftool: use "env_array" to simplify memory managementÆvar Arnfjörð Bjarmason, Nov 25, 2021
  57. 9/9 run-command API: remove "env" member, always use "env_array"Ævar Arnfjörð Bjarmason, Nov 25, 2021
  58. Enzo MatsumiyaNov 22, 2021
  59. Ævar Arnfjörð BjarmasonNov 22, 2021
  60. Enzo MatsumiyaNov 22, 2021
  61. Ævar Arnfjörð BjarmasonNov 22, 2021
  62. Jeff KingNov 22, 2021
  63. Junio C HamanoNov 22, 2021
  64. Jeff KingNov 22, 2021
  65. Junio C HamanoNov 22, 2021
  66. Junio C HamanoNov 22, 2021
  67. Jeff KingNov 22, 2021
  68. Junio C HamanoNov 22, 2021
  69. Junio C HamanoNov 22, 2021
  70. Ævar Arnfjörð BjarmasonNov 22, 2021
  71. Jeff KingNov 22, 2021

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.