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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

On Fri, May 31, 2024 at 07:35:30AM -0400, Jeff King wrote:
> 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?

> @@ -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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]