From: Patrick Steinhardt <ps@pks.im>
Date: Mon, 08 Dec 2025 18:27:16 GMT
Subject: Re: [PATCH 15/17] t/helper/test-read-midx.c: plug memory leak when selecting layer
Message-ID: <aTcYhKOIu7ebJ_xV@pks.im>
In-Reply-To: <43ef3d0013f1534c7ff8b1a7c4a17fb0c9b68684.1765053054.git.me@ttaylorr.com>

On Sat, Dec 06, 2025 at 03:31:43PM -0500, Taylor Blau wrote:
> diff --git a/t/helper/test-read-midx.c b/t/helper/test-read-midx.c
> index dee603b3cd0..6e03aabca79 100644
> --- a/t/helper/test-read-midx.c
> +++ b/t/helper/test-read-midx.c
> @@ -26,9 +26,10 @@ static int read_midx_file(const char *object_dir, const char *checksum,
>  			  int show_objects)
>  {
>  	uint32_t i;
> -	struct multi_pack_index *m;
> +	struct multi_pack_index *m, *tip;
> +	int ret = 0;
>  
> -	m = setup_midx(object_dir);
> +	m = tip = setup_midx(object_dir);
>  
>  	if (!m)
>  		return 1;

I was briefly wondering whether we should also convert this into a `goto
out`. It's of course not required, as setting up the MIDX has just
failed. But it would simplfy things a bit as we now have a single exit
path, only.

I don't mind this too much though.

> @@ -36,8 +37,11 @@ static int read_midx_file(const char *object_dir, const char *checksum,
>  	if (checksum) {
>  		while (m && strcmp(get_midx_checksum(m), checksum))
>  			m = m->base_midx;
> -		if (!m)
> -			return 1;
> +		if (!m) {
> +			ret = error(_("could not find MIDX with checksum %s"),
> +				    checksum);
> +			goto out;
> +		}
>  	}
>  
>  	printf("header: %08x %d %d %d %d\n",

We change the return code from 1 to -1, but that ultimately shouldn't
matter much.

I'll stop reviewing here and will have a look at the remaining two
patches with some fresh eyes. But so far this was a nice read, thanks!

Patrick