On Sun, May 28, 2017 at 08:51:07PM +0200, Ævar Arnfjörð Bjarmason wrote: > On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@xxxxxxxxxxxxx> wrote: > > Implement all git stash functionality as a builtin command > > > > Signed-off-by: Joel Teichroeb <joel@xxxxxxxxxxxxx> > > --- > > General note on this that I missed in my first E-Mail, you have ~20 > calls to argv_array_init() but none to argv_array_clear(). So you're > leaking memory, and it obscures potential other issues with valgrind. I haven't looked carefully at the patches, but note that the argv array in a child_process is handled automatically by start/finish_command. So: > @@ -1107,9 +1111,9 @@ static int list_stash(int argc, const char > **argv, const char *prefix) > argv_array_pushv(&args, argv); > argv_array_push(&args, ref_stash); > if (cmd_log(args.argc, args.argv, prefix)) > - return 1; > - > - return 0; > + ret = 1; > + argv_array_clear(&args); > + return ret; > } This one does need a clear, because it's calling an internal function. But... > @@ -741,13 +740,18 @@ static int do_push_stash(const char *prefix, > const char *message, > argv_array_push(&cp.args, "--"); > argv_array_pushv(&cp.args, argv); > pipe_command(&cp, NULL, 0, &out, 0, NULL, 0); > + argv_array_clear(&cp.args); This one does not, because the array will have been cleared by calling pipe_command (because it does the start/finish block itself; and yes, before you go checking, start_command() clears it even when it returns error). > + child_process_clear(&cp); This should not be necessary for the same reason. > - cp2.git_cmd = 1; > - argv_array_push(&cp2.args, "checkout-index"); > - argv_array_push(&cp2.args, "-z"); > - argv_array_push(&cp2.args, "--force"); > - argv_array_push(&cp2.args, "--stdin"); > - pipe_command(&cp2, out.buf, out.len, NULL, 0, NULL, 0); > + child_process_init(&cp); > + cp.git_cmd = 1; > + argv_array_push(&cp.args, "checkout-index"); > + argv_array_push(&cp.args, "-z"); > + argv_array_push(&cp.args, "--force"); > + argv_array_push(&cp.args, "--stdin"); > + pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0); > + argv_array_clear(&cp.args); > + child_process_clear(&cp); And ditto here. I'd also encourage using CHILD_PROCESS_INIT and ARGV_ARRAY_INIT constant initializers instead of their function-call counterparts, as that matches our usual style. -Peff