Re: [PATCH 67/83] builtin/apply: make build_fake_ancestor() return -1 on error

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 2, 2016, 07:32 UTC
Message-ID: <CAPig+cSMLCg49O3tf=g2qDi-TUz-xsdW-ac_Eze9g=XsCKSSJA@mail.gmail.com>
In-Reply-To: <1461504863-15946-68-git-send-email-chriscool@tuxfamily.org>

On Sun, Apr 24, 2016 at 9:34 AM, Christian Couder <christian.couder@gmail.com> wrote:

Show 13 quoted lines

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> diff --git a/builtin/apply.c b/builtin/apply.c
> @@ -3913,31 +3913,34 @@ static void build_fake_ancestor(struct patch *list, const char *filename)
>                 ce = make_cache_entry(patch->old_mode, sha1, name, 0, 0);
>                 if (!ce)
> -                       die(_("make_cache_entry failed for path '%s'"), name);
> +                       return error(_("make_cache_entry failed for path '%s'"),
> +                                    name);
>                 if (add_index_entry(&result, ce, ADD_CACHE_OK_TO_ADD))
> -                       die ("Could not add %s to temporary index", name);
> +                       return error("Could not add %s to temporary index",
> +                                    name);

What happens to 'ce' here? Does it leak?

Show 6 quoted lines

>         }
>
>         hold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);
>         if (write_locked_index(&result, &lock, COMMIT_LOCK))
> -               die ("Could not write temporary index to %s", filename);
> +               return error("Could not write temporary index to %s", filename);

What happens with 'result'? Does it need to be discarded?

Show 14 quoted lines

>         discard_index(&result);
> +       return 0;
>  }
>
>  static void stat_patch_list(struct apply_state *state, struct patch *patch)
> @@ -4470,8 +4473,9 @@ static int apply_patch(struct apply_state *state,
>                 return 1;
>         }
>
> -       if (state->fake_ancestor)
> -               build_fake_ancestor(list, state->fake_ancestor);
> +       if (state->fake_ancestor &&
> +           build_fake_ancestor(list, state->fake_ancestor))
> +               return -1;

Same comment as earlier patches: Leaking 'list', 'buf', 'fn_table'.

>         if (state->diffstat)
>                 stat_patch_list(state, list);