Re: [PATCH 6/8] builtin/repack.c: store existing cruft packs separately

From: Jeff King <peff@peff.net>
Date: Sep 7, 2023, 08:09 UTC
Message-ID: <20230907080913.GF1260718@coredump.intra.peff.net>
In-Reply-To: <414a558883830a29924710126960074b37ab97fc.1693946195.git.me@ttaylorr.com>

On Tue, Sep 05, 2023 at 04:36:54PM -0400, Taylor Blau wrote:

Show 18 quoted lines

> When repacking with the `--write-midx` option, we invoke the function
> `midx_included_packs()` in order to produce the list of packs we want to
> include in the resulting MIDX.
> 
> This list is comprised of:
> 
>   - existing .keep packs
>   - any pack(s) which were written earlier in the same process
>   - any unchanged packs when doing a `--geometric` repack
>   - any cruft packs
> 
> Prior to this patch, we stored pre-existing cruft and non-cruft packs
> together (provided those packs are non-kept). This meant we needed an
> additional bit to indicate which non-kept pack(s) were cruft versus
> those that aren't.
> 
> But alternatively we can store cruft packs in a separate list, avoiding
> the need for this extra bit, and simplifying the code below.

OK. Getting rid of the extra bit is nice. We shorten code that only cares about cruft packs like:

  for each pack
    if (pack is cruft)
       ...

to just:

  for each cruft_pack
    ...

which is good. But the flip side is that any existing code which looks at the combined list now has to do:

  for each pack
     ...
  for each cruft_pack
     ...

I think there's just one case of that, here:

Show 12 quoted lines

> @@ -707,6 +706,12 @@ static void midx_included_packs(struct string_list *include,
>  				continue;
>  			string_list_insert(include, xstrfmt("%s.idx", item->string));
>  		}
> +
> +		for_each_string_list_item(item, &existing->cruft_packs) {
> +			if ((uintptr_t)item->util & DELETE_PACK)
> +				continue;
> +			string_list_insert(include, xstrfmt("%s.idx", item->string));
> +		}
>  	}
>  }

It may be an OK price to pay if this lets us keep cleaning things up (especially if we could get rid of that util casting entirely!). Let's read on...

-Peff