Re: [PATCH v2] builtin/repack.c: avoid making cruft packs preferred

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 5, 2023, 11:14 UTC
Message-ID: <ZR6ajQa8bh5HKsnr@tanuki>
In-Reply-To: <035393935108d02aaf8927189b05102f4f74f340.1696370003.git.me@ttaylorr.com>

On Tue, Oct 03, 2023 at 05:54:19PM -0400, Taylor Blau wrote: [snip]

Show 8 quoted lines

> @@ -801,6 +814,38 @@ static int write_midx_included_packs(struct string_list *include,
>  	if (preferred)
>  		strvec_pushf(&cmd.args, "--preferred-pack=%s",
>  			     pack_basename(preferred));
> +	else if (names->nr) {
> +		/* The largest pack was repacked, meaning that either
> +		 * one or two packs exist depending on whether the
> +		 * repository has a cruft pack or not.

Nit: this comment will grow stale soonish once your patch series lands
that introduces a maximum packfile size for cruft packs as there can be
arbitrarily many cruft packs from thereon.

Show 8 quoted lines

> +		 * Select the non-cruft one as preferred to encourage
> +		 * pack-reuse among packs containing reachable objects
> +		 * over unreachable ones.
> +		 *
> +		 * (Note we could write multiple packs here if
> +		 * `--max-pack-size` was given, but any one of them
> +		 * will suffice, so pick the first one.)
> +		 */

Well, okay, you kind of acknowledge this here.

The rest of this patch series looks good to me and makes sense. I don't really think that this comment here is worth a reroll.

Patrick