Re: [PATCH] builtin/: add UNLEAKs

From: Jeff King <peff@peff.net>
Date: Oct 2, 2017, 06:25 UTC
Message-ID: <20171002062528.naa5hrqoao5sifs3@sigill.intra.peff.net>
In-Reply-To: <20171001174208.1693753-1-martin.agren@gmail.com>

On Sun, Oct 01, 2017 at 07:42:08PM +0200, Martin Ågren wrote:

> Add some UNLEAKs where we are about to return from `cmd_*`. UNLEAK the
> variables in the same order as we've declared them. While addressing
> `msg` in builtin/tag.c, convert the existing `strbuf_release()` calls as
> well.

It might have raised Junio's eyebrows less to say something like:

   ...convert the existing strbuf_release() calls as well (they're not
   wrong, but they also accomplish nothing and create an inconsistency
   with the UNLEAKed variables).

That aside, the patch looks good.

Show 12 quoted lines

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 3345a0d16..2daa4412a 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1298,6 +1298,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>  		strbuf_release(&buf);
>  	}
>  
> +	UNLEAK(opts);
>  	if (opts.patch_mode || opts.pathspec.nr)
>  		return checkout_paths(&opts, new.name);
>  	else

Seeing hunks like this makes me happy with the UNLEAK() solution. It would have been a real pain to do this via actual freeing.

-Peff