Re: [PATCH v2 5/8] pack-objects: fix error when same packfile is included and excluded

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 12, 2023, 21:52 UTC
Message-ID: <ZDcoKN1rEzqxZAe2@nand.local>
In-Reply-To: <9b278a4c91a5631d1b7b11bf56ab560c6bb58645.1681294715.git.ps@pks.im>

On Wed, Apr 12, 2023 at 12:22:48PM +0200, Patrick Steinhardt wrote:

Show 17 quoted lines

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index fdf3f440be..522eb4dd31 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -3366,11 +3366,9 @@ static void read_packs_list_from_stdin(void)
>  	for (p = get_all_packs(the_repository); p; p = p->next) {
>  		const char *pack_name = pack_basename(p);
>
> -		item = string_list_lookup(&include_packs, pack_name);
> -		if (!item)
> -			item = string_list_lookup(&exclude_packs, pack_name);
> -
> -		if (item)
> +		if ((item = string_list_lookup(&include_packs, pack_name)))
> +			item->util = p;
> +		if ((item = string_list_lookup(&exclude_packs, pack_name)))
>  			item->util = p;

Oof. I was hoping that we could avoid having to look through both lists. But that relies on us having disjoint sets of packs in the include and exclude lists.

We probably *could* just ban this combination outright, but that would also involve some work to try and detect that case. So I think that doing this (and ensuring that the resulting pack is empty, that is that the exclude set takes precedence here) is the right thing to do.

This (and the elided test below) look great to me.

Thanks, Taylor