Re: [PATCH 53/83] builtin/apply: make find_header() return -1 instead of die()ing

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 27, 2016, 18:08 UTC
Message-ID: <CAPig+cRw7_6hbLZ7HfvCHwy7UcujnfOTToMSXaRbx30WTXvaQA@mail.gmail.com>
In-Reply-To: <1461504863-15946-54-git-send-email-chriscool@tuxfamily.org>

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

Show 33 quoted lines

> To be compatible with the rest of the error handling in builtin/apply.c,
> find_header() should return -1 instead of calling die().
>
> Unfortunately find_header() already returns -1 when no header is found,
> so let's make it return -2 instead in this case.
>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> diff --git a/builtin/apply.c b/builtin/apply.c
> @@ -1588,18 +1596,18 @@ static int find_header(struct apply_state *state,
>                                 continue;
>                         if (!patch->old_name && !patch->new_name) {
>                                 if (!patch->def_name)
> -                                       die(Q_("git diff header lacks filename information when removing "
> -                                              "%d leading pathname component (line %d)",
> -                                              "git diff header lacks filename information when removing "
> -                                              "%d leading pathname components (line %d)",
> -                                              state->p_value),
> -                                           state->p_value, state->linenr);
> +                                       return error(Q_("git diff header lacks filename information when removing "
> +                                                       "%d leading pathname component (line %d)",
> +                                                       "git diff header lacks filename information when removing "
> +                                                       "%d leading pathname components (line %d)",
> +                                                       state->p_value),
> +                                                    state->p_value, state->linenr);
>                                 patch->old_name = xstrdup(patch->def_name);
>                                 patch->new_name = xstrdup(patch->def_name);
>                         }
>                         if (!patch->is_delete && !patch->new_name)
> -                               die("git diff header lacks filename information "
> -                                   "(line %d)", state->linenr);
> +                               return error("git diff header lacks filename information "
> +                                            "(line %d)", state->linenr);

I realize that the caller in this patch currently just die()'s, and I haven't looked at subsequent patches yet, but is this new 'return' going to cause the caller to start leaking patch->old_name and patch->new_name which are xstrdup()'d just above?

Show 12 quoted lines

>                         patch->is_toplevel_relative = 1;
>                         *hdrsize = git_hdr_len;
>                         return offset;
> @@ -2115,6 +2123,9 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si
>         int hdrsize, patchsize;
>         int offset = find_header(state, buffer, size, &hdrsize, patch);
>
> +       if (offset == -1)
> +               exit(1);
> +
>         if (offset < 0)
>                 return offset;