From 33c997a411b14b0ac54452d79c6951cbf109475c Mon Sep 17 00:00:00 2001 From: Eric Sunshine Date: Thu, 25 Nov 2021 23:52:16 +0100 Subject: [PATCH 1/9] worktree: stop being overly intimate with run_command() internals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit add_worktree() reuses a `child_process` for three run_command() invocations, but to do so, it has overly-intimate knowledge of run-command.c internals. In particular, it knows that it must reset child_process::argv to NULL for each subsequent invocation[*] in order for start_command() to latch the newly-populated child_process::args for each invocation, even though this behavior is not a part of the documented API. Beyond having overly-intimate knowledge of run-command.c internals, the reuse of one `child_process` for three run_command() invocations smells like an unnecessary micro-optimization. Therefore, stop sharing one `child_process` and instead use a new one for each run_command() call. [*] If child_process::argv is not reset to NULL, then subsequent run_command() invocations will instead incorrectly access a dangling pointer to freed memory which had been allocated by child_process::args on the previous run. This is due to the following code in start_command(): if (!cmd->argv) cmd->argv = cmd->args.v; Signed-off-by: Eric Sunshine Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/worktree.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin/worktree.c b/builtin/worktree.c index d22ece93e1..9edd3e2829 100644 --- a/builtin/worktree.c +++ b/builtin/worktree.c @@ -355,8 +355,8 @@ static int add_worktree(const char *path, const char *refname, goto done; if (opts->checkout) { - cp.argv = NULL; - strvec_clear(&cp.args); + struct child_process cp = CHILD_PROCESS_INIT; + cp.git_cmd = 1; strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL); if (opts->quiet) strvec_push(&cp.args, "--quiet"); @@ -385,12 +385,11 @@ done: const char *hook = find_hook("post-checkout"); if (hook) { const char *env[] = { "GIT_DIR", "GIT_WORK_TREE", NULL }; - cp.git_cmd = 0; + struct child_process cp = CHILD_PROCESS_INIT; cp.no_stdin = 1; cp.stdout_to_stderr = 1; cp.dir = path; cp.env = env; - cp.argv = NULL; cp.trace2_hook_name = "post-checkout"; strvec_pushl(&cp.args, absolute_path(hook), oid_to_hex(null_oid()), From c8a4cd55d915a51e666863e576bb1cc2adbecabe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 25 Nov 2021 23:52:17 +0100 Subject: [PATCH 2/9] upload-archive: use regular "struct child_process" pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This pattern added [1] in seems to have been intentional, but since [2] and [3] we've wanted do initialization of what's now the "struct strvec" "args" and "env_array" members. Let's not trample on that initialization here. 1. 1bc01efed17 (upload-archive: use start_command instead of fork, 2011-11-19) 2. c460c0ecdca (run-command: store an optional argv_array, 2014-05-15) 3. 9a583dc39e (run-command: add env_array, an optional argv_array for env, 2014-10-19) Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/upload-archive.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c index 24654b4c9b..98d028dae6 100644 --- a/builtin/upload-archive.c +++ b/builtin/upload-archive.c @@ -77,7 +77,7 @@ static ssize_t process_input(int child_fd, int band) int cmd_upload_archive(int argc, const char **argv, const char *prefix) { - struct child_process writer = { argv }; + struct child_process writer = CHILD_PROCESS_INIT; if (argc == 2 && !strcmp(argv[1], "-h")) usage(upload_archive_usage); @@ -89,9 +89,10 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix) * multiplexed out to our fd#1. If the child dies, we tell the other * end over channel #3. */ - argv[0] = "upload-archive--writer"; writer.out = writer.err = -1; writer.git_cmd = 1; + strvec_push(&writer.args, "upload-archive--writer"); + strvec_pushv(&writer.args, argv + 1); if (start_command(&writer)) { int err = errno; packet_write_fmt(1, "NACK unable to spawn subprocess\n"); From 6def0ff8785eb12ccc24ceebea04355e13ae24b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 25 Nov 2021 23:52:18 +0100 Subject: [PATCH 3/9] run-command API users: use strvec_pushv(), not argv assignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migrate those run-command API users that assign directly to the "argv" member to use a strvec_pushv() of "args" instead. In these cases it did not make sense to further refactor these callers, e.g. daemon.c could be made to construct the arguments closer to handle(), but that would require moving the construction from its cmd_main() and pass "argv" through two intermediate functions. It would be possible for a change like this to introduce a regression if we were doing: cp.argv = argv; argv[1] = "foo"; And changed the code, as is being done here, to: strvec_pushv(&cp.args, argv); argv[1] = "foo"; But as viewing this change with the "-W" flag reveals none of these functions modify variable that's being pushed afterwards in a way that would introduce such a logic error. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- add-patch.c | 4 ++-- daemon.c | 2 +- http-backend.c | 2 +- http.c | 5 +++-- remote-curl.c | 2 +- run-command.c | 2 +- t/helper/test-subprocess.c | 2 +- 7 files changed, 10 insertions(+), 9 deletions(-) diff --git a/add-patch.c b/add-patch.c index 8c41cdfe39..573eef0cc4 100644 --- a/add-patch.c +++ b/add-patch.c @@ -413,7 +413,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) strvec_push(&args, ps->items[i].original); setup_child_process(s, &cp, NULL); - cp.argv = args.v; + strvec_pushv(&cp.args, args.v); res = capture_command(&cp, plain, 0); if (res) { strvec_clear(&args); @@ -431,7 +431,7 @@ static int parse_diff(struct add_p_state *s, const struct pathspec *ps) setup_child_process(s, &colored_cp, NULL); xsnprintf((char *)args.v[color_arg_index], 8, "--color"); - colored_cp.argv = args.v; + strvec_pushv(&colored_cp.args, args.v); colored = &s->colored; res = capture_command(&colored_cp, colored, 0); strvec_clear(&args); diff --git a/daemon.c b/daemon.c index b1fcbe0d6f..8df21f2130 100644 --- a/daemon.c +++ b/daemon.c @@ -922,7 +922,7 @@ static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen) #endif } - cld.argv = cld_argv.v; + strvec_pushv(&cld.args, cld_argv.v); cld.in = incoming; cld.out = dup(incoming); diff --git a/http-backend.c b/http-backend.c index 3d6e2ff17f..4dd4d939f8 100644 --- a/http-backend.c +++ b/http-backend.c @@ -480,7 +480,7 @@ static void run_service(const char **argv, int buffer_input) strvec_pushf(&cld.env_array, "GIT_COMMITTER_EMAIL=%s@http.%s", user, host); - cld.argv = argv; + strvec_pushv(&cld.args, argv); if (buffer_input || gzipped_request || req_len >= 0) cld.in = -1; cld.git_cmd = 1; diff --git a/http.c b/http.c index f92859f43f..229da4d148 100644 --- a/http.c +++ b/http.c @@ -2126,8 +2126,9 @@ int finish_http_pack_request(struct http_pack_request *preq) ip.git_cmd = 1; ip.in = tmpfile_fd; - ip.argv = preq->index_pack_args ? preq->index_pack_args - : default_index_pack_args; + strvec_pushv(&ip.args, preq->index_pack_args ? + preq->index_pack_args : + default_index_pack_args); if (preq->preserve_index_pack_stdout) ip.out = 0; diff --git a/remote-curl.c b/remote-curl.c index d69156312b..0dabef2dd7 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -1061,7 +1061,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads, client.in = -1; client.out = -1; client.git_cmd = 1; - client.argv = client_argv; + strvec_pushv(&client.args, client_argv); if (start_command(&client)) exit(1); write_or_die(client.in, preamble->buf, preamble->len); diff --git a/run-command.c b/run-command.c index f40df01c77..620a06ca2f 100644 --- a/run-command.c +++ b/run-command.c @@ -1039,7 +1039,7 @@ 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; - cmd.argv = argv; + 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; diff --git a/t/helper/test-subprocess.c b/t/helper/test-subprocess.c index 92b69de635..ff22f2fa2c 100644 --- a/t/helper/test-subprocess.c +++ b/t/helper/test-subprocess.c @@ -15,6 +15,6 @@ int cmd__subprocess(int argc, const char **argv) argv++; } cp.git_cmd = 1; - cp.argv = (const char **)argv + 1; + strvec_pushv(&cp.args, (const char **)argv + 1); return run_command(&cp); } From 87ee87dd6bb633cd5171e244fb69cd4b66d294aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 25 Nov 2021 23:52:19 +0100 Subject: [PATCH 4/9] run-command tests: use strvec_pushv(), not argv assignment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As in the preceding commit change this API user to use strvec_pushv() instead of assigning to the "argv" member directly. This leaves us without test coverage of how the "argv" assignment in this API works, but we'll be removing it in a subsequent commit. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- t/helper/test-run-command.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c index 3c4fb86223..913775a14b 100644 --- a/t/helper/test-run-command.c +++ b/t/helper/test-run-command.c @@ -31,7 +31,7 @@ static int parallel_next(struct child_process *cp, if (number_callbacks >= 4) return 0; - strvec_pushv(&cp->args, d->argv); + strvec_pushv(&cp->args, d->args.v); strbuf_addstr(err, "preloaded output of a child\n"); number_callbacks++; return 1; @@ -274,7 +274,7 @@ static int quote_stress_test(int argc, const char **argv) if (i < skip) continue; - cp.argv = args.v; + strvec_pushv(&cp.args, args.v); strbuf_reset(&out); if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0) < 0) return error("Failed to spawn child process"); @@ -396,7 +396,7 @@ int cmd__run_command(int argc, const char **argv) } if (argc < 3) return 1; - proc.argv = (const char **)argv + 2; + strvec_pushv(&proc.args, (const char **)argv + 2); if (!strcmp(argv[1], "start-command-ENOENT")) { if (start_command(&proc) < 0 && errno == ENOENT) @@ -408,7 +408,8 @@ int cmd__run_command(int argc, const char **argv) exit(run_command(&proc)); jobs = atoi(argv[2]); - proc.argv = (const char **)argv + 3; + strvec_clear(&proc.args); + strvec_pushv(&proc.args, (const char **)argv + 3); if (!strcmp(argv[1], "run-command-parallel")) exit(run_processes_parallel(jobs, parallel_next, From 2b7098936c9e91d527aa53b8d4af0b25d7e912b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 25 Nov 2021 23:52:20 +0100 Subject: [PATCH 5/9] run-command API users: use strvec_pushl(), not argv construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change a pattern of hardcoding an "argv" array size, populating it and assigning to the "argv" member of "struct child_process" to instead use "strvec_pushl()" to add data to the "args" member. This implements the same behavior as before in fewer lines of code, and moves us further towards being able to remove the "argv" member in a subsequent commit. Since we've entirely removed the "argv" variable(s) we can be sure that no potential logic errors of the type discussed in a preceding commit are being introduced here, i.e. ones where the local "argv" was being modified after the assignment to "struct child_process"'s "argv". Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/add.c | 7 ++----- builtin/fsck.c | 12 ++++-------- builtin/help.c | 3 +-- builtin/merge.c | 3 +-- builtin/notes.c | 5 ++--- builtin/receive-pack.c | 38 +++++++++++++------------------------- builtin/replace.c | 3 +-- editor.c | 4 +--- sequencer.c | 10 +++------- upload-pack.c | 5 +---- 10 files changed, 29 insertions(+), 61 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index ef6b619c45..a010b2c325 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -302,15 +302,11 @@ int interactive_add(const char **argv, const char *prefix, int patch) static int edit_patch(int argc, const char **argv, const char *prefix) { char *file = git_pathdup("ADD_EDIT.patch"); - const char *apply_argv[] = { "apply", "--recount", "--cached", - NULL, NULL }; struct child_process child = CHILD_PROCESS_INIT; struct rev_info rev; int out; struct stat st; - apply_argv[3] = file; - git_config(git_diff_basic_config, NULL); /* no "diff" UI options */ if (read_cache() < 0) @@ -338,7 +334,8 @@ static int edit_patch(int argc, const char **argv, const char *prefix) die(_("Empty patch. Aborted.")); child.git_cmd = 1; - child.argv = apply_argv; + strvec_pushl(&child.args, "apply", "--recount", "--cached", file, + NULL); if (run_command(&child)) die(_("Could not apply '%s'"), file); diff --git a/builtin/fsck.c b/builtin/fsck.c index 27b9e78094..9e54892311 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -944,15 +944,13 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) if (the_repository->settings.core_commit_graph) { struct child_process commit_graph_verify = CHILD_PROCESS_INIT; - const char *verify_argv[] = { "commit-graph", "verify", NULL, NULL, NULL }; prepare_alt_odb(the_repository); for (odb = the_repository->objects->odb; odb; odb = odb->next) { child_process_init(&commit_graph_verify); - commit_graph_verify.argv = verify_argv; commit_graph_verify.git_cmd = 1; - verify_argv[2] = "--object-dir"; - verify_argv[3] = odb->path; + strvec_pushl(&commit_graph_verify.args, "commit-graph", + "verify", "--object-dir", odb->path, NULL); if (run_command(&commit_graph_verify)) errors_found |= ERROR_COMMIT_GRAPH; } @@ -960,15 +958,13 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) if (the_repository->settings.core_multi_pack_index) { struct child_process midx_verify = CHILD_PROCESS_INIT; - const char *midx_argv[] = { "multi-pack-index", "verify", NULL, NULL, NULL }; prepare_alt_odb(the_repository); for (odb = the_repository->objects->odb; odb; odb = odb->next) { child_process_init(&midx_verify); - midx_verify.argv = midx_argv; midx_verify.git_cmd = 1; - midx_argv[2] = "--object-dir"; - midx_argv[3] = odb->path; + strvec_pushl(&midx_verify.args, "multi-pack-index", + "verify", "--object-dir", odb->path, NULL); if (run_command(&midx_verify)) errors_found |= ERROR_MULTI_PACK_INDEX; } diff --git a/builtin/help.c b/builtin/help.c index 75cd2fb407..d387131dd8 100644 --- a/builtin/help.c +++ b/builtin/help.c @@ -212,11 +212,10 @@ static int check_emacsclient_version(void) { struct strbuf buffer = STRBUF_INIT; struct child_process ec_process = CHILD_PROCESS_INIT; - const char *argv_ec[] = { "emacsclient", "--version", NULL }; int version; /* emacsclient prints its version number on stderr */ - ec_process.argv = argv_ec; + strvec_pushl(&ec_process.args, "emacsclient", "--version", NULL); ec_process.err = -1; ec_process.stdout_to_stderr = 1; if (start_command(&ec_process)) diff --git a/builtin/merge.c b/builtin/merge.c index ea3112e0c0..5f0476b0b7 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -310,10 +310,9 @@ static int save_state(struct object_id *stash) int len; struct child_process cp = CHILD_PROCESS_INIT; struct strbuf buffer = STRBUF_INIT; - const char *argv[] = {"stash", "create", NULL}; int rc = -1; - cp.argv = argv; + strvec_pushl(&cp.args, "stash", "create", NULL); cp.out = -1; cp.git_cmd = 1; diff --git a/builtin/notes.c b/builtin/notes.c index 71c59583a1..85d1abad88 100644 --- a/builtin/notes.c +++ b/builtin/notes.c @@ -134,14 +134,13 @@ static void copy_obj_to_fd(int fd, const struct object_id *oid) static void write_commented_object(int fd, const struct object_id *object) { - const char *show_args[5] = - {"show", "--stat", "--no-notes", oid_to_hex(object), NULL}; struct child_process show = CHILD_PROCESS_INIT; struct strbuf buf = STRBUF_INIT; struct strbuf cbuf = STRBUF_INIT; /* Invoke "git show --stat --no-notes $object" */ - show.argv = show_args; + strvec_pushl(&show.args, "show", "--stat", "--no-notes", + oid_to_hex(object), NULL); show.no_stdin = 1; show.out = -1; show.err = 0; diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 49b846d960..6149d50796 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -1370,22 +1370,10 @@ static const char *push_to_deploy(unsigned char *sha1, struct strvec *env, const char *work_tree) { - const char *update_refresh[] = { - "update-index", "-q", "--ignore-submodules", "--refresh", NULL - }; - const char *diff_files[] = { - "diff-files", "--quiet", "--ignore-submodules", "--", NULL - }; - const char *diff_index[] = { - "diff-index", "--quiet", "--cached", "--ignore-submodules", - NULL, "--", NULL - }; - const char *read_tree[] = { - "read-tree", "-u", "-m", NULL, NULL - }; struct child_process child = CHILD_PROCESS_INIT; - child.argv = update_refresh; + strvec_pushl(&child.args, "update-index", "-q", "--ignore-submodules", + "--refresh", NULL); child.env = env->v; child.dir = work_tree; child.no_stdin = 1; @@ -1396,7 +1384,8 @@ static const char *push_to_deploy(unsigned char *sha1, /* run_command() does not clean up completely; reinitialize */ child_process_init(&child); - child.argv = diff_files; + strvec_pushl(&child.args, "diff-files", "--quiet", + "--ignore-submodules", "--", NULL); child.env = env->v; child.dir = work_tree; child.no_stdin = 1; @@ -1405,11 +1394,12 @@ static const char *push_to_deploy(unsigned char *sha1, if (run_command(&child)) return "Working directory has unstaged changes"; - /* diff-index with either HEAD or an empty tree */ - diff_index[4] = head_has_history() ? "HEAD" : empty_tree_oid_hex(); - child_process_init(&child); - child.argv = diff_index; + strvec_pushl(&child.args, "diff-index", "--quiet", "--cached", + "--ignore-submodules", + /* diff-index with either HEAD or an empty tree */ + head_has_history() ? "HEAD" : empty_tree_oid_hex(), + "--", NULL); child.env = env->v; child.no_stdin = 1; child.no_stdout = 1; @@ -1418,9 +1408,9 @@ static const char *push_to_deploy(unsigned char *sha1, if (run_command(&child)) return "Working directory has staged changes"; - read_tree[3] = hash_to_hex(sha1); child_process_init(&child); - child.argv = read_tree; + strvec_pushl(&child.args, "read-tree", "-u", "-m", hash_to_hex(sha1), + NULL); child.env = env->v; child.dir = work_tree; child.no_stdin = 1; @@ -2575,16 +2565,14 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) run_update_post_hook(commands); string_list_clear(&push_options, 0); if (auto_gc) { - const char *argv_gc_auto[] = { - "gc", "--auto", "--quiet", NULL, - }; struct child_process proc = CHILD_PROCESS_INIT; proc.no_stdin = 1; proc.stdout_to_stderr = 1; proc.err = use_sideband ? -1 : 0; proc.git_cmd = proc.close_object_store = 1; - proc.argv = argv_gc_auto; + strvec_pushl(&proc.args, "gc", "--auto", "--quiet", + NULL); if (!start_command(&proc)) { if (use_sideband) diff --git a/builtin/replace.c b/builtin/replace.c index 946938d011..6ff1734d58 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -258,11 +258,10 @@ static int import_object(struct object_id *oid, enum object_type type, return error_errno(_("unable to open %s for reading"), filename); if (!raw && type == OBJ_TREE) { - const char *argv[] = { "mktree", NULL }; struct child_process cmd = CHILD_PROCESS_INIT; struct strbuf result = STRBUF_INIT; - cmd.argv = argv; + strvec_push(&cmd.args, "mktree"); cmd.git_cmd = 1; cmd.in = fd; cmd.out = -1; diff --git a/editor.c b/editor.c index fdd3eeafa9..d92a8d9ab5 100644 --- a/editor.c +++ b/editor.c @@ -55,7 +55,6 @@ static int launch_specified_editor(const char *editor, const char *path, if (strcmp(editor, ":")) { struct strbuf realpath = STRBUF_INIT; - const char *args[] = { editor, NULL, NULL }; struct child_process p = CHILD_PROCESS_INIT; int ret, sig; int print_waiting_for_editor = advice_enabled(ADVICE_WAITING_FOR_EDITOR) && isatty(2); @@ -77,9 +76,8 @@ static int launch_specified_editor(const char *editor, const char *path, } strbuf_realpath(&realpath, path, 1); - args[1] = realpath.buf; - p.argv = args; + strvec_pushl(&p.args, editor, realpath.buf, NULL); p.env = env; p.use_shell = 1; p.trace2_child_class = "editor"; diff --git a/sequencer.c b/sequencer.c index ea96837cde..6e02210db7 100644 --- a/sequencer.c +++ b/sequencer.c @@ -1164,18 +1164,14 @@ static int run_rewrite_hook(const struct object_id *oldoid, const struct object_id *newoid) { struct child_process proc = CHILD_PROCESS_INIT; - const char *argv[3]; int code; struct strbuf sb = STRBUF_INIT; + const char *hook_path = find_hook("post-rewrite"); - argv[0] = find_hook("post-rewrite"); - if (!argv[0]) + if (!hook_path) return 0; - argv[1] = "amend"; - argv[2] = NULL; - - proc.argv = argv; + strvec_pushl(&proc.args, hook_path, "amend", NULL); proc.in = -1; proc.stdout_to_stderr = 1; proc.trace2_hook_name = "post-rewrite"; diff --git a/upload-pack.c b/upload-pack.c index c78d55bc67..9b5db32623 100644 --- a/upload-pack.c +++ b/upload-pack.c @@ -596,14 +596,11 @@ static int do_reachable_revlist(struct child_process *cmd, struct object_array *reachable, enum allow_uor allow_uor) { - static const char *argv[] = { - "rev-list", "--stdin", NULL, - }; struct object *o; FILE *cmd_in = NULL; int i; - cmd->argv = argv; + strvec_pushl(&cmd->args, "rev-list", "--stdin", NULL); cmd->git_cmd = 1; cmd->no_stderr = 1; cmd->in = -1; From 7f14609e296d5737f34607d4c3e40bb1b063ef04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 25 Nov 2021 23:52:21 +0100 Subject: [PATCH 6/9] run-command API users: use strvec_push(), not argv construction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change a pattern of hardcoding an "argv" array size, populating it and assigning to the "argv" member of "struct child_process" to instead use "strvec_push()" to add data to the "args" member. As noted in the preceding commit this moves us further towards being able to remove the "argv" member in a subsequent commit These callers could have used strvec_pushl(), but moving to strvec_push() makes the diff easier to read, and keeps the arguments aligned as before. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- archive-tar.c | 9 +++------ builtin/receive-pack.c | 31 ++++++++++++------------------- daemon.c | 18 +++++++----------- diff.c | 8 ++------ prompt.c | 7 ++----- transport.c | 11 +++++------ 6 files changed, 31 insertions(+), 53 deletions(-) diff --git a/archive-tar.c b/archive-tar.c index 05d2455870..3c74db1746 100644 --- a/archive-tar.c +++ b/archive-tar.c @@ -430,7 +430,6 @@ static int write_tar_filter_archive(const struct archiver *ar, { struct strbuf cmd = STRBUF_INIT; struct child_process filter = CHILD_PROCESS_INIT; - const char *argv[2]; int r; if (!ar->data) @@ -440,14 +439,12 @@ static int write_tar_filter_archive(const struct archiver *ar, if (args->compression_level >= 0) strbuf_addf(&cmd, " -%d", args->compression_level); - argv[0] = cmd.buf; - argv[1] = NULL; - filter.argv = argv; + strvec_push(&filter.args, cmd.buf); filter.use_shell = 1; filter.in = -1; if (start_command(&filter) < 0) - die_errno(_("unable to start '%s' filter"), argv[0]); + die_errno(_("unable to start '%s' filter"), cmd.buf); close(1); if (dup2(filter.in, 1) < 0) die_errno(_("unable to redirect descriptor")); @@ -457,7 +454,7 @@ static int write_tar_filter_archive(const struct archiver *ar, close(1); if (finish_command(&filter) != 0) - die(_("'%s' filter reported error"), argv[0]); + die(_("'%s' filter reported error"), cmd.buf); strbuf_release(&cmd); return r; diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 6149d50796..48c99c8ee4 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -812,16 +812,13 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed, { struct child_process proc = CHILD_PROCESS_INIT; struct async muxer; - const char *argv[2]; int code; + const char *hook_path = find_hook(hook_name); - argv[0] = find_hook(hook_name); - if (!argv[0]) + if (!hook_path) return 0; - argv[1] = NULL; - - proc.argv = argv; + strvec_push(&proc.args, hook_path); proc.in = -1; proc.stdout_to_stderr = 1; proc.trace2_hook_name = hook_name; @@ -943,23 +940,21 @@ static int run_receive_hook(struct command *commands, static int run_update_hook(struct command *cmd) { - const char *argv[5]; struct child_process proc = CHILD_PROCESS_INIT; int code; + const char *hook_path = find_hook("update"); - argv[0] = find_hook("update"); - if (!argv[0]) + if (!hook_path) return 0; - argv[1] = cmd->ref_name; - argv[2] = oid_to_hex(&cmd->old_oid); - argv[3] = oid_to_hex(&cmd->new_oid); - argv[4] = NULL; + strvec_push(&proc.args, hook_path); + strvec_push(&proc.args, cmd->ref_name); + strvec_push(&proc.args, oid_to_hex(&cmd->old_oid)); + strvec_push(&proc.args, oid_to_hex(&cmd->new_oid)); proc.no_stdin = 1; proc.stdout_to_stderr = 1; proc.err = use_sideband ? -1 : 0; - proc.argv = argv; proc.trace2_hook_name = "update"; code = start_command(&proc); @@ -1117,22 +1112,20 @@ static int run_proc_receive_hook(struct command *commands, struct child_process proc = CHILD_PROCESS_INIT; struct async muxer; struct command *cmd; - const char *argv[2]; struct packet_reader reader; struct strbuf cap = STRBUF_INIT; struct strbuf errmsg = STRBUF_INIT; int hook_use_push_options = 0; int version = 0; int code; + const char *hook_path = find_hook("proc-receive"); - argv[0] = find_hook("proc-receive"); - if (!argv[0]) { + if (!hook_path) { rp_error("cannot find hook 'proc-receive'"); return -1; } - argv[1] = NULL; - proc.argv = argv; + strvec_push(&proc.args, hook_path); proc.in = -1; proc.out = -1; proc.trace2_hook_name = "proc-receive"; diff --git a/daemon.c b/daemon.c index 8df21f2130..4a000ee4af 100644 --- a/daemon.c +++ b/daemon.c @@ -326,22 +326,18 @@ static int run_access_hook(struct daemon_service *service, const char *dir, { struct child_process child = CHILD_PROCESS_INIT; struct strbuf buf = STRBUF_INIT; - const char *argv[8]; - const char **arg = argv; char *eol; int seen_errors = 0; - *arg++ = access_hook; - *arg++ = service->name; - *arg++ = path; - *arg++ = hi->hostname.buf; - *arg++ = get_canon_hostname(hi); - *arg++ = get_ip_address(hi); - *arg++ = hi->tcp_port.buf; - *arg = NULL; + strvec_push(&child.args, access_hook); + strvec_push(&child.args, service->name); + strvec_push(&child.args, path); + strvec_push(&child.args, hi->hostname.buf); + strvec_push(&child.args, get_canon_hostname(hi)); + strvec_push(&child.args, get_ip_address(hi)); + strvec_push(&child.args, hi->tcp_port.buf); child.use_shell = 1; - child.argv = argv; child.no_stdin = 1; child.no_stderr = 1; child.out = -1; diff --git a/diff.c b/diff.c index 861282db1c..4107685742 100644 --- a/diff.c +++ b/diff.c @@ -6921,19 +6921,15 @@ static char *run_textconv(struct repository *r, size_t *outsize) { struct diff_tempfile *temp; - const char *argv[3]; - const char **arg = argv; struct child_process child = CHILD_PROCESS_INIT; struct strbuf buf = STRBUF_INIT; int err = 0; temp = prepare_temp_file(r, spec->path, spec); - *arg++ = pgm; - *arg++ = temp->name; - *arg = NULL; + strvec_push(&child.args, pgm); + strvec_push(&child.args, temp->name); child.use_shell = 1; - child.argv = argv; child.out = -1; if (start_command(&child)) { remove_tempfile(); diff --git a/prompt.c b/prompt.c index 5ded21a017..50df17279d 100644 --- a/prompt.c +++ b/prompt.c @@ -8,15 +8,12 @@ static char *do_askpass(const char *cmd, const char *prompt) { struct child_process pass = CHILD_PROCESS_INIT; - const char *args[3]; static struct strbuf buffer = STRBUF_INIT; int err = 0; - args[0] = cmd; - args[1] = prompt; - args[2] = NULL; + strvec_push(&pass.args, cmd); + strvec_push(&pass.args, prompt); - pass.argv = args; pass.out = -1; if (start_command(&pass)) diff --git a/transport.c b/transport.c index e4f1decae2..92ab9a3fa6 100644 --- a/transport.c +++ b/transport.c @@ -1204,16 +1204,15 @@ static int run_pre_push_hook(struct transport *transport, struct ref *r; struct child_process proc = CHILD_PROCESS_INIT; struct strbuf buf; - const char *argv[4]; + const char *hook_path = find_hook("pre-push"); - if (!(argv[0] = find_hook("pre-push"))) + if (!hook_path) return 0; - argv[1] = transport->remote->name; - argv[2] = transport->url; - argv[3] = NULL; + strvec_push(&proc.args, hook_path); + strvec_push(&proc.args, transport->remote->name); + strvec_push(&proc.args, transport->url); - proc.argv = argv; proc.in = -1; proc.trace2_hook_name = "pre-push"; From d3b2159712019a06f1f495d3e42bd6aa6e76e848 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 25 Nov 2021 23:52:22 +0100 Subject: [PATCH 7/9] run-command API: remove "argv" member, always use "args" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Remove the "argv" member from the run-command API, ever since "args" was added in c460c0ecdca (run-command: store an optional argv_array, 2014-05-15) being able to provide either "argv" or "args" has led to some confusion and bugs. If we hadn't gone in that direction and only had an "argv" our problems wouldn't have been solved either, as noted in [1] (and in the documentation amended here) it comes with inherent memory management issues: The caller would have to hang on to the "argv" until the run-command API was finished. If the "argv" was an argument to main() this wasn't an issue, but if it it was manually constructed using the API might be painful. We also have a recent report[2] of a user of the API segfaulting, which is a direct result of it being complex to use. This commit addresses the root cause of that bug. This change is larger than I'd like, but there's no easy way to avoid it that wouldn't involve even more verbose intermediate steps. We use the "argv" as the source of truth over the "args", so we need to change all parts of run-command.[ch] itself, as well as the trace2 logging at the same time. The resulting Windows-specific code in start_command() is a bit nasty, as we're now assigning to a strvec's "v" member, instead of to our own "argv". There was a suggestion of some alternate approaches in reply to an earlier version of this commit[3], but let's leave larger a larger and needless refactoring of this code for now. 1. http://lore.kernel.org/git/YT6BnnXeAWn8BycF@coredump.intra.peff.net 2. https://lore.kernel.org/git/20211120194048.12125-1-ematsumiya@suse.de/ 3. https://lore.kernel.org/git/patch-5.5-ea1011f7473-20211122T153605Z-avarab@gmail.com/ Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- run-command.c | 42 ++++++++++++++++++++--------------------- run-command.h | 20 ++++++++------------ sub-process.c | 2 +- trace2/tr2_tgt_event.c | 2 +- trace2/tr2_tgt_normal.c | 2 +- trace2/tr2_tgt_perf.c | 4 ++-- 6 files changed, 33 insertions(+), 39 deletions(-) diff --git a/run-command.c b/run-command.c index 620a06ca2f..99dc93e730 100644 --- a/run-command.c +++ b/run-command.c @@ -380,7 +380,7 @@ static void child_err_spew(struct child_process *cmd, struct child_err *cerr) switch (cerr->err) { case CHILD_ERR_CHDIR: error_errno("exec '%s': cd to '%s' failed", - cmd->argv[0], cmd->dir); + cmd->args.v[0], cmd->dir); break; case CHILD_ERR_DUP2: error_errno("dup2() in child failed"); @@ -392,12 +392,12 @@ static void child_err_spew(struct child_process *cmd, struct child_err *cerr) error_errno("sigprocmask failed restoring signals"); break; case CHILD_ERR_ENOENT: - error_errno("cannot run %s", cmd->argv[0]); + error_errno("cannot run %s", cmd->args.v[0]); break; case CHILD_ERR_SILENT: break; case CHILD_ERR_ERRNO: - error_errno("cannot exec '%s'", cmd->argv[0]); + error_errno("cannot exec '%s'", cmd->args.v[0]); break; } set_error_routine(old_errfn); @@ -405,7 +405,7 @@ static void child_err_spew(struct child_process *cmd, struct child_err *cerr) static int prepare_cmd(struct strvec *out, const struct child_process *cmd) { - if (!cmd->argv[0]) + if (!cmd->args.v[0]) BUG("command is empty"); /* @@ -415,11 +415,11 @@ static int prepare_cmd(struct strvec *out, const struct child_process *cmd) strvec_push(out, SHELL_PATH); if (cmd->git_cmd) { - prepare_git_cmd(out, cmd->argv); + prepare_git_cmd(out, cmd->args.v); } else if (cmd->use_shell) { - prepare_shell_cmd(out, cmd->argv); + prepare_shell_cmd(out, cmd->args.v); } else { - strvec_pushv(out, cmd->argv); + strvec_pushv(out, cmd->args.v); } /* @@ -663,7 +663,7 @@ static void trace_run_command(const struct child_process *cp) trace_add_env(&buf, cp->env); if (cp->git_cmd) strbuf_addstr(&buf, " git"); - sq_quote_argv_pretty(&buf, cp->argv); + sq_quote_argv_pretty(&buf, cp->args.v); trace_printf("%s", buf.buf); strbuf_release(&buf); @@ -676,8 +676,6 @@ int start_command(struct child_process *cmd) int failed_errno; char *str; - if (!cmd->argv) - cmd->argv = cmd->args.v; if (!cmd->env) cmd->env = cmd->env_array.v; @@ -729,7 +727,7 @@ int start_command(struct child_process *cmd) str = "standard error"; fail_pipe: error("cannot create %s pipe for %s: %s", - str, cmd->argv[0], strerror(failed_errno)); + str, cmd->args.v[0], strerror(failed_errno)); child_process_clear(cmd); errno = failed_errno; return -1; @@ -758,7 +756,7 @@ fail_pipe: failed_errno = errno; cmd->pid = -1; if (!cmd->silent_exec_failure) - error_errno("cannot run %s", cmd->argv[0]); + error_errno("cannot run %s", cmd->args.v[0]); goto end_of_spawn; } @@ -868,7 +866,7 @@ fail_pipe: } atfork_parent(&as); if (cmd->pid < 0) - error_errno("cannot fork() for %s", cmd->argv[0]); + error_errno("cannot fork() for %s", cmd->args.v[0]); else if (cmd->clean_on_exit) mark_child_for_cleanup(cmd->pid, cmd); @@ -885,7 +883,7 @@ fail_pipe: * At this point we know that fork() succeeded, but exec() * failed. Errors have been reported to our stderr. */ - wait_or_whine(cmd->pid, cmd->argv[0], 0); + wait_or_whine(cmd->pid, cmd->args.v[0], 0); child_err_spew(cmd, &cerr); failed_errno = errno; cmd->pid = -1; @@ -902,7 +900,7 @@ end_of_spawn: #else { int fhin = 0, fhout = 1, fherr = 2; - const char **sargv = cmd->argv; + const char **sargv = cmd->args.v; struct strvec nargv = STRVEC_INIT; if (cmd->no_stdin) @@ -929,20 +927,20 @@ end_of_spawn: fhout = dup(cmd->out); if (cmd->git_cmd) - cmd->argv = prepare_git_cmd(&nargv, cmd->argv); + cmd->args.v = prepare_git_cmd(&nargv, sargv); else if (cmd->use_shell) - cmd->argv = prepare_shell_cmd(&nargv, cmd->argv); + cmd->args.v = prepare_shell_cmd(&nargv, sargv); - cmd->pid = mingw_spawnvpe(cmd->argv[0], cmd->argv, (char**) cmd->env, + cmd->pid = mingw_spawnvpe(cmd->args.v[0], cmd->args.v, (char**) cmd->env, cmd->dir, fhin, fhout, fherr); failed_errno = errno; if (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT)) - error_errno("cannot spawn %s", cmd->argv[0]); + error_errno("cannot spawn %s", cmd->args.v[0]); if (cmd->clean_on_exit && cmd->pid >= 0) mark_child_for_cleanup(cmd->pid, cmd); strvec_clear(&nargv); - cmd->argv = sargv; + cmd->args.v = sargv; if (fhin != 0) close(fhin); if (fhout != 1) @@ -992,7 +990,7 @@ end_of_spawn: int finish_command(struct child_process *cmd) { - int ret = wait_or_whine(cmd->pid, cmd->argv[0], 0); + int ret = wait_or_whine(cmd->pid, cmd->args.v[0], 0); trace2_child_exit(cmd, ret); child_process_clear(cmd); invalidate_lstat_cache(); @@ -1001,7 +999,7 @@ int finish_command(struct child_process *cmd) int finish_command_in_signal(struct child_process *cmd) { - int ret = wait_or_whine(cmd->pid, cmd->argv[0], 1); + int ret = wait_or_whine(cmd->pid, cmd->args.v[0], 1); trace2_child_exit(cmd, ret); return ret; } diff --git a/run-command.h b/run-command.h index 4987826258..c0d1210cc6 100644 --- a/run-command.h +++ b/run-command.h @@ -44,21 +44,17 @@ struct child_process { /** - * The .argv member is set up as an array of string pointers (NULL - * terminated), of which .argv[0] is the program name to run (usually - * without a path). If the command to run is a git command, set argv[0] to - * the command name without the 'git-' prefix and set .git_cmd = 1. + * The .args is a `struct strvec', use that API to manipulate + * it, e.g. strvec_pushv() to add an existing "const char **" + * vector. * - * Note that the ownership of the memory pointed to by .argv stays with the - * caller, but it should survive until `finish_command` completes. If the - * .argv member is NULL, `start_command` will point it at the .args - * `strvec` (so you may use one or the other, but you must use exactly - * one). The memory in .args will be cleaned up automatically during + * If the command to run is a git command, set the first + * element in the strvec to the command name without the + * 'git-' prefix and set .git_cmd = 1. + * + * The memory in .args will be cleaned up automatically during * `finish_command` (or during `start_command` when it is unsuccessful). - * */ - const char **argv; - struct strvec args; struct strvec env_array; pid_t pid; diff --git a/sub-process.c b/sub-process.c index dfa790d3ff..cae56ae6b8 100644 --- a/sub-process.c +++ b/sub-process.c @@ -187,7 +187,7 @@ static int handshake_capabilities(struct child_process *process, *supported_capabilities |= capabilities[i].flag; } else { die("subprocess '%s' requested unsupported capability '%s'", - process->argv[0], p); + process->args.v[0], p); } } diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c index 3a0014417c..bd17ecdc32 100644 --- a/trace2/tr2_tgt_event.c +++ b/trace2/tr2_tgt_event.c @@ -354,7 +354,7 @@ static void fn_child_start_fl(const char *file, int line, jw_object_inline_begin_array(&jw, "argv"); if (cmd->git_cmd) jw_array_string(&jw, "git"); - jw_array_argv(&jw, cmd->argv); + jw_array_argv(&jw, cmd->args.v); jw_end(&jw); jw_end(&jw); diff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c index 58d9e430f0..6e429a3fb9 100644 --- a/trace2/tr2_tgt_normal.c +++ b/trace2/tr2_tgt_normal.c @@ -232,7 +232,7 @@ static void fn_child_start_fl(const char *file, int line, strbuf_addch(&buf_payload, ' '); if (cmd->git_cmd) strbuf_addstr(&buf_payload, "git "); - sq_append_quote_argv_pretty(&buf_payload, cmd->argv); + sq_append_quote_argv_pretty(&buf_payload, cmd->args.v); normal_io_write_fl(file, line, &buf_payload); strbuf_release(&buf_payload); diff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c index e4acca13d6..2ff9cf7083 100644 --- a/trace2/tr2_tgt_perf.c +++ b/trace2/tr2_tgt_perf.c @@ -335,10 +335,10 @@ static void fn_child_start_fl(const char *file, int line, strbuf_addstr(&buf_payload, " argv:["); if (cmd->git_cmd) { strbuf_addstr(&buf_payload, "git"); - if (cmd->argv[0]) + if (cmd->args.nr) strbuf_addch(&buf_payload, ' '); } - sq_append_quote_argv_pretty(&buf_payload, cmd->argv); + sq_append_quote_argv_pretty(&buf_payload, cmd->args.v); strbuf_addch(&buf_payload, ']'); perf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute, From 26a15355d60a0a5be08250512c05e1447036dce3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 25 Nov 2021 23:52:23 +0100 Subject: [PATCH 8/9] difftool: use "env_array" to simplify memory management MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Amend code added in 03831ef7b50 (difftool: implement the functionality in the builtin, 2017-01-19) to use the "env_array" in the run_command.[ch] API. Now we no longer need to manage our own "index_env" buffer. Signed-off-by: Ævar Arnfjörð Bjarmason Signed-off-by: Junio C Hamano --- builtin/difftool.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/builtin/difftool.c b/builtin/difftool.c index 4931c10845..4ee40fe3a0 100644 --- a/builtin/difftool.c +++ b/builtin/difftool.c @@ -202,15 +202,10 @@ static void changed_files(struct hashmap *result, const char *index_path, { struct child_process update_index = CHILD_PROCESS_INIT; struct child_process diff_files = CHILD_PROCESS_INIT; - struct strbuf index_env = STRBUF_INIT, buf = STRBUF_INIT; - const char *git_dir = absolute_path(get_git_dir()), *env[] = { - NULL, NULL - }; + struct strbuf buf = STRBUF_INIT; + const char *git_dir = absolute_path(get_git_dir()); FILE *fp; - strbuf_addf(&index_env, "GIT_INDEX_FILE=%s", index_path); - env[0] = index_env.buf; - strvec_pushl(&update_index.args, "--git-dir", git_dir, "--work-tree", workdir, "update-index", "--really-refresh", "-q", @@ -222,7 +217,7 @@ static void changed_files(struct hashmap *result, const char *index_path, update_index.use_shell = 0; update_index.clean_on_exit = 1; update_index.dir = workdir; - update_index.env = env; + strvec_pushf(&update_index.env_array, "GIT_INDEX_FILE=%s", index_path); /* Ignore any errors of update-index */ run_command(&update_index); @@ -235,7 +230,7 @@ static void changed_files(struct hashmap *result, const char *index_path, diff_files.clean_on_exit = 1; diff_files.out = -1; diff_files.dir = workdir; - diff_files.env = env; + strvec_pushf(&diff_files.env_array, "GIT_INDEX_FILE=%s", index_path); if (start_command(&diff_files)) die("could not obtain raw diff"); fp = xfdopen(diff_files.out, "r"); @@ -248,7 +243,6 @@ static void changed_files(struct hashmap *result, const char *index_path, fclose(fp); if (finish_command(&diff_files)) die("diff-files did not exit properly"); - strbuf_release(&index_env); strbuf_release(&buf); } From c7c4bdeccf3e737e6e674cd9f0828922e629ab06 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Thu, 25 Nov 2021 23:52:24 +0100 Subject: [PATCH 9/9] run-command API: remove "env" member, always use "env_array" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Signed-off-by: Junio C Hamano --- 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 48c99c8ee4..3979752cec 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 9edd3e2829..962d71cf98 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 @@ 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 35bd4a2638..ed3025e7a2 100644 --- 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 d92a8d9ab5..8b9648281d 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 c3d866a287..2fc1bed417 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 99dc93e730..ebade08670 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 @@ 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 c0d1210cc6..2be5f5d642 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 7c7cb61a94..1b12f77d94 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;