Re: [PATCH 09/13] sparse-checkout: refactor temporary sparse_checkout_patterns

From: Patrick Steinhardt <ps@pks.im>
Date: Jun 4, 2024, 07:42 UTC
Message-ID: <Zl7Fe6132gy1JclD@framework>
In-Reply-To: <20240531113530.GI428814@coredump.intra.peff.net>

On Fri, May 31, 2024 at 07:35:30AM -0400, Jeff King wrote:

Show 10 quoted lines

> In update_working_directory(), we take in a pattern_list, attach it to
> the repository index by assigning it to index->sparse_checkout_patterns,
> and then call unpack_trees. Afterwards, we remove it by setting
> index->sparse_checkout_patterns back to NULL.
> 
> But there are two possible leaks here:
> 
>   1. If the index already had a populated sparse_checkout_patterns,
>      we've obliterated it. We can fix this by saving and restoring it,
>      rather than always setting it back to NULL.

So this isn't only a leak, but also a potential bug because we did not restore the old pattern list?

Show 13 quoted lines

> @@ -241,7 +243,12 @@ static int update_working_directory(struct pattern_list *pl)
>  
>  	clean_tracked_sparse_directories(r);
>  
> -	r->index->sparse_checkout_patterns = NULL;
> +	if (r->index->sparse_checkout_patterns != pl) {
> +		clear_pattern_list(r->index->sparse_checkout_patterns);
> +		FREE_AND_NULL(r->index->sparse_checkout_patterns);
> +	} else {
> +		r->index->sparse_checkout_patterns = old_pl;
> +	}
>  	return result;
>  }

What I find weird is that we sometimes restore the old value, and sometimes we don't. I get that it makes sense to conditionally free only the lazy-loaded list. But shouldn't we then unconditionally assign `old_pl`?

Makes me wonder whether I miss some subtlety here.

Patrick