Re: [PATCH] Allocate msg only after fatal checks to avoid leaks

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> And while it may be tempting to say "well, it does not hurt to free them
> on the die() path", in my opinion that way madness lies. You may have
> access to some local variables that can be freed, but there will be many
> other heap allocations that you don't even know about! Here's a toy
> example from a similar discussion a few years ago:
>
>   https://lore.kernel.org/git/YNypPeoZTRiOxPPQ@coredump.intra.peff.net/

Yeah, I recall that discussion and the example.  Yes, we should not
have to crawl up from a direct caller of die() and free everything
these stack frames hold.

> I.e., this:
>
> diff --git a/builtin/notes.c b/builtin/notes.c
> index cc1163242f..f3d5eda104 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -321,12 +321,8 @@ static int parse_reuse_arg(const struct option *opt, const char *arg, int unset)
>  		die(_("failed to resolve '%s' as a valid ref."), arg);
>  	if (!(value = odb_read_object(the_repository->objects, &object, &type, &len)))
>  		die(_("failed to read object '%s'."), arg);
> -	if (type != OBJ_BLOB) {
> -		strbuf_release(&msg->buf);
> -		free(value);
> -		free(msg);
> +	if (type != OBJ_BLOB)
>  		die(_("cannot read note data from non-blob object '%s'."), arg);
> -	}

Much nicer.