* [PATCH 1/5] archive-tar: use our own cmd.buf in error message
2021-11-22 16:04 ` [PATCH 0/5] run-command API: get rid of "argv" Ævar Arnfjörð Bjarmason
@ 2021-11-22 16:04 ` Ævar Arnfjörð Bjarmason
2021-11-22 21:04 ` Junio C Hamano
2021-11-22 16:04 ` [PATCH 2/5] upload-archive: use regular "struct child_process" pattern Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
6 siblings, 1 reply; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 16:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
Use the "cmd.buf" we just created in this function, instead of the
argv[0], which we assigned "cmd.buf" for. This is in preparation for
getting rid of the use of "argv" in this function.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
archive-tar.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index 05d2455870d..4154d9a0953 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -447,7 +447,7 @@ static int write_tar_filter_archive(const struct archiver *ar,
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 +457,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;
--
2.34.0.822.gb876f875f1b
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH 1/5] archive-tar: use our own cmd.buf in error message
2021-11-22 16:04 ` [PATCH 1/5] archive-tar: use our own cmd.buf in error message Ævar Arnfjörð Bjarmason
@ 2021-11-22 21:04 ` Junio C Hamano
0 siblings, 0 replies; 71+ messages in thread
From: Junio C Hamano @ 2021-11-22 21:04 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Enzo Matsumiya
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Use the "cmd.buf" we just created in this function, instead of the
> argv[0], which we assigned "cmd.buf" for. This is in preparation for
> getting rid of the use of "argv" in this function.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> archive-tar.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/archive-tar.c b/archive-tar.c
> index 05d2455870d..4154d9a0953 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -447,7 +447,7 @@ static int write_tar_filter_archive(const struct archiver *ar,
> 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 +457,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;
Is this really needed to be a separate step? If we update this
function to
- lose local argv[] array;
- instead use the embedded filter.args
i.e. something like the attached, such a "modern" code that uses
child_process .args would not require further changes when the
run-command API is streamlined by the remainder of the series, no?
IOW, if can arrange this step to be trivially and obviously correct
so that it can go in without fixing any bugs (if exists) in the main
part of the series, and the API update would not require such code
that already uses cp.args not cp.argv, that would be much easier to
grok. With this "this is half a change in preparation", reviewers
need to keep this promise in their heads that argv[] will be then
removed from the function.
archive-tar.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git c/archive-tar.c w/archive-tar.c
index 05d2455870..3c74db1746 100644
--- c/archive-tar.c
+++ w/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;
^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH 2/5] upload-archive: use regular "struct child_process" pattern
2021-11-22 16:04 ` [PATCH 0/5] run-command API: get rid of "argv" Ævar Arnfjörð Bjarmason
2021-11-22 16:04 ` [PATCH 1/5] archive-tar: use our own cmd.buf in error message Ævar Arnfjörð Bjarmason
@ 2021-11-22 16:04 ` Ævar Arnfjörð Bjarmason
2021-11-22 17:02 ` Jeff King
2021-11-22 20:53 ` Ævar Arnfjörð Bjarmason
2021-11-22 16:04 ` [PATCH 3/5] run-command API users: use strvec_pushv(), not argv assignment Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
6 siblings, 2 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 16:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/upload-archive.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 24654b4c9bf..b4b9b3a6262 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);
@@ -92,6 +92,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
argv[0] = "upload-archive--writer";
writer.out = writer.err = -1;
writer.git_cmd = 1;
+ strvec_push(&writer.args, "upload-archive--writer");
+ if (argc > 1)
+ strvec_pushv(&writer.args, &argv[1]);
if (start_command(&writer)) {
int err = errno;
packet_write_fmt(1, "NACK unable to spawn subprocess\n");
--
2.34.0.822.gb876f875f1b
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH 2/5] upload-archive: use regular "struct child_process" pattern
2021-11-22 16:04 ` [PATCH 2/5] upload-archive: use regular "struct child_process" pattern Ævar Arnfjörð Bjarmason
@ 2021-11-22 17:02 ` Jeff King
2021-11-22 20:53 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 71+ messages in thread
From: Jeff King @ 2021-11-22 17:02 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Enzo Matsumiya
On Mon, Nov 22, 2021 at 05:04:04PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.
Yes, I came across this one, too, while looking at the same topic. I
think this is a good change, but:
> 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);
> @@ -92,6 +92,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
> argv[0] = "upload-archive--writer";
You can drop this assignment over argv[0] now.
> writer.out = writer.err = -1;
> writer.git_cmd = 1;
> + strvec_push(&writer.args, "upload-archive--writer");
> + if (argc > 1)
> + strvec_pushv(&writer.args, &argv[1]);
The "argc > 1" isn't necessary. If it is not true, strvec_pushv() will
see the terminating NULL in the list and just become a noop.
(I'd also spell it "argv + 1" to make it more obvious that we are
interested in the list and not a single element, but that may be a
matter of taste).
-Peff
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH 2/5] upload-archive: use regular "struct child_process" pattern
2021-11-22 16:04 ` [PATCH 2/5] upload-archive: use regular "struct child_process" pattern Ævar Arnfjörð Bjarmason
2021-11-22 17:02 ` Jeff King
@ 2021-11-22 20:53 ` Ævar Arnfjörð Bjarmason
2021-11-22 21:10 ` Jeff King
1 sibling, 1 reply; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 20:53 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
On Mon, Nov 22 2021, Ævar Arnfjörð Bjarmason wrote:
> 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 <avarab@gmail.com>
> ---
> builtin/upload-archive.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
> index 24654b4c9bf..b4b9b3a6262 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);
> @@ -92,6 +92,9 @@ int cmd_upload_archive(int argc, const char **argv, const char *prefix)
> argv[0] = "upload-archive--writer";
> writer.out = writer.err = -1;
> writer.git_cmd = 1;
> + strvec_push(&writer.args, "upload-archive--writer");
> + if (argc > 1)
> + strvec_pushv(&writer.args, &argv[1]);
> if (start_command(&writer)) {
> int err = errno;
> packet_write_fmt(1, "NACK unable to spawn subprocess\n");
Changing it to argv + 1 makes sense, I don't know why I did it like
that. It works, but is unusual.
But as to skipping the "argc > 1" test I've got this still:
@@ -89,9 +89,11 @@ 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");
+ if (argc > 1)
+ strvec_pushv(&writer.args, argv + 1);
if (start_command(&writer)) {
int err = errno;
packet_write_fmt(1, "NACK unable to spawn subprocess\n");
We'll segfault if we give NULL to strvec_pushv() so we still need that
check. Were you thinking of strvec_pushl(), or am I missing something?
(Even with strvec_pushl() we can't interpolate it as-is into the list,
since it'll point to uninitialized memory in this case).
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH 2/5] upload-archive: use regular "struct child_process" pattern
2021-11-22 20:53 ` Ævar Arnfjörð Bjarmason
@ 2021-11-22 21:10 ` Jeff King
2021-11-22 21:36 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 71+ messages in thread
From: Jeff King @ 2021-11-22 21:10 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Enzo Matsumiya
On Mon, Nov 22, 2021 at 09:53:34PM +0100, Ævar Arnfjörð Bjarmason wrote:
> But as to skipping the "argc > 1" test I've got this still:
>
> @@ -89,9 +89,11 @@ 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");
> + if (argc > 1)
> + strvec_pushv(&writer.args, argv + 1);
> if (start_command(&writer)) {
> int err = errno;
> packet_write_fmt(1, "NACK unable to spawn subprocess\n");
>
> We'll segfault if we give NULL to strvec_pushv() so we still need that
> check. Were you thinking of strvec_pushl(), or am I missing something?
We wouldn't be giving NULL to strvec_pushv(). We'd be giving argv+1,
which is guaranteed non-NULL, but may point to NULL. In that case the
loop condition in strvec_pushv() would turn it into a noop.
-Peff
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH 2/5] upload-archive: use regular "struct child_process" pattern
2021-11-22 21:10 ` Jeff King
@ 2021-11-22 21:36 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 21:36 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Enzo Matsumiya
On Mon, Nov 22 2021, Jeff King wrote:
> On Mon, Nov 22, 2021 at 09:53:34PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> But as to skipping the "argc > 1" test I've got this still:
>>
>> @@ -89,9 +89,11 @@ 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");
>> + if (argc > 1)
>> + strvec_pushv(&writer.args, argv + 1);
>> if (start_command(&writer)) {
>> int err = errno;
>> packet_write_fmt(1, "NACK unable to spawn subprocess\n");
>>
>> We'll segfault if we give NULL to strvec_pushv() so we still need that
>> check. Were you thinking of strvec_pushl(), or am I missing something?
>
> We wouldn't be giving NULL to strvec_pushv(). We'd be giving argv+1,
> which is guaranteed non-NULL, but may point to NULL. In that case the
> loop condition in strvec_pushv() would turn it into a noop.
D'oh. Thanks. Yes I was stupidly conflating a case where I'd tested it
with strvec_pushl() and moved that dereferenced version to
strvec_pushv(), sorry.
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 3/5] run-command API users: use strvec_pushv(), not argv assignment
2021-11-22 16:04 ` [PATCH 0/5] run-command API: get rid of "argv" Ævar Arnfjörð Bjarmason
2021-11-22 16:04 ` [PATCH 1/5] archive-tar: use our own cmd.buf in error message Ævar Arnfjörð Bjarmason
2021-11-22 16:04 ` [PATCH 2/5] upload-archive: use regular "struct child_process" pattern Ævar Arnfjörð Bjarmason
@ 2021-11-22 16:04 ` Ævar Arnfjörð Bjarmason
2021-11-22 21:19 ` Junio C Hamano
2021-11-22 16:04 ` [PATCH 4/5] run-command API users: use strvec_pushl(), not argv construction Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
6 siblings, 1 reply; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 16:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
Migrate those run-command API users that assign directly to the "argv"
member to use a strvec_pushv() of "args" instead, but exclude those
cases where we can't easily get rid of the construction of the "argv"
variable being given to the resulting "strvec_pushv()".
This is in preparation for getting rid of the "argv" member from the
run-command API itself.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
add-patch.c | 4 ++--
builtin/notes.c | 2 +-
builtin/receive-pack.c | 16 ++++++++--------
daemon.c | 2 +-
editor.c | 2 +-
http-backend.c | 2 +-
http.c | 5 +++--
remote-curl.c | 2 +-
run-command.c | 2 +-
sequencer.c | 2 +-
t/helper/test-subprocess.c | 2 +-
transport.c | 2 +-
12 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/add-patch.c b/add-patch.c
index 8c41cdfe39b..573eef0cc4a 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/builtin/notes.c b/builtin/notes.c
index 71c59583a17..2b2bac43f31 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -141,7 +141,7 @@ static void write_commented_object(int fd, const struct object_id *object)
struct strbuf cbuf = STRBUF_INIT;
/* Invoke "git show --stat --no-notes $object" */
- show.argv = show_args;
+ strvec_pushv(&show.args, show_args);
show.no_stdin = 1;
show.out = -1;
show.err = 0;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 49b846d9605..33815631010 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -821,7 +821,7 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
argv[1] = NULL;
- proc.argv = argv;
+ strvec_pushv(&proc.args, argv);
proc.in = -1;
proc.stdout_to_stderr = 1;
proc.trace2_hook_name = hook_name;
@@ -959,7 +959,7 @@ static int run_update_hook(struct command *cmd)
proc.no_stdin = 1;
proc.stdout_to_stderr = 1;
proc.err = use_sideband ? -1 : 0;
- proc.argv = argv;
+ strvec_pushv(&proc.args, argv);
proc.trace2_hook_name = "update";
code = start_command(&proc);
@@ -1132,7 +1132,7 @@ static int run_proc_receive_hook(struct command *commands,
}
argv[1] = NULL;
- proc.argv = argv;
+ strvec_pushv(&proc.args, argv);
proc.in = -1;
proc.out = -1;
proc.trace2_hook_name = "proc-receive";
@@ -1385,7 +1385,7 @@ static const char *push_to_deploy(unsigned char *sha1,
};
struct child_process child = CHILD_PROCESS_INIT;
- child.argv = update_refresh;
+ strvec_pushv(&child.args, update_refresh);
child.env = env->v;
child.dir = work_tree;
child.no_stdin = 1;
@@ -1396,7 +1396,7 @@ 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_pushv(&child.args, diff_files);
child.env = env->v;
child.dir = work_tree;
child.no_stdin = 1;
@@ -1409,7 +1409,7 @@ static const char *push_to_deploy(unsigned char *sha1,
diff_index[4] = head_has_history() ? "HEAD" : empty_tree_oid_hex();
child_process_init(&child);
- child.argv = diff_index;
+ strvec_pushv(&child.args, diff_index);
child.env = env->v;
child.no_stdin = 1;
child.no_stdout = 1;
@@ -1420,7 +1420,7 @@ static const char *push_to_deploy(unsigned char *sha1,
read_tree[3] = hash_to_hex(sha1);
child_process_init(&child);
- child.argv = read_tree;
+ strvec_pushv(&child.args, read_tree);
child.env = env->v;
child.dir = work_tree;
child.no_stdin = 1;
@@ -2584,7 +2584,7 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix)
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_pushv(&proc.args, argv_gc_auto);
if (!start_command(&proc)) {
if (use_sideband)
diff --git a/daemon.c b/daemon.c
index b1fcbe0d6fa..8df21f2130c 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/editor.c b/editor.c
index 674309eed8b..90f3379285c 100644
--- a/editor.c
+++ b/editor.c
@@ -82,7 +82,7 @@ static int launch_specified_editor(const char *editor, const char *path,
strbuf_realpath(&realpath, path, 1);
args[1] = realpath.buf;
- p.argv = args;
+ strvec_pushv(&p.args, args);
p.env = env;
p.use_shell = 1;
p.trace2_child_class = "editor";
diff --git a/http-backend.c b/http-backend.c
index 3d6e2ff17f8..4dd4d939f8a 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 f92859f43fa..229da4d1488 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 d69156312bd..0dabef2dd7c 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 f40df01c772..620a06ca2f5 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/sequencer.c b/sequencer.c
index ea96837cde3..60ede96461c 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1175,7 +1175,7 @@ static int run_rewrite_hook(const struct object_id *oldoid,
argv[1] = "amend";
argv[2] = NULL;
- proc.argv = argv;
+ strvec_pushv(&proc.args, argv);
proc.in = -1;
proc.stdout_to_stderr = 1;
proc.trace2_hook_name = "post-rewrite";
diff --git a/t/helper/test-subprocess.c b/t/helper/test-subprocess.c
index 92b69de6352..ff22f2fa2c5 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);
}
diff --git a/transport.c b/transport.c
index e4f1decae20..2361f54bd48 100644
--- a/transport.c
+++ b/transport.c
@@ -1213,7 +1213,7 @@ static int run_pre_push_hook(struct transport *transport,
argv[2] = transport->url;
argv[3] = NULL;
- proc.argv = argv;
+ strvec_pushv(&proc.args, argv);
proc.in = -1;
proc.trace2_hook_name = "pre-push";
--
2.34.0.822.gb876f875f1b
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH 3/5] run-command API users: use strvec_pushv(), not argv assignment
2021-11-22 16:04 ` [PATCH 3/5] run-command API users: use strvec_pushv(), not argv assignment Ævar Arnfjörð Bjarmason
@ 2021-11-22 21:19 ` Junio C Hamano
2021-11-22 21:30 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 71+ messages in thread
From: Junio C Hamano @ 2021-11-22 21:19 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason; +Cc: git, Jeff King, Enzo Matsumiya
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> diff --git a/add-patch.c b/add-patch.c
> index 8c41cdfe39b..573eef0cc4a 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);
We used to use the caller-supplied args, run-command API borrowed
that strvec by pointing at cp.argv, and because it is borrowed,
capture_command() did not use cp.args/cp.env_array and there was no
leak inside run-command API side, whether capture_command succeeded
or failed. The code was using its own args, so it already correctly
releases it (we can see one such strvec_clear() here).
OK.
I gave the remainder only a cursory look so I cannot call it quite
"reviewed", but presumably all the other changes in this patch are
the same way?
Thanks.
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH 3/5] run-command API users: use strvec_pushv(), not argv assignment
2021-11-22 21:19 ` Junio C Hamano
@ 2021-11-22 21:30 ` Ævar Arnfjörð Bjarmason
0 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 21:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Enzo Matsumiya
On Mon, Nov 22 2021, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> diff --git a/add-patch.c b/add-patch.c
>> index 8c41cdfe39b..573eef0cc4a 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);
>
> We used to use the caller-supplied args, run-command API borrowed
> that strvec by pointing at cp.argv, and because it is borrowed,
> capture_command() did not use cp.args/cp.env_array and there was no
> leak inside run-command API side, whether capture_command succeeded
> or failed. The code was using its own args, so it already correctly
> releases it (we can see one such strvec_clear() here).
>
> OK.
>
> I gave the remainder only a cursory look so I cannot call it quite
> "reviewed", but presumably all the other changes in this patch are
> the same way?
Yes, I tried to group all of these cases together.
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH 4/5] run-command API users: use strvec_pushl(), not argv construction
2021-11-22 16:04 ` [PATCH 0/5] run-command API: get rid of "argv" Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-11-22 16:04 ` [PATCH 3/5] run-command API users: use strvec_pushv(), not argv assignment Ævar Arnfjörð Bjarmason
@ 2021-11-22 16:04 ` Ævar Arnfjörð Bjarmason
2021-11-22 16:04 ` [PATCH 5/5] run-command API: remove "argv" member, always use "args" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
6 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 16:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
Migrate those run-command API users that assign directly to the "argv"
member to use a strvec_pushl() of a list instead, this gets rid of the
intermediate "const char *args[]" these callers were using.
This is in preparation for getting rid of the "argv" member from the
run-command API itself.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
archive-tar.c | 5 +----
builtin/add.c | 6 +-----
builtin/fsck.c | 12 ++++--------
builtin/help.c | 3 +--
builtin/merge.c | 3 +--
builtin/replace.c | 3 +--
daemon.c | 15 +++------------
diff.c | 7 +------
prompt.c | 7 +------
upload-pack.c | 5 +----
10 files changed, 15 insertions(+), 51 deletions(-)
diff --git a/archive-tar.c b/archive-tar.c
index 4154d9a0953..3c74db17468 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,9 +439,7 @@ 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;
diff --git a/builtin/add.c b/builtin/add.c
index ef6b619c45e..6357c0c3f9a 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,7 @@ 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 27b9e78094d..9e54892311d 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 75cd2fb407f..d387131dd83 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 ea3112e0c0b..5f0476b0b76 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/replace.c b/builtin/replace.c
index 946938d011e..6ff1734d587 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/daemon.c b/daemon.c
index 8df21f2130c..cc278077d2f 100644
--- a/daemon.c
+++ b/daemon.c
@@ -326,22 +326,13 @@ 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;
-
child.use_shell = 1;
- child.argv = argv;
+ strvec_pushl(&child.args, access_hook, service->name, path,
+ hi->hostname.buf, get_canon_hostname(hi),
+ get_ip_address(hi), hi->tcp_port.buf, NULL);
child.no_stdin = 1;
child.no_stderr = 1;
child.out = -1;
diff --git a/diff.c b/diff.c
index 861282db1c3..70711090b29 100644
--- a/diff.c
+++ b/diff.c
@@ -6921,19 +6921,14 @@ 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;
child.use_shell = 1;
- child.argv = argv;
+ strvec_pushl(&child.args, pgm, temp->name, NULL);
child.out = -1;
if (start_command(&child)) {
remove_tempfile();
diff --git a/prompt.c b/prompt.c
index 5ded21a017f..aff9d7b65b2 100644
--- a/prompt.c
+++ b/prompt.c
@@ -8,15 +8,10 @@
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;
-
- pass.argv = args;
+ strvec_pushl(&pass.args, cmd, prompt, NULL);
pass.out = -1;
if (start_command(&pass))
diff --git a/upload-pack.c b/upload-pack.c
index c78d55bc674..9b5db32623f 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;
--
2.34.0.822.gb876f875f1b
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH 5/5] run-command API: remove "argv" member, always use "args"
2021-11-22 16:04 ` [PATCH 0/5] run-command API: get rid of "argv" Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-11-22 16:04 ` [PATCH 4/5] run-command API users: use strvec_pushl(), not argv construction Ævar Arnfjörð Bjarmason
@ 2021-11-22 16:04 ` Ævar Arnfjörð Bjarmason
2021-11-22 17:32 ` Jeff King
2021-11-22 17:52 ` [PATCH 0/5] run-command API: get rid of "argv" Jeff King
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
6 siblings, 1 reply; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 16:04 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
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. A test being
added here would segfault before this change.
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.
We can also similarly follow-up and remove "env" member in favor of
"env_array", which is a much smaller change than this series removing
"argv", but let's stop at "argv" for now. We can always do "env" as a
follow-up.
I have not carefully tested the Windows-specific code in
start_command() as I don't have a Windows system, but it seems to pass
CI + runs the tests there.
1. http://lore.kernel.org/git/YT6BnnXeAWn8BycF@coredump.intra.peff.net
2. https://lore.kernel.org/git/20211120194048.12125-1-ematsumiya@suse.de/
Reported-by: Enzo Matsumiya <ematsumiya@suse.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/worktree.c | 2 --
run-command.c | 51 ++++++++++++++++++++-----------------
run-command.h | 20 ++++++---------
sub-process.c | 2 +-
t/helper/test-run-command.c | 10 +++++---
| 4 +++
trace2/tr2_tgt_event.c | 2 +-
trace2/tr2_tgt_normal.c | 2 +-
trace2/tr2_tgt_perf.c | 4 +--
9 files changed, 51 insertions(+), 46 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d22ece93e1a..7264a5b5de0 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -355,7 +355,6 @@ static int add_worktree(const char *path, const char *refname,
goto done;
if (opts->checkout) {
- cp.argv = NULL;
strvec_clear(&cp.args);
strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
if (opts->quiet)
@@ -390,7 +389,6 @@ static int add_worktree(const char *path, const char *refname,
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()),
diff --git a/run-command.c b/run-command.c
index 620a06ca2f5..6158807ed13 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 @@ int start_command(struct child_process *cmd)
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 @@ int start_command(struct child_process *cmd)
}
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 @@ int start_command(struct child_process *cmd)
* 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,8 +900,9 @@ int start_command(struct child_process *cmd)
#else
{
int fhin = 0, fhout = 1, fherr = 2;
- const char **sargv = cmd->argv;
+ const char **sargv = strvec_detach(&cmd->args);
struct strvec nargv = STRVEC_INIT;
+ const char **temp_argv = NULL;
if (cmd->no_stdin)
fhin = open("/dev/null", O_RDWR);
@@ -929,20 +928,26 @@ int start_command(struct child_process *cmd)
fhout = dup(cmd->out);
if (cmd->git_cmd)
- cmd->argv = prepare_git_cmd(&nargv, cmd->argv);
+ temp_argv = prepare_git_cmd(&nargv, sargv);
else if (cmd->use_shell)
- cmd->argv = prepare_shell_cmd(&nargv, cmd->argv);
+ temp_argv = prepare_shell_cmd(&nargv, sargv);
+ else
+ temp_argv = sargv;
+ if (!temp_argv)
+ BUG("should have some cmd->args to set");
+ strvec_pushv(&cmd->args, temp_argv);
+ strvec_clear(&nargv);
- 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;
+ strvec_pushv(&cmd->args, sargv);;
+ free(sargv);
if (fhin != 0)
close(fhin);
if (fhout != 1)
@@ -992,7 +997,7 @@ int start_command(struct child_process *cmd)
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 +1006,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 49878262584..c0d1210cc63 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
- * `finish_command` (or during `start_command` when it is unsuccessful).
+ * 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 dfa790d3ff9..cae56ae6b80 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/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 3c4fb862234..b5519f92a19 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,8 @@ int cmd__run_command(int argc, const char **argv)
}
if (argc < 3)
return 1;
- proc.argv = (const char **)argv + 2;
+ strvec_clear(&proc.args);
+ strvec_pushv(&proc.args, (const char **)argv + 2);
if (!strcmp(argv[1], "start-command-ENOENT")) {
if (start_command(&proc) < 0 && errno == ENOENT)
@@ -408,7 +409,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,
--git a/t/t7006-pager.sh b/t/t7006-pager.sh
index 0e7cf75435e..d75a874b5be 100755
--- a/t/t7006-pager.sh
+++ b/t/t7006-pager.sh
@@ -16,6 +16,10 @@ test_expect_success 'setup' '
test_commit initial
'
+test_expect_success 'non-existent pager does not segfault' '
+ git -c pager.show=INVALID show
+'
+
test_expect_success TTY 'some commands use a pager' '
rm -f paginated.out &&
test_terminal git log &&
diff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c
index 3a0014417cc..bd17ecdc321 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 58d9e430f05..6e429a3fb9e 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 e4acca13d64..2ff9cf70835 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,
--
2.34.0.822.gb876f875f1b
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH 5/5] run-command API: remove "argv" member, always use "args"
2021-11-22 16:04 ` [PATCH 5/5] run-command API: remove "argv" member, always use "args" Ævar Arnfjörð Bjarmason
@ 2021-11-22 17:32 ` Jeff King
2021-11-22 18:19 ` Ævar Arnfjörð Bjarmason
0 siblings, 1 reply; 71+ messages in thread
From: Jeff King @ 2021-11-22 17:32 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Enzo Matsumiya
On Mon, Nov 22, 2021 at 05:04:07PM +0100, Ævar Arnfjörð Bjarmason wrote:
> 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.
Yeah, agreed most of this needs to come in a bundle. But at least few spots
could be split out into the earlier patches:
> builtin/worktree.c | 2 --
> t/helper/test-run-command.c | 10 +++++---
as they are just users of the API.
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index 3c4fb862234..b5519f92a19 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> [...]
> @@ -396,7 +396,8 @@ int cmd__run_command(int argc, const char **argv)
> }
> if (argc < 3)
> return 1;
> - proc.argv = (const char **)argv + 2;
> + strvec_clear(&proc.args);
> + strvec_pushv(&proc.args, (const char **)argv + 2);
>
> if (!strcmp(argv[1], "start-command-ENOENT")) {
> if (start_command(&proc) < 0 && errno == ENOENT)
> @@ -408,7 +409,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,
These two in particular are tricky. This second strvec_clear() is
necessary because we are overwriting what was put into proc.args by the
earlier strvec_pushv(). I think this is one of two interesting ways
these kinds of trivial conversions can fail:
- somebody is relying on the fact that "argv = whatever" is an
assignment, not an append (which is the case here)
- somebody is relying on the fact that assignment of the pointer is
shallow, whereas strvec_push() is doing a deep copy. So converting:
cp.argv = argv;
argv[1] = "foo";
to:
strvec_pushv(&cp.args, argv);
argv[1] = "foo";
is not correct. We wouldn't use the updated "foo". I didn't notice
any cases like this during my rough own rough conversion, but they
could be lurking.
The strvec_clear() in the first hunk above is also not necessary (nobody
has written anything before it), but I don't mind it for consistency /
being defensive.
> @@ -902,8 +900,9 @@ int start_command(struct child_process *cmd)
> #else
> {
> int fhin = 0, fhout = 1, fherr = 2;
> - const char **sargv = cmd->argv;
> + const char **sargv = strvec_detach(&cmd->args);
> struct strvec nargv = STRVEC_INIT;
> + const char **temp_argv = NULL;
I wondered about detaching here, because other parts of the code
(finish_command(), tracing, etc) will be looking at cmd->args.v[0] for
the command name.
But we eventually overwrite it with munged results:
> @@ -929,20 +928,26 @@ int start_command(struct child_process *cmd)
> fhout = dup(cmd->out);
>
> if (cmd->git_cmd)
> - cmd->argv = prepare_git_cmd(&nargv, cmd->argv);
> + temp_argv = prepare_git_cmd(&nargv, sargv);
> else if (cmd->use_shell)
> - cmd->argv = prepare_shell_cmd(&nargv, cmd->argv);
> + temp_argv = prepare_shell_cmd(&nargv, sargv);
> + else
> + temp_argv = sargv;
> + if (!temp_argv)
> + BUG("should have some cmd->args to set");
> + strvec_pushv(&cmd->args, temp_argv);
> + strvec_clear(&nargv);
So we have to do some replacement. I think the memory management gets
confusing here, though.
At this point "temp_argv" might point to nargv.v (in which case it is a
dangling pointer after we clear nargv) or to the original sargv (in
which case it is memory owned by us that must be freed). The former is
OK, because we never look at it again. And the latter we eventually
reattach into &cmd->args, but:
> - strvec_clear(&nargv);
> - cmd->argv = sargv;
> + strvec_pushv(&cmd->args, sargv);;
> + free(sargv);
I think we still have all the entries we pushed into cmd->args from
temp_argv earlier. So we'd need to strvec_clear() it before pushing
sargv again.
And then the free(sargv) is too shallow. It's just freeing the outer
array, but sargv[0], etc, are leaked.
I think what you really want is the equivalent of strvec_attach(). We
don't have that, but it's basically just:
void strvec_attach(struct strvec *s, const char **v)
{
/*
* this is convenient for our caller, but if we wanted to find
* potential misuses, we could also BUG() if s.nr > 0
*/
strvec_clear(&s);
/* take over ownership */
s->v = v;
/* v must be NULL-terminated; count up to set number */
s->nr = 0;
for (; *v; v++)
s->nr++;
/*
* we don't know how big the original allocation was, so we can
* assume only nr. But add 1 to account for the NULL.
*/
s->alloc = s->nr + 1;
}
I do think this whole thing would be easier to read if we left cmd->argv
intact, and just operated on a separate argv strvec. That's what the
non-Windows side does. But that distinction is nothing new in your
patch, and I'm not sure if there is a reason that the Windows code does
it differently.
-Peff
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH 5/5] run-command API: remove "argv" member, always use "args"
2021-11-22 17:32 ` Jeff King
@ 2021-11-22 18:19 ` Ævar Arnfjörð Bjarmason
2021-11-22 18:47 ` Jeff King
0 siblings, 1 reply; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 18:19 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Enzo Matsumiya
On Mon, Nov 22 2021, Jeff King wrote:
> On Mon, Nov 22, 2021 at 05:04:07PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> 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.
>
> Yeah, agreed most of this needs to come in a bundle. But at least few spots
> could be split out into the earlier patches:
>
>> builtin/worktree.c | 2 --
>> t/helper/test-run-command.c | 10 +++++---
>
> as they are just users of the API.
Will split these out, I had test-run-command.c split initially, but
squashed it locally, which did away with having to explain leaving this
area untested as an intermediate step. But will split again.
>> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
>> index 3c4fb862234..b5519f92a19 100644
>> --- a/t/helper/test-run-command.c
>> +++ b/t/helper/test-run-command.c
>> [...]
>> @@ -396,7 +396,8 @@ int cmd__run_command(int argc, const char **argv)
>> }
>> if (argc < 3)
>> return 1;
>> - proc.argv = (const char **)argv + 2;
>> + strvec_clear(&proc.args);
>> + strvec_pushv(&proc.args, (const char **)argv + 2);
>>
>> if (!strcmp(argv[1], "start-command-ENOENT")) {
>> if (start_command(&proc) < 0 && errno == ENOENT)
>> @@ -408,7 +409,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,
>
> These two in particular are tricky. This second strvec_clear() is
> necessary because we are overwriting what was put into proc.args by the
> earlier strvec_pushv(). I think this is one of two interesting ways
> these kinds of trivial conversions can fail:
>
> - somebody is relying on the fact that "argv = whatever" is an
> assignment, not an append (which is the case here)
>
> - somebody is relying on the fact that assignment of the pointer is
> shallow, whereas strvec_push() is doing a deep copy. So converting:
>
> cp.argv = argv;
> argv[1] = "foo";
>
> to:
>
> strvec_pushv(&cp.args, argv);
> argv[1] = "foo";
>
> is not correct. We wouldn't use the updated "foo". I didn't notice
> any cases like this during my rough own rough conversion, but they
> could be lurking.
Yes, I tried to eyeball all the code involved & see if there were any. I
could amend this to start renaming variables, but that'll be a much
larger change.
> The strvec_clear() in the first hunk above is also not necessary (nobody
> has written anything before it), but I don't mind it for consistency /
> being defensive.
*nod*, will remove.
>> @@ -902,8 +900,9 @@ int start_command(struct child_process *cmd)
>> #else
>> {
>> int fhin = 0, fhout = 1, fherr = 2;
>> - const char **sargv = cmd->argv;
>> + const char **sargv = strvec_detach(&cmd->args);
>> struct strvec nargv = STRVEC_INIT;
>> + const char **temp_argv = NULL;
>
> I wondered about detaching here, because other parts of the code
> (finish_command(), tracing, etc) will be looking at cmd->args.v[0] for
> the command name.
>
> But we eventually overwrite it with munged results:
>
>> @@ -929,20 +928,26 @@ int start_command(struct child_process *cmd)
>> fhout = dup(cmd->out);
>>
>> if (cmd->git_cmd)
>> - cmd->argv = prepare_git_cmd(&nargv, cmd->argv);
>> + temp_argv = prepare_git_cmd(&nargv, sargv);
>> else if (cmd->use_shell)
>> - cmd->argv = prepare_shell_cmd(&nargv, cmd->argv);
>> + temp_argv = prepare_shell_cmd(&nargv, sargv);
>> + else
>> + temp_argv = sargv;
>> + if (!temp_argv)
>> + BUG("should have some cmd->args to set");
>> + strvec_pushv(&cmd->args, temp_argv);
>> + strvec_clear(&nargv);
>
> So we have to do some replacement. I think the memory management gets
> confusing here, though.
>
> At this point "temp_argv" might point to nargv.v (in which case it is a
> dangling pointer after we clear nargv) or to the original sargv (in
> which case it is memory owned by us that must be freed). The former is
> OK, because we never look at it again. And the latter we eventually
> reattach into &cmd->args, but:
>
>> - strvec_clear(&nargv);
>> - cmd->argv = sargv;
>> + strvec_pushv(&cmd->args, sargv);;
>> + free(sargv);
>
> I think we still have all the entries we pushed into cmd->args from
> temp_argv earlier. So we'd need to strvec_clear() it before pushing
> sargv again.
>
> And then the free(sargv) is too shallow. It's just freeing the outer
> array, but sargv[0], etc, are leaked.
I'll try to fix that, but updates to this part are very painful, since
I've needed to wait 30m for each change in the Windows CI.
> I think what you really want is the equivalent of strvec_attach(). We
> don't have that, but it's basically just:
>
> void strvec_attach(struct strvec *s, const char **v)
> {
> /*
> * this is convenient for our caller, but if we wanted to find
> * potential misuses, we could also BUG() if s.nr > 0
> */
> strvec_clear(&s);
>
> /* take over ownership */
> s->v = v;
>
> /* v must be NULL-terminated; count up to set number */
> s->nr = 0;
> for (; *v; v++)
> s->nr++;
> /*
> * we don't know how big the original allocation was, so we can
> * assume only nr. But add 1 to account for the NULL.
> */
> s->alloc = s->nr + 1;
> }
>
> I do think this whole thing would be easier to read if we left cmd->argv
> intact, and just operated on a separate argv strvec. That's what the
> non-Windows side does. But that distinction is nothing new in your
> patch, and I'm not sure if there is a reason that the Windows code does
> it differently.
I did have this with a strvec_attach() locally, but figured I'd get
comments about growing scope & with just one caller.
This version seems to be duplicating things in the existing API though,
I just had the below, which I think works just as well without the
duplication. Perhaps you missed strvec_push_nodup()?
diff --git a/strvec.c b/strvec.c
index 61a76ce6cb9..c10008d792f 100644
--- a/strvec.c
+++ b/strvec.c
@@ -106,3 +106,9 @@ const char **strvec_detach(struct strvec *array)
return ret;
}
}
+
+void strvec_attach(struct strvec *array, const char **items)
+{
+ for (; *items; items++)
+ strvec_push_nodup(array, *items);
+}
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH 5/5] run-command API: remove "argv" member, always use "args"
2021-11-22 18:19 ` Ævar Arnfjörð Bjarmason
@ 2021-11-22 18:47 ` Jeff King
0 siblings, 0 replies; 71+ messages in thread
From: Jeff King @ 2021-11-22 18:47 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Enzo Matsumiya
On Mon, Nov 22, 2021 at 07:19:14PM +0100, Ævar Arnfjörð Bjarmason wrote:
> I did have this with a strvec_attach() locally, but figured I'd get
> comments about growing scope & with just one caller.
I do think I'd rather see us avoiding have to do this attach() by
refactoring the Windows code. But I sympathize with your pain in the
guess-and-wait-for-CI loop with Windows (I have an unrelated series I've
done that with, and it's rather awful).
> This version seems to be duplicating things in the existing API though,
> I just had the below, which I think works just as well without the
> duplication. Perhaps you missed strvec_push_nodup()?
>
> diff --git a/strvec.c b/strvec.c
> index 61a76ce6cb9..c10008d792f 100644
> --- a/strvec.c
> +++ b/strvec.c
> @@ -106,3 +106,9 @@ const char **strvec_detach(struct strvec *array)
> return ret;
> }
> }
> +
> +void strvec_attach(struct strvec *array, const char **items)
> +{
> + for (; *items; items++)
> + strvec_push_nodup(array, *items);
> +}
This isn't taking ownership of "items", though. We've attached the
things it points to, but not the array itself. It would perhaps be OK to
free() it here under the notion that we took ownership of it and it is
ours to do with as we please. Potentially a caller might expect that
we'd continue to use the attached buffer, but any such assumption is
invalid once another strvec_push() is called, since that could replace
the array anyway.
It's also slightly less efficient (it grows a new array unnecessarily),
but I doubt that matters much in practice.
-Peff
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 0/5] run-command API: get rid of "argv"
2021-11-22 16:04 ` [PATCH 0/5] run-command API: get rid of "argv" Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-11-22 16:04 ` [PATCH 5/5] run-command API: remove "argv" member, always use "args" Ævar Arnfjörð Bjarmason
@ 2021-11-22 17:52 ` Jeff King
2021-11-22 18:11 ` Junio C Hamano
2021-11-22 18:26 ` Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
6 siblings, 2 replies; 71+ messages in thread
From: Jeff King @ 2021-11-22 17:52 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: git, Junio C Hamano, Enzo Matsumiya
On Mon, Nov 22, 2021 at 05:04:02PM +0100, Ævar Arnfjörð Bjarmason wrote:
> This series is an alternate but more thorough way to solve the pager
> segfault reported by Enzo Matsumiya[1], and more generally avoids
> similar issues in the future.
>
> That the run-command API exposed two subtly different ways of doing
> the same thing wouldn't only lead to the sort of bug reported in [1],
> but also made memory management around it rather painful. As noted by
> Jeff King in[2]:
>
> I'd like to eventually get rid of the argv interface entirely
> because it has memory-ownership semantics that are easy to get
> wrong.
Yeah, unsurprisingly I'm in favor of this direction (and in fact started
looking at myself before seeing your responses). It's big and complex
enough that I do worry about prepending it in front of the segfault bug
fix being discussed.
> As noted in 5/5 we've still got a similar issue with "env" and
> "env_array". I've got a follow-up series that similarly removes "env"
> which we can do at some point (it's much smaller than this one), but
> for now let's focus on "argv".
I think we should probably do both, though I am OK with doing it
separately. There are fewer callers for "env", but I found more
ancillary cleanup necessary (e.g., "const char **" versus "const char
*const *" headaches).
> Ævar Arnfjörð Bjarmason (5):
> archive-tar: use our own cmd.buf in error message
> upload-archive: use regular "struct child_process" pattern
> run-command API users: use strvec_pushv(), not argv assignment
> run-command API users: use strvec_pushl(), not argv construction
> run-command API: remove "argv" member, always use "args"
I left a few comments on individual patches. I had done a rough cut at
this, too. One big difference is that I used the opportunity to clean up
some ugly and error-prone uses of argv that are now unnecessary. For
instance:
diff --git a/builtin/notes.c b/builtin/notes.c
index 2b2bac43f3..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" */
- strvec_pushv(&show.args, 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;
The show_args variable is error-prone in two ways:
- the magic number "5" must be in sync with the rest of the array. In
this case it's superfluous and could just be removed, but I'll give
a related example below.
- we have to remember to include the trailing NULL. We have to for
pushl(), too, but in that case the compiler will warn us when we
omit it.
Here's another one:
@@ -943,23 +941,22 @@ static int run_receive_hook(struct command *commands,
static int run_update_hook(struct command *cmd)
{
- const char *argv[5];
+ const char *hook_cmd;
struct child_process proc = CHILD_PROCESS_INIT;
int code;
- argv[0] = find_hook("update");
- if (!argv[0])
+ hook_cmd = find_hook("update");
+ if (!hook_cmd)
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_cmd);
+ 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;
- strvec_pushv(&proc.args, argv);
proc.trace2_hook_name = "update";
In this case the magic "5" really is important, and we get rid of it
(and again don't need to worry about the terminating NULL).
I'm on the fence on how important it is to do these cleanups. IMHO they
are half of what really sells the change in the first place (since the
other bug can pretty easily be fixed without it).
But maybe it is piling too much onto what is already a pretty big
change. The cleanups could be done individually later.
diff --git a/daemon.c b/daemon.c
index cc278077d2..4a000ee4af 100644
--- a/daemon.c
+++ b/daemon.c
@@ -329,10 +329,15 @@ static int run_access_hook(struct daemon_service *service, const char *dir,
char *eol;
int seen_errors = 0;
+ 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;
- strvec_pushl(&child.args, access_hook, service->name, path,
- hi->hostname.buf, get_canon_hostname(hi),
- get_ip_address(hi), hi->tcp_port.buf, NULL);
child.no_stdin = 1;
child.no_stderr = 1;
child.out = -1;
I had other changes from yours like this. This is purely cosmetic, and I
could see arguments either way. I find the one-per-line version a bit
easier to read. Even though it repeats child.args over and over, it's
easy to look past since it's all aligned.
I'm OK calling that bike-shedding, but I offer it mostly in case you
didn't try it the other way and actually like my color. ;)
-Peff
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH 0/5] run-command API: get rid of "argv"
2021-11-22 17:52 ` [PATCH 0/5] run-command API: get rid of "argv" Jeff King
@ 2021-11-22 18:11 ` Junio C Hamano
2021-11-22 18:33 ` Ævar Arnfjörð Bjarmason
2021-11-22 18:26 ` Ævar Arnfjörð Bjarmason
1 sibling, 1 reply; 71+ messages in thread
From: Junio C Hamano @ 2021-11-22 18:11 UTC (permalink / raw)
To: Jeff King; +Cc: Ævar Arnfjörð Bjarmason, git, Enzo Matsumiya
Jeff King <peff@peff.net> writes:
> I'm on the fence on how important it is to do these cleanups. IMHO they
> are half of what really sells the change in the first place (since the
> other bug can pretty easily be fixed without it).
Yes. I actually think these have much better value for their
complexity, compared to the other "half" of the topic ;-)
> But maybe it is piling too much onto what is already a pretty big
> change. The cleanups could be done individually later.
I am OK with that, too. But I do agree that the series is too big
to sit in front of a fix for a bug, for which a much simpler and
focused approach has already been discussed, to block it.
Thanks.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 0/5] run-command API: get rid of "argv"
2021-11-22 18:11 ` Junio C Hamano
@ 2021-11-22 18:33 ` Ævar Arnfjörð Bjarmason
2021-11-22 18:49 ` Jeff King
0 siblings, 1 reply; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 18:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Enzo Matsumiya
On Mon, Nov 22 2021, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
>> I'm on the fence on how important it is to do these cleanups. IMHO they
>> are half of what really sells the change in the first place (since the
>> other bug can pretty easily be fixed without it).
>
> Yes. I actually think these have much better value for their
> complexity, compared to the other "half" of the topic ;-)
>
>> But maybe it is piling too much onto what is already a pretty big
>> change. The cleanups could be done individually later.
>
> I am OK with that, too. But I do agree that the series is too big
> to sit in front of a fix for a bug, for which a much simpler and
> focused approach has already been discussed, to block it.
I'm happy to re-roll this on top of the more narrow fix. FWIW the bug's
there since at least v2.30.0 (just tested that, probably much older), so
in that sense there's no urgency either way.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 0/5] run-command API: get rid of "argv"
2021-11-22 18:33 ` Ævar Arnfjörð Bjarmason
@ 2021-11-22 18:49 ` Jeff King
0 siblings, 0 replies; 71+ messages in thread
From: Jeff King @ 2021-11-22 18:49 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Junio C Hamano, git, Enzo Matsumiya
On Mon, Nov 22, 2021 at 07:33:10PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
> On Mon, Nov 22 2021, Junio C Hamano wrote:
>
> > Jeff King <peff@peff.net> writes:
> >
> >> I'm on the fence on how important it is to do these cleanups. IMHO they
> >> are half of what really sells the change in the first place (since the
> >> other bug can pretty easily be fixed without it).
> >
> > Yes. I actually think these have much better value for their
> > complexity, compared to the other "half" of the topic ;-)
> >
> >> But maybe it is piling too much onto what is already a pretty big
> >> change. The cleanups could be done individually later.
> >
> > I am OK with that, too. But I do agree that the series is too big
> > to sit in front of a fix for a bug, for which a much simpler and
> > focused approach has already been discussed, to block it.
>
> I'm happy to re-roll this on top of the more narrow fix. FWIW the bug's
> there since at least v2.30.0 (just tested that, probably much older), so
> in that sense there's no urgency either way.
I suspect it has been a problem since 43b0190224 (pager: lose a separate
argv[], 2016-02-16) in v2.7.3. So yeah, definitely not urgent, but I do
think we can get out a 2-line minimal fix to start with.
-Peff
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH 0/5] run-command API: get rid of "argv"
2021-11-22 17:52 ` [PATCH 0/5] run-command API: get rid of "argv" Jeff King
2021-11-22 18:11 ` Junio C Hamano
@ 2021-11-22 18:26 ` Ævar Arnfjörð Bjarmason
1 sibling, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-22 18:26 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Enzo Matsumiya
On Mon, Nov 22 2021, Jeff King wrote:
> On Mon, Nov 22, 2021 at 05:04:02PM +0100, Ævar Arnfjörð Bjarmason wrote:
>
>> This series is an alternate but more thorough way to solve the pager
>> segfault reported by Enzo Matsumiya[1], and more generally avoids
>> similar issues in the future.
>>
>> That the run-command API exposed two subtly different ways of doing
>> the same thing wouldn't only lead to the sort of bug reported in [1],
>> but also made memory management around it rather painful. As noted by
>> Jeff King in[2]:
>>
>> I'd like to eventually get rid of the argv interface entirely
>> because it has memory-ownership semantics that are easy to get
>> wrong.
>
> Yeah, unsurprisingly I'm in favor of this direction (and in fact started
> looking at myself before seeing your responses). It's big and complex
> enough that I do worry about prepending it in front of the segfault bug
> fix being discussed.
>
>> As noted in 5/5 we've still got a similar issue with "env" and
>> "env_array". I've got a follow-up series that similarly removes "env"
>> which we can do at some point (it's much smaller than this one), but
>> for now let's focus on "argv".
>
> I think we should probably do both, though I am OK with doing it
> separately. There are fewer callers for "env", but I found more
> ancillary cleanup necessary (e.g., "const char **" versus "const char
> *const *" headaches).
>
>> Ævar Arnfjörð Bjarmason (5):
>> archive-tar: use our own cmd.buf in error message
>> upload-archive: use regular "struct child_process" pattern
>> run-command API users: use strvec_pushv(), not argv assignment
>> run-command API users: use strvec_pushl(), not argv construction
>> run-command API: remove "argv" member, always use "args"
>
> I left a few comments on individual patches. I had done a rough cut at
> this, too. One big difference is that I used the opportunity to clean up
> some ugly and error-prone uses of argv that are now unnecessary. For
> instance:
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 2b2bac43f3..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" */
> - strvec_pushv(&show.args, 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;
>
> The show_args variable is error-prone in two ways:
>
> - the magic number "5" must be in sync with the rest of the array. In
> this case it's superfluous and could just be removed, but I'll give
> a related example below.
>
> - we have to remember to include the trailing NULL. We have to for
> pushl(), too, but in that case the compiler will warn us when we
> omit it.
>
> Here's another one:
>
> @@ -943,23 +941,22 @@ static int run_receive_hook(struct command *commands,
>
> static int run_update_hook(struct command *cmd)
> {
> - const char *argv[5];
> + const char *hook_cmd;
> struct child_process proc = CHILD_PROCESS_INIT;
> int code;
>
> - argv[0] = find_hook("update");
> - if (!argv[0])
> + hook_cmd = find_hook("update");
> + if (!hook_cmd)
> 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_cmd);
> + 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;
> - strvec_pushv(&proc.args, argv);
> proc.trace2_hook_name = "update";
>
> In this case the magic "5" really is important, and we get rid of it
> (and again don't need to worry about the terminating NULL).
>
> I'm on the fence on how important it is to do these cleanups. IMHO they
> are half of what really sells the change in the first place (since the
> other bug can pretty easily be fixed without it).
>
> But maybe it is piling too much onto what is already a pretty big
> change. The cleanups could be done individually later.
Yeah, those are nice. I did do most/all those initially myself, but
ended up ejecting them in anticipation of getting comments about runaway
refactoring, as they're not strictly necessary. But I can include them
again if you/Junio would like...
> diff --git a/daemon.c b/daemon.c
> index cc278077d2..4a000ee4af 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -329,10 +329,15 @@ static int run_access_hook(struct daemon_service *service, const char *dir,
> char *eol;
> int seen_errors = 0;
>
> + 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;
> - strvec_pushl(&child.args, access_hook, service->name, path,
> - hi->hostname.buf, get_canon_hostname(hi),
> - get_ip_address(hi), hi->tcp_port.buf, NULL);
> child.no_stdin = 1;
> child.no_stderr = 1;
> child.out = -1;
>
> I had other changes from yours like this. This is purely cosmetic, and I
> could see arguments either way. I find the one-per-line version a bit
> easier to read. Even though it repeats child.args over and over, it's
> easy to look past since it's all aligned.
>
> I'm OK calling that bike-shedding, but I offer it mostly in case you
> didn't try it the other way and actually like my color. ;)
I do like it better :) It's another thing I did like that initiall, but
ended up moving to strvec_pushl(). IIRC because I got the opposite
request on a recent bundle.c topic of mine (now landed). I.e. it used
multiple aligned strvec_push() initailly, and it was suggested to use
strvec_pushl() instead...
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH v2 0/9] run-command API: get rid of "argv" and "env"
2021-11-22 16:04 ` [PATCH 0/5] run-command API: get rid of "argv" Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-11-22 17:52 ` [PATCH 0/5] run-command API: get rid of "argv" Jeff King
@ 2021-11-23 12:06 ` Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv Ævar Arnfjörð Bjarmason
` (9 more replies)
6 siblings, 10 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
That the run-command API exposed two subtly different ways of doing
the same thing wouldn't only lead to the sort of bug reported in [1],
but also made memory management around it rather painful. As noted by
Jeff King in[2]:
I'd like to eventually get rid of the argv interface entirely
because it has memory-ownership semantics that are easy to get
wrong.
As noted in v1[1] there's probably never going to be a perfect time to
do this as a change to this widely used API will probably impact
things in-flight.
Now seems to be a particularly good time though, merging this to
"seen" only conflicts with my ab/config-based-hooks-2 in
builtin/worktree.c. The resolution is trivial (just use the new hook
API added in that topic).
This re-rolled v2 also had a semantic conflict with that topic's use
of the "env" member, which is solved with a just-submitted update of
mine[2].
Changes since v2:
* I ejected the test for the segfault this topic solves, I understood
that Junio wanted to fast-track a more limited fix based on Enzo
Matsumiya's [3]. In any case the two won't semantically conflict as
far as the test goes, this larger change also fixes the segfault.
* I'm now removing the "env" member in addition to "argv". The two
patches at the end are new. Doing so was a much smaller change than
"argv".
* I changed all the minimal refactorings to get rid of "env" to
larger changes to get rid of any function-local "argv" arrays with
a hardcoded size.
* As a result, the mid-series is now structured in such a way as to
group changes where we can trivially prove that we're not
introducing a logic error (since the "argv" variable went away),
and those cases where we still keep the "argv" (usually from an
existing "struct strvec").
* The whole Windows-specific complexity in v1 is gone. I didn't
really need to change the behavior of that code, now it's just a
simple search-replacement of s/cmd->argv/cmd->args.v/. It means we
clobber the "env_array" member as we were doing with the "env"
before, but since that's nothing new it doesn't require careful
review of change semantics.
1. https://lore.kernel.org/git/cover-0.5-00000000000-20211122T153605Z-avarab@gmail.com/
2. https://lore.kernel.org/git/cover-v5-00.17-00000000000-20211123T114206Z-avarab@gmail.com/
3. https://lore.kernel.org/git/20211120194048.12125-1-ematsumiya@suse.de/
Ævar Arnfjörð Bjarmason (9):
worktree: remove redundant NULL-ing of "cp.argv
upload-archive: use regular "struct child_process" pattern
run-command API users: use strvec_pushv(), not argv assignment
run-command tests: use strvec_pushv(), not argv assignment
run-command API users: use strvec_pushl(), not argv construction
run-command API users: use strvec_push(), not argv construction
run-command API: remove "argv" member, always use "args"
difftool: use "env_array" to simplify memory management
run-command API: remove "env" member, always use "env_array"
add-patch.c | 4 +-
archive-tar.c | 9 ++---
builtin/add.c | 7 +---
builtin/difftool.c | 14 ++-----
builtin/fsck.c | 12 ++----
builtin/help.c | 3 +-
builtin/merge.c | 3 +-
builtin/notes.c | 5 +--
builtin/receive-pack.c | 80 ++++++++++++++-----------------------
builtin/replace.c | 3 +-
builtin/upload-archive.c | 5 ++-
builtin/worktree.c | 8 ++--
connected.c | 3 +-
daemon.c | 20 ++++------
diff.c | 8 +---
editor.c | 8 ++--
http-backend.c | 2 +-
http.c | 5 ++-
object-file.c | 2 +-
prompt.c | 7 +---
remote-curl.c | 2 +-
run-command.c | 62 +++++++++++++---------------
run-command.h | 54 ++++++++++++-------------
sequencer.c | 10 ++---
sub-process.c | 2 +-
t/helper/test-run-command.c | 9 +++--
t/helper/test-subprocess.c | 2 +-
trace2/tr2_tgt_event.c | 2 +-
trace2/tr2_tgt_normal.c | 2 +-
trace2/tr2_tgt_perf.c | 4 +-
trailer.c | 2 +-
transport.c | 11 +++--
upload-pack.c | 5 +--
33 files changed, 154 insertions(+), 221 deletions(-)
Range-diff against v1:
-: ----------- > 1: 9cc220ce5a3 worktree: remove redundant NULL-ing of "cp.argv
2: a411098699d ! 2: bfa65e5afd7 upload-archive: use regular "struct child_process" pattern
@@ builtin/upload-archive.c: static ssize_t process_input(int child_fd, int band)
if (argc == 2 && !strcmp(argv[1], "-h"))
usage(upload_archive_usage);
@@ builtin/upload-archive.c: int cmd_upload_archive(int argc, const char **argv, const char *prefix)
- argv[0] = "upload-archive--writer";
+ * 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");
-+ if (argc > 1)
-+ strvec_pushv(&writer.args, &argv[1]);
++ strvec_pushv(&writer.args, argv + 1);
if (start_command(&writer)) {
int err = errno;
packet_write_fmt(1, "NACK unable to spawn subprocess\n");
3: fd6c4c960ba ! 3: 61e4eb8e173 run-command API users: use strvec_pushv(), not argv assignment
@@ Commit message
run-command API users: use strvec_pushv(), not argv assignment
Migrate those run-command API users that assign directly to the "argv"
- member to use a strvec_pushv() of "args" instead, but exclude those
- cases where we can't easily get rid of the construction of the "argv"
- variable being given to the resulting "strvec_pushv()".
+ member to use a strvec_pushv() of "args" instead.
- This is in preparation for getting rid of the "argv" member from the
- run-command API itself.
+ 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 <avarab@gmail.com>
@@ add-patch.c: static int parse_diff(struct add_p_state *s, const struct pathspec
res = capture_command(&colored_cp, colored, 0);
strvec_clear(&args);
- ## builtin/notes.c ##
-@@ builtin/notes.c: static void write_commented_object(int fd, const struct object_id *object)
- struct strbuf cbuf = STRBUF_INIT;
-
- /* Invoke "git show --stat --no-notes $object" */
-- show.argv = show_args;
-+ strvec_pushv(&show.args, show_args);
- show.no_stdin = 1;
- show.out = -1;
- show.err = 0;
-
- ## builtin/receive-pack.c ##
-@@ builtin/receive-pack.c: static int run_and_feed_hook(const char *hook_name, feed_fn feed,
-
- argv[1] = NULL;
-
-- proc.argv = argv;
-+ strvec_pushv(&proc.args, argv);
- proc.in = -1;
- proc.stdout_to_stderr = 1;
- proc.trace2_hook_name = hook_name;
-@@ builtin/receive-pack.c: static int run_update_hook(struct command *cmd)
- proc.no_stdin = 1;
- proc.stdout_to_stderr = 1;
- proc.err = use_sideband ? -1 : 0;
-- proc.argv = argv;
-+ strvec_pushv(&proc.args, argv);
- proc.trace2_hook_name = "update";
-
- code = start_command(&proc);
-@@ builtin/receive-pack.c: static int run_proc_receive_hook(struct command *commands,
- }
- argv[1] = NULL;
-
-- proc.argv = argv;
-+ strvec_pushv(&proc.args, argv);
- proc.in = -1;
- proc.out = -1;
- proc.trace2_hook_name = "proc-receive";
-@@ builtin/receive-pack.c: static const char *push_to_deploy(unsigned char *sha1,
- };
- struct child_process child = CHILD_PROCESS_INIT;
-
-- child.argv = update_refresh;
-+ strvec_pushv(&child.args, update_refresh);
- child.env = env->v;
- child.dir = work_tree;
- child.no_stdin = 1;
-@@ builtin/receive-pack.c: 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_pushv(&child.args, diff_files);
- child.env = env->v;
- child.dir = work_tree;
- child.no_stdin = 1;
-@@ builtin/receive-pack.c: static const char *push_to_deploy(unsigned char *sha1,
- diff_index[4] = head_has_history() ? "HEAD" : empty_tree_oid_hex();
-
- child_process_init(&child);
-- child.argv = diff_index;
-+ strvec_pushv(&child.args, diff_index);
- child.env = env->v;
- child.no_stdin = 1;
- child.no_stdout = 1;
-@@ builtin/receive-pack.c: static const char *push_to_deploy(unsigned char *sha1,
-
- read_tree[3] = hash_to_hex(sha1);
- child_process_init(&child);
-- child.argv = read_tree;
-+ strvec_pushv(&child.args, read_tree);
- child.env = env->v;
- child.dir = work_tree;
- child.no_stdin = 1;
-@@ builtin/receive-pack.c: int cmd_receive_pack(int argc, const char **argv, const char *prefix)
- 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_pushv(&proc.args, argv_gc_auto);
-
- if (!start_command(&proc)) {
- if (use_sideband)
-
## daemon.c ##
@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addrlen)
#endif
@@ daemon.c: static void handle(int incoming, struct sockaddr *addr, socklen_t addr
cld.out = dup(incoming);
- ## editor.c ##
-@@ editor.c: static int launch_specified_editor(const char *editor, const char *path,
- strbuf_realpath(&realpath, path, 1);
- args[1] = realpath.buf;
-
-- p.argv = args;
-+ strvec_pushv(&p.args, args);
- p.env = env;
- p.use_shell = 1;
- p.trace2_child_class = "editor";
-
## http-backend.c ##
@@ http-backend.c: static void run_service(const char **argv, int buffer_input)
strvec_pushf(&cld.env_array,
@@ run-command.c: int run_command_v_opt_cd_env_tr2(const char **argv, int opt, cons
cmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;
cmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;
- ## sequencer.c ##
-@@ sequencer.c: static int run_rewrite_hook(const struct object_id *oldoid,
- argv[1] = "amend";
- argv[2] = NULL;
-
-- proc.argv = argv;
-+ strvec_pushv(&proc.args, argv);
- proc.in = -1;
- proc.stdout_to_stderr = 1;
- proc.trace2_hook_name = "post-rewrite";
-
## t/helper/test-subprocess.c ##
@@ t/helper/test-subprocess.c: int cmd__subprocess(int argc, const char **argv)
argv++;
@@ t/helper/test-subprocess.c: int cmd__subprocess(int argc, const char **argv)
+ strvec_pushv(&cp.args, (const char **)argv + 1);
return run_command(&cp);
}
-
- ## transport.c ##
-@@ transport.c: static int run_pre_push_hook(struct transport *transport,
- argv[2] = transport->url;
- argv[3] = NULL;
-
-- proc.argv = argv;
-+ strvec_pushv(&proc.args, argv);
- proc.in = -1;
- proc.trace2_hook_name = "pre-push";
-
-: ----------- > 4: a2ee10e214c run-command tests: use strvec_pushv(), not argv assignment
4: 61751f55ff2 ! 5: 2b446606eb9 run-command API users: use strvec_pushl(), not argv construction
@@ Metadata
## Commit message ##
run-command API users: use strvec_pushl(), not argv construction
- Migrate those run-command API users that assign directly to the "argv"
- member to use a strvec_pushl() of a list instead, this gets rid of the
- intermediate "const char *args[]" these callers were using.
+ 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 is in preparation for getting rid of the "argv" member from the
- run-command API itself.
+ 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.
- Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+ 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".
- ## archive-tar.c ##
-@@ archive-tar.c: 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)
-@@ archive-tar.c: 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;
-
+ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## builtin/add.c ##
@@ builtin/add.c: int interactive_add(const char **argv, const char *prefix, int patch)
@@ builtin/add.c: static int edit_patch(int argc, const char **argv, const char *pr
child.git_cmd = 1;
- child.argv = apply_argv;
-+ strvec_pushl(&child.args, "apply", "--recount", "--cached", file, NULL);
++ strvec_pushl(&child.args, "apply", "--recount", "--cached", file,
++ NULL);
if (run_command(&child))
die(_("Could not apply '%s'"), file);
@@ builtin/merge.c: static int save_state(struct object_id *stash)
cp.git_cmd = 1;
+ ## builtin/notes.c ##
+@@ builtin/notes.c: 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;
+
+ ## builtin/receive-pack.c ##
+@@ builtin/receive-pack.c: 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;
+@@ builtin/receive-pack.c: 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;
+@@ builtin/receive-pack.c: 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;
+@@ builtin/receive-pack.c: 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;
+@@ builtin/receive-pack.c: 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)
+
## builtin/replace.c ##
@@ builtin/replace.c: static int import_object(struct object_id *oid, enum object_type type,
return error_errno(_("unable to open %s for reading"), filename);
@@ builtin/replace.c: static int import_object(struct object_id *oid, enum object_t
cmd.in = fd;
cmd.out = -1;
- ## daemon.c ##
-@@ daemon.c: 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;
--
- child.use_shell = 1;
-- child.argv = argv;
-+ strvec_pushl(&child.args, access_hook, service->name, path,
-+ hi->hostname.buf, get_canon_hostname(hi),
-+ get_ip_address(hi), hi->tcp_port.buf, NULL);
- child.no_stdin = 1;
- child.no_stderr = 1;
- child.out = -1;
+ ## editor.c ##
+@@ editor.c: 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);
+@@ editor.c: 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.c ##
-@@ diff.c: static char *run_textconv(struct repository *r,
- size_t *outsize)
+ ## sequencer.c ##
+@@ sequencer.c: static int run_rewrite_hook(const struct object_id *oldoid,
+ const struct object_id *newoid)
{
- struct diff_tempfile *temp;
+ struct child_process proc = CHILD_PROCESS_INIT;
- 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;
-
- child.use_shell = 1;
-- child.argv = argv;
-+ strvec_pushl(&child.args, pgm, temp->name, NULL);
- child.out = -1;
- if (start_command(&child)) {
- remove_tempfile();
-
- ## prompt.c ##
-@@
- 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;
--
-- pass.argv = args;
-+ strvec_pushl(&pass.args, cmd, prompt, NULL);
- pass.out = -1;
+ int code;
+ struct strbuf sb = STRBUF_INIT;
++ const char *hook_path = find_hook("post-rewrite");
- if (start_command(&pass))
+- 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";
## upload-pack.c ##
@@ upload-pack.c: static int do_reachable_revlist(struct child_process *cmd,
1: 1d8cab554bb ! 6: fad420dc563 archive-tar: use our own cmd.buf in error message
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- archive-tar: use our own cmd.buf in error message
+ run-command API users: use strvec_push(), not argv construction
- Use the "cmd.buf" we just created in this function, instead of the
- argv[0], which we assigned "cmd.buf" for. This is in preparation for
- getting rid of the use of "argv" in this function.
+ 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 <avarab@gmail.com>
## archive-tar.c ##
@@ archive-tar.c: 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)
+@@ archive-tar.c: 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)
@@ archive-tar.c: static int write_tar_filter_archive(const struct archiver *ar,
strbuf_release(&cmd);
return r;
+
+ ## builtin/receive-pack.c ##
+@@ builtin/receive-pack.c: 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;
+@@ builtin/receive-pack.c: 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);
+@@ builtin/receive-pack.c: 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";
+
+ ## daemon.c ##
+@@ daemon.c: 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.c ##
+@@ diff.c: 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();
+
+ ## prompt.c ##
+@@
+ 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))
+
+ ## transport.c ##
+@@ transport.c: 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";
+
5: ea1011f7473 ! 7: 67ab5114ed7 run-command API: remove "argv" member, always use "args"
@@ Commit message
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. A test being
- added here would segfault before this change.
+ 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
@@ Commit message
change all parts of run-command.[ch] itself, as well as the trace2
logging at the same time.
- We can also similarly follow-up and remove "env" member in favor of
- "env_array", which is a much smaller change than this series removing
- "argv", but let's stop at "argv" for now. We can always do "env" as a
- follow-up.
-
- I have not carefully tested the Windows-specific code in
- start_command() as I don't have a Windows system, but it seems to pass
- CI + runs the tests there.
+ 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/
- Reported-by: Enzo Matsumiya <ematsumiya@suse.de>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- ## builtin/worktree.c ##
-@@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname,
- goto done;
-
- if (opts->checkout) {
-- cp.argv = NULL;
- strvec_clear(&cp.args);
- strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
- if (opts->quiet)
-@@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname,
- 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()),
-
## run-command.c ##
@@ run-command.c: static void child_err_spew(struct child_process *cmd, struct child_err *cerr)
switch (cerr->err) {
@@ run-command.c: int start_command(struct child_process *cmd)
{
int fhin = 0, fhout = 1, fherr = 2;
- const char **sargv = cmd->argv;
-+ const char **sargv = strvec_detach(&cmd->args);
++ const char **sargv = cmd->args.v;
struct strvec nargv = STRVEC_INIT;
-+ const char **temp_argv = NULL;
if (cmd->no_stdin)
- fhin = open("/dev/null", O_RDWR);
@@ run-command.c: int start_command(struct child_process *cmd)
fhout = dup(cmd->out);
if (cmd->git_cmd)
- cmd->argv = prepare_git_cmd(&nargv, cmd->argv);
-+ temp_argv = prepare_git_cmd(&nargv, sargv);
++ cmd->args.v = prepare_git_cmd(&nargv, sargv);
else if (cmd->use_shell)
- cmd->argv = prepare_shell_cmd(&nargv, cmd->argv);
-+ temp_argv = prepare_shell_cmd(&nargv, sargv);
-+ else
-+ temp_argv = sargv;
-+ if (!temp_argv)
-+ BUG("should have some cmd->args to set");
-+ strvec_pushv(&cmd->args, temp_argv);
-+ strvec_clear(&nargv);
++ 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,
@@ run-command.c: int start_command(struct child_process *cmd)
if (cmd->clean_on_exit && cmd->pid >= 0)
mark_child_for_cleanup(cmd->pid, cmd);
-- strvec_clear(&nargv);
+ strvec_clear(&nargv);
- cmd->argv = sargv;
-+ strvec_pushv(&cmd->args, sargv);;
-+ free(sargv);
++ cmd->args.v = sargv;
if (fhin != 0)
close(fhin);
if (fhout != 1)
@@ sub-process.c: static int handshake_capabilities(struct child_process *process,
}
- ## t/helper/test-run-command.c ##
-@@ t/helper/test-run-command.c: 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;
-@@ t/helper/test-run-command.c: 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");
-@@ t/helper/test-run-command.c: int cmd__run_command(int argc, const char **argv)
- }
- if (argc < 3)
- return 1;
-- proc.argv = (const char **)argv + 2;
-+ strvec_clear(&proc.args);
-+ strvec_pushv(&proc.args, (const char **)argv + 2);
-
- if (!strcmp(argv[1], "start-command-ENOENT")) {
- if (start_command(&proc) < 0 && errno == ENOENT)
-@@ t/helper/test-run-command.c: 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,
-
- ## t/t7006-pager.sh ##
-@@ t/t7006-pager.sh: test_expect_success 'setup' '
- test_commit initial
- '
-
-+test_expect_success 'non-existent pager does not segfault' '
-+ git -c pager.show=INVALID show
-+'
-+
- test_expect_success TTY 'some commands use a pager' '
- rm -f paginated.out &&
- test_terminal git log &&
-
## trace2/tr2_tgt_event.c ##
@@ trace2/tr2_tgt_event.c: static void fn_child_start_fl(const char *file, int line,
jw_object_inline_begin_array(&jw, "argv");
-: ----------- > 8: b8387a4a76d difftool: use "env_array" to simplify memory management
-: ----------- > 9: 6bd9f508a3d run-command API: remove "env" member, always use "env_array"
--
2.34.0.831.gd33babec0d1
^ permalink raw reply [flat|nested] 71+ messages in thread* [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
@ 2021-11-23 12:06 ` Ævar Arnfjörð Bjarmason
2021-11-23 15:26 ` Eric Sunshine
2021-11-23 12:06 ` [PATCH v2 2/9] upload-archive: use regular "struct child_process" pattern Ævar Arnfjörð Bjarmason
` (8 subsequent siblings)
9 siblings, 1 reply; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
The clearing of "argv" was added in 7f44e3d1de0 (worktree: make setup
of new HEAD distinct from worktree population, 2015-07-17) when the
"cp" variable wasn't initialized. It hasn't been needed since
542aa25d974 (use CHILD_PROCESS_INIT to initialize automatic variables,
2016-08-05).
Let's remove it to make a later change that gets rid of the "argv"
member from "struct child_process" smaller.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/worktree.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d22ece93e1a..7264a5b5de0 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -355,7 +355,6 @@ static int add_worktree(const char *path, const char *refname,
goto done;
if (opts->checkout) {
- cp.argv = NULL;
strvec_clear(&cp.args);
strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
if (opts->quiet)
@@ -390,7 +389,6 @@ static int add_worktree(const char *path, const char *refname,
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()),
--
2.34.0.831.gd33babec0d1
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv
2021-11-23 12:06 ` [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv Ævar Arnfjörð Bjarmason
@ 2021-11-23 15:26 ` Eric Sunshine
2021-11-24 1:54 ` Junio C Hamano
2021-11-24 5:44 ` Eric Sunshine
0 siblings, 2 replies; 71+ messages in thread
From: Eric Sunshine @ 2021-11-23 15:26 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, Enzo Matsumiya
On Tue, Nov 23, 2021 at 7:08 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> The clearing of "argv" was added in 7f44e3d1de0 (worktree: make setup
> of new HEAD distinct from worktree population, 2015-07-17) when the
> "cp" variable wasn't initialized. It hasn't been needed since
> 542aa25d974 (use CHILD_PROCESS_INIT to initialize automatic variables,
> 2016-08-05).
>
> Let's remove it to make a later change that gets rid of the "argv"
> member from "struct child_process" smaller.
Let me preface with the caveat that (due to lack of time) I haven't
dug into this deeply and I haven't been following changes to the
run_command() machinery closely, so what I say below may be
inaccurate, but...
Although it will be safe to drop these builtin/worktree.c assignments
when the series eventually removes the child_process::argv member, I
think that the commit message is misleading (or outright wrong), and
that this change so early in the series potentially breaks
git-worktree, leaving it in a state where it works only "by accident".
The reason that this code repeatedly clears `cp.argv` is that the `cp`
structure is reused for multiple run_command() invocations (which, in
retrospect, is pretty ugly since worktree.c is too familiar with the
internals of run-command). The reason that `cp.argv` needs to be
cleared between each invocation is due to this code from
start_command():
if (!cmd->argv)
cmd->argv = cmd->args.v;
worktree.c re-populates child_process::args between run_command()
invocations and needs to clear child_process::argv to ensure that the
latter gets re-initialized from child_process::args each time. The use
of CHILD_PROCESS_INIT does not change anything in regard to the
requirement; child_process::argv still needs to be cleared between
run_command() invocations.
As to why this change so early in the series potentially breaks
git-worktree: with the removal of these assignments,
child_process::argv now _always_ points at the _initial_ value of
child_process::args.v even though that vector gets cleared between
run_command() invocations. At best, following this change,
git-worktree is only working "by accident" if the underlying
child_process::args.v doesn't get reallocated between run_command()
invocations. Relying upon this "by accident" behavior feels rather
unsafe.
I think perhaps the simplest thing to do is merely to squash this
patch into the patch which ultimately removes the child_process::argv
member (and the removal of these lines from worktree.c probably
doesn't even need mention in the commit message -- or maybe just a
minor mention).
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index d22ece93e1a..7264a5b5de0 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -355,7 +355,6 @@ static int add_worktree(const char *path, const char *refname,
> goto done;
>
> if (opts->checkout) {
> - cp.argv = NULL;
> strvec_clear(&cp.args);
> strvec_pushl(&cp.args, "reset", "--hard", "--no-recurse-submodules", NULL);
> if (opts->quiet)
> @@ -390,7 +389,6 @@ static int add_worktree(const char *path, const char *refname,
> 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()),
> --
> 2.34.0.831.gd33babec0d1
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv
2021-11-23 15:26 ` Eric Sunshine
@ 2021-11-24 1:54 ` Junio C Hamano
2021-11-24 6:00 ` Eric Sunshine
2021-11-24 5:44 ` Eric Sunshine
1 sibling, 1 reply; 71+ messages in thread
From: Junio C Hamano @ 2021-11-24 1:54 UTC (permalink / raw)
To: Eric Sunshine
Cc: Ævar Arnfjörð Bjarmason, Git List, Jeff King,
Enzo Matsumiya
Eric Sunshine <sunshine@sunshineco.com> writes:
> ... At best, following this change,
> git-worktree is only working "by accident" if the underlying
> child_process::args.v doesn't get reallocated between run_command()
> invocations. Relying upon this "by accident" behavior feels rather
> unsafe.
Very true. Relying on the "if argv is null, point it at args.v"
assignment at the very beginning of the start_command() function is
safe because by that time the reallocations have happened already
if needed.
The pattern with or without NULLing is
initialize cp
push to cp.args
use cp
/* cp.argv = NULL */
strvec_clear(&cp.args);
push to cp.args
and strvec_clear() frees the underying array, and the first push
will reallocates from NULL, so there is no guarantee that cp.argv
in the first use that used to be pointing at cp.args that has
already been freed is still valid.
Thanks for spotting this. Has this patch ever been tested with
sanitizer? Do we have gap in test coverage?
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv
2021-11-24 1:54 ` Junio C Hamano
@ 2021-11-24 6:00 ` Eric Sunshine
2021-11-24 6:12 ` Eric Sunshine
0 siblings, 1 reply; 71+ messages in thread
From: Eric Sunshine @ 2021-11-24 6:00 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Git List, Jeff King,
Enzo Matsumiya
On Tue, Nov 23, 2021 at 8:54 PM Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
> > ... At best, following this change,
> > git-worktree is only working "by accident" if the underlying
> > child_process::args.v doesn't get reallocated between run_command()
> > invocations. Relying upon this "by accident" behavior feels rather
> > unsafe.
>
> The pattern with or without NULLing is
> /* cp.argv = NULL */
> strvec_clear(&cp.args);
> push to cp.args
>
> and strvec_clear() frees the underying array, and the first push
> will reallocates from NULL, so there is no guarantee that cp.argv
> in the first use that used to be pointing at cp.args that has
> already been freed is still valid.
Indeed, so this is even worse than I thought. I was somewhat pressed
for time when I wrote the review, thus didn't look at the
implementation of strvec_clear(), and incorrectly thought that it only
reset its `nr` member to 0 but kept the array allocated (much like
strbuf_reset()). That's why I thought it was only working "by
accident". But, as you point out, strvec_clear() does free its
allocated array (much like strbuf_release()), so -- with this patch
applied -- each subsequent run_command() invocation is _definitely_
accessing the dangling pointer in child_process::argv, and that
dangling pointer would (in the "best" case) be referencing the
original populated value of child_process::args, not the repopulated
value. So, even if it didn't crash outright, it would just re-run the
same command three times (unless by chance it reallocated the same
memory it had freed earlier.)
> Thanks for spotting this. Has this patch ever been tested with
> sanitizer? Do we have gap in test coverage?
The question about potential gap in test coverage is a good one.
Maybe, by chance it reallocated the same memory that it had earlier
freed, thus did indeed work "by accident". Another possibility is that
Ævar only ran the tests after applying the full patch series, in which
case this dangling-pointer bug would be gone, rather than running the
tests after each patch.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv
2021-11-24 6:00 ` Eric Sunshine
@ 2021-11-24 6:12 ` Eric Sunshine
0 siblings, 0 replies; 71+ messages in thread
From: Eric Sunshine @ 2021-11-24 6:12 UTC (permalink / raw)
To: Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason, Git List, Jeff King,
Enzo Matsumiya
On Wed, Nov 24, 2021 at 1:00 AM Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Nov 23, 2021 at 8:54 PM Junio C Hamano <gitster@pobox.com> wrote:
> > Thanks for spotting this. Has this patch ever been tested with
> > sanitizer? Do we have gap in test coverage?
>
> The question about potential gap in test coverage is a good one.
> Maybe, by chance it reallocated the same memory that it had earlier
> freed, thus did indeed work "by accident". Another possibility is that
> Ævar only ran the tests after applying the full patch series, in which
> case this dangling-pointer bug would be gone, rather than running the
> tests after each patch.
As a follow-up, I just applied this patch alone and ran the tests, and
they do indeed fail as expected (on my macOS). In
t2400-worktree-add.sh, alone, 44 out of 71 tests failed, thus I don't
think there's a gap in test coverage. So, the most likely explanation
of how this problem slipped through is that Ævar only tested after
applying the full series, in which case the dangling pointer bug would
be gone, rather than testing after each patch.
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv
2021-11-23 15:26 ` Eric Sunshine
2021-11-24 1:54 ` Junio C Hamano
@ 2021-11-24 5:44 ` Eric Sunshine
1 sibling, 0 replies; 71+ messages in thread
From: Eric Sunshine @ 2021-11-24 5:44 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, Enzo Matsumiya
On Tue, Nov 23, 2021 at 10:26:56AM -0500, Eric Sunshine wrote:
> I think perhaps the simplest thing to do is merely to squash this
> patch into the patch which ultimately removes the child_process::argv
> member (and the removal of these lines from worktree.c probably
> doesn't even need mention in the commit message -- or maybe just a
> minor mention).
On second thought, squashing this patch into the patch which
ultimately retires child_process::argv is probably not necessary. If
the patch is instead rewritten as below, then it prepares
builtin/worktree.c for eventual retirement of child_process::argv
without breaking git-worktree functionality.
(You could also extend this patch so it prepares for removal of
child_process::env, as well, or keep it minimal as I did here.)
-- >8 --
From: Eric Sunshine <sunshine@sunshineco.com>
Subject: [PATCH] worktree: stop being overly intimate with run_command()
internals
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 <sunshine@sunshineco.com>
---
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 @@ static int add_worktree(const char *path, const char *refname,
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()),
--
2.34.0.399.gdf2c515fd2
-- >8 --
^ permalink raw reply related [flat|nested] 71+ messages in thread
* [PATCH v2 2/9] upload-archive: use regular "struct child_process" pattern
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv Ævar Arnfjörð Bjarmason
@ 2021-11-23 12:06 ` Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 3/9] run-command API users: use strvec_pushv(), not argv assignment Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
9 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 24654b4c9bf..98d028dae67 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");
--
2.34.0.831.gd33babec0d1
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v2 3/9] run-command API users: use strvec_pushv(), not argv assignment
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 1/9] worktree: remove redundant NULL-ing of "cp.argv Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 2/9] upload-archive: use regular "struct child_process" pattern Ævar Arnfjörð Bjarmason
@ 2021-11-23 12:06 ` Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 4/9] run-command tests: " Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
9 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 8c41cdfe39b..573eef0cc4a 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 b1fcbe0d6fa..8df21f2130c 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 3d6e2ff17f8..4dd4d939f8a 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 f92859f43fa..229da4d1488 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 d69156312bd..0dabef2dd7c 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 f40df01c772..620a06ca2f5 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 92b69de6352..ff22f2fa2c5 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);
}
--
2.34.0.831.gd33babec0d1
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v2 4/9] run-command tests: use strvec_pushv(), not argv assignment
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-11-23 12:06 ` [PATCH v2 3/9] run-command API users: use strvec_pushv(), not argv assignment Ævar Arnfjörð Bjarmason
@ 2021-11-23 12:06 ` Ævar Arnfjörð Bjarmason
2021-11-24 1:33 ` Eric Sunshine
2021-11-23 12:06 ` [PATCH v2 5/9] run-command API users: use strvec_pushl(), not argv construction Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
9 siblings, 1 reply; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
As in the preceding commit change this API user to use strvec_pushv()
instead of assigning to the "argv" member directly. This leaves is
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 <avarab@gmail.com>
---
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 3c4fb862234..913775a14b7 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,
--
2.34.0.831.gd33babec0d1
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH v2 4/9] run-command tests: use strvec_pushv(), not argv assignment
2021-11-23 12:06 ` [PATCH v2 4/9] run-command tests: " Ævar Arnfjörð Bjarmason
@ 2021-11-24 1:33 ` Eric Sunshine
0 siblings, 0 replies; 71+ messages in thread
From: Eric Sunshine @ 2021-11-24 1:33 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, Enzo Matsumiya
On Tue, Nov 23, 2021 at 7:08 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> As in the preceding commit change this API user to use strvec_pushv()
> instead of assigning to the "argv" member directly. This leaves is
s/leaves is/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 <avarab@gmail.com>
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH v2 5/9] run-command API users: use strvec_pushl(), not argv construction
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-11-23 12:06 ` [PATCH v2 4/9] run-command tests: " Ævar Arnfjörð Bjarmason
@ 2021-11-23 12:06 ` Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 6/9] run-command API users: use strvec_push(), " Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
9 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 ef6b619c45e..a010b2c325f 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 27b9e78094d..9e54892311d 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 75cd2fb407f..d387131dd83 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 ea3112e0c0b..5f0476b0b76 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 71c59583a17..85d1abad884 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 49b846d9605..6149d507965 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 946938d011e..6ff1734d587 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 674309eed8b..b7d2a9b73c3 100644
--- a/editor.c
+++ b/editor.c
@@ -58,7 +58,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);
@@ -80,9 +79,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 ea96837cde3..6e02210db7a 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 c78d55bc674..9b5db32623f 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;
--
2.34.0.831.gd33babec0d1
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v2 6/9] run-command API users: use strvec_push(), not argv construction
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-11-23 12:06 ` [PATCH v2 5/9] run-command API users: use strvec_pushl(), not argv construction Ævar Arnfjörð Bjarmason
@ 2021-11-23 12:06 ` Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 7/9] run-command API: remove "argv" member, always use "args" Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
9 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 05d2455870d..3c74db17468 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 6149d507965..48c99c8ee45 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 8df21f2130c..4a000ee4afa 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 861282db1c3..41076857428 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 5ded21a017f..50df17279d1 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 e4f1decae20..92ab9a3fa6b 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";
--
2.34.0.831.gd33babec0d1
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v2 7/9] run-command API: remove "argv" member, always use "args"
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-11-23 12:06 ` [PATCH v2 6/9] run-command API users: use strvec_push(), " Ævar Arnfjörð Bjarmason
@ 2021-11-23 12:06 ` Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 8/9] difftool: use "env_array" to simplify memory management Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
9 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 620a06ca2f5..99dc93e7300 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 @@ int start_command(struct child_process *cmd)
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 @@ int start_command(struct child_process *cmd)
}
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 @@ int start_command(struct child_process *cmd)
* 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 @@ int start_command(struct child_process *cmd)
#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 @@ int start_command(struct child_process *cmd)
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 @@ int start_command(struct child_process *cmd)
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 49878262584..c0d1210cc63 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
- * `finish_command` (or during `start_command` when it is unsuccessful).
+ * 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 dfa790d3ff9..cae56ae6b80 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 3a0014417cc..bd17ecdc321 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 58d9e430f05..6e429a3fb9e 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 e4acca13d64..2ff9cf70835 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,
--
2.34.0.831.gd33babec0d1
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v2 8/9] difftool: use "env_array" to simplify memory management
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-11-23 12:06 ` [PATCH v2 7/9] run-command API: remove "argv" member, always use "args" Ævar Arnfjörð Bjarmason
@ 2021-11-23 12:06 ` Ævar Arnfjörð Bjarmason
2021-11-23 12:06 ` [PATCH v2 9/9] run-command API: remove "env" member, always use "env_array" Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
9 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/difftool.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4931c108451..4ee40fe3a06 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);
}
--
2.34.0.831.gd33babec0d1
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v2 9/9] run-command API: remove "env" member, always use "env_array"
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-11-23 12:06 ` [PATCH v2 8/9] difftool: use "env_array" to simplify memory management Ævar Arnfjörð Bjarmason
@ 2021-11-23 12:06 ` Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
9 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-23 12:06 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya,
Ævar Arnfjörð Bjarmason
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 7264a5b5de0..36b3d6c40da 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;
@@ -359,7 +359,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;
@@ -388,7 +388,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 b7d2a9b73c3..7044ad5f72e 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"
#include "compat/terminal.h"
@@ -81,7 +82,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";
term_fail = save_term(1);
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.0.831.gd33babec0d1
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v3 0/9] run-command API: get rid of "argv" and "env"
2021-11-23 12:06 ` [PATCH v2 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (8 preceding siblings ...)
2021-11-23 12:06 ` [PATCH v2 9/9] run-command API: remove "env" member, always use "env_array" Ævar Arnfjörð Bjarmason
@ 2021-11-25 22:52 ` Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 1/9] worktree: stop being overly intimate with run_command() internals Ævar Arnfjörð Bjarmason
` (8 more replies)
9 siblings, 9 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-25 22:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya, Eric Sunshine,
Ævar Arnfjörð Bjarmason
See v2[1] for the overall summary of this topic.
New since v2:
* Ejected my old 1/9 in favor of Eric Sunshine's version at
[2]. Thanks a lot Eric. And this time I ran the full tests for
every commit with "rebase -x", so it all passes now.
* Minor typo correction, also pointed out by Eric.
1. https://lore.kernel.org/git/cover-v2-0.9-00000000000-20211123T115551Z-avarab@gmail.com/
2. https://lore.kernel.org/git/YZ3RUkGpixYAREcI@flurp.local/
Eric Sunshine (1):
worktree: stop being overly intimate with run_command() internals
Ævar Arnfjörð Bjarmason (8):
upload-archive: use regular "struct child_process" pattern
run-command API users: use strvec_pushv(), not argv assignment
run-command tests: use strvec_pushv(), not argv assignment
run-command API users: use strvec_pushl(), not argv construction
run-command API users: use strvec_push(), not argv construction
run-command API: remove "argv" member, always use "args"
difftool: use "env_array" to simplify memory management
run-command API: remove "env" member, always use "env_array"
add-patch.c | 4 +-
archive-tar.c | 9 ++---
builtin/add.c | 7 +---
builtin/difftool.c | 14 ++-----
builtin/fsck.c | 12 ++----
builtin/help.c | 3 +-
builtin/merge.c | 3 +-
builtin/notes.c | 5 +--
builtin/receive-pack.c | 80 ++++++++++++++-----------------------
builtin/replace.c | 3 +-
builtin/upload-archive.c | 5 ++-
builtin/worktree.c | 13 +++---
connected.c | 3 +-
daemon.c | 20 ++++------
diff.c | 8 +---
editor.c | 8 ++--
http-backend.c | 2 +-
http.c | 5 ++-
object-file.c | 2 +-
prompt.c | 7 +---
remote-curl.c | 2 +-
run-command.c | 62 +++++++++++++---------------
run-command.h | 54 ++++++++++++-------------
sequencer.c | 10 ++---
sub-process.c | 2 +-
t/helper/test-run-command.c | 9 +++--
t/helper/test-subprocess.c | 2 +-
trace2/tr2_tgt_event.c | 2 +-
trace2/tr2_tgt_normal.c | 2 +-
trace2/tr2_tgt_perf.c | 4 +-
trailer.c | 2 +-
transport.c | 11 +++--
upload-pack.c | 5 +--
33 files changed, 157 insertions(+), 223 deletions(-)
Range-diff against v2:
1: 9cc220ce5a3 ! 1: 1c3f9de33ad worktree: remove redundant NULL-ing of "cp.argv
@@
## Metadata ##
-Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
+Author: Eric Sunshine <sunshine@sunshineco.com>
## Commit message ##
- worktree: remove redundant NULL-ing of "cp.argv
+ worktree: stop being overly intimate with run_command() internals
- The clearing of "argv" was added in 7f44e3d1de0 (worktree: make setup
- of new HEAD distinct from worktree population, 2015-07-17) when the
- "cp" variable wasn't initialized. It hasn't been needed since
- 542aa25d974 (use CHILD_PROCESS_INIT to initialize automatic variables,
- 2016-08-05).
+ 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.
- Let's remove it to make a later change that gets rid of the "argv"
- member from "struct child_process" smaller.
+ [*] 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 <sunshine@sunshineco.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## builtin/worktree.c ##
@@ builtin/worktree.c: static int add_worktree(const char *path, const char *refnam
if (opts->checkout) {
- cp.argv = NULL;
- strvec_clear(&cp.args);
+- 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");
@@ builtin/worktree.c: static int add_worktree(const char *path, const char *refname,
+ 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;
2: bfa65e5afd7 = 2: d3a418b3809 upload-archive: use regular "struct child_process" pattern
3: 61e4eb8e173 = 3: 595ff9a775d run-command API users: use strvec_pushv(), not argv assignment
4: a2ee10e214c ! 4: 764c9b813fb run-command tests: use strvec_pushv(), not argv assignment
@@ Commit message
run-command tests: use strvec_pushv(), not argv assignment
As in the preceding commit change this API user to use strvec_pushv()
- instead of assigning to the "argv" member directly. This leaves is
+ 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.
5: 2b446606eb9 = 5: 45803236764 run-command API users: use strvec_pushl(), not argv construction
6: fad420dc563 = 6: 7bdf963ed04 run-command API users: use strvec_push(), not argv construction
7: 67ab5114ed7 = 7: 275535a447e run-command API: remove "argv" member, always use "args"
8: b8387a4a76d = 8: 203f44a91f4 difftool: use "env_array" to simplify memory management
9: 6bd9f508a3d ! 9: 4e4db79ea53 run-command API: remove "env" member, always use "env_array"
@@ editor.c
+#include "strvec.h"
#include "run-command.h"
#include "sigchain.h"
- #include "compat/terminal.h"
+
@@ editor.c: static int launch_specified_editor(const char *editor, const char *path,
strbuf_realpath(&realpath, path, 1);
@@ editor.c: static int launch_specified_editor(const char *editor, const char *pat
+ strvec_pushv(&p.env_array, (const char **)env);
p.use_shell = 1;
p.trace2_child_class = "editor";
- term_fail = save_term(1);
+ if (start_command(&p) < 0) {
## object-file.c ##
@@ object-file.c: static void fill_alternate_refs_command(struct child_process *cmd,
--
2.34.1.838.g779e9098efb
^ permalink raw reply [flat|nested] 71+ messages in thread* [PATCH v3 1/9] worktree: stop being overly intimate with run_command() internals
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
@ 2021-11-25 22:52 ` Ævar Arnfjörð Bjarmason
2021-11-26 9:48 ` Eric Sunshine
2021-11-25 22:52 ` [PATCH v3 2/9] upload-archive: use regular "struct child_process" pattern Ævar Arnfjörð Bjarmason
` (7 subsequent siblings)
8 siblings, 1 reply; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-25 22:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya, Eric Sunshine,
Ævar Arnfjörð Bjarmason
From: Eric Sunshine <sunshine@sunshineco.com>
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 <sunshine@sunshineco.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
builtin/worktree.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/builtin/worktree.c b/builtin/worktree.c
index d22ece93e1a..9edd3e2829b 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 @@ static int add_worktree(const char *path, const char *refname,
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()),
--
2.34.1.838.g779e9098efb
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [PATCH v3 1/9] worktree: stop being overly intimate with run_command() internals
2021-11-25 22:52 ` [PATCH v3 1/9] worktree: stop being overly intimate with run_command() internals Ævar Arnfjörð Bjarmason
@ 2021-11-26 9:48 ` Eric Sunshine
0 siblings, 0 replies; 71+ messages in thread
From: Eric Sunshine @ 2021-11-26 9:48 UTC (permalink / raw)
To: Ævar Arnfjörð Bjarmason
Cc: Git List, Junio C Hamano, Jeff King, Enzo Matsumiya
On Thu, Nov 25, 2021 at 5:52 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> From: Eric Sunshine <sunshine@sunshineco.com>
>
> 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 people feel uncomfortable with the way this patch shadows `cp` in
nested blocks, an alternative would be to call child_process_init(&cp)
to reuse the existing `cp`, similar to the fix[1] applied to pager.c
when reusing a `child_process`. I don't feel strongly about it either
way.
[1]: https://lore.kernel.org/git/20211125000239.2336-1-ematsumiya@suse.de/
> [*] 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 <sunshine@sunshineco.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> builtin/worktree.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index d22ece93e1a..9edd3e2829b 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 @@ static int add_worktree(const char *path, const char *refname,
> 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()),
> --
> 2.34.1.838.g779e9098efb
^ permalink raw reply [flat|nested] 71+ messages in thread
* [PATCH v3 2/9] upload-archive: use regular "struct child_process" pattern
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 1/9] worktree: stop being overly intimate with run_command() internals Ævar Arnfjörð Bjarmason
@ 2021-11-25 22:52 ` Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 3/9] run-command API users: use strvec_pushv(), not argv assignment Ævar Arnfjörð Bjarmason
` (6 subsequent siblings)
8 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-25 22:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya, Eric Sunshine,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 24654b4c9bf..98d028dae67 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");
--
2.34.1.838.g779e9098efb
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v3 3/9] run-command API users: use strvec_pushv(), not argv assignment
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 1/9] worktree: stop being overly intimate with run_command() internals Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 2/9] upload-archive: use regular "struct child_process" pattern Ævar Arnfjörð Bjarmason
@ 2021-11-25 22:52 ` Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 4/9] run-command tests: " Ævar Arnfjörð Bjarmason
` (5 subsequent siblings)
8 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-25 22:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya, Eric Sunshine,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 8c41cdfe39b..573eef0cc4a 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 b1fcbe0d6fa..8df21f2130c 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 3d6e2ff17f8..4dd4d939f8a 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 f92859f43fa..229da4d1488 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 d69156312bd..0dabef2dd7c 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 f40df01c772..620a06ca2f5 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 92b69de6352..ff22f2fa2c5 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);
}
--
2.34.1.838.g779e9098efb
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v3 4/9] run-command tests: use strvec_pushv(), not argv assignment
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (2 preceding siblings ...)
2021-11-25 22:52 ` [PATCH v3 3/9] run-command API users: use strvec_pushv(), not argv assignment Ævar Arnfjörð Bjarmason
@ 2021-11-25 22:52 ` Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 5/9] run-command API users: use strvec_pushl(), not argv construction Ævar Arnfjörð Bjarmason
` (4 subsequent siblings)
8 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-25 22:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya, Eric Sunshine,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 3c4fb862234..913775a14b7 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,
--
2.34.1.838.g779e9098efb
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v3 5/9] run-command API users: use strvec_pushl(), not argv construction
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (3 preceding siblings ...)
2021-11-25 22:52 ` [PATCH v3 4/9] run-command tests: " Ævar Arnfjörð Bjarmason
@ 2021-11-25 22:52 ` Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 6/9] run-command API users: use strvec_push(), " Ævar Arnfjörð Bjarmason
` (3 subsequent siblings)
8 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-25 22:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya, Eric Sunshine,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 ef6b619c45e..a010b2c325f 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 27b9e78094d..9e54892311d 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 75cd2fb407f..d387131dd83 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 ea3112e0c0b..5f0476b0b76 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 71c59583a17..85d1abad884 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 49b846d9605..6149d507965 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 946938d011e..6ff1734d587 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 fdd3eeafa94..d92a8d9ab5b 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 ea96837cde3..6e02210db7a 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 c78d55bc674..9b5db32623f 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;
--
2.34.1.838.g779e9098efb
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v3 6/9] run-command API users: use strvec_push(), not argv construction
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (4 preceding siblings ...)
2021-11-25 22:52 ` [PATCH v3 5/9] run-command API users: use strvec_pushl(), not argv construction Ævar Arnfjörð Bjarmason
@ 2021-11-25 22:52 ` Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 7/9] run-command API: remove "argv" member, always use "args" Ævar Arnfjörð Bjarmason
` (2 subsequent siblings)
8 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-25 22:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya, Eric Sunshine,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 05d2455870d..3c74db17468 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 6149d507965..48c99c8ee45 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 8df21f2130c..4a000ee4afa 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 861282db1c3..41076857428 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 5ded21a017f..50df17279d1 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 e4f1decae20..92ab9a3fa6b 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";
--
2.34.1.838.g779e9098efb
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v3 7/9] run-command API: remove "argv" member, always use "args"
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (5 preceding siblings ...)
2021-11-25 22:52 ` [PATCH v3 6/9] run-command API users: use strvec_push(), " Ævar Arnfjörð Bjarmason
@ 2021-11-25 22:52 ` Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 8/9] difftool: use "env_array" to simplify memory management Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 9/9] run-command API: remove "env" member, always use "env_array" Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-25 22:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya, Eric Sunshine,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
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 620a06ca2f5..99dc93e7300 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 @@ int start_command(struct child_process *cmd)
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 @@ int start_command(struct child_process *cmd)
}
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 @@ int start_command(struct child_process *cmd)
* 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 @@ int start_command(struct child_process *cmd)
#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 @@ int start_command(struct child_process *cmd)
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 @@ int start_command(struct child_process *cmd)
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 49878262584..c0d1210cc63 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
- * `finish_command` (or during `start_command` when it is unsuccessful).
+ * 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 dfa790d3ff9..cae56ae6b80 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 3a0014417cc..bd17ecdc321 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 58d9e430f05..6e429a3fb9e 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 e4acca13d64..2ff9cf70835 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,
--
2.34.1.838.g779e9098efb
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v3 8/9] difftool: use "env_array" to simplify memory management
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (6 preceding siblings ...)
2021-11-25 22:52 ` [PATCH v3 7/9] run-command API: remove "argv" member, always use "args" Ævar Arnfjörð Bjarmason
@ 2021-11-25 22:52 ` Ævar Arnfjörð Bjarmason
2021-11-25 22:52 ` [PATCH v3 9/9] run-command API: remove "env" member, always use "env_array" Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-25 22:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya, Eric Sunshine,
Ævar Arnfjörð Bjarmason
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 <avarab@gmail.com>
---
builtin/difftool.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)
diff --git a/builtin/difftool.c b/builtin/difftool.c
index 4931c108451..4ee40fe3a06 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);
}
--
2.34.1.838.g779e9098efb
^ permalink raw reply related [flat|nested] 71+ messages in thread* [PATCH v3 9/9] run-command API: remove "env" member, always use "env_array"
2021-11-25 22:52 ` [PATCH v3 0/9] run-command API: get rid of "argv" and "env" Ævar Arnfjörð Bjarmason
` (7 preceding siblings ...)
2021-11-25 22:52 ` [PATCH v3 8/9] difftool: use "env_array" to simplify memory management Ævar Arnfjörð Bjarmason
@ 2021-11-25 22:52 ` Ævar Arnfjörð Bjarmason
8 siblings, 0 replies; 71+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-11-25 22:52 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Jeff King, Enzo Matsumiya, Eric Sunshine,
Ævar Arnfjörð Bjarmason
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
^ permalink raw reply related [flat|nested] 71+ messages in thread