From: Jonathan Tan <jonathantanmy@google.com>
Date: Fri, 24 Feb 2023 22:37:30 GMT
Subject: Re: [PATCH 05/11] sparse-checkout: avoid using internal API of unpack-trees
Message-ID: <20230224223730.1510266-1-jonathantanmy@google.com>
In-Reply-To: <508837fc182b0adba96c3fcbd468f47f8f0a0aef.1677143700.git.gitgitgadget@gmail.com>

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index c3738154918..4b7390ce367 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -219,14 +219,13 @@ static int update_working_directory(struct pattern_list *pl)
>  	o.dst_index = r->index;
>  	index_state_init(&o.result, r);
>  	o.skip_sparse_checkout = 0;
> -	o.pl = pl;
>  
>  	setup_work_tree();
>  
>  	repo_hold_locked_index(r, &lock_file, LOCK_DIE_ON_ERROR);
>  
>  	setup_unpack_trees_porcelain(&o, "sparse-checkout");
> -	result = update_sparsity(&o);
> +	result = update_sparsity(&o, pl);

This makes sense - setup_unpack_trees_porcelain() does not use o.pl, so
there is no logic change.

> @@ -2111,11 +2111,12 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
>  	trace_performance_enter();
>  
>  	/* If we weren't given patterns, use the recorded ones */
> -	if (!o->pl) {
> -		memset(&pl, 0, sizeof(pl));
> +	if (!pl) {
>  		free_pattern_list = 1;
> -		populate_from_existing_patterns(o, &pl);
> +		pl = xcalloc(1, sizeof(*pl));
> +		populate_from_existing_patterns(o, pl);
>  	}
> +	o->pl = pl;
>  
>  	/* Expand sparse directories as needed */
>  	expand_index(o->src_index, o->pl);
> @@ -2147,8 +2148,10 @@ enum update_sparsity_result update_sparsity(struct unpack_trees_options *o)
>  
>  	display_warning_msgs(o);
>  	o->show_all_errors = old_show_all_errors;
> -	if (free_pattern_list)
> -		clear_pattern_list(&pl);
> +	if (free_pattern_list) {
> +		clear_pattern_list(pl);
> +		free(pl);
> +	}

When free_pattern_list is true, we free "pl" which was previously
assigned to "o->pl". Do we thus need to also clear "o->pl"?