Re: [PATCH v3 4/4] stash: implement builtin stash

From: Jeff King <peff@peff.net>
Date: May 28, 2017, 19:21 UTC
Message-ID: <20170528192149.dgzibu334n5ja57e@sigill.intra.peff.net>
In-Reply-To: <CACBZZX6uXnE+BTfsiLNF2OT3Dsr-J99uUFEwcu-qK45OrU+1hQ@mail.gmail.com>

On Sun, May 28, 2017 at 08:51:07PM +0200, Ævar Arnfjörð Bjarmason wrote:

Show 9 quoted lines

> On Sun, May 28, 2017 at 6:56 PM, Joel Teichroeb <joel@teichroeb.net> wrote:
> > Implement all git stash functionality as a builtin command
> >
> > Signed-off-by: Joel Teichroeb <joel@teichroeb.net>
> > ---
> 
> 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:

Show 12 quoted lines

> @@ -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...

Show 6 quoted lines

> @@ -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.

Show 15 quoted lines

> -                       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