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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 2, 2016, 17:42 UTC
Message-ID: <CAPig+cTpTYowkeO-cDd-87aAw_tRHvdhV4BaBveg25MvsDAd4w@mail.gmail.com>
In-Reply-To: <1461504863-15946-74-git-send-email-chriscool@tuxfamily.org>

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

Show 9 quoted lines

> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> diff --git a/builtin/apply.c b/builtin/apply.c
> @@ -4381,7 +4387,7 @@ static int write_out_results(struct apply_state *state, struct patch *list)
>                                 errs = 1;
>                         else {
>                                 if (write_out_one_result(state, l, phase))
> -                                       exit(1);
> +                                       return -1;

Isn't this leaking 'string_list cpath'?

Show 13 quoted lines

>                                 if (phase == 1) {
>                                         if (write_out_one_reject(state, l))
>                                                 errs = 1;
> @@ -4484,11 +4490,16 @@ static int apply_patch(struct apply_state *state,
>             !state->apply_with_reject)
>                 return -1;
>
> -       if (state->apply && write_out_results(state, list)) {
> -               if (state->apply_with_reject)
> +       if (state->apply) {
> +               int res = write_out_results(state, list);
> +               if (res < 0)
>                         return -1;

Mentioned previously: Leaking 'list', 'buf', 'fn_table'.

Show 7 quoted lines

> -               /* with --3way, we still need to write the index out */
> -               return 1;
> +               if (res > 0) {
> +                       if (state->apply_with_reject)
> +                               return -1;
> +                       /* with --3way, we still need to write the index out */
> +                       return 1;

Not the fault of this patch, but this 'return' in the original code was also leaking 'list', 'buf', 'fn_table', right?

> +               }
>         }
>
>         if (state->fake_ancestor &&