On Tue, Oct 04 2022, René Scharfe wrote: > The strvec "argv" is used to build a command for run_command_v_opt(), > but never freed. Use the "args" strvec of struct child_process and > run_command() instead, which releases the allocated memory both on > success and on error. We just also need to set the "git_cmd" bit > directly. > > Signed-off-by: René Scharfe <l.s.r@xxxxxx> > --- > builtin/bisect--helper.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 501245fac9..9fe0c08479 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -765,11 +765,12 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a > strbuf_read_file(&start_head, git_path_bisect_start(), 0); > strbuf_trim(&start_head); > if (!no_checkout) { > - struct strvec argv = STRVEC_INIT; > + struct child_process cmd = CHILD_PROCESS_INIT; > > - strvec_pushl(&argv, "checkout", start_head.buf, > + cmd.git_cmd = 1; > + strvec_pushl(&cmd.args, "checkout", start_head.buf, > "--", NULL); > - if (run_command_v_opt(argv.v, RUN_GIT_CMD)) { > + if (run_command(&cmd)) { > res = error(_("checking out '%s' failed." > " Try 'git bisect start " > "<valid-branch>'."), Okey, so we leak the strvec, and instead of adding a strvec_clear() you're just switching the lower-level API, which we'd need in some cases with this API, and would often be cleaner. But I don't get it in this case, why not just: diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 4e97817fba5..f9645a9d0df 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -763,11 +763,9 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a strbuf_read_file(&start_head, git_path_bisect_start(), 0); strbuf_trim(&start_head); if (!no_checkout) { - struct strvec argv = STRVEC_INIT; + const char *argv[] = { "checkout", start_head.buf, "--", NULL }; - strvec_pushl(&argv, "checkout", start_head.buf, - "--", NULL); - if (run_command_v_opt(argv.v, RUN_GIT_CMD)) { + if (run_command_v_opt(argv, RUN_GIT_CMD)) { res = error(_("checking out '%s' failed." " Try 'git bisect start " "<valid-branch>'."), The common pattern for run_command_v_opt() callers that don't need a dynamic list is exactly that, e.g.: builtin/difftool.c=static int print_tool_help(void) builtin/difftool.c-{ builtin/difftool.c- const char *argv[] = { "mergetool", "--tool-help=diff", NULL }; builtin/difftool.c: return run_command_v_opt(argv, RUN_GIT_CMD); builtin/difftool.c-} And: fsmonitor-ipc.c=static int spawn_daemon(void) fsmonitor-ipc.c-{ fsmonitor-ipc.c- const char *args[] = { "fsmonitor--daemon", "start", NULL }; fsmonitor-ipc.c- fsmonitor-ipc.c: return run_command_v_opt_tr2(args, RUN_COMMAND_NO_STDIN | RUN_GIT_CMD, fsmonitor-ipc.c- "fsmonitor"); fsmonitor-ipc.c-} Here we have the "start_head" which we'll need to strbuf_release(), but we're not returning directly, and the function is doing that already. Your version is slightly more memory efficient, i.e. we'll end up having to push this to a "struct child_process"'s argv anyway, but this is less code & we don't need to carefully eyeball run_command_v_opt_cd_env_tr2() to see that it's correct (which I did, your version does the right thing too).