{"thread":{"id":"58562","subject":"PATCH] bisect--helper: plug strvec leak in bisect_start()","startedAt":"2022-10-04T16:06:46Z","lastAt":"2022-10-15T18:21:51Z","messageCount":21,"participants":["René Scharfe","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"464180","messageId":"5c6a4c30-d454-51b6-ec57-9af036b9c4e0@web.de","threadId":"58562","inReplyTo":null,"subject":"PATCH] bisect--helper: plug strvec leak in bisect_start()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-04T16:06:28Z","receivedAt":"2022-10-04T16:06:46Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The strvec \"argv\" is used to build a command for run_command_v_opt(),\nbut never freed. Use the \"args\" strvec of struct child_process and\nrun_command() instead, which releases the allocated memory both on\nsuccess and on error. We just also need to set the \"git_cmd\" bit\ndirectly.\n\nSigned-off-by: René Scharfe \n---\n builtin/bisect--helper.c | 7 ++++---\n 1 file changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 501245fac9..9fe0c08479 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -765,11 +765,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n \t\tstrbuf_trim(&start_head);\n \t\tif (!no_checkout) {\n-\t\t\tstruct strvec argv = STRVEC_INIT;\n+\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n+\t\t\tcmd.git_cmd = 1;\n+\t\t\tstrvec_pushl(&cmd.args, \"checkout\", start_head.buf,\n \t\t\t\t \"--\", NULL);\n-\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\t\t\tif (run_command(&cmd)) {\n \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n \t\t\t\t\t\t \" Try 'git bisect start \"\n \t\t\t\t\t\t \"'.\"),\n--\n2.37.3\n"},{"id":"464217","messageId":"221005.8635c2u3k5.gmgdl@evledraar.gmail.com","threadId":"58562","inReplyTo":"5c6a4c30-d454-51b6-ec57-9af036b9c4e0@web.de","subject":"Re: PATCH] bisect--helper: plug strvec leak in bisect_start()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-05T07:29:13Z","receivedAt":"2022-10-05T07:34:27Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 04 2022, René Scharfe wrote:\n\n> The strvec \"argv\" is used to build a command for run_command_v_opt(),\n> but never freed. Use the \"args\" strvec of struct child_process and\n> run_command() instead, which releases the allocated memory both on\n> success and on error. We just also need to set the \"git_cmd\" bit\n> directly.\n>\n> Signed-off-by: René Scharfe \n> ---\n> builtin/bisect--helper.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 501245fac9..9fe0c08479 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -765,11 +765,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n> \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n> \t\tstrbuf_trim(&start_head);\n> \t\tif (!no_checkout) {\n> -\t\t\tstruct strvec argv = STRVEC_INIT;\n> +\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n>\n> -\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n> +\t\t\tcmd.git_cmd = 1;\n> +\t\t\tstrvec_pushl(&cmd.args, \"checkout\", start_head.buf,\n> \t\t\t\t \"--\", NULL);\n> -\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n> +\t\t\tif (run_command(&cmd)) {\n> \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n> \t\t\t\t\t\t \" Try 'git bisect start \"\n> \t\t\t\t\t\t \"'.\"),\n\nOkey, so we leak the strvec, and instead of adding a strvec_clear()\nyou're just switching the lower-level API, which we'd need in some cases\nwith this API, and would often be cleaner.\n\nBut I don't get it in this case, why not just:\n\t\n\tdiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n\tindex 4e97817fba5..f9645a9d0df 100644\n\t--- a/builtin/bisect--helper.c\n\t+++ b/builtin/bisect--helper.c\n\t@@ -763,11 +763,9 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n\t \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n\t \t\tstrbuf_trim(&start_head);\n\t \t\tif (!no_checkout) {\n\t-\t\t\tstruct strvec argv = STRVEC_INIT;\n\t+\t\t\tconst char *argv[] = { \"checkout\", start_head.buf, \"--\", NULL };\n\t \n\t-\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n\t-\t\t\t\t \"--\", NULL);\n\t-\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n\t+\t\t\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n\t \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n\t \t\t\t\t\t\t \" Try 'git bisect start \"\n\t \t\t\t\t\t\t \"'.\"),\n\nThe common pattern for run_command_v_opt() callers that don't need a\ndynamic list is exactly that, e.g.:\n\t\n\tbuiltin/difftool.c=static int print_tool_help(void)\n\tbuiltin/difftool.c-{\n\tbuiltin/difftool.c- const char *argv[] = { \"mergetool\", \"--tool-help=diff\", NULL };\n\tbuiltin/difftool.c: return run_command_v_opt(argv, RUN_GIT_CMD);\n\tbuiltin/difftool.c-}\n\t\nAnd:\n\t\n\tfsmonitor-ipc.c=static int spawn_daemon(void)\n\tfsmonitor-ipc.c-{\n\tfsmonitor-ipc.c- const char *args[] = { \"fsmonitor--daemon\", \"start\", NULL };\n\tfsmonitor-ipc.c-\n\tfsmonitor-ipc.c: return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,\n\tfsmonitor-ipc.c- \"fsmonitor\");\n\tfsmonitor-ipc.c-}\n\nHere we have the \"start_head\" which we'll need to strbuf_release(), but\nwe're not returning directly, and the function is doing that already.\n\nYour version is slightly more memory efficient, i.e. we'll end up having\nto push this to a \"struct child_process\"'s argv anyway, but this is less\ncode & we don't need to carefully eyeball run_command_v_opt_cd_env_tr2()\nto see that it's correct (which I did, your version does the right thing\ntoo).\n"},{"id":"464242","messageId":"04d393cc-d8c7-ddf7-b235-12082f81c560@web.de","threadId":"58562","inReplyTo":"221005.8635c2u3k5.gmgdl@evledraar.gmail.com","subject":"Re: PATCH] bisect--helper: plug strvec leak in bisect_start()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-05T15:43:04Z","receivedAt":"2022-10-05T15:43:17Z","isPatch":false,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 05.10.22 um 09:29 schrieb Ævar Arnfjörð Bjarmason:\n>\n> On Tue, Oct 04 2022, René Scharfe wrote:\n>\n>> The strvec \"argv\" is used to build a command for run_command_v_opt(),\n>> but never freed. Use the \"args\" strvec of struct child_process and\n>> run_command() instead, which releases the allocated memory both on\n>> success and on error. We just also need to set the \"git_cmd\" bit\n>> directly.\n>>\n>> Signed-off-by: René Scharfe \n>> ---\n>> builtin/bisect--helper.c | 7 ++++---\n>> 1 file changed, 4 insertions(+), 3 deletions(-)\n>>\n>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n>> index 501245fac9..9fe0c08479 100644\n>> --- a/builtin/bisect--helper.c\n>> +++ b/builtin/bisect--helper.c\n>> @@ -765,11 +765,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n>> \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n>> \t\tstrbuf_trim(&start_head);\n>> \t\tif (!no_checkout) {\n>> -\t\t\tstruct strvec argv = STRVEC_INIT;\n>> +\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n>>\n>> -\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n>> +\t\t\tcmd.git_cmd = 1;\n>> +\t\t\tstrvec_pushl(&cmd.args, \"checkout\", start_head.buf,\n>> \t\t\t\t \"--\", NULL);\n>> -\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n>> +\t\t\tif (run_command(&cmd)) {\n>> \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n>> \t\t\t\t\t\t \" Try 'git bisect start \"\n>> \t\t\t\t\t\t \"'.\"),\n>\n> Okey, so we leak the strvec, and instead of adding a strvec_clear()\n> you're just switching the lower-level API, which we'd need in some cases\n> with this API, and would often be cleaner.\n>\n> But I don't get it in this case, why not just:\n>\n> \tdiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> \tindex 4e97817fba5..f9645a9d0df 100644\n> \t--- a/builtin/bisect--helper.c\n> \t+++ b/builtin/bisect--helper.c\n> \t@@ -763,11 +763,9 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n> \t \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n> \t \t\tstrbuf_trim(&start_head);\n> \t \t\tif (!no_checkout) {\n> \t-\t\t\tstruct strvec argv = STRVEC_INIT;\n> \t+\t\t\tconst char *argv[] = { \"checkout\", start_head.buf, \"--\", NULL };\n>\n> \t-\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n> \t-\t\t\t\t \"--\", NULL);\n> \t-\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n> \t+\t\t\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n> \t \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n> \t \t\t\t\t\t\t \" Try 'git bisect start \"\n> \t \t\t\t\t\t\t \"'.\"),\n\nThat looks even better. I didn't think of that because we had a phase\nwhere we could only use constant expressions for initialization. We do\nhave a precedent in 359f0d754a (range-diff/format-patch: handle commit\nranges other than A..B, 2021-02-05), though, which does use a variable:\n\n+ char *copy = xstrdup(arg); /* setup_revisions() modifies it */\n+ const char *argv[] = { \"\", copy, \"--\", NULL };\n\n(There may be earlier ones, that's just the one I found quickly.) So is\nit time for that C99 feature already? Yay! 🥳\n\n> The common pattern for run_command_v_opt() callers that don't need a\n> dynamic list is exactly that, e.g.:\n>\n> \tbuiltin/difftool.c=static int print_tool_help(void)\n> \tbuiltin/difftool.c-{\n> \tbuiltin/difftool.c- const char *argv[] = { \"mergetool\", \"--tool-help=diff\", NULL };\n> \tbuiltin/difftool.c: return run_command_v_opt(argv, RUN_GIT_CMD);\n> \tbuiltin/difftool.c-}\n>\n> And:\n>\n> \tfsmonitor-ipc.c=static int spawn_daemon(void)\n> \tfsmonitor-ipc.c-{\n> \tfsmonitor-ipc.c- const char *args[] = { \"fsmonitor--daemon\", \"start\", NULL };\n> \tfsmonitor-ipc.c-\n> \tfsmonitor-ipc.c: return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,\n> \tfsmonitor-ipc.c- \"fsmonitor\");\n> \tfsmonitor-ipc.c-}\n>\n> Here we have the \"start_head\" which we'll need to strbuf_release(), but\n> we're not returning directly, and the function is doing that already.\n>\n> Your version is slightly more memory efficient, i.e. we'll end up having\n> to push this to a \"struct child_process\"'s argv anyway, but this is less\n> code & we don't need to carefully eyeball run_command_v_opt_cd_env_tr2()\n> to see that it's correct (which I did, your version does the right thing\n> too).\n\nHaving two ways to set the same flags adds some unnecessary friction.\n\nRené\n"},{"id":"464255","messageId":"xmqq4jwioy61.fsf@gitster.g","threadId":"58562","inReplyTo":"5c6a4c30-d454-51b6-ec57-9af036b9c4e0@web.de","subject":"Re: PATCH] bisect--helper: plug strvec leak in bisect_start()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-05T19:41:58Z","receivedAt":"2022-10-05T19:42:04Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> The strvec \"argv\" is used to build a command for run_command_v_opt(),\n> but never freed. Use the \"args\" strvec of struct child_process and\n> run_command() instead, which releases the allocated memory both on\n> success and on error. We just also need to set the \"git_cmd\" bit\n> directly.\n\nWell reasoned and explained.\n\nThanks.\n\n>\n> Signed-off-by: René Scharfe \n> ---\n> builtin/bisect--helper.c | 7 ++++---\n> 1 file changed, 4 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 501245fac9..9fe0c08479 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -765,11 +765,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n> \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n> \t\tstrbuf_trim(&start_head);\n> \t\tif (!no_checkout) {\n> -\t\t\tstruct strvec argv = STRVEC_INIT;\n> +\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n>\n> -\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n> +\t\t\tcmd.git_cmd = 1;\n> +\t\t\tstrvec_pushl(&cmd.args, \"checkout\", start_head.buf,\n> \t\t\t\t \"--\", NULL);\n> -\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n> +\t\t\tif (run_command(&cmd)) {\n> \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n> \t\t\t\t\t\t \" Try 'git bisect start \"\n> \t\t\t\t\t\t \"'.\"),\n> --\n> 2.37.3\n"},{"id":"464256","messageId":"xmqqy1tunjgp.fsf@gitster.g","threadId":"58562","inReplyTo":"221005.8635c2u3k5.gmgdl@evledraar.gmail.com","subject":"Re: PATCH] bisect--helper: plug strvec leak in bisect_start()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-05T19:44:54Z","receivedAt":"2022-10-05T19:45:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> But I don't get it in this case, why not just:\n> \t\n> \tdiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> \tindex 4e97817fba5..f9645a9d0df 100644\n> \t--- a/builtin/bisect--helper.c\n> \t+++ b/builtin/bisect--helper.c\n> \t@@ -763,11 +763,9 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n> \t \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n> \t \t\tstrbuf_trim(&start_head);\n> \t \t\tif (!no_checkout) {\n> \t-\t\t\tstruct strvec argv = STRVEC_INIT;\n> \t+\t\t\tconst char *argv[] = { \"checkout\", start_head.buf, \"--\", NULL };\n> \t \n> \t-\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n> \t-\t\t\t\t \"--\", NULL);\n> \t-\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n> \t+\t\t\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n> \t \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n> \t \t\t\t\t\t\t \" Try 'git bisect start \"\n> \t \t\t\t\t\t\t \"'.\"),\n>\n> The common pattern for run_command_v_opt() callers that don't need a\n> dynamic list is exactly that.\n\nI think you answered it yourself. start_head.buf is not known at\ncompilation time, and there may be some superstition (it may not be\na mere superstition, but conservatism) about older compiler not\ngrokking it.\n"},{"id":"464334","messageId":"221006.86a668r5mf.gmgdl@evledraar.gmail.com","threadId":"58562","inReplyTo":"xmqqy1tunjgp.fsf@gitster.g","subject":"Re: PATCH] bisect--helper: plug strvec leak in bisect_start()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-06T21:35:19Z","receivedAt":"2022-10-06T21:42:39Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Oct 05 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> But I don't get it in this case, why not just:\n>> \t\n>> \tdiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n>> \tindex 4e97817fba5..f9645a9d0df 100644\n>> \t--- a/builtin/bisect--helper.c\n>> \t+++ b/builtin/bisect--helper.c\n>> \t@@ -763,11 +763,9 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n>> \t \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n>> \t \t\tstrbuf_trim(&start_head);\n>> \t \t\tif (!no_checkout) {\n>> \t-\t\t\tstruct strvec argv = STRVEC_INIT;\n>> \t+\t\t\tconst char *argv[] = { \"checkout\", start_head.buf, \"--\", NULL };\n>> \t \n>> \t-\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n>> \t-\t\t\t\t \"--\", NULL);\n>> \t-\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n>> \t+\t\t\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n>> \t \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n>> \t \t\t\t\t\t\t \" Try 'git bisect start \"\n>> \t \t\t\t\t\t\t \"'.\"),\n>>\n>> The common pattern for run_command_v_opt() callers that don't need a\n>> dynamic list is exactly that.\n>\n> I think you answered it yourself. start_head.buf is not known at\n> compilation time, and there may be some superstition (it may not be\n> a mere superstition, but conservatism) about older compiler not\n> grokking it.\n\nI think we're thoroughly past that hump as we have a hard requirement on\ndesignated initializers.\n\nAnyway, I believe GCC's -std=c89 wtith -pedantic catches this, e.g. for\nbisect--helper.c (the latter is the above patch):\n\n\t$ make -k git-objs CFLAGS=-std=c89 2>&1|grep 'initializer element is not computable at load time'|grep bisect\n\tbuiltin/bisect--helper.c:534:43: error: initializer element is not computable at load time [-Werror=pedantic]\n\tbuiltin/bisect--helper.c:768:60: error: initializer element is not computable at load time [-Werror=pedantic]\n\nFor the former we've had:\n\n\tstatic int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)\n\t[...]\n\t\tstruct add_bisect_ref_data cb = { revs };\n\nIn the same file since 517ecb3161d (bisect--helper: reimplement\n`bisect_next` and `bisect_auto_next` shell functions in C, 2020-09-24).\n\nOther prior art, just taking the char[] ones (and not even all of them):\n\t\n\tbuiltin/merge-index.c:12:37: error: initializer element is not computable at load time [-Werror=pedantic]\n\t 12 | const char *arguments[] = { pgm, \"\", \"\", \"\", path, \"\", \"\", \"\", NULL };\n\tbuiltin/remote.c:95:41: error: initializer element is not computable at load time [-Werror=pedantic]\n\t 95 | const char *argv[] = { \"fetch\", name, NULL, NULL };\n\tarchive.c:408:33: error: initializer element is not computable at load time [-Werror=pedantic]\n\t 408 | const char *paths[] = { path, NULL };\n\tmerge-ort.c:1699:45: error: initializer element is not computable at load time [-Werror=pedantic]\n\t 1699 | \"--all\", merged_revision, NULL };\n\nSo I think we can safely use this.\t\n\n\n"},{"id":"464336","messageId":"xmqqk05cipq8.fsf@gitster.g","threadId":"58562","inReplyTo":"221006.86a668r5mf.gmgdl@evledraar.gmail.com","subject":"Re: PATCH] bisect--helper: plug strvec leak in bisect_start()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-06T21:53:03Z","receivedAt":"2022-10-06T21:53:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>>> The common pattern for run_command_v_opt() callers that don't need a\n>>> dynamic list is exactly that.\n\nSo, scratching \"that don't need a dynamic list\", and with ...\n\n> Other prior art, just taking the char[] ones (and not even all of them):\n> ...\n> \tbuiltin/merge-index.c:12:37: error: initializer element is not computable at load time [-Werror=pedantic]\n> \t 12 | const char *arguments[] = { pgm, \"\", \"\", \"\", path, \"\", \"\", \"\", NULL };\n> \tbuiltin/remote.c:95:41: error: initializer element is not computable at load time [-Werror=pedantic]\n> \t 95 | const char *argv[] = { \"fetch\", name, NULL, NULL };\n> \tarchive.c:408:33: error: initializer element is not computable at load time [-Werror=pedantic]\n> \t 408 | const char *paths[] = { path, NULL };\n> \tmerge-ort.c:1699:45: error: initializer element is not computable at load time [-Werror=pedantic]\n> \t 1699 | \"--all\", merged_revision, NULL };\n\n... these, I agree that we are not making anything worse.\n\nThanks.\n"},{"id":"464379","messageId":"1965b54b-122a-c965-f886-1a7dd6afbfb4@web.de","threadId":"58562","inReplyTo":"xmqqk05cipq8.fsf@gitster.g","subject":"[PATCH v2] bisect--helper: plug strvec leak","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-07T15:08:42Z","receivedAt":"2022-10-07T15:08:52Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"The strvec \"argv\" is used to build a command for run_command_v_opt(),\nbut never freed. Use a constant string array instead, which doesn't\nrequire any cleanup.\n\nSuggested-by: Ævar Arnfjörð Bjarmason \nSigned-off-by: René Scharfe \n---\n builtin/bisect--helper.c | 7 +++----\n 1 file changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 501245fac9..28ef7ec2a4 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n \t\tstrbuf_trim(&start_head);\n \t\tif (!no_checkout) {\n-\t\t\tstruct strvec argv = STRVEC_INIT;\n+\t\t\tconst char *argv[] = { \"checkout\", start_head.buf,\n+\t\t\t\t\t \"--\", NULL };\n\n-\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n-\t\t\t\t \"--\", NULL);\n-\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\t\t\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n \t\t\t\t\t\t \" Try 'git bisect start \"\n \t\t\t\t\t\t \"'.\"),\n--\n2.38.0\n\n"},{"id":"464386","messageId":"xmqq4jwfh7mc.fsf@gitster.g","threadId":"58562","inReplyTo":"1965b54b-122a-c965-f886-1a7dd6afbfb4@web.de","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-07T17:21:47Z","receivedAt":"2022-10-07T17:21:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> The strvec \"argv\" is used to build a command for run_command_v_opt(),\n> but never freed. Use a constant string array instead, which doesn't\n> require any cleanup.\n>\n> Suggested-by: Ævar Arnfjörð Bjarmason \n> Signed-off-by: René Scharfe \n> ---\n> builtin/bisect--helper.c | 7 +++----\n> 1 file changed, 3 insertions(+), 4 deletions(-)\n\nThanks, both. Looking good.\n\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 501245fac9..28ef7ec2a4 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n> \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n> \t\tstrbuf_trim(&start_head);\n> \t\tif (!no_checkout) {\n> -\t\t\tstruct strvec argv = STRVEC_INIT;\n> +\t\t\tconst char *argv[] = { \"checkout\", start_head.buf,\n> +\t\t\t\t\t \"--\", NULL };\n>\n> -\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n> -\t\t\t\t \"--\", NULL);\n> -\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n> +\t\t\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n> \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n> \t\t\t\t\t\t \" Try 'git bisect start \"\n> \t\t\t\t\t\t \"'.\"),\n> --\n> 2.38.0\n"},{"id":"464585","messageId":"Y0TXTl0gSBOFQa9B@coredump.intra.peff.net","threadId":"58562","inReplyTo":"1965b54b-122a-c965-f886-1a7dd6afbfb4@web.de","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T02:39:10Z","receivedAt":"2022-10-11T02:39:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 07, 2022 at 05:08:42PM +0200, René Scharfe wrote:\n\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 501245fac9..28ef7ec2a4 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n> \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n> \t\tstrbuf_trim(&start_head);\n> \t\tif (!no_checkout) {\n> -\t\t\tstruct strvec argv = STRVEC_INIT;\n> +\t\t\tconst char *argv[] = { \"checkout\", start_head.buf,\n> +\t\t\t\t\t \"--\", NULL };\n> \n> -\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n> -\t\t\t\t \"--\", NULL);\n> -\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n> +\t\t\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n\nThis is OK with me, but note that one thing we lose is compiler\nprotection that we remembered the trailing NULL pointer in the argv\narray (whereas strvec_pushl() has an attribute that makes sure the last\nargument is a NULL).\n\nProbably not that big a deal in practice. It would be nice if there was\na way to annotate this for the compiler, but I don't think there's any\nattribute for \"this pointer-to-pointer parameter should have a NULL at\nthe end\".\n\n-Peff\n"},{"id":"464588","messageId":"xmqq35buykz1.fsf@gitster.g","threadId":"58562","inReplyTo":"Y0TXTl0gSBOFQa9B@coredump.intra.peff.net","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-11T05:42:42Z","receivedAt":"2022-10-11T05:42:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Oct 07, 2022 at 05:08:42PM +0200, René Scharfe wrote:\n>\n>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n>> index 501245fac9..28ef7ec2a4 100644\n>> --- a/builtin/bisect--helper.c\n>> +++ b/builtin/bisect--helper.c\n>> @@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n>> \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n>> \t\tstrbuf_trim(&start_head);\n>> \t\tif (!no_checkout) {\n>> -\t\t\tstruct strvec argv = STRVEC_INIT;\n>> +\t\t\tconst char *argv[] = { \"checkout\", start_head.buf,\n>> +\t\t\t\t\t \"--\", NULL };\n>> \n>> -\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n>> -\t\t\t\t \"--\", NULL);\n>> -\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n>> +\t\t\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n>\n> This is OK with me, but note that one thing we lose is compiler\n> protection that we remembered the trailing NULL pointer in the argv\n> array (whereas strvec_pushl() has an attribute that makes sure the last\n> argument is a NULL).\n\nThe first parameter to run_command_v_opt() must be a NULL terminated\narray of strings. argv.v[] after strvec_push*() is such a NULL\nterminated array, and is suitable to be passed to the function.\n\nThat much human programmers would know.\n\nBut does the compiler know that run_command_v_opt() requires a NULL\nterminated array of strings, and does it know to check that argv.v[]\ncame from strvec_pushl() without any annotation in the first place?\n\nFor such a check to happen, I think we need to tell the compiler\nwith some annotation that the first parameter to run_command_v_opt()\nis supposed to be a NULL terminated char *[] array.\n\nIn the code before the patch, strvec_pushl() is annotated to require\nthe last arg to be NULL, but without following data/control flow, it\nmay not be clear to the compiler that argv.v[] will be NULL terminated\nafter the function returns. If the compiler is sufficiently clever\nto figure it out, with the knowledge that run_command_v_opt() must\nbe given a NULL terminated array of strings, we do have compiler\nprotection to make the run_command_v_opt() safe.\n\nBut if the code tells the compiler that run_command_v_opt() must be\ngiven a NULL terminated array of strings, it is obvious that the\narray passed by the code after the patch, i.e. argv[], is NULL\nterminated, and the compiler would be happy, as well.\n\n> Probably not that big a deal in practice. It would be nice if there was\n> a way to annotate this for the compiler, but I don't think there's any\n> attribute for \"this pointer-to-pointer parameter should have a NULL at\n> the end\".\n\nBut the code before this patch is safe only for strvec_pushl() call,\nnot run_command_v_opt() call, so we are not losing anything, I would\nthink.\n\n"},{"id":"464592","messageId":"221011.86k056q0l4.gmgdl@evledraar.gmail.com","threadId":"58562","inReplyTo":"xmqq35buykz1.fsf@gitster.g","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-11T07:29:01Z","receivedAt":"2022-10-11T07:30:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Oct 10 2022, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n>> On Fri, Oct 07, 2022 at 05:08:42PM +0200, René Scharfe wrote:\n>>\n>>> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n>>> index 501245fac9..28ef7ec2a4 100644\n>>> --- a/builtin/bisect--helper.c\n>>> +++ b/builtin/bisect--helper.c\n>>> @@ -765,11 +765,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n>>> \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n>>> \t\tstrbuf_trim(&start_head);\n>>> \t\tif (!no_checkout) {\n>>> -\t\t\tstruct strvec argv = STRVEC_INIT;\n>>> +\t\t\tconst char *argv[] = { \"checkout\", start_head.buf,\n>>> +\t\t\t\t\t \"--\", NULL };\n>>> \n>>> -\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n>>> -\t\t\t\t \"--\", NULL);\n>>> -\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n>>> +\t\t\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n>>\n>> This is OK with me, but note that one thing we lose is compiler\n>> protection that we remembered the trailing NULL pointer in the argv\n>> array (whereas strvec_pushl() has an attribute that makes sure the last\n>> argument is a NULL).\n>\n> The first parameter to run_command_v_opt() must be a NULL terminated\n> array of strings. argv.v[] after strvec_push*() is such a NULL\n> terminated array, and is suitable to be passed to the function.\n>\n> That much human programmers would know.\n>\n> But does the compiler know that run_command_v_opt() requires a NULL\n> terminated array of strings, and does it know to check that argv.v[]\n> came from strvec_pushl() without any annotation in the first place?\n>\n> For such a check to happen, I think we need to tell the compiler\n> with some annotation that the first parameter to run_command_v_opt()\n> is supposed to be a NULL terminated char *[] array.\n>\n> In the code before the patch, strvec_pushl() is annotated to require\n> the last arg to be NULL, but without following data/control flow, it\n> may not be clear to the compiler that argv.v[] will be NULL terminated\n> after the function returns. If the compiler is sufficiently clever\n> to figure it out, with the knowledge that run_command_v_opt() must\n> be given a NULL terminated array of strings, we do have compiler\n> protection to make the run_command_v_opt() safe.\n>\n> But if the code tells the compiler that run_command_v_opt() must be\n> given a NULL terminated array of strings, it is obvious that the\n> array passed by the code after the patch, i.e. argv[], is NULL\n> terminated, and the compiler would be happy, as well.\n>\n>> Probably not that big a deal in practice. It would be nice if there was\n>> a way to annotate this for the compiler, but I don't think there's any\n>> attribute for \"this pointer-to-pointer parameter should have a NULL at\n>> the end\".\n>\n> But the code before this patch is safe only for strvec_pushl() call,\n> not run_command_v_opt() call, so we are not losing anything, I would\n> think.\n\nYes, and if we suppose a bug like this sneaking in one way or the other:\n\t\n\tdiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n\tindex 28ef7ec2a48..a7f9d43a6f1 100644\n\t--- a/builtin/bisect--helper.c\n\t+++ b/builtin/bisect--helper.c\n\t@@ -766,7 +766,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n\t strbuf_trim(&start_head);\n\t if (!no_checkout) {\n\t const char *argv[] = { \"checkout\", start_head.buf,\n\t- \"--\", NULL };\n\t+ \"--\" };\n\t \n\t if (run_command_v_opt(argv, RUN_GIT_CMD)) {\n\t res = error(_(\"checking out '%s' failed.\"\n\nI don't know a way to statically flag that, but we'll catch it with\nSANITIZE=address:\n\t\n\t+ git bisect start 32a594a3fdac2d57cf6d02987e30eec68511498c 88bcdc1839f0ad191ffdd65cae2a2a862d682151 --\n\t=================================================================\n\t==1734==ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7ffe6bbf5c18 at pc 0x55d31729bc7a bp 0x7ffe6bbf5920 sp 0x7ffe6bbf5918\n\tREAD of size 8 at 0x7ffe6bbf5c18 thread T0\n\t #0 0x55d31729bc79 in strvec_pushv strvec.c:55\n\t #1 0x55d317232be9 in run_command_v_opt_cd_env_tr2 run-command.c:1026\n\t #2 0x55d317232a3c in run_command_v_opt_cd_env run-command.c:1019\n\t #3 0x55d3172329d1 in run_command_v_opt run-command.c:1009\n\t #4 0x55d316c1337a in bisect_start builtin/bisect--helper.c:771\n\t #5 0x55d316c17d06 in cmd_bisect__helper builtin/bisect--helper.c:1346\n\t #6 0x55d316bf190f in run_builtin git.c:466\n\t #7 0x55d316bf22bb in handle_builtin git.c:721\n\t #8 0x55d316bf29e5 in run_argv git.c:788\n\t #9 0x55d316bf375f in cmd_main git.c:921\n\t #10 0x55d316e79a06 in main common-main.c:57\n\t #11 0x7fceab0a0209 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58\n\t #12 0x7fceab0a02bb in __libc_start_main_impl ../csu/libc-start.c:389\n\t #13 0x55d316bed210 in _start (git+0x1d7210)\n\nSo I'm not worried about it happening in the first place, but if it does\nwe'd also need to have no test coverage of the relevant code to miss it.\n"},{"id":"464607","messageId":"Y0VtkmNwjKcXcemP@coredump.intra.peff.net","threadId":"58562","inReplyTo":"xmqq35buykz1.fsf@gitster.g","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T13:20:18Z","receivedAt":"2022-10-11T13:21:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 10, 2022 at 10:42:42PM -0700, Junio C Hamano wrote:\n\n> >> -\t\t\tstruct strvec argv = STRVEC_INIT;\n> >> +\t\t\tconst char *argv[] = { \"checkout\", start_head.buf,\n> >> +\t\t\t\t\t \"--\", NULL };\n> >> \n> >> -\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n> >> -\t\t\t\t \"--\", NULL);\n> >> -\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n> >> +\t\t\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n> >\n> > This is OK with me, but note that one thing we lose is compiler\n> > protection that we remembered the trailing NULL pointer in the argv\n> > array (whereas strvec_pushl() has an attribute that makes sure the last\n> > argument is a NULL).\n> \n> The first parameter to run_command_v_opt() must be a NULL terminated\n> array of strings. argv.v[] after strvec_push*() is such a NULL\n> terminated array, and is suitable to be passed to the function.\n> \n> That much human programmers would know.\n> \n> But does the compiler know that run_command_v_opt() requires a NULL\n> terminated array of strings, and does it know to check that argv.v[]\n> came from strvec_pushl() without any annotation in the first place?\n\nNo, but I don't think that's the interesting part. If you're using\nstrvec, it does the right thing, and it's hard to get it wrong. I'm more\nconcerned about places where we manually write a list of strings, and\nit's easy to forget the trailing NULL.\n\nIn the existing code, that's done in the interface of strvec_pushl(),\nwhich will remind you if you write:\n\n strvec_pushl(&arg, \"checkout\", start_head.buf, \"--\");\n\nBut after it is done in an initializer, which has no clue about the\nexpected semantics. We only have to get strvec's invariants right once.\nBut every ad-hoc command argv has to remember the trailing NULL.\n\n> For such a check to happen, I think we need to tell the compiler\n> with some annotation that the first parameter to run_command_v_opt()\n> is supposed to be a NULL terminated char *[] array.\n\nRight, but I would not expect the compiler to realize that strvec\nmaintains the ends-in-NULL invariant. It would have to be quite a clever\ncompiler.\n\nIn theory it could realize that argv is declared as an array locally,\nand could make sure it ends in NULL as a compile-time check.\n\nSo it would have to be: \"check this statically if you can, but otherwise\nassume it's OK\" kind of warning. But it's all kind of moot since I don't\nthink any such annotation exists. :)\n\nPossibly a linter like sparse could complain about declaring a variable\ncalled argv that doesn't end in NULL. I don't think it's worth anybody\nspending too much time on it, though. This hasn't historically been a\nbig source of bugs.\n\n> > Probably not that big a deal in practice. It would be nice if there was\n> > a way to annotate this for the compiler, but I don't think there's any\n> > attribute for \"this pointer-to-pointer parameter should have a NULL at\n> > the end\".\n> \n> But the code before this patch is safe only for strvec_pushl() call,\n> not run_command_v_opt() call, so we are not losing anything, I would\n> think.\n\nThe bug I'm worried about it is in a human writing the list of strings\nand forgetting the NULL, so there we are losing the (admittedly minor)\nprotection.\n\n-Peff\n"},{"id":"464608","messageId":"Y0Vt4eAfUET1GEIp@coredump.intra.peff.net","threadId":"58562","inReplyTo":"221011.86k056q0l4.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-11T13:21:37Z","receivedAt":"2022-10-11T13:22:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 11, 2022 at 09:29:01AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > But the code before this patch is safe only for strvec_pushl() call,\n> > not run_command_v_opt() call, so we are not losing anything, I would\n> > think.\n> \n> Yes, and if we suppose a bug like this sneaking in one way or the other:\n> \t\n> \tdiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> \tindex 28ef7ec2a48..a7f9d43a6f1 100644\n> \t--- a/builtin/bisect--helper.c\n> \t+++ b/builtin/bisect--helper.c\n> \t@@ -766,7 +766,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n> \t strbuf_trim(&start_head);\n> \t if (!no_checkout) {\n> \t const char *argv[] = { \"checkout\", start_head.buf,\n> \t- \"--\", NULL };\n> \t+ \"--\" };\n> \t \n> \t if (run_command_v_opt(argv, RUN_GIT_CMD)) {\n> \t res = error(_(\"checking out '%s' failed.\"\n> \n> I don't know a way to statically flag that, but we'll catch it with\n> SANITIZE=address:\n\nI'd expect we'd even catch it in a non-sanitizing build, since we'd\nlikely feed garbage to exec (unless we get lucky and there's a NULL on\nthe stack). I like catching bugs as early as possible, but I agree this\nkind is not likely to get very far (assuming there's test coverage).\n\n-Peff\n"},{"id":"464623","messageId":"xmqqpmeyuvxt.fsf@gitster.g","threadId":"58562","inReplyTo":"Y0VtkmNwjKcXcemP@coredump.intra.peff.net","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-11T17:11:42Z","receivedAt":"2022-10-11T17:11:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The bug I'm worried about it is in a human writing the list of strings\n> and forgetting the NULL, so there we are losing the (admittedly minor)\n> protection.\n\nI expect that this story will repeat itself, especially given that\nwe asserted that it is OK to initialize such an array with variable\nreference recently in this thread.\n\nHere are a couple that I found with a quick eyeballing of the output\nof \"git grep -e 'run_command_v_opt([^,]*\\.v,' \\*.c\" command.\n\n\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ed8d44bb6a..c93345bc75 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -651,9 +651,8 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \n static int git_sparse_checkout_init(const char *repo)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n \tint result = 0;\n-\tstrvec_pushl(&argv, \"-C\", repo, \"sparse-checkout\", \"set\", NULL);\n+\tconst char *argv[] = { \"-C\", repo, \"sparse-checkout\", \"set\", NULL };\n \n \t/*\n \t * We must apply the setting in the current process\n@@ -661,12 +660,11 @@ static int git_sparse_checkout_init(const char *repo)\n \t */\n \tcore_apply_sparse_checkout = 1;\n \n-\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n \t\terror(_(\"failed to initialize sparse-checkout\"));\n \t\tresult = 1;\n \t}\n \n-\tstrvec_clear(&argv);\n \treturn result;\n }\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 0a0ca8b7c4..d261bc652f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -384,24 +384,20 @@ static void reset_hard(const struct object_id *oid, int verbose)\n static void restore_state(const struct object_id *head,\n \t\t\t const struct object_id *stash)\n {\n-\tstruct strvec args = STRVEC_INIT;\n-\n \treset_hard(head, 1);\n \n-\tif (is_null_oid(stash))\n-\t\tgoto refresh_cache;\n-\n-\tstrvec_pushl(&args, \"stash\", \"apply\", \"--index\", \"--quiet\", NULL);\n-\tstrvec_push(&args, oid_to_hex(stash));\n+\tif (!is_null_oid(stash)) {\n+\t\tconst char *argv[] = {\n+\t\t\t\"stash\", \"apply\", \"--index\", \"--quiet\", oid_to_hex(stash), NULL\n+\t\t};\n \n-\t/*\n-\t * It is OK to ignore error here, for example when there was\n-\t * nothing to restore.\n-\t */\n-\trun_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n+\t\t/*\n+\t\t * It is OK to ignore error here, for example when there was\n+\t\t * nothing to restore.\n+\t\t */\n+\t\trun_command_v_opt(argv, RUN_GIT_CMD);\n+\t}\n \n-refresh_cache:\n \tif (discard_cache() < 0 || read_cache() < 0)\n \t\tdie(_(\"could not read index\"));\n }\n"},{"id":"464627","messageId":"221011.86czayns5x.gmgdl@evledraar.gmail.com","threadId":"58562","inReplyTo":"xmqqpmeyuvxt.fsf@gitster.g","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-10-11T18:13:20Z","receivedAt":"2022-10-11T18:15:17Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Oct 11 2022, Junio C Hamano wrote:\n\n> Jeff King writes:\n>\n>> The bug I'm worried about it is in a human writing the list of strings\n>> and forgetting the NULL, so there we are losing the (admittedly minor)\n>> protection.\n>\n> I expect that this story will repeat itself, especially given that\n> we asserted that it is OK to initialize such an array with variable\n> reference recently in this thread.\n>\n> Here are a couple that I found with a quick eyeballing of the output\n> of \"git grep -e 'run_command_v_opt([^,]*\\.v,' \\*.c\" command.\n>\n>\n>\n> diff --git a/builtin/clone.c b/builtin/clone.c\n> index ed8d44bb6a..c93345bc75 100644\n> --- a/builtin/clone.c\n> +++ b/builtin/clone.c\n> @@ -651,9 +651,8 @@ static void update_head(const struct ref *our, const struct ref *remote,\n> \n> static int git_sparse_checkout_init(const char *repo)\n> {\n> -\tstruct strvec argv = STRVEC_INIT;\n> \tint result = 0;\n> -\tstrvec_pushl(&argv, \"-C\", repo, \"sparse-checkout\", \"set\", NULL);\n> +\tconst char *argv[] = { \"-C\", repo, \"sparse-checkout\", \"set\", NULL };\n> \n> \t/*\n> \t * We must apply the setting in the current process\n> @@ -661,12 +660,11 @@ static int git_sparse_checkout_init(const char *repo)\n> \t */\n> \tcore_apply_sparse_checkout = 1;\n> \n> -\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n> +\tif (run_command_v_opt(argv, RUN_GIT_CMD)) {\n> \t\terror(_(\"failed to initialize sparse-checkout\"));\n> \t\tresult = 1;\n> \t}\n> \n> -\tstrvec_clear(&argv);\n> \treturn result;\n> }\n> \n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 0a0ca8b7c4..d261bc652f 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -384,24 +384,20 @@ static void reset_hard(const struct object_id *oid, int verbose)\n> static void restore_state(const struct object_id *head,\n> \t\t\t const struct object_id *stash)\n> {\n> -\tstruct strvec args = STRVEC_INIT;\n> -\n> \treset_hard(head, 1);\n> \n> -\tif (is_null_oid(stash))\n> -\t\tgoto refresh_cache;\n> -\n> -\tstrvec_pushl(&args, \"stash\", \"apply\", \"--index\", \"--quiet\", NULL);\n> -\tstrvec_push(&args, oid_to_hex(stash));\n> +\tif (!is_null_oid(stash)) {\n> +\t\tconst char *argv[] = {\n> +\t\t\t\"stash\", \"apply\", \"--index\", \"--quiet\", oid_to_hex(stash), NULL\n> +\t\t};\n> \n> -\t/*\n> -\t * It is OK to ignore error here, for example when there was\n> -\t * nothing to restore.\n> -\t */\n> -\trun_command_v_opt(args.v, RUN_GIT_CMD);\n> -\tstrvec_clear(&args);\n> +\t\t/*\n> +\t\t * It is OK to ignore error here, for example when there was\n> +\t\t * nothing to restore.\n> +\t\t */\n> +\t\trun_command_v_opt(argv, RUN_GIT_CMD);\n> +\t}\n> \n> -refresh_cache:\n> \tif (discard_cache() < 0 || read_cache() < 0)\n> \t\tdie(_(\"could not read index\"));\n> }\n\nI was experimenting with implementing a run_command_opt_l() earlier\nwhich would give us the safety Jeff notes. The relevant end-state for\nthese two files is (there's more conversions, and I manually edited the\ndiff to remove an unrelated change):\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ed8d44bb6ab..8dc986b5196 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -651,23 +651,18 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \n static int git_sparse_checkout_init(const char *repo)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n-\tint result = 0;\n-\tstrvec_pushl(&argv, \"-C\", repo, \"sparse-checkout\", \"set\", NULL);\n-\n \t/*\n \t * We must apply the setting in the current process\n \t * for the later checkout to use the sparse-checkout file.\n \t */\n \tcore_apply_sparse_checkout = 1;\n \n-\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\tif (run_command_opt_l(RUN_GIT_CMD, \"-C\", repo, \"sparse-checkout\",\n+\t\t\t \"set\", NULL)) {\n \t\terror(_(\"failed to initialize sparse-checkout\"));\n-\t\tresult = 1;\n+\t\treturn 1;\n \t}\n-\n-\tstrvec_clear(&argv);\n-\treturn result;\n+\treturn 0;\n }\n \n@@ -862,11 +856,11 @@ static void write_refspec_config(const char *src_ref_prefix,\n \n static void dissociate_from_references(void)\n {\n-\tstatic const char* argv[] = { \"repack\", \"-a\", \"-d\", NULL };\n \tchar *alternates = git_pathdup(\"objects/info/alternates\");\n \n \tif (!access(alternates, F_OK)) {\n-\t\tif (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))\n+\t\tif (run_command_opt_l(RUN_GIT_CMD|RUN_COMMAND_NO_STDIN,\n+\t\t\t\t \"repack\", \"-a\", \"-d\", NULL))\n \t\t\tdie(_(\"cannot repack to clean up\"));\n \t\tif (unlink(alternates) && errno != ENOENT)\n \t\t\tdie_errno(_(\"cannot unlink temporary alternates file\"));\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5900b81729d..9c08de57113 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -345,60 +345,34 @@ static int save_state(struct object_id *stash)\n \treturn rc;\n }\n \n-static void read_empty(const struct object_id *oid, int verbose)\n+static void read_empty(const struct object_id *oid)\n {\n-\tint i = 0;\n-\tconst char *args[7];\n-\n-\targs[i++] = \"read-tree\";\n-\tif (verbose)\n-\t\targs[i++] = \"-v\";\n-\targs[i++] = \"-m\";\n-\targs[i++] = \"-u\";\n-\targs[i++] = empty_tree_oid_hex();\n-\targs[i++] = oid_to_hex(oid);\n-\targs[i] = NULL;\n-\n-\tif (run_command_v_opt(args, RUN_GIT_CMD))\n+\tif (run_command_opt_l(RUN_GIT_CMD, \"read-tree\", \"-m\", \"-u\",\n+\t\t\t empty_tree_oid_hex(), oid_to_hex(oid), NULL))\n \t\tdie(_(\"read-tree failed\"));\n }\n \n-static void reset_hard(const struct object_id *oid, int verbose)\n+static void reset_hard(const struct object_id *oid)\n {\n-\tint i = 0;\n-\tconst char *args[6];\n-\n-\targs[i++] = \"read-tree\";\n-\tif (verbose)\n-\t\targs[i++] = \"-v\";\n-\targs[i++] = \"--reset\";\n-\targs[i++] = \"-u\";\n-\targs[i++] = oid_to_hex(oid);\n-\targs[i] = NULL;\n-\n-\tif (run_command_v_opt(args, RUN_GIT_CMD))\n+\tif (run_command_opt_l(RUN_GIT_CMD, \"read-tree\", \"-v\", \"--reset\", \"-u\",\n+\t\t\t oid_to_hex(oid), NULL))\n \t\tdie(_(\"read-tree failed\"));\n }\n \n static void restore_state(const struct object_id *head,\n \t\t\t const struct object_id *stash)\n {\n-\tstruct strvec args = STRVEC_INIT;\n-\n-\treset_hard(head, 1);\n+\treset_hard(head);\n \n \tif (is_null_oid(stash))\n \t\tgoto refresh_cache;\n \n-\tstrvec_pushl(&args, \"stash\", \"apply\", \"--index\", \"--quiet\", NULL);\n-\tstrvec_push(&args, oid_to_hex(stash));\n-\n \t/*\n \t * It is OK to ignore error here, for example when there was\n \t * nothing to restore.\n \t */\n-\trun_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n+\trun_command_opt_l(RUN_GIT_CMD, \"stash\", \"apply\", \"--index\", \"--quiet\",\n+\t\t\t oid_to_hex(stash), NULL);\n \n refresh_cache:\n \tif (discard_cache() < 0 || read_cache() < 0)\n@@ -1470,7 +1444,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t check_trust_level);\n \n \t\tremote_head_oid = &remoteheads->item->object.oid;\n-\t\tread_empty(remote_head_oid, 0);\n+\t\tread_empty(remote_head_oid);\n \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head_oid, NULL, 0,\n \t\t\t UPDATE_REFS_DIE_ON_ERR);\n \t\tgoto done;\n"},{"id":"464633","messageId":"xmqq8rlmujcz.fsf@gitster.g","threadId":"58562","inReplyTo":"221011.86czayns5x.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-11T21:43:24Z","receivedAt":"2022-10-11T21:43:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I was experimenting with implementing a run_command_opt_l() earlier\n> which would give us the safety Jeff notes. The relevant end-state for\n> these two files is (there's more conversions, and I manually edited the\n> diff to remove an unrelated change):\n\nThere still are some changes that would not belong here, but for the\npurpose of illustrating the usage of _l variant, this is good enough.\nOf course, if we want to follow the existing naming scheme, the new\nfunction must be called _l_opt(), not _opt_l(). _v_ in the _v_opt()\ncomes from execv() and you are adding execl() analogue here.\n\n> -\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n> +\tif (run_command_opt_l(RUN_GIT_CMD, \"-C\", repo, \"sparse-checkout\",\n> +\t\t\t \"set\", NULL)) {\n\nAnd this does give us protection from the \"Programmers can give\nunterminated list to run_command_v_opt() by mistake\", which is not\nreally solved mechanically even if the list is prepared with the\nstrvec API (because the compiler has to be smart enough to know that\nargv.v was prepared with proper use of the API), which is nice.\n\nHaving to give the option flags as the first argument, of course it\nis necessary because we are talking about the _l variant with\nvarargs, looks ugly, though.\n\nAlso, I am not sure how useful this new function actually is. Many\nhits from my quick \"grep\" in the message you are responding to did\nuse strvec because they wanted to dynamically build up the command\nline, and _l variant does not really shine in these places.\n\nWe may probably be able to refactor some (but not all) of these\nsites so that the command line is not built dynamically with unknown\nnumber of elements in unknown order, but that feels like a solution\nlooking for a problem, changing code to use the new function, tail\nwagging the dog. So, I dunno...\n"},{"id":"464981","messageId":"Y0m8AU+Yf7PREI2E@coredump.intra.peff.net","threadId":"58562","inReplyTo":"xmqq8rlmujcz.fsf@gitster.g","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-14T19:44:01Z","receivedAt":"2022-10-14T19:44:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 11, 2022 at 02:43:24PM -0700, Junio C Hamano wrote:\n\n> > -\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n> > +\tif (run_command_opt_l(RUN_GIT_CMD, \"-C\", repo, \"sparse-checkout\",\n> > +\t\t\t \"set\", NULL)) {\n> \n> And this does give us protection from the \"Programmers can give\n> unterminated list to run_command_v_opt() by mistake\", which is not\n> really solved mechanically even if the list is prepared with the\n> strvec API (because the compiler has to be smart enough to know that\n> argv.v was prepared with proper use of the API), which is nice.\n\nYeah, I agree this addresses the point I raised (which I am somewhat\nregretting raising, as IMHO it was not worth the amount of discussion\nthat has ensued).\n\nSince nobody asked, my _real_ opinion is that I prefer René's original\nthat used an actual struct, and its auto-freeing strvec. And I'd be\nhappy to see all of the run_command_v() variants go away entirely. It\ndoes save a few lines, but with modern niceties, it's not very many, and\nit's much less flexible.\n\nI resisted saying so earlier because I do not think it is even worth\nanybody's time to think about, let alone implement.\n\n-Peff\n"},{"id":"464985","messageId":"xmqqczau888n.fsf@gitster.g","threadId":"58562","inReplyTo":"Y0m8AU+Yf7PREI2E@coredump.intra.peff.net","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-14T20:23:36Z","receivedAt":"2022-10-14T20:23:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Oct 11, 2022 at 02:43:24PM -0700, Junio C Hamano wrote:\n>\n>> > -\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n>> > +\tif (run_command_opt_l(RUN_GIT_CMD, \"-C\", repo, \"sparse-checkout\",\n>> > +\t\t\t \"set\", NULL)) {\n>> \n>> And this does give us protection from the \"Programmers can give\n>> unterminated list to run_command_v_opt() by mistake\", which is not\n>> really solved mechanically even if the list is prepared with the\n>> strvec API (because the compiler has to be smart enough to know that\n>> argv.v was prepared with proper use of the API), which is nice.\n>\n> Yeah, I agree this addresses the point I raised (which I am somewhat\n> regretting raising, as IMHO it was not worth the amount of discussion\n> that has ensued).\n>\n> Since nobody asked, my _real_ opinion is that I prefer René's original\n> that used an actual struct, and its auto-freeing strvec.\n\nYup, it was you who worried about forgotten NULL at the end, though\n;-)\n"},{"id":"465014","messageId":"9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de","threadId":"58562","inReplyTo":"Y0m8AU+Yf7PREI2E@coredump.intra.peff.net","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2022-10-15T06:51:34Z","receivedAt":"2022-10-15T06:51:57Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 14.10.22 um 21:44 schrieb Jeff King:\n> And I'd be happy to see all of the run_command_v() variants go away\n> entirely. It does save a few lines, but with modern niceties, it's\n> not very many, and it's much less flexible.\nThat would look something like below.\n\n---\n add-interactive.c | 9 ++-\n bisect.c | 12 ++--\n builtin/add.c | 19 +++--\n builtin/am.c | 12 ++--\n builtin/bisect--helper.c | 64 ++++++++---------\n builtin/clone.c | 41 ++++++-----\n builtin/difftool.c | 24 ++++---\n builtin/fetch.c | 9 ++-\n builtin/gc.c | 51 ++++++++------\n builtin/merge-index.c | 4 +-\n builtin/merge.c | 42 +++++------\n builtin/pull.c | 147 +++++++++++++++++++--------------------\n builtin/remote.c | 40 +++++------\n compat/mingw.c | 11 +--\n diff.c | 27 ++++---\n fsmonitor-ipc.c | 10 ++-\n git.c | 15 ++--\n ll-merge.c | 7 +-\n merge.c | 18 ++---\n run-command.c | 35 ----------\n run-command.h | 30 --------\n scalar.c | 13 ++--\n sequencer.c | 32 ++++-----\n shell.c | 17 +++--\n t/helper/test-fake-ssh.c | 7 +-\n t/helper/test-trace2.c | 4 +-\n tmp-objdir.h | 4 +-\n 27 files changed, 330 insertions(+), 374 deletions(-)\n\ndiff --git a/add-interactive.c b/add-interactive.c\nindex f071b2a1b4..ecc5ae1b24 100644\n--- a/add-interactive.c\n+++ b/add-interactive.c\n@@ -997,18 +997,17 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,\n \tcount = list_and_choose(s, files, opts);\n \topts->flags = 0;\n \tif (count > 0) {\n-\t\tstruct strvec args = STRVEC_INIT;\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\t\tstrvec_pushl(&args, \"git\", \"diff\", \"-p\", \"--cached\",\n+\t\tstrvec_pushl(&cmd.args, \"git\", \"diff\", \"-p\", \"--cached\",\n \t\t\t oid_to_hex(!is_initial ? &oid :\n \t\t\t\t\ts->r->hash_algo->empty_tree),\n \t\t\t \"--\", NULL);\n \t\tfor (i = 0; i < files->items.nr; i++)\n \t\t\tif (files->selected[i])\n-\t\t\t\tstrvec_push(&args,\n+\t\t\t\tstrvec_push(&cmd.args,\n \t\t\t\t\t files->items.items[i].string);\n-\t\tres = run_command_v_opt(args.v, 0);\n-\t\tstrvec_clear(&args);\n+\t\tres = run_command(&cmd);\n \t}\n\n \tputchar('\\n');\ndiff --git a/bisect.c b/bisect.c\nindex fd581b85a7..ec7487e683 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -22,8 +22,6 @@ static struct oid_array skipped_revs;\n\n static struct object_id *current_bad_oid;\n\n-static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n-\n static const char *term_bad;\n static const char *term_good;\n\n@@ -729,20 +727,22 @@ static int is_expected_rev(const struct object_id *oid)\n enum bisect_error bisect_checkout(const struct object_id *bisect_rev,\n \t\t\t\t int no_checkout)\n {\n-\tchar bisect_rev_hex[GIT_MAX_HEXSZ + 1];\n \tstruct commit *commit;\n \tstruct pretty_print_context pp = {0};\n \tstruct strbuf commit_msg = STRBUF_INIT;\n\n-\toid_to_hex_r(bisect_rev_hex, bisect_rev);\n \tupdate_ref(NULL, \"BISECT_EXPECTED_REV\", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n\n-\targv_checkout[2] = bisect_rev_hex;\n \tif (no_checkout) {\n \t\tupdate_ref(NULL, \"BISECT_HEAD\", bisect_rev, NULL, 0,\n \t\t\t UPDATE_REFS_DIE_ON_ERR);\n \t} else {\n-\t\tif (run_command_v_opt(argv_checkout, RUN_GIT_CMD))\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\t\tcmd.git_cmd = 1;\n+\t\tstrvec_pushl(&cmd.args, \"checkout\", \"-q\",\n+\t\t\t oid_to_hex(bisect_rev), \"--\", NULL);\n+\t\tif (run_command(&cmd))\n \t\t\t/*\n \t\t\t * Errors in `run_command()` itself, signaled by res < 0,\n \t\t\t * and errors in the child process, signaled by res > 0\ndiff --git a/builtin/add.c b/builtin/add.c\nindex f84372964c..626c71ec6a 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -240,8 +240,8 @@ static int refresh(int verbose, const struct pathspec *pathspec)\n int run_add_interactive(const char *revision, const char *patch_mode,\n \t\t\tconst struct pathspec *pathspec)\n {\n-\tint status, i;\n-\tstruct strvec argv = STRVEC_INIT;\n+\tint i;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint use_builtin_add_i =\n \t\tgit_env_bool(\"GIT_TEST_ADD_I_USE_BUILTIN\", -1);\n\n@@ -272,19 +272,18 @@ int run_add_interactive(const char *revision, const char *patch_mode,\n \t\treturn !!run_add_p(the_repository, mode, revision, pathspec);\n \t}\n\n-\tstrvec_push(&argv, \"add--interactive\");\n+\tstrvec_push(&cmd.args, \"add--interactive\");\n \tif (patch_mode)\n-\t\tstrvec_push(&argv, patch_mode);\n+\t\tstrvec_push(&cmd.args, patch_mode);\n \tif (revision)\n-\t\tstrvec_push(&argv, revision);\n-\tstrvec_push(&argv, \"--\");\n+\t\tstrvec_push(&cmd.args, revision);\n+\tstrvec_push(&cmd.args, \"--\");\n \tfor (i = 0; i < pathspec->nr; i++)\n \t\t/* pass original pathspec, to be re-parsed */\n-\t\tstrvec_push(&argv, pathspec->items[i].original);\n+\t\tstrvec_push(&cmd.args, pathspec->items[i].original);\n\n-\tstatus = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&argv);\n-\treturn status;\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n\n int interactive_add(const char **argv, const char *prefix, int patch)\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 39fea24833..20aea0d248 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2187,14 +2187,12 @@ static int show_patch(struct am_state *state, enum show_patch_type sub_mode)\n \tint len;\n\n \tif (!is_null_oid(&state->orig_commit)) {\n-\t\tconst char *av[4] = { \"show\", NULL, \"--\", NULL };\n-\t\tchar *new_oid_str;\n-\t\tint ret;\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\t\tav[1] = new_oid_str = xstrdup(oid_to_hex(&state->orig_commit));\n-\t\tret = run_command_v_opt(av, RUN_GIT_CMD);\n-\t\tfree(new_oid_str);\n-\t\treturn ret;\n+\t\tstrvec_pushl(&cmd.args, \"show\", oid_to_hex(&state->orig_commit),\n+\t\t\t \"--\", NULL);\n+\t\tcmd.git_cmd = 1;\n+\t\treturn run_command(&cmd);\n \t}\n\n \tswitch (sub_mode) {\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 501245fac9..542f0cb8f5 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -220,18 +220,17 @@ static int bisect_reset(const char *commit)\n \t}\n\n \tif (!ref_exists(\"BISECT_HEAD\")) {\n-\t\tstruct strvec argv = STRVEC_INIT;\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\t\tstrvec_pushl(&argv, \"checkout\", branch.buf, \"--\", NULL);\n-\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\t\tstrvec_pushl(&cmd.args, \"checkout\", branch.buf, \"--\", NULL);\n+\t\tcmd.git_cmd = 1;\n+\t\tif (run_command(&cmd)) {\n \t\t\terror(_(\"could not check out original\"\n \t\t\t\t\" HEAD '%s'. Try 'git bisect\"\n \t\t\t\t\" reset '.\"), branch.buf);\n \t\t\tstrbuf_release(&branch);\n-\t\t\tstrvec_clear(&argv);\n \t\t\treturn -1;\n \t\t}\n-\t\tstrvec_clear(&argv);\n \t}\n\n \tstrbuf_release(&branch);\n@@ -765,11 +764,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a\n \t\tstrbuf_read_file(&start_head, git_path_bisect_start(), 0);\n \t\tstrbuf_trim(&start_head);\n \t\tif (!no_checkout) {\n-\t\t\tstruct strvec argv = STRVEC_INIT;\n+\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\t\t\tstrvec_pushl(&argv, \"checkout\", start_head.buf,\n+\t\t\tstrvec_pushl(&cmd.args, \"checkout\", start_head.buf,\n \t\t\t\t \"--\", NULL);\n-\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\t\t\tcmd.git_cmd = 1;\n+\t\t\tif (run_command(&cmd)) {\n \t\t\t\tres = error(_(\"checking out '%s' failed.\"\n \t\t\t\t\t\t \" Try 'git bisect start \"\n \t\t\t\t\t\t \"'.\"),\n@@ -1099,40 +1099,38 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar\n\n static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)\n {\n-\tstruct strvec args = STRVEC_INIT;\n-\tint flags = RUN_COMMAND_NO_STDIN, res = 0;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct strbuf sb = STRBUF_INIT;\n\n \tif (bisect_next_check(terms, NULL) != 0)\n \t\treturn BISECT_FAILED;\n\n+\tcmd.no_stdin = 1;\n \tif (!argc) {\n \t\tif ((getenv(\"DISPLAY\") || getenv(\"SESSIONNAME\") || getenv(\"MSYSTEM\") ||\n \t\t getenv(\"SECURITYSESSIONID\")) && exists_in_PATH(\"gitk\")) {\n-\t\t\tstrvec_push(&args, \"gitk\");\n+\t\t\tstrvec_push(&cmd.args, \"gitk\");\n \t\t} else {\n-\t\t\tstrvec_push(&args, \"log\");\n-\t\t\tflags |= RUN_GIT_CMD;\n+\t\t\tstrvec_push(&cmd.args, \"log\");\n+\t\t\tcmd.git_cmd = 1;\n \t\t}\n \t} else {\n \t\tif (argv[0][0] == '-') {\n-\t\t\tstrvec_push(&args, \"log\");\n-\t\t\tflags |= RUN_GIT_CMD;\n+\t\t\tstrvec_push(&cmd.args, \"log\");\n+\t\t\tcmd.git_cmd = 1;\n \t\t} else if (strcmp(argv[0], \"tig\") && !starts_with(argv[0], \"git\"))\n-\t\t\tflags |= RUN_GIT_CMD;\n+\t\t\tcmd.git_cmd = 1;\n\n-\t\tstrvec_pushv(&args, argv);\n+\t\tstrvec_pushv(&cmd.args, argv);\n \t}\n\n-\tstrvec_pushl(&args, \"--bisect\", \"--\", NULL);\n+\tstrvec_pushl(&cmd.args, \"--bisect\", \"--\", NULL);\n\n \tstrbuf_read_file(&sb, git_path_bisect_names(), 0);\n-\tsq_dequote_to_strvec(sb.buf, &args);\n+\tsq_dequote_to_strvec(sb.buf, &cmd.args);\n \tstrbuf_release(&sb);\n\n-\tres = run_command_v_opt(args.v, flags);\n-\tstrvec_clear(&args);\n-\treturn res;\n+\treturn run_command(&cmd);\n }\n\n static int get_first_good(const char *refname UNUSED,\n@@ -1143,8 +1141,7 @@ static int get_first_good(const char *refname UNUSED,\n \treturn 1;\n }\n\n-static int verify_good(const struct bisect_terms *terms,\n-\t\t const char **quoted_argv)\n+static int verify_good(const struct bisect_terms *terms, const char *command)\n {\n \tint rc;\n \tenum bisect_error res;\n@@ -1152,6 +1149,7 @@ static int verify_good(const struct bisect_terms *terms,\n \tstruct object_id current_rev;\n \tchar *good_glob = xstrfmt(\"%s-*\", terms->term_good);\n \tint no_checkout = ref_exists(\"BISECT_HEAD\");\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n \tfor_each_glob_ref_in(get_first_good, good_glob, \"refs/bisect/\",\n \t\t\t &good_rev);\n@@ -1164,8 +1162,10 @@ static int verify_good(const struct bisect_terms *terms,\n \tif (res != BISECT_OK)\n \t\treturn -1;\n\n-\tprintf(_(\"running %s\\n\"), quoted_argv[0]);\n-\trc = run_command_v_opt(quoted_argv, RUN_USING_SHELL);\n+\tprintf(_(\"running %s\\n\"), command);\n+\tstrvec_push(&cmd.args, command);\n+\tcmd.use_shell = 1;\n+\trc = run_command(&cmd);\n\n \tres = bisect_checkout(¤t_rev, no_checkout);\n \tif (res != BISECT_OK)\n@@ -1178,7 +1178,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)\n {\n \tint res = BISECT_OK;\n \tstruct strbuf command = STRBUF_INIT;\n-\tstruct strvec run_args = STRVEC_INIT;\n \tconst char *new_state;\n \tint temporary_stdout_fd, saved_stdout;\n \tint is_first_run = 1;\n@@ -1193,11 +1192,13 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)\n \t\treturn BISECT_FAILED;\n \t}\n\n-\tstrvec_push(&run_args, command.buf);\n-\n \twhile (1) {\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n \t\tprintf(_(\"running %s\\n\"), command.buf);\n-\t\tres = run_command_v_opt(run_args.v, RUN_USING_SHELL);\n+\t\tstrvec_push(&cmd.args, command.buf);\n+\t\tcmd.use_shell = 1;\n+\t\tres = run_command(&cmd);\n\n \t\t/*\n \t\t * Exit code 126 and 127 can either come from the shell\n@@ -1207,7 +1208,7 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)\n \t\t * missing or non-executable script.\n \t\t */\n \t\tif (is_first_run && (res == 126 || res == 127)) {\n-\t\t\tint rc = verify_good(terms, run_args.v);\n+\t\t\tint rc = verify_good(terms, command.buf);\n \t\t\tis_first_run = 0;\n \t\t\tif (rc < 0) {\n \t\t\t\terror(_(\"unable to verify '%s' on good\"\n@@ -1274,7 +1275,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)\n \t}\n\n \tstrbuf_release(&command);\n-\tstrvec_clear(&run_args);\n \treturn res;\n }\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex ed8d44bb6a..219cfbd735 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -651,9 +651,9 @@ static void update_head(const struct ref *our, const struct ref *remote,\n\n static int git_sparse_checkout_init(const char *repo)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint result = 0;\n-\tstrvec_pushl(&argv, \"-C\", repo, \"sparse-checkout\", \"set\", NULL);\n+\tstrvec_pushl(&cmd.args, \"-C\", repo, \"sparse-checkout\", \"set\", NULL);\n\n \t/*\n \t * We must apply the setting in the current process\n@@ -661,12 +661,12 @@ static int git_sparse_checkout_init(const char *repo)\n \t */\n \tcore_apply_sparse_checkout = 1;\n\n-\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\tcmd.git_cmd = 1;\n+\tif (run_command(&cmd)) {\n \t\terror(_(\"failed to initialize sparse-checkout\"));\n \t\tresult = 1;\n \t}\n\n-\tstrvec_clear(&argv);\n \treturn result;\n }\n\n@@ -731,37 +731,38 @@ static int checkout(int submodule_progress, int filter_submodules)\n \t\t\t oid_to_hex(&oid), \"1\", NULL);\n\n \tif (!err && (option_recurse_submodules.nr > 0)) {\n-\t\tstruct strvec args = STRVEC_INIT;\n-\t\tstrvec_pushl(&args, \"submodule\", \"update\", \"--require-init\", \"--recursive\", NULL);\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\t\tstrvec_pushl(&cmd.args, \"submodule\", \"update\", \"--require-init\",\n+\t\t\t \"--recursive\", NULL);\n\n \t\tif (option_shallow_submodules == 1)\n-\t\t\tstrvec_push(&args, \"--depth=1\");\n+\t\t\tstrvec_push(&cmd.args, \"--depth=1\");\n\n \t\tif (max_jobs != -1)\n-\t\t\tstrvec_pushf(&args, \"--jobs=%d\", max_jobs);\n+\t\t\tstrvec_pushf(&cmd.args, \"--jobs=%d\", max_jobs);\n\n \t\tif (submodule_progress)\n-\t\t\tstrvec_push(&args, \"--progress\");\n+\t\t\tstrvec_push(&cmd.args, \"--progress\");\n\n \t\tif (option_verbosity < 0)\n-\t\t\tstrvec_push(&args, \"--quiet\");\n+\t\t\tstrvec_push(&cmd.args, \"--quiet\");\n\n \t\tif (option_remote_submodules) {\n-\t\t\tstrvec_push(&args, \"--remote\");\n-\t\t\tstrvec_push(&args, \"--no-fetch\");\n+\t\t\tstrvec_push(&cmd.args, \"--remote\");\n+\t\t\tstrvec_push(&cmd.args, \"--no-fetch\");\n \t\t}\n\n \t\tif (filter_submodules && filter_options.choice)\n-\t\t\tstrvec_pushf(&args, \"--filter=%s\",\n+\t\t\tstrvec_pushf(&cmd.args, \"--filter=%s\",\n \t\t\t\t expand_list_objects_filter_spec(&filter_options));\n\n \t\tif (option_single_branch >= 0)\n-\t\t\tstrvec_push(&args, option_single_branch ?\n+\t\t\tstrvec_push(&cmd.args, option_single_branch ?\n \t\t\t\t\t \"--single-branch\" :\n \t\t\t\t\t \"--no-single-branch\");\n\n-\t\terr = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\t\tstrvec_clear(&args);\n+\t\tcmd.git_cmd = 1;\n+\t\terr = run_command(&cmd);\n \t}\n\n \treturn err;\n@@ -862,11 +863,15 @@ static void write_refspec_config(const char *src_ref_prefix,\n\n static void dissociate_from_references(void)\n {\n-\tstatic const char* argv[] = { \"repack\", \"-a\", \"-d\", NULL };\n \tchar *alternates = git_pathdup(\"objects/info/alternates\");\n\n \tif (!access(alternates, F_OK)) {\n-\t\tif (run_command_v_opt(argv, RUN_GIT_CMD|RUN_COMMAND_NO_STDIN))\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\t\tstrvec_pushl(&cmd.args, \"repack\", \"-a\", \"-d\", NULL);\n+\t\tcmd.git_cmd = 1;\n+\t\tcmd.no_stdin = 1;\n+\t\tif (run_command(&cmd))\n \t\t\tdie(_(\"cannot repack to clean up\"));\n \t\tif (unlink(alternates) && errno != ENOENT)\n \t\t\tdie_errno(_(\"cannot unlink temporary alternates file\"));\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 4b10ad1a36..d6c262e15f 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -44,8 +44,10 @@ static int difftool_config(const char *var, const char *value, void *cb)\n\n static int print_tool_help(void)\n {\n-\tconst char *argv[] = { \"mergetool\", \"--tool-help=diff\", NULL };\n-\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\tstrvec_pushl(&cmd.args, \"mergetool\", \"--tool-help=diff\", NULL);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n\n static int parse_index_info(char *p, int *mode1, int *mode2,\n@@ -360,8 +362,8 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \tstruct pair_entry *entry;\n \tstruct index_state wtindex;\n \tstruct checkout lstate, rstate;\n-\tint flags = RUN_GIT_CMD, err = 0;\n-\tconst char *helper_argv[] = { \"difftool--helper\", NULL, NULL, NULL };\n+\tint err = 0;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct hashmap wt_modified, tmp_modified;\n \tint indices_loaded = 0;\n\n@@ -563,16 +565,18 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t}\n\n \tstrbuf_setlen(&ldir, ldir_len);\n-\thelper_argv[1] = ldir.buf;\n \tstrbuf_setlen(&rdir, rdir_len);\n-\thelper_argv[2] = rdir.buf;\n\n \tif (extcmd) {\n-\t\thelper_argv[0] = extcmd;\n-\t\tflags = 0;\n-\t} else\n+\t\tstrvec_push(&cmd.args, extcmd);\n+\t} else {\n+\t\tstrvec_push(&cmd.args, \"difftool--helper\");\n+\t\tcmd.git_cmd = 1;\n \t\tsetenv(\"GIT_DIFFTOOL_DIRDIFF\", \"true\", 1);\n-\tret = run_command_v_opt(helper_argv, flags);\n+\t}\n+\tstrvec_pushl(&cmd.args, ldir.buf, rdir.buf, NULL);\n+\n+\tret = run_command(&cmd);\n\n \t/* TODO: audit for interaction with sparse-index. */\n \tensure_full_index(&wtindex);\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a0fca93bb6..dd060dd65a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1965,14 +1965,17 @@ static int fetch_multiple(struct string_list *list, int max_children)\n \t} else\n \t\tfor (i = 0; i < list->nr; i++) {\n \t\t\tconst char *name = list->items[i].string;\n-\t\t\tstrvec_push(&argv, name);\n+\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\t\t\tstrvec_pushv(&cmd.args, argv.v);\n+\t\t\tstrvec_push(&cmd.args, name);\n \t\t\tif (verbosity >= 0)\n \t\t\t\tprintf(_(\"Fetching %s\\n\"), name);\n-\t\t\tif (run_command_v_opt(argv.v, RUN_GIT_CMD)) {\n+\t\t\tcmd.git_cmd = 1;\n+\t\t\tif (run_command(&cmd)) {\n \t\t\t\terror(_(\"could not fetch %s\"), name);\n \t\t\t\tresult = 1;\n \t\t\t}\n-\t\t\tstrvec_pop(&argv);\n \t\t}\n\n \tstrvec_clear(&argv);\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 243ee85d28..b47e53c210 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -167,9 +167,11 @@ static void gc_config(void)\n struct maintenance_run_opts;\n static int maintenance_task_pack_refs(MAYBE_UNUSED struct maintenance_run_opts *opts)\n {\n-\tconst char *argv[] = { \"pack-refs\", \"--all\", \"--prune\", NULL };\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\treturn run_command_v_opt(argv, RUN_GIT_CMD);\n+\tstrvec_pushl(&cmd.args, \"pack-refs\", \"--all\", \"--prune\", NULL);\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n\n static int too_many_loose_objects(void)\n@@ -521,6 +523,16 @@ static int report_last_gc_error(void)\n \treturn ret;\n }\n\n+static void run_git_or_die(const char **argv)\n+{\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tstrvec_pushv(&cmd.args, argv);\n+\tcmd.git_cmd = 1;\n+\tif (run_command(&cmd))\n+\t\tdie(FAILED_RUN, argv[0]);\n+}\n+\n static void gc_before_repack(void)\n {\n \t/*\n@@ -535,8 +547,8 @@ static void gc_before_repack(void)\n \tif (pack_refs && maintenance_task_pack_refs(NULL))\n \t\tdie(FAILED_RUN, \"pack-refs\");\n\n-\tif (prune_reflogs && run_command_v_opt(reflog.v, RUN_GIT_CMD))\n-\t\tdie(FAILED_RUN, reflog.v[0]);\n+\tif (prune_reflogs)\n+\t\trun_git_or_die(reflog.v);\n }\n\n int cmd_gc(int argc, const char **argv, const char *prefix)\n@@ -671,8 +683,12 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \tgc_before_repack();\n\n \tif (!repository_format_precious_objects) {\n-\t\tif (run_command_v_opt(repack.v,\n-\t\t\t\t RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE))\n+\t\tstruct child_process repack_cmd = CHILD_PROCESS_INIT;\n+\n+\t\tstrvec_pushv(&repack_cmd.args, repack.v);\n+\t\trepack_cmd.git_cmd = 1;\n+\t\trepack_cmd.close_object_store = 1;\n+\t\tif (run_command(&repack_cmd))\n \t\t\tdie(FAILED_RUN, repack.v[0]);\n\n \t\tif (prune_expire) {\n@@ -683,19 +699,16 @@ int cmd_gc(int argc, const char **argv, const char *prefix)\n \t\t\tif (has_promisor_remote())\n \t\t\t\tstrvec_push(&prune,\n \t\t\t\t\t \"--exclude-promisor-objects\");\n-\t\t\tif (run_command_v_opt(prune.v, RUN_GIT_CMD))\n-\t\t\t\tdie(FAILED_RUN, prune.v[0]);\n+\t\t\trun_git_or_die(prune.v);\n \t\t}\n \t}\n\n \tif (prune_worktrees_expire) {\n \t\tstrvec_push(&prune_worktrees, prune_worktrees_expire);\n-\t\tif (run_command_v_opt(prune_worktrees.v, RUN_GIT_CMD))\n-\t\t\tdie(FAILED_RUN, prune_worktrees.v[0]);\n+\t\trun_git_or_die(prune_worktrees.v);\n \t}\n\n-\tif (run_command_v_opt(rerere.v, RUN_GIT_CMD))\n-\t\tdie(FAILED_RUN, rerere.v[0]);\n+\trun_git_or_die(rerere.v);\n\n \treport_garbage = report_pack_garbage;\n \treprepare_packed_git(the_repository);\n@@ -1910,20 +1923,16 @@ static char *schtasks_task_name(const char *frequency)\n static int schtasks_remove_task(enum schedule_priority schedule)\n {\n \tconst char *cmd = \"schtasks\";\n-\tint result;\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n \tconst char *frequency = get_frequency(schedule);\n \tchar *name = schtasks_task_name(frequency);\n\n \tget_schedule_cmd(&cmd, NULL);\n-\tstrvec_split(&args, cmd);\n-\tstrvec_pushl(&args, \"/delete\", \"/tn\", name, \"/f\", NULL);\n-\n-\tresult = run_command_v_opt(args.v, 0);\n-\n-\tstrvec_clear(&args);\n+\tstrvec_split(&child.args, cmd);\n+\tstrvec_pushl(&child.args, \"/delete\", \"/tn\", name, \"/f\", NULL);\n \tfree(name);\n-\treturn result;\n+\n+\treturn run_command(&child);\n }\n\n static int schtasks_remove_tasks(void)\ndiff --git a/builtin/merge-index.c b/builtin/merge-index.c\nindex c0383fe9df..012f52bd00 100644\n--- a/builtin/merge-index.c\n+++ b/builtin/merge-index.c\n@@ -12,6 +12,7 @@ static int merge_entry(int pos, const char *path)\n \tconst char *arguments[] = { pgm, \"\", \"\", \"\", path, \"\", \"\", \"\", NULL };\n \tchar hexbuf[4][GIT_MAX_HEXSZ + 1];\n \tchar ownbuf[4][60];\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n \tif (pos >= active_nr)\n \t\tdie(\"git merge-index: %s not in the cache\", path);\n@@ -31,7 +32,8 @@ static int merge_entry(int pos, const char *path)\n \tif (!found)\n \t\tdie(\"git merge-index: %s not in the cache\", path);\n\n-\tif (run_command_v_opt(arguments, 0)) {\n+\tstrvec_pushv(&cmd.args, arguments);\n+\tif (run_command(&cmd)) {\n \t\tif (one_shot)\n \t\t\terr++;\n \t\telse {\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5900b81729..5e14decc22 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -347,58 +347,52 @@ static int save_state(struct object_id *stash)\n\n static void read_empty(const struct object_id *oid, int verbose)\n {\n-\tint i = 0;\n-\tconst char *args[7];\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\targs[i++] = \"read-tree\";\n+\tstrvec_push(&cmd.args, \"read-tree\");\n \tif (verbose)\n-\t\targs[i++] = \"-v\";\n-\targs[i++] = \"-m\";\n-\targs[i++] = \"-u\";\n-\targs[i++] = empty_tree_oid_hex();\n-\targs[i++] = oid_to_hex(oid);\n-\targs[i] = NULL;\n+\t\tstrvec_push(&cmd.args, \"-v\");\n+\tstrvec_pushl(&cmd.args, \"-m\", \"-u\", empty_tree_oid_hex(),\n+\t\t oid_to_hex(oid), NULL);\n\n-\tif (run_command_v_opt(args, RUN_GIT_CMD))\n+\tcmd.git_cmd = 1;\n+\tif (run_command(&cmd))\n \t\tdie(_(\"read-tree failed\"));\n }\n\n static void reset_hard(const struct object_id *oid, int verbose)\n {\n-\tint i = 0;\n-\tconst char *args[6];\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\targs[i++] = \"read-tree\";\n+\tstrvec_push(&cmd.args, \"read-tree\");\n \tif (verbose)\n-\t\targs[i++] = \"-v\";\n-\targs[i++] = \"--reset\";\n-\targs[i++] = \"-u\";\n-\targs[i++] = oid_to_hex(oid);\n-\targs[i] = NULL;\n+\t\tstrvec_push(&cmd.args, \"-v\");\n+\tstrvec_pushl(&cmd.args, \"--reset\", \"-u\", oid_to_hex(oid), NULL);\n\n-\tif (run_command_v_opt(args, RUN_GIT_CMD))\n+\tcmd.git_cmd = 1;\n+\tif (run_command(&cmd))\n \t\tdie(_(\"read-tree failed\"));\n }\n\n static void restore_state(const struct object_id *head,\n \t\t\t const struct object_id *stash)\n {\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n \treset_hard(head, 1);\n\n \tif (is_null_oid(stash))\n \t\tgoto refresh_cache;\n\n-\tstrvec_pushl(&args, \"stash\", \"apply\", \"--index\", \"--quiet\", NULL);\n-\tstrvec_push(&args, oid_to_hex(stash));\n+\tstrvec_pushl(&cmd.args, \"stash\", \"apply\", \"--index\", \"--quiet\", NULL);\n+\tstrvec_push(&cmd.args, oid_to_hex(stash));\n\n \t/*\n \t * It is OK to ignore error here, for example when there was\n \t * nothing to restore.\n \t */\n-\trun_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n+\tcmd.git_cmd = 1;\n+\trun_command(&cmd);\n\n refresh_cache:\n \tif (discard_cache() < 0 || read_cache() < 0)\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 403a24d7ca..b21edd767a 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -515,76 +515,75 @@ static void parse_repo_refspecs(int argc, const char **argv, const char **repo,\n */\n static int run_fetch(const char *repo, const char **refspecs)\n {\n-\tstruct strvec args = STRVEC_INIT;\n-\tint ret;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\tstrvec_pushl(&args, \"fetch\", \"--update-head-ok\", NULL);\n+\tstrvec_pushl(&cmd.args, \"fetch\", \"--update-head-ok\", NULL);\n\n \t/* Shared options */\n-\targv_push_verbosity(&args);\n+\targv_push_verbosity(&cmd.args);\n \tif (opt_progress)\n-\t\tstrvec_push(&args, opt_progress);\n+\t\tstrvec_push(&cmd.args, opt_progress);\n\n \t/* Options passed to git-fetch */\n \tif (opt_all)\n-\t\tstrvec_push(&args, opt_all);\n+\t\tstrvec_push(&cmd.args, opt_all);\n \tif (opt_append)\n-\t\tstrvec_push(&args, opt_append);\n+\t\tstrvec_push(&cmd.args, opt_append);\n \tif (opt_upload_pack)\n-\t\tstrvec_push(&args, opt_upload_pack);\n-\targv_push_force(&args);\n+\t\tstrvec_push(&cmd.args, opt_upload_pack);\n+\targv_push_force(&cmd.args);\n \tif (opt_tags)\n-\t\tstrvec_push(&args, opt_tags);\n+\t\tstrvec_push(&cmd.args, opt_tags);\n \tif (opt_prune)\n-\t\tstrvec_push(&args, opt_prune);\n+\t\tstrvec_push(&cmd.args, opt_prune);\n \tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n \t\tswitch (recurse_submodules_cli) {\n \t\tcase RECURSE_SUBMODULES_ON:\n-\t\t\tstrvec_push(&args, \"--recurse-submodules=on\");\n+\t\t\tstrvec_push(&cmd.args, \"--recurse-submodules=on\");\n \t\t\tbreak;\n \t\tcase RECURSE_SUBMODULES_OFF:\n-\t\t\tstrvec_push(&args, \"--recurse-submodules=no\");\n+\t\t\tstrvec_push(&cmd.args, \"--recurse-submodules=no\");\n \t\t\tbreak;\n \t\tcase RECURSE_SUBMODULES_ON_DEMAND:\n-\t\t\tstrvec_push(&args, \"--recurse-submodules=on-demand\");\n+\t\t\tstrvec_push(&cmd.args, \"--recurse-submodules=on-demand\");\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tBUG(\"submodule recursion option not understood\");\n \t\t}\n \tif (max_children)\n-\t\tstrvec_push(&args, max_children);\n+\t\tstrvec_push(&cmd.args, max_children);\n \tif (opt_dry_run)\n-\t\tstrvec_push(&args, \"--dry-run\");\n+\t\tstrvec_push(&cmd.args, \"--dry-run\");\n \tif (opt_keep)\n-\t\tstrvec_push(&args, opt_keep);\n+\t\tstrvec_push(&cmd.args, opt_keep);\n \tif (opt_depth)\n-\t\tstrvec_push(&args, opt_depth);\n+\t\tstrvec_push(&cmd.args, opt_depth);\n \tif (opt_unshallow)\n-\t\tstrvec_push(&args, opt_unshallow);\n+\t\tstrvec_push(&cmd.args, opt_unshallow);\n \tif (opt_update_shallow)\n-\t\tstrvec_push(&args, opt_update_shallow);\n+\t\tstrvec_push(&cmd.args, opt_update_shallow);\n \tif (opt_refmap)\n-\t\tstrvec_push(&args, opt_refmap);\n+\t\tstrvec_push(&cmd.args, opt_refmap);\n \tif (opt_ipv4)\n-\t\tstrvec_push(&args, opt_ipv4);\n+\t\tstrvec_push(&cmd.args, opt_ipv4);\n \tif (opt_ipv6)\n-\t\tstrvec_push(&args, opt_ipv6);\n+\t\tstrvec_push(&cmd.args, opt_ipv6);\n \tif (opt_show_forced_updates > 0)\n-\t\tstrvec_push(&args, \"--show-forced-updates\");\n+\t\tstrvec_push(&cmd.args, \"--show-forced-updates\");\n \telse if (opt_show_forced_updates == 0)\n-\t\tstrvec_push(&args, \"--no-show-forced-updates\");\n+\t\tstrvec_push(&cmd.args, \"--no-show-forced-updates\");\n \tif (set_upstream)\n-\t\tstrvec_push(&args, set_upstream);\n-\tstrvec_pushv(&args, opt_fetch.v);\n+\t\tstrvec_push(&cmd.args, set_upstream);\n+\tstrvec_pushv(&cmd.args, opt_fetch.v);\n\n \tif (repo) {\n-\t\tstrvec_push(&args, repo);\n-\t\tstrvec_pushv(&args, refspecs);\n+\t\tstrvec_push(&cmd.args, repo);\n+\t\tstrvec_pushv(&cmd.args, refspecs);\n \t} else if (*refspecs)\n \t\tBUG(\"refspecs without repo?\");\n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD | RUN_CLOSE_OBJECT_STORE);\n-\tstrvec_clear(&args);\n-\treturn ret;\n+\tcmd.git_cmd = 1;\n+\tcmd.close_object_store = 1;\n+\treturn run_command(&cmd);\n }\n\n /**\n@@ -653,52 +652,50 @@ static int update_submodules(void)\n */\n static int run_merge(void)\n {\n-\tint ret;\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\tstrvec_pushl(&args, \"merge\", NULL);\n+\tstrvec_pushl(&cmd.args, \"merge\", NULL);\n\n \t/* Shared options */\n-\targv_push_verbosity(&args);\n+\targv_push_verbosity(&cmd.args);\n \tif (opt_progress)\n-\t\tstrvec_push(&args, opt_progress);\n+\t\tstrvec_push(&cmd.args, opt_progress);\n\n \t/* Options passed to git-merge */\n \tif (opt_diffstat)\n-\t\tstrvec_push(&args, opt_diffstat);\n+\t\tstrvec_push(&cmd.args, opt_diffstat);\n \tif (opt_log)\n-\t\tstrvec_push(&args, opt_log);\n+\t\tstrvec_push(&cmd.args, opt_log);\n \tif (opt_signoff)\n-\t\tstrvec_push(&args, opt_signoff);\n+\t\tstrvec_push(&cmd.args, opt_signoff);\n \tif (opt_squash)\n-\t\tstrvec_push(&args, opt_squash);\n+\t\tstrvec_push(&cmd.args, opt_squash);\n \tif (opt_commit)\n-\t\tstrvec_push(&args, opt_commit);\n+\t\tstrvec_push(&cmd.args, opt_commit);\n \tif (opt_edit)\n-\t\tstrvec_push(&args, opt_edit);\n+\t\tstrvec_push(&cmd.args, opt_edit);\n \tif (cleanup_arg)\n-\t\tstrvec_pushf(&args, \"--cleanup=%s\", cleanup_arg);\n+\t\tstrvec_pushf(&cmd.args, \"--cleanup=%s\", cleanup_arg);\n \tif (opt_ff)\n-\t\tstrvec_push(&args, opt_ff);\n+\t\tstrvec_push(&cmd.args, opt_ff);\n \tif (opt_verify)\n-\t\tstrvec_push(&args, opt_verify);\n+\t\tstrvec_push(&cmd.args, opt_verify);\n \tif (opt_verify_signatures)\n-\t\tstrvec_push(&args, opt_verify_signatures);\n-\tstrvec_pushv(&args, opt_strategies.v);\n-\tstrvec_pushv(&args, opt_strategy_opts.v);\n+\t\tstrvec_push(&cmd.args, opt_verify_signatures);\n+\tstrvec_pushv(&cmd.args, opt_strategies.v);\n+\tstrvec_pushv(&cmd.args, opt_strategy_opts.v);\n \tif (opt_gpg_sign)\n-\t\tstrvec_push(&args, opt_gpg_sign);\n+\t\tstrvec_push(&cmd.args, opt_gpg_sign);\n \tif (opt_autostash == 0)\n-\t\tstrvec_push(&args, \"--no-autostash\");\n+\t\tstrvec_push(&cmd.args, \"--no-autostash\");\n \telse if (opt_autostash == 1)\n-\t\tstrvec_push(&args, \"--autostash\");\n+\t\tstrvec_push(&cmd.args, \"--autostash\");\n \tif (opt_allow_unrelated_histories > 0)\n-\t\tstrvec_push(&args, \"--allow-unrelated-histories\");\n+\t\tstrvec_push(&cmd.args, \"--allow-unrelated-histories\");\n\n-\tstrvec_push(&args, \"FETCH_HEAD\");\n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n-\treturn ret;\n+\tstrvec_push(&cmd.args, \"FETCH_HEAD\");\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n\n /**\n@@ -879,43 +876,41 @@ static int get_rebase_newbase_and_upstream(struct object_id *newbase,\n static int run_rebase(const struct object_id *newbase,\n \t\tconst struct object_id *upstream)\n {\n-\tint ret;\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\tstrvec_push(&args, \"rebase\");\n+\tstrvec_push(&cmd.args, \"rebase\");\n\n \t/* Shared options */\n-\targv_push_verbosity(&args);\n+\targv_push_verbosity(&cmd.args);\n\n \t/* Options passed to git-rebase */\n \tif (opt_rebase == REBASE_MERGES)\n-\t\tstrvec_push(&args, \"--rebase-merges\");\n+\t\tstrvec_push(&cmd.args, \"--rebase-merges\");\n \telse if (opt_rebase == REBASE_INTERACTIVE)\n-\t\tstrvec_push(&args, \"--interactive\");\n+\t\tstrvec_push(&cmd.args, \"--interactive\");\n \tif (opt_diffstat)\n-\t\tstrvec_push(&args, opt_diffstat);\n-\tstrvec_pushv(&args, opt_strategies.v);\n-\tstrvec_pushv(&args, opt_strategy_opts.v);\n+\t\tstrvec_push(&cmd.args, opt_diffstat);\n+\tstrvec_pushv(&cmd.args, opt_strategies.v);\n+\tstrvec_pushv(&cmd.args, opt_strategy_opts.v);\n \tif (opt_gpg_sign)\n-\t\tstrvec_push(&args, opt_gpg_sign);\n+\t\tstrvec_push(&cmd.args, opt_gpg_sign);\n \tif (opt_signoff)\n-\t\tstrvec_push(&args, opt_signoff);\n+\t\tstrvec_push(&cmd.args, opt_signoff);\n \tif (opt_autostash == 0)\n-\t\tstrvec_push(&args, \"--no-autostash\");\n+\t\tstrvec_push(&cmd.args, \"--no-autostash\");\n \telse if (opt_autostash == 1)\n-\t\tstrvec_push(&args, \"--autostash\");\n+\t\tstrvec_push(&cmd.args, \"--autostash\");\n \tif (opt_verify_signatures &&\n \t !strcmp(opt_verify_signatures, \"--verify-signatures\"))\n \t\twarning(_(\"ignoring --verify-signatures for rebase\"));\n\n-\tstrvec_push(&args, \"--onto\");\n-\tstrvec_push(&args, oid_to_hex(newbase));\n+\tstrvec_push(&cmd.args, \"--onto\");\n+\tstrvec_push(&cmd.args, oid_to_hex(newbase));\n\n-\tstrvec_push(&args, oid_to_hex(upstream));\n+\tstrvec_push(&cmd.args, oid_to_hex(upstream));\n\n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n-\treturn ret;\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n\n static int get_can_ff(struct object_id *orig_head,\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 910f7b9316..0cde2e244a 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -92,13 +92,15 @@ static int verbose;\n\n static int fetch_remote(const char *name)\n {\n-\tconst char *argv[] = { \"fetch\", name, NULL, NULL };\n-\tif (verbose) {\n-\t\targv[1] = \"-v\";\n-\t\targv[2] = name;\n-\t}\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\tstrvec_push(&cmd.args, \"fetch\");\n+\tif (verbose)\n+\t\tstrvec_push(&cmd.args, \"-v\");\n+\tstrvec_push(&cmd.args, name);\n \tprintf_ln(_(\"Updating %s\"), name);\n-\tif (run_command_v_opt(argv, RUN_GIT_CMD))\n+\tcmd.git_cmd = 1;\n+\tif (run_command(&cmd))\n \t\treturn error(_(\"Could not fetch %s\"), name);\n \treturn 0;\n }\n@@ -1508,37 +1510,35 @@ static int update(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"prune remotes after fetching\")),\n \t\tOPT_END()\n \t};\n-\tstruct strvec fetch_argv = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint default_defined = 0;\n-\tint retval;\n\n \targc = parse_options(argc, argv, prefix, options,\n \t\t\t builtin_remote_update_usage,\n \t\t\t PARSE_OPT_KEEP_ARGV0);\n\n-\tstrvec_push(&fetch_argv, \"fetch\");\n+\tstrvec_push(&cmd.args, \"fetch\");\n\n \tif (prune != -1)\n-\t\tstrvec_push(&fetch_argv, prune ? \"--prune\" : \"--no-prune\");\n+\t\tstrvec_push(&cmd.args, prune ? \"--prune\" : \"--no-prune\");\n \tif (verbose)\n-\t\tstrvec_push(&fetch_argv, \"-v\");\n-\tstrvec_push(&fetch_argv, \"--multiple\");\n+\t\tstrvec_push(&cmd.args, \"-v\");\n+\tstrvec_push(&cmd.args, \"--multiple\");\n \tif (argc < 2)\n-\t\tstrvec_push(&fetch_argv, \"default\");\n+\t\tstrvec_push(&cmd.args, \"default\");\n \tfor (i = 1; i < argc; i++)\n-\t\tstrvec_push(&fetch_argv, argv[i]);\n+\t\tstrvec_push(&cmd.args, argv[i]);\n\n-\tif (strcmp(fetch_argv.v[fetch_argv.nr-1], \"default\") == 0) {\n+\tif (strcmp(cmd.args.v[cmd.args.nr-1], \"default\") == 0) {\n \t\tgit_config(get_remote_default, &default_defined);\n \t\tif (!default_defined) {\n-\t\t\tstrvec_pop(&fetch_argv);\n-\t\t\tstrvec_push(&fetch_argv, \"--all\");\n+\t\t\tstrvec_pop(&cmd.args);\n+\t\t\tstrvec_push(&cmd.args, \"--all\");\n \t\t}\n \t}\n\n-\tretval = run_command_v_opt(fetch_argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&fetch_argv);\n-\treturn retval;\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n\n static int remove_all_fetch_refspecs(const char *key)\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 901375d584..d614f156df 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -196,16 +196,19 @@ static int read_yes_no_answer(void)\n static int ask_yes_no_if_possible(const char *format, ...)\n {\n \tchar question[4096];\n-\tconst char *retry_hook[] = { NULL, NULL, NULL };\n+\tconst char *retry_hook;\n \tva_list args;\n\n \tva_start(args, format);\n \tvsnprintf(question, sizeof(question), format, args);\n \tva_end(args);\n\n-\tif ((retry_hook[0] = mingw_getenv(\"GIT_ASK_YESNO\"))) {\n-\t\tretry_hook[1] = question;\n-\t\treturn !run_command_v_opt(retry_hook, 0);\n+\tretry_hook = mingw_getenv(\"GIT_ASK_YESNO\");\n+\tif (retry_hook) {\n+\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n+\t\tstrvec_pushl(&cmd.args, retry_hook, question, NULL);\n+\t\treturn !run_command(&cmd);\n \t}\n\n \tif (!isatty(_fileno(stdin)) || !isatty(_fileno(stderr)))\ndiff --git a/diff.c b/diff.c\nindex 648f6717a5..8451c230d9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4278,35 +4278,34 @@ static void run_external_diff(const char *pgm,\n \t\t\t const char *xfrm_msg,\n \t\t\t struct diff_options *o)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n-\tstruct strvec env = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n\n-\tstrvec_push(&argv, pgm);\n-\tstrvec_push(&argv, name);\n+\tstrvec_push(&cmd.args, pgm);\n+\tstrvec_push(&cmd.args, name);\n\n \tif (one && two) {\n-\t\tadd_external_diff_name(o->repo, &argv, name, one);\n+\t\tadd_external_diff_name(o->repo, &cmd.args, name, one);\n \t\tif (!other)\n-\t\t\tadd_external_diff_name(o->repo, &argv, name, two);\n+\t\t\tadd_external_diff_name(o->repo, &cmd.args, name, two);\n \t\telse {\n-\t\t\tadd_external_diff_name(o->repo, &argv, other, two);\n-\t\t\tstrvec_push(&argv, other);\n-\t\t\tstrvec_push(&argv, xfrm_msg);\n+\t\t\tadd_external_diff_name(o->repo, &cmd.args, other, two);\n+\t\t\tstrvec_push(&cmd.args, other);\n+\t\t\tstrvec_push(&cmd.args, xfrm_msg);\n \t\t}\n \t}\n\n-\tstrvec_pushf(&env, \"GIT_DIFF_PATH_COUNTER=%d\", ++o->diff_path_counter);\n-\tstrvec_pushf(&env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n+\tstrvec_pushf(&cmd.env, \"GIT_DIFF_PATH_COUNTER=%d\",\n+\t\t ++o->diff_path_counter);\n+\tstrvec_pushf(&cmd.env, \"GIT_DIFF_PATH_TOTAL=%d\", q->nr);\n\n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n-\tif (run_command_v_opt_cd_env(argv.v, RUN_USING_SHELL, NULL, env.v))\n+\tcmd.use_shell = 1;\n+\tif (run_command(&cmd))\n \t\tdie(_(\"external diff died, stopping at %s\"), name);\n\n \tremove_tempfile();\n-\tstrvec_clear(&argv);\n-\tstrvec_clear(&env);\n }\n\n static int similarity_index(struct diff_filepair *p)\ndiff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c\nindex 789e7397ba..8d0b0b157c 100644\n--- a/fsmonitor-ipc.c\n+++ b/fsmonitor-ipc.c\n@@ -56,10 +56,14 @@ enum ipc_active_state fsmonitor_ipc__get_state(void)\n\n static int spawn_daemon(void)\n {\n-\tconst char *args[] = { \"fsmonitor--daemon\", \"start\", NULL };\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\treturn run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD,\n-\t\t\t\t \"fsmonitor\");\n+\tstrvec_pushl(&cmd.args, \"fsmonitor--daemon\", \"start\", NULL);\n+\n+\tcmd.git_cmd = 1;\n+\tcmd.no_stdin = 1;\n+\tcmd.trace2_child_class = \"fsmonitor\";\n+\treturn run_command(&cmd);\n }\n\n int fsmonitor_ipc__send_query(const char *since_token,\ndiff --git a/git.c b/git.c\nindex da411c5382..ccf444417b 100644\n--- a/git.c\n+++ b/git.c\n@@ -787,7 +787,7 @@ static int run_argv(int *argcp, const char ***argv)\n \t\tif (!done_alias)\n \t\t\thandle_builtin(*argcp, *argv);\n \t\telse if (get_builtin(**argv)) {\n-\t\t\tstruct strvec args = STRVEC_INIT;\n+\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \t\t\tint i;\n\n \t\t\t/*\n@@ -804,18 +804,21 @@ static int run_argv(int *argcp, const char ***argv)\n\n \t\t\tcommit_pager_choice();\n\n-\t\t\tstrvec_push(&args, \"git\");\n+\t\t\tstrvec_push(&cmd.args, \"git\");\n \t\t\tfor (i = 0; i < *argcp; i++)\n-\t\t\t\tstrvec_push(&args, (*argv)[i]);\n+\t\t\t\tstrvec_push(&cmd.args, (*argv)[i]);\n\n-\t\t\ttrace_argv_printf(args.v, \"trace: exec:\");\n+\t\t\ttrace_argv_printf(cmd.args.v, \"trace: exec:\");\n\n \t\t\t/*\n \t\t\t * if we fail because the command is not found, it is\n \t\t\t * OK to return. Otherwise, we just pass along the status code.\n \t\t\t */\n-\t\t\ti = run_command_v_opt_tr2(args.v, RUN_SILENT_EXEC_FAILURE |\n-\t\t\t\t\t\t RUN_CLEAN_ON_EXIT | RUN_WAIT_AFTER_CLEAN, \"git_alias\");\n+\t\t\tcmd.silent_exec_failure = 1;\n+\t\t\tcmd.clean_on_exit = 1;\n+\t\t\tcmd.wait_after_clean = 1;\n+\t\t\tcmd.trace2_child_class = \"git_alias\";\n+\t\t\ti = run_command(&cmd);\n \t\t\tif (i >= 0 || errno != ENOENT)\n \t\t\t\texit(i);\n \t\t\tdie(\"could not execute builtin %s\", **argv);\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 8955d7e1f6..b20491043e 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -193,7 +193,7 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n \tstruct strbuf cmd = STRBUF_INIT;\n \tstruct strbuf_expand_dict_entry dict[6];\n \tstruct strbuf path_sq = STRBUF_INIT;\n-\tconst char *args[] = { NULL, NULL };\n+\tstruct child_process child = CHILD_PROCESS_INIT;\n \tint status, fd, i;\n \tstruct stat st;\n \tenum ll_merge_result ret;\n@@ -219,8 +219,9 @@ static enum ll_merge_result ll_ext_merge(const struct ll_merge_driver *fn,\n\n \tstrbuf_expand(&cmd, fn->cmdline, strbuf_expand_dict_cb, &dict);\n\n-\targs[0] = cmd.buf;\n-\tstatus = run_command_v_opt(args, RUN_USING_SHELL);\n+\tstrvec_push(&child.args, cmd.buf);\n+\tchild.use_shell = 1;\n+\tstatus = run_command(&child);\n \tfd = open(temp[1], O_RDONLY);\n \tif (fd < 0)\n \t\tgoto bad;\ndiff --git a/merge.c b/merge.c\nindex 2382ff66d3..445b4f19aa 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -19,22 +19,22 @@ int try_merge_command(struct repository *r,\n \t\t const char **xopts, struct commit_list *common,\n \t\t const char *head_arg, struct commit_list *remotes)\n {\n-\tstruct strvec args = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint i, ret;\n \tstruct commit_list *j;\n\n-\tstrvec_pushf(&args, \"merge-%s\", strategy);\n+\tstrvec_pushf(&cmd.args, \"merge-%s\", strategy);\n \tfor (i = 0; i < xopts_nr; i++)\n-\t\tstrvec_pushf(&args, \"--%s\", xopts[i]);\n+\t\tstrvec_pushf(&cmd.args, \"--%s\", xopts[i]);\n \tfor (j = common; j; j = j->next)\n-\t\tstrvec_push(&args, merge_argument(j->item));\n-\tstrvec_push(&args, \"--\");\n-\tstrvec_push(&args, head_arg);\n+\t\tstrvec_push(&cmd.args, merge_argument(j->item));\n+\tstrvec_push(&cmd.args, \"--\");\n+\tstrvec_push(&cmd.args, head_arg);\n \tfor (j = remotes; j; j = j->next)\n-\t\tstrvec_push(&args, merge_argument(j->item));\n+\t\tstrvec_push(&cmd.args, merge_argument(j->item));\n\n-\tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n-\tstrvec_clear(&args);\n+\tcmd.git_cmd = 1;\n+\tret = run_command(&cmd);\n\n \tdiscard_index(r->index);\n \tif (repo_read_index(r) < 0)\ndiff --git a/run-command.c b/run-command.c\nindex 5ec3a46dcc..23e100dffc 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1004,41 +1004,6 @@ int run_command(struct child_process *cmd)\n \treturn finish_command(cmd);\n }\n\n-int run_command_v_opt(const char **argv, int opt)\n-{\n-\treturn run_command_v_opt_cd_env(argv, opt, NULL, NULL);\n-}\n-\n-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class)\n-{\n-\treturn run_command_v_opt_cd_env_tr2(argv, opt, NULL, NULL, tr2_class);\n-}\n-\n-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env)\n-{\n-\treturn run_command_v_opt_cd_env_tr2(argv, opt, dir, env, NULL);\n-}\n-\n-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n-\t\t\t\t const char *const *env, const char *tr2_class)\n-{\n-\tstruct child_process cmd = CHILD_PROCESS_INIT;\n-\tstrvec_pushv(&cmd.args, argv);\n-\tcmd.no_stdin = opt & RUN_COMMAND_NO_STDIN ? 1 : 0;\n-\tcmd.git_cmd = opt & RUN_GIT_CMD ? 1 : 0;\n-\tcmd.stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n-\tcmd.silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n-\tcmd.use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n-\tcmd.clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n-\tcmd.wait_after_clean = opt & RUN_WAIT_AFTER_CLEAN ? 1 : 0;\n-\tcmd.close_object_store = opt & RUN_CLOSE_OBJECT_STORE ? 1 : 0;\n-\tcmd.dir = dir;\n-\tif (env)\n-\t\tstrvec_pushv(&cmd.env, (const char **)env);\n-\tcmd.trace2_child_class = tr2_class;\n-\treturn run_command(&cmd);\n-}\n-\n #ifndef NO_PTHREADS\n static pthread_t main_thread;\n static int main_thread_set;\ndiff --git a/run-command.h b/run-command.h\nindex 0e85e5846a..57354fb7df 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -224,36 +224,6 @@ int run_command(struct child_process *);\n */\n int run_auto_maintenance(int quiet);\n\n-#define RUN_COMMAND_NO_STDIN\t\t(1<<0)\n-#define RUN_GIT_CMD\t\t\t(1<<1)\n-#define RUN_COMMAND_STDOUT_TO_STDERR\t(1<<2)\n-#define RUN_SILENT_EXEC_FAILURE\t\t(1<<3)\n-#define RUN_USING_SHELL\t\t\t(1<<4)\n-#define RUN_CLEAN_ON_EXIT\t\t(1<<5)\n-#define RUN_WAIT_AFTER_CLEAN\t\t(1<<6)\n-#define RUN_CLOSE_OBJECT_STORE\t\t(1<<7)\n-\n-/**\n- * Convenience functions that encapsulate a sequence of\n- * start_command() followed by finish_command(). The argument argv\n- * specifies the program and its arguments. The argument opt is zero\n- * or more of the flags `RUN_COMMAND_NO_STDIN`, `RUN_GIT_CMD`,\n- * `RUN_COMMAND_STDOUT_TO_STDERR`, or `RUN_SILENT_EXEC_FAILURE`\n- * that correspond to the members .no_stdin, .git_cmd,\n- * .stdout_to_stderr, .silent_exec_failure of `struct child_process`.\n- * The argument dir corresponds the member .dir. The argument env\n- * corresponds to the member .env.\n- */\n-int run_command_v_opt(const char **argv, int opt);\n-int run_command_v_opt_tr2(const char **argv, int opt, const char *tr2_class);\n-/*\n- * env (the environment) is to be formatted like environ: \"VAR=VALUE\".\n- * To unset an environment variable use just \"VAR\".\n- */\n-int run_command_v_opt_cd_env(const char **argv, int opt, const char *dir, const char *const *env);\n-int run_command_v_opt_cd_env_tr2(const char **argv, int opt, const char *dir,\n-\t\t\t\t const char *const *env, const char *tr2_class);\n-\n /**\n * Execute the given command, sending \"in\" to its stdin, and capturing its\n * stdout and stderr in the \"out\" and \"err\" strbufs. Any of the three may\ndiff --git a/scalar.c b/scalar.c\nindex 6de9c0ee52..03f9e480dd 100644\n--- a/scalar.c\n+++ b/scalar.c\n@@ -69,21 +69,18 @@ static void setup_enlistment_directory(int argc, const char **argv,\n\n static int run_git(const char *arg, ...)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tva_list args;\n \tconst char *p;\n-\tint res;\n\n \tva_start(args, arg);\n-\tstrvec_push(&argv, arg);\n+\tstrvec_push(&cmd.args, arg);\n \twhile ((p = va_arg(args, const char *)))\n-\t\tstrvec_push(&argv, p);\n+\t\tstrvec_push(&cmd.args, p);\n \tva_end(args);\n\n-\tres = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\n-\tstrvec_clear(&argv);\n-\treturn res;\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n\n struct scalar_config {\ndiff --git a/sequencer.c b/sequencer.c\nindex debb2ecbaf..9b9d6a5582 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3183,18 +3183,15 @@ static int rollback_is_safe(void)\n\n static int reset_merge(const struct object_id *oid)\n {\n-\tint ret;\n-\tstruct strvec argv = STRVEC_INIT;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n-\tstrvec_pushl(&argv, \"reset\", \"--merge\", NULL);\n+\tstrvec_pushl(&cmd.args, \"reset\", \"--merge\", NULL);\n\n \tif (!is_null_oid(oid))\n-\t\tstrvec_push(&argv, oid_to_hex(oid));\n-\n-\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&argv);\n+\t\tstrvec_push(&cmd.args, oid_to_hex(oid));\n\n-\treturn ret;\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n\n static int rollback_single_pick(struct repository *r)\n@@ -3558,12 +3555,13 @@ static int error_failed_squash(struct repository *r,\n\n static int do_exec(struct repository *r, const char *command_line)\n {\n-\tconst char *child_argv[] = { NULL, NULL };\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint dirty, status;\n\n \tfprintf(stderr, _(\"Executing: %s\\n\"), command_line);\n-\tchild_argv[0] = command_line;\n-\tstatus = run_command_v_opt(child_argv, RUN_USING_SHELL);\n+\tstrvec_push(&cmd.args, command_line);\n+\tcmd.use_shell = 1;\n+\tstatus = run_command(&cmd);\n\n \t/* force re-reading of the cache */\n \tif (discard_index(r->index) < 0 || repo_read_index(r) < 0)\n@@ -4867,14 +4865,13 @@ static int pick_commits(struct repository *r,\n\n static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n {\n-\tstruct strvec argv = STRVEC_INIT;\n-\tint ret;\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n \tif (!refs_ref_exists(get_main_ref_store(r), \"CHERRY_PICK_HEAD\") &&\n \t !refs_ref_exists(get_main_ref_store(r), \"REVERT_HEAD\"))\n \t\treturn error(_(\"no cherry-pick or revert in progress\"));\n\n-\tstrvec_push(&argv, \"commit\");\n+\tstrvec_push(&cmd.args, \"commit\");\n\n \t/*\n \t * continue_single_pick() handles the case of recovering from a\n@@ -4887,11 +4884,10 @@ static int continue_single_pick(struct repository *r, struct replay_opts *opts)\n \t\t * Include --cleanup=strip as well because we don't want the\n \t\t * \"# Conflicts:\" messages.\n \t\t */\n-\t\tstrvec_pushl(&argv, \"--no-edit\", \"--cleanup=strip\", NULL);\n+\t\tstrvec_pushl(&cmd.args, \"--no-edit\", \"--cleanup=strip\", NULL);\n\n-\tret = run_command_v_opt(argv.v, RUN_GIT_CMD);\n-\tstrvec_clear(&argv);\n-\treturn ret;\n+\tcmd.git_cmd = 1;\n+\treturn run_command(&cmd);\n }\n\n static int commit_staged_changes(struct repository *r,\ndiff --git a/shell.c b/shell.c\nindex 811e13b9c9..4abc14eeee 100644\n--- a/shell.c\n+++ b/shell.c\n@@ -50,21 +50,24 @@ static void cd_to_homedir(void)\n static void run_shell(void)\n {\n \tint done = 0;\n-\tstatic const char *help_argv[] = { HELP_COMMAND, NULL };\n+\tstruct child_process help_cmd = CHILD_PROCESS_INIT;\n\n \tif (!access(NOLOGIN_COMMAND, F_OK)) {\n \t\t/* Interactive login disabled. */\n-\t\tconst char *argv[] = { NOLOGIN_COMMAND, NULL };\n+\t\tstruct child_process nologin_cmd = CHILD_PROCESS_INIT;\n \t\tint status;\n\n-\t\tstatus = run_command_v_opt(argv, 0);\n+\t\tstrvec_push(&nologin_cmd.args, NOLOGIN_COMMAND);\n+\t\tstatus = run_command(&nologin_cmd);\n \t\tif (status < 0)\n \t\t\texit(127);\n \t\texit(status);\n \t}\n\n \t/* Print help if enabled */\n-\trun_command_v_opt(help_argv, RUN_SILENT_EXEC_FAILURE);\n+\tstrvec_push(&help_cmd.args, HELP_COMMAND);\n+\thelp_cmd.silent_exec_failure = 1;\n+\trun_command(&help_cmd);\n\n \tdo {\n \t\tstruct strbuf line = STRBUF_INIT;\n@@ -99,9 +102,13 @@ static void run_shell(void)\n \t\t\t !strcmp(prog, \"exit\") || !strcmp(prog, \"bye\")) {\n \t\t\tdone = 1;\n \t\t} else if (is_valid_cmd_name(prog)) {\n+\t\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n+\n \t\t\tfull_cmd = make_cmd(prog);\n \t\t\targv[0] = full_cmd;\n-\t\t\tcode = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);\n+\t\t\tstrvec_pushv(&cmd.args, argv);\n+\t\t\tcmd.silent_exec_failure = 1;\n+\t\t\tcode = run_command(&cmd);\n \t\t\tif (code == -1 && errno == ENOENT) {\n \t\t\t\tfprintf(stderr, \"unrecognized command '%s'\\n\", prog);\n \t\t\t}\ndiff --git a/t/helper/test-fake-ssh.c b/t/helper/test-fake-ssh.c\nindex 12beee99ad..42185f3fc0 100644\n--- a/t/helper/test-fake-ssh.c\n+++ b/t/helper/test-fake-ssh.c\n@@ -8,7 +8,7 @@ int cmd_main(int argc, const char **argv)\n \tstruct strbuf buf = STRBUF_INIT;\n \tFILE *f;\n \tint i;\n-\tconst char *child_argv[] = { NULL, NULL };\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n\n \t/* First, print all parameters into $TRASH_DIRECTORY/ssh-output */\n \tif (!trash_directory)\n@@ -25,6 +25,7 @@ int cmd_main(int argc, const char **argv)\n \t/* Now, evaluate the *last* parameter */\n \tif (argc < 2)\n \t\treturn 0;\n-\tchild_argv[0] = argv[argc - 1];\n-\treturn run_command_v_opt(child_argv, RUN_USING_SHELL);\n+\tstrvec_push(&cmd.args, argv[argc - 1]);\n+\tcmd.use_shell = 1;\n+\treturn run_command(&cmd);\n }\ndiff --git a/t/helper/test-trace2.c b/t/helper/test-trace2.c\nindex a714130ece..94fd8ccf51 100644\n--- a/t/helper/test-trace2.c\n+++ b/t/helper/test-trace2.c\n@@ -132,6 +132,7 @@ static int ut_003error(int argc, const char **argv)\n */\n static int ut_004child(int argc, const char **argv)\n {\n+\tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tint result;\n\n \t/*\n@@ -141,7 +142,8 @@ static int ut_004child(int argc, const char **argv)\n \tif (!argc)\n \t\treturn 0;\n\n-\tresult = run_command_v_opt(argv, 0);\n+\tstrvec_pushv(&cmd.args, argv);\n+\tresult = run_command(&cmd);\n \texit(result);\n }\n\ndiff --git a/tmp-objdir.h b/tmp-objdir.h\nindex 76efc7edee..615b188573 100644\n--- a/tmp-objdir.h\n+++ b/tmp-objdir.h\n@@ -11,8 +11,8 @@\n * Example:\n *\n *\tstruct tmp_objdir *t = tmp_objdir_create(\"incoming\");\n- *\tif (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&\n- *\t !tmp_objdir_migrate(t))\n+ *\tstrvec_pushv(&cmd.env, tmp_objdir_env(t));\n+ *\tif (!run_command(&cmd)) && !tmp_objdir_migrate(t))\n *\t\tprintf(\"success!\\n\");\n *\telse\n *\t\tdie(\"failed...tmp_objdir will clean up for us\");\n--\n2.38.0\n"},{"id":"465027","messageId":"Y0r6LaxDLjpiRNqS@coredump.intra.peff.net","threadId":"58562","inReplyTo":"9d924a5d-5c72-fbe6-270c-a8f6c5fc5850@web.de","subject":"Re: [PATCH v2] bisect--helper: plug strvec leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-15T18:21:33Z","receivedAt":"2022-10-15T18:21:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 15, 2022 at 08:51:34AM +0200, René Scharfe wrote:\n\n> Am 14.10.22 um 21:44 schrieb Jeff King:\n> > And I'd be happy to see all of the run_command_v() variants go away\n> > entirely. It does save a few lines, but with modern niceties, it's\n> > not very many, and it's much less flexible.\n> That would look something like below.\n>\n> [...]\n> 27 files changed, 330 insertions(+), 374 deletions(-)\n\nI was a little surprised we ended up with fewer lines, but I guess most\nof that is not in the callers themselves, but deleting the function and\nflag definitions.\n\nMost call sites seem to be about as long. IMHO the result is quite\nreadable, though of course any big change like this carries risk of\nregression.\n\n> diff --git a/add-interactive.c b/add-interactive.c\n> index f071b2a1b4..ecc5ae1b24 100644\n> --- a/add-interactive.c\n> +++ b/add-interactive.c\n> @@ -997,18 +997,17 @@ static int run_diff(struct add_i_state *s, const struct pathspec *ps,\n> \tcount = list_and_choose(s, files, opts);\n> \topts->flags = 0;\n> \tif (count > 0) {\n> -\t\tstruct strvec args = STRVEC_INIT;\n> +\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \n> -\t\tstrvec_pushl(&args, \"git\", \"diff\", \"-p\", \"--cached\",\n> +\t\tstrvec_pushl(&cmd.args, \"git\", \"diff\", \"-p\", \"--cached\",\n> \t\t\t oid_to_hex(!is_initial ? &oid :\n> \t\t\t\t\ts->r->hash_algo->empty_tree),\n> \t\t\t \"--\", NULL);\n> \t\tfor (i = 0; i < files->items.nr; i++)\n> \t\t\tif (files->selected[i])\n> -\t\t\t\tstrvec_push(&args,\n> +\t\t\t\tstrvec_push(&cmd.args,\n> \t\t\t\t\t files->items.items[i].string);\n> -\t\tres = run_command_v_opt(args.v, 0);\n> -\t\tstrvec_clear(&args);\n> +\t\tres = run_command(&cmd);\n> \t}\n\nSo ones like this are just swapping \"args\" for \"cmd.args\". It's nice\nthat we don't have to remember to free (and in some cases, that lets us\nreturn directly rather than stashing the result of run_command(),\ncleaning up, and then returning it).\n\n> -static const char *argv_checkout[] = {\"checkout\", \"-q\", NULL, \"--\", NULL};\n> -\n> static const char *term_bad;\n> static const char *term_good;\n> \n> @@ -729,20 +727,22 @@ static int is_expected_rev(const struct object_id *oid)\n> enum bisect_error bisect_checkout(const struct object_id *bisect_rev,\n> \t\t\t\t int no_checkout)\n> {\n> -\tchar bisect_rev_hex[GIT_MAX_HEXSZ + 1];\n> \tstruct commit *commit;\n> \tstruct pretty_print_context pp = {0};\n> \tstruct strbuf commit_msg = STRBUF_INIT;\n> \n> -\toid_to_hex_r(bisect_rev_hex, bisect_rev);\n> \tupdate_ref(NULL, \"BISECT_EXPECTED_REV\", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);\n> \n> -\targv_checkout[2] = bisect_rev_hex;\n> \tif (no_checkout) {\n> \t\tupdate_ref(NULL, \"BISECT_HEAD\", bisect_rev, NULL, 0,\n> \t\t\t UPDATE_REFS_DIE_ON_ERR);\n> \t} else {\n> -\t\tif (run_command_v_opt(argv_checkout, RUN_GIT_CMD))\n> +\t\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> +\n> +\t\tcmd.git_cmd = 1;\n> +\t\tstrvec_pushl(&cmd.args, \"checkout\", \"-q\",\n> +\t\t\t oid_to_hex(bisect_rev), \"--\", NULL);\n> +\t\tif (run_command(&cmd))\n\nThis one is a little longer, but IMHO worth it regardless to avoid the\nmagic \"2\" that must match the NULL stuffed into argv_checkout.\n\nThat is mostly coming from using a strvec at all, though. It _could_ use\none and then run_command_v_opt() or whatever, though I like the full\nconversion.\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 5900b81729..5e14decc22 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -347,58 +347,52 @@ static int save_state(struct object_id *stash)\n> \n> static void read_empty(const struct object_id *oid, int verbose)\n> {\n> -\tint i = 0;\n> -\tconst char *args[7];\n> +\tstruct child_process cmd = CHILD_PROCESS_INIT;\n> \n> -\targs[i++] = \"read-tree\";\n> +\tstrvec_push(&cmd.args, \"read-tree\");\n> \tif (verbose)\n> -\t\targs[i++] = \"-v\";\n> -\targs[i++] = \"-m\";\n> -\targs[i++] = \"-u\";\n> -\targs[i++] = empty_tree_oid_hex();\n> -\targs[i++] = oid_to_hex(oid);\n> -\targs[i] = NULL;\n> +\t\tstrvec_push(&cmd.args, \"-v\");\n> +\tstrvec_pushl(&cmd.args, \"-m\", \"-u\", empty_tree_oid_hex(),\n> +\t\t oid_to_hex(oid), NULL);\n> \n> -\tif (run_command_v_opt(args, RUN_GIT_CMD))\n> +\tcmd.git_cmd = 1;\n> +\tif (run_command(&cmd))\n> \t\tdie(_(\"read-tree failed\"));\n> }\n\nLikewise this one, which is even worse, as that \"7\" must match the\nnumber of \"i++\" lines. So regardless of any run_command_v() conversion,\nI am happy to see this move to a strvec (and again, IMHO you might as\nwell go straight to using cmd.args rather than a separate strvec).\n\n-Peff\n"}]}