@matheusbsbs / matheus-bagliano / commits / c7c4bdeccf

run-command API: remove "env" member, always use "env_array"

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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Nov 25, 2021 at 23:52 UTC c7c4bdeccf3e737e6e674cd9f0828922e629ab06

8 files changed +40 -42

builtin/receive-pack.c

+6 -5

@@ -1367,7 +1367,7 @@ static const char *push_to_deploy(unsigned char *sha1,

1367

1368 strvec_pushl(&child.args, "update-index", "-q", "--ignore-submodules",

1369 "--refresh", NULL);

1370 - child.env = env->v;

1370 + strvec_pushv(&child.env_array, env->v);

1371 child.dir = work_tree;

1372 child.no_stdin = 1;

1373 child.stdout_to_stderr = 1;

@@ -1379,7 +1379,7 @@ static const char *push_to_deploy(unsigned char *sha1,

1379 child_process_init(&child);

1380 strvec_pushl(&child.args, "diff-files", "--quiet",

1381 "--ignore-submodules", "--", NULL);

1382 - child.env = env->v;

1382 + strvec_pushv(&child.env_array, env->v);

1383 child.dir = work_tree;

1384 child.no_stdin = 1;

1385 child.stdout_to_stderr = 1;

@@ -1393,7 +1393,7 @@ static const char *push_to_deploy(unsigned char *sha1,

1393 /* diff-index with either HEAD or an empty tree */

1394 head_has_history() ? "HEAD" : empty_tree_oid_hex(),

1395 "--", NULL);

1396 - child.env = env->v;

1396 + strvec_pushv(&child.env_array, env->v);

1397 child.no_stdin = 1;

1398 child.no_stdout = 1;

1399 child.stdout_to_stderr = 0;

@@ -1404,7 +1404,7 @@ static const char *push_to_deploy(unsigned char *sha1,

1404 child_process_init(&child);

1405 strvec_pushl(&child.args, "read-tree", "-u", "-m", hash_to_hex(sha1),

1406 NULL);

1407 - child.env = env->v;

1407 + strvec_pushv(&child.env_array, env->v);

1408 child.dir = work_tree;

1409 child.no_stdin = 1;

1410 child.no_stdout = 1;

@@ -2202,7 +2202,8 @@ static const char *unpack(int err_fd, struct shallow_info *si)

2202 close(err_fd);

2203 return "unable to create temporary object directory";

2204 }

2205 - child.env = tmp_objdir_env(tmp_objdir);

2205 + if (tmp_objdir)

2206 + strvec_pushv(&child.env_array, tmp_objdir_env(tmp_objdir));

2207

2208 /*

2209 * Normally we just pass the tmp_objdir environment to the child

builtin/worktree.c

+3 -3

@@ -349,7 +349,7 @@ static int add_worktree(const char *path, const char *refname,

349 strvec_push(&cp.args, "--quiet");

350 }

351

352 - cp.env = child_env.v;

352 + strvec_pushv(&cp.env_array, child_env.v);

353 ret = run_command(&cp);

354 if (ret)

355 goto done;

@@ -360,7 +360,7 @@ static int add_worktree(const char *path, const char *refname,

360 strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);

361 if (opts->quiet)

362 strvec_push(&cp.args, "--quiet");

363 - cp.env = child_env.v;

363 + strvec_pushv(&cp.env_array, child_env.v);

364 ret = run_command(&cp);

365 if (ret)

366 goto done;

@@ -389,7 +389,7 @@ done:

389 cp.no_stdin = 1;

390 cp.stdout_to_stderr = 1;

391 cp.dir = path;

392 - cp.env = env;

392 + strvec_pushv(&cp.env_array, env);

393 cp.trace2_hook_name = "post-checkout";

394 strvec_pushl(&cp.args, absolute_path(hook),

395 oid_to_hex(null_oid()),

connected.c

+2 -1

@@ -109,7 +109,8 @@ no_promisor_pack_found:

109 _("Checking connectivity"));

110

111 rev_list.git_cmd = 1;

112 - rev_list.env = opt->env;

112 + if (opt->env)

113 + strvec_pushv(&rev_list.env_array, opt->env);

114 rev_list.in = -1;

115 rev_list.no_stdout = 1;

116 if (opt->err_fd)

editor.c

+3 -1

@@ -1,6 +1,7 @@

1 #include "cache.h"

2 #include "config.h"

3 #include "strbuf.h"

4 +#include "strvec.h"

5 #include "run-command.h"

6 #include "sigchain.h"

@@ -78,7 +79,8 @@ static int launch_specified_editor(const char *editor, const char *path,

79 strbuf_realpath(&realpath, path, 1);

81 strvec_pushl(&p.args, editor, realpath.buf, NULL);

81 - p.env = env;

82 + if (env)

83 + strvec_pushv(&p.env_array, (const char **)env);

84 p.use_shell = 1;

85 p.trace2_child_class = "editor";

86 if (start_command(&p) < 0) {

object-file.c

+1 -1

@@ -797,7 +797,7 @@ static void fill_alternate_refs_command(struct child_process *cmd,

797 }

798 }

799

800 - cmd->env = local_repo_env;

800 + strvec_pushv(&cmd->env_array, (const char **)local_repo_env);

801 cmd->out = -1;

802 }

803

run-command.c

+7 -13

@@ -655,12 +655,7 @@ static void trace_run_command(const struct child_process *cp)

655 sq_quote_buf_pretty(&buf, cp->dir);

656 strbuf_addch(&buf, ';');

657 }

658 - /*

659 - * The caller is responsible for initializing cp->env from

660 - * cp->env_array if needed. We only check one place.

661 - */

662 - if (cp->env)

663 - trace_add_env(&buf, cp->env);

658 + trace_add_env(&buf, cp->env_array.v);

659 if (cp->git_cmd)

660 strbuf_addstr(&buf, " git");

661 sq_quote_argv_pretty(&buf, cp->args.v);

@@ -676,9 +671,6 @@ int start_command(struct child_process *cmd)

671 int failed_errno;

672 char *str;

673

679 - if (!cmd->env)

680 - cmd->env = cmd->env_array.v;

681 -

674 /*

675 * In case of errors we must keep the promise to close FDs

676 * that have been passed in via ->in and ->out.

@@ -768,7 +760,7 @@ fail_pipe:

760 set_cloexec(null_fd);

761 }

762

771 - childenv = prep_childenv(cmd->env);

763 + childenv = prep_childenv(cmd->env_array.v);

764 atfork_prepare(&as);

765

766 /*

@@ -931,7 +923,7 @@ end_of_spawn:

923 else if (cmd->use_shell)

924 cmd->args.v = prepare_shell_cmd(&nargv, sargv);

925

934 - cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v, (char**) cmd->env,

926 + cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v, (char**) cmd->env_array.v,

927 cmd->dir, fhin, fhout, fherr);

928 failed_errno = errno;

929 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,

1039 cmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;

1040 cmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;

1041 cmd.dir = dir;

1050 - cmd.env = env;

1042 + if (env)

1043 + strvec_pushv(&cmd.env_array, (const char **)env);

1044 cmd.trace2_child_class = tr2_class;

1045 return run_command(&cmd);

1046 }

@@ -1333,7 +1326,8 @@ int run_hook_ve(const char *const *env, const char *name, va_list args)

1326 strvec_push(&hook.args, p);

1327 while ((p = va_arg(args, const char *)))

1328 strvec_push(&hook.args, p);

1336 - hook.env = env;

1329 + if (env)

1330 + strvec_pushv(&hook.env_array, (const char **)env);

1331 hook.no_stdin = 1;

1332 hook.stdout_to_stderr = 1;

1333 hook.trace2_hook_name = name;

run-command.h

+17 -17

@@ -56,6 +56,23 @@ struct child_process {

56 * `finish_command` (or during `start_command` when it is unsuccessful).

57 */

58 struct strvec args;

59 +

60 + /**

61 + * Like .args the .env_array is a `struct strvec'.

62 + *

63 + * To modify the environment of the sub-process, specify an array of

64 + * environment settings. Each string in the array manipulates the

65 + * environment.

66 + *

67 + * - If the string is of the form "VAR=value", i.e. it contains '='

68 + * the variable is added to the child process's environment.

69 + *

70 + * - If the string does not contain '=', it names an environment

71 + * variable that will be removed from the child process's environment.

72 + *

73 + * The memory in .env_array will be cleaned up automatically during

74 + * `finish_command` (or during `start_command` when it is unsuccessful).

75 + */

76 struct strvec env_array;

77 pid_t pid;

@@ -92,23 +109,6 @@ struct child_process {

109 */

110 const char *dir;

111

95 - /**

96 - * To modify the environment of the sub-process, specify an array of

97 - * string pointers (NULL terminated) in .env:

98 - *

99 - * - If the string is of the form "VAR=value", i.e. it contains '='

100 - * the variable is added to the child process's environment.

101 - *

102 - * - If the string does not contain '=', it names an environment

103 - * variable that will be removed from the child process's environment.

104 - *

105 - * If the .env member is NULL, `start_command` will point it at the

106 - * .env_array `strvec` (so you may use one or the other, but not both).

107 - * The memory in .env_array will be cleaned up automatically during

108 - * `finish_command` (or during `start_command` when it is unsuccessful).

109 - */

110 - const char *const *env;

111 -

112 unsigned no_stdin:1;

113 unsigned no_stdout:1;

114 unsigned no_stderr:1;

trailer.c

+1 -1

@@ -236,7 +236,7 @@ static char *apply_command(struct conf_info *conf, const char *arg)

236 strbuf_replace(&cmd, TRAILER_ARG_STRING, arg);

237 strvec_push(&cp.args, cmd.buf);

238 }

239 - cp.env = local_repo_env;

239 + strvec_pushv(&cp.env_array, (const char **)local_repo_env);

240 cp.no_stdin = 1;

241 cp.use_shell = 1;

242