Re: [PATCH v2 2/2] fix(gc): make --prune=now compatible with --expire-to

From: Jeff King <peff@peff.net>
Date: Jan 13, 2025, 09:17 UTC
Message-ID: <20250113091750.GA794747@coredump.intra.peff.net>
In-Reply-To: <579757957d21faaa8dd9228a191d82f663e93c03.1735611513.git.gitgitgadget@gmail.com>

On Tue, Dec 31, 2024 at 02:18:33AM +0000, ZheNing Hu via GitGitGadget wrote:

Show 12 quoted lines

> diff --git a/builtin/gc.c b/builtin/gc.c
> index 77904694c9f..8656e1caff0 100644
> --- a/builtin/gc.c
> +++ b/builtin/gc.c
> @@ -433,7 +433,8 @@ static int keep_one_pack(struct string_list_item *item, void *data UNUSED)
>  static void add_repack_all_option(struct gc_config *cfg,
>  				  struct string_list *keep_pack)
>  {
> -	if (cfg->prune_expire && !strcmp(cfg->prune_expire, "now"))
> +	if (cfg->prune_expire && !strcmp(cfg->prune_expire, "now")
> +		&& !(cfg->cruft_packs && cfg->repack_expire_to))
>  		strvec_push(&repack, "-a");

I expected to see a mention of repack_expire_to here, but not cfg->cruft_packs. These two are AND-ed together so we are only disabling "repack -a" when both options ("--expire-to" and "--cruft") are passed. Can we --expire-to without cruft? I.e., what should happen with:

  git gc --expire-to=some-path --prune=now --no-cruft

Looking at the underlying git-repack, it seems that we only respect --expire-to at all when used with "--cruft", and don't otherwise consider it. Which is what the manpage says ("Only useful with --cruft -d").

But if we look at this proposed patch for example:

  https://lore.kernel.org/git/48438876fb42a889110e100a6c42ca84e93aac49.1733011259.git.me@ttaylorr.com/

then it is expanding how --expire-to is used during the pruning step. OTOH, I think the way your patch 1 is structured means that we'd always pass --expire-to to git-repack anyway, and I _think_ even with the patch linked above that "repack -a -d --expire-to=whatever" would do the right thing.

In which case the problem really is the combination of cruft packs and expire-to. Just cruft packs by themselves do not need to override using "-a" for "--prune=now" because we know that any such cruft pack would be empty.

So I think this logic is correct. Taylor might have more thoughts, though (and ideas on whether he intends to revisit that earlier patch).

I do think this change should probably be done as part of patch 1, rather than introducing a buggy state and then fixing it in patch 2.

-Peff