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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: May 28, 2017, 18:51 UTC
Message-ID: <CACBZZX6uXnE+BTfsiLNF2OT3Dsr-J99uUFEwcu-qK45OrU+1hQ@mail.gmail.com>
In-Reply-To: <20170528165642.14699-5-joel@teichroeb.net>

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.

A lot of that's easy to solve, but sometimes requires a temporary variable since the code is now returning directly, e.g:

@@ -1091,6 +1094,7 @@ static int list_stash(int argc, const char
**argv, const char *prefix)
        struct object_id obj;
        struct object_context unused;
        struct argv_array args;
+       int ret = 0;

        argc = parse_options(argc, argv, prefix, options,
                                 git_stash_list_usage, PARSE_OPT_KEEP_UNKNOWN);
@@ -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;
 }

But more generally this goes a long way to resolving the issue where
you have variables like out1, out2 or cp1, cp2 etc. which Christian
pointed out. I.e. you're not freeing/clearing strbufs either, instead
just creating new ones that also aren't freed, or not clearing
child_process structs, e.g. this on top allows you to re-use the same
variable and stops leaking memory:

diff --git a/builtin/stash.c b/builtin/stash.c
index bf36ff8f9b..4e7344501a 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -729,7 +729,6 @@ static int do_push_stash(const char *prefix, const
char *message,

                if (keep_index) {
                        struct child_process cp = CHILD_PROCESS_INIT;
-                       struct child_process cp2 = CHILD_PROCESS_INIT;
                        struct strbuf out = STRBUF_INIT;

                        reset_tree(info.i_tree, 0, 1);
@@ -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);
+                       child_process_clear(&cp);

-                       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);
                }
        } else {
                struct child_process cp2 = CHILD_PROCESS_INIT;