Re: [PATCH] builtin/apply: exit when parse_binary() fails
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 16, 2016, 19:44 UTC
- Message-ID
- <xmqqshzq438s.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1458156675-26109-1-git-send-email-chriscool@tuxfamily.org>
Christian Couder <christian.couder@gmail.com> writes:
Show 11 quoted lines
> In parse_binary() there is:
>
> forward = parse_binary_hunk(&buffer, &size, &status, &used);
> if (!forward && !status)
> /* there has to be one hunk (forward hunk) */
> return error(_("unrecognized binary patch at line %d"), linenr-1);
>
> so parse_binary() can return -1, because that's what error() returns.
>
> Also parse_binary_hunk() sets "status" to -1 in case of error and
> parse_binary() does "if (status) return status;".All of the above sounds sensible, and your follow-up patch expects parse_chunk() to return -1 on failure--it is a bit sad that you make parse_chunk() to directly call exit(2). In the spirit of eventually libifying this codepath, shouldn't we be turning existing die() to an error return, instead of introducing more calls to exit(2)?
> In this case parse_chunk() should just exit, rather than add -1 to the > patchsize it computes.
Show 30 quoted lines
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
> builtin/apply.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index 42c610e..18dec0f 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -1872,6 +1872,11 @@ static struct fragment *parse_binary_hunk(char **buf_p,
> return NULL;
> }
>
> +/*
> + * Returns:
> + * -1 in case of error,
> + * the length of the parsed binary patch otherwise
> + */
> static int parse_binary(char *buffer, unsigned long size, struct patch *patch)
> {
> /*
> @@ -2017,6 +2022,8 @@ static int parse_chunk(char *buffer, unsigned long size, struct patch *patch)
> linenr++;
> used = parse_binary(buffer + hd + llen,
> size - hd - llen, patch);
> + if (used < 0)
> + exit(1);
> if (used)
> patchsize = used + llen;
> else