Re: [PATCH 1/4] stash: convert apply to builtin

From: Christian Couder <christian.couder@gmail.com>
Date: Mar 25, 2018, 09:27 UTC
Message-ID: <CAP8UFD1f=VM7VLWGPOzn5PSwHr_m2PohP0gOCM6wk7FdoZ3-Yg@mail.gmail.com>
In-Reply-To: <CAPig+cSkQLSvOroB0bLLLBAXy9UBDN+s=i97COtNDpO0FbLJkg@mail.gmail.com>

On Sun, Mar 25, 2018 at 8:40 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:

> On Sat, Mar 24, 2018 at 1:37 PM, Joel Teichroeb <joel@teichroeb.net> wrote:

Show 25 quoted lines

>> +static int do_apply_stash(const char *prefix, struct stash_info *info, int index)
>> +{
>> +       if (index) {
>> +               if (!oidcmp(&info->b_tree, &info->i_tree) || !oidcmp(&c_tree, &info->i_tree)) {
>> +                       has_index = 0;
>> +               } else {
>> +                       struct child_process cp = CHILD_PROCESS_INIT;
>> +                       struct strbuf out = STRBUF_INIT;
>> +                       struct argv_array args = ARGV_ARRAY_INIT;
>> +                       cp.git_cmd = 1;
>> +                       argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL);
>> +                       argv_array_pushf(&cp.args, "%s^2^..%s^2", sha1_to_hex(info->w_commit.hash), sha1_to_hex(info->w_commit.hash));
>> +                       if (pipe_command(&cp, NULL, 0, &out, 0, NULL, 0))
>> +                               return -1;
>
> Leaking 'out'?
>
>> +
>> +                       child_process_init(&cp);
>> +                       cp.git_cmd = 1;
>> +                       argv_array_pushl(&cp.args, "apply", "--cached", NULL);
>> +                       if (pipe_command(&cp, out.buf, out.len, NULL, 0, NULL, 0))
>> +                               return -1;
>
> Leaking 'out'.

It might be a good idea to have small functions encapsulating the forks of git commands. For example:

static int diff_tree_binary(struct strbuf *out, struct object_id *w_commit)
{
    struct child_process cp = CHILD_PROCESS_INIT;
    const char *w_commit_hex = oid_to_hex(w_commit);

    cp.git_cmd = 1;
    argv_array_pushl(&cp.args, "diff-tree", "--binary", NULL);
    argv_array_pushf(&cp.args, "%s^2^..%s^2", w_commit_hex, w_commit_hex);

    return pipe_command(&cp, NULL, 0, out, 0, NULL, 0);
}

static int apply_cached(struct strbuf *out)
{
    struct child_process cp = CHILD_PROCESS_INIT;

    cp.git_cmd = 1;
    argv_array_pushl(&cp.args, "apply", "--cached", NULL);
    return pipe_command(&cp, out->buf, out->len, NULL, 0, NULL, 0);
}

This could help find leaks and maybe later make it easier to call libified code instead of forking a git command.