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>

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 48c99c8ee4561e85bf03818db707fbeeef3ee3d4..3979752ceca10549e5a3beda2c23b552a5d15b3a 100644 (file)
--- 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 9edd3e2829bb1f9b1148c4c834c4720dd8eb2144..962d71cf987e5b6d228981ca42cbee742fa21814 100644 (file)
--- 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 @@ done:
                        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 35bd4a26382a445302aa6239c8661b223b478781..ed3025e7a2a7cffdc6607aa9408d097f6d537768 100644 (file)
--- a/connected.c
+++ b/connected.c
@@ -109,7 +109,8 @@ no_promisor_pack_found:
                             _("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 d92a8d9ab5bb4f48d2eff713464f1da744ce50b1..8b9648281d7b53ad34a8dffca32ae2940b3a81c5 100644 (file)
--- 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 c3d866a287e03ad0ae5de97ab6a352887c92e233..2fc1bed417cdc8547961009461978e8609d3d343 100644 (file)
--- 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 99dc93e73000b121f2d36eb19a786856672a9dbb..ebade086700d0ccc411d642c21486ef7209ab4c5 100644 (file)
--- 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 @@ fail_pipe:
                set_cloexec(null_fd);
        }
 
-       childenv = prep_childenv(cmd->env);
+       childenv = prep_childenv(cmd->env_array.v);
        atfork_prepare(&as);
 
        /*
@@ -931,7 +923,7 @@ end_of_spawn:
        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 c0d1210cc63c24738d0e54f3f104ed421e53134a..2be5f5d6422e54b84df300abd41974df3cd169da 100644 (file)
--- 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 7c7cb61a945c7ba2634685d6d49e3ad7ac23204a..1b12f77d945f423aae5b8141d9494c06424f7180 100644 (file)
--- 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;