From: Jeff King <peff@peff•net>
To: ZheNing Hu via GitGitGadget <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, gitster@pobox•com, me@ttaylorr•com,
ZheNing Hu <adlternative@gmail•com>
Subject: Re: [PATCH v2 2/2] fix(gc): make --prune=now compatible with --expire-to
Date: Mon, 13 Jan 2025 04:17:50 -0500 [thread overview]
Message-ID: <20250113091750.GA794747@coredump.intra.peff.net> (raw)
In-Reply-To: <579757957d21faaa8dd9228a191d82f663e93c03.1735611513.git.gitgitgadget@gmail.com>
On Tue, Dec 31, 2024 at 02:18:33AM +0000, ZheNing Hu via GitGitGadget wrote:
> 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
next prev parent reply other threads:[~2025-01-13 9:17 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-24 11:52 [PATCH] gc: add `--expire-to` option ZheNing Hu via GitGitGadget
2024-12-31 2:18 ` [PATCH v2 0/2] gc: add --expire-to option ZheNing Hu via GitGitGadget
2024-12-31 2:18 ` [PATCH v2 1/2] gc: add `--expire-to` option ZheNing Hu via GitGitGadget
2025-01-03 4:57 ` ZheNing Hu
2025-01-13 7:12 ` ZheNing Hu
2024-12-31 2:18 ` [PATCH v2 2/2] fix(gc): make --prune=now compatible with --expire-to ZheNing Hu via GitGitGadget
2025-01-13 9:17 ` Jeff King [this message]
2025-01-15 7:56 ` ZheNing Hu
2025-01-16 2:35 ` [PATCH v3] gc: add `--expire-to` option ZheNing Hu via GitGitGadget
2025-01-16 18:23 ` Junio C Hamano
2025-01-23 3:42 ` ZheNing Hu
2025-01-24 7:49 ` [PATCH v4] " ZheNing Hu via GitGitGadget
2025-02-04 18:15 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250113091750.GA794747@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=adlternative@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=gitster@pobox$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox