From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Taylor Blau <me@ttaylorr.com>,
git@vger.kernel.org, Chris Torek <chris.torek@gmail.com>,
Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`
Date: Fri, 12 May 2023 20:11:17 -0400 [thread overview]
Message-ID: <20230513001117.GA2974285@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqq4johnnjr.fsf@gitster.g>
On Fri, May 12, 2023 at 04:21:44PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > It does make me wonder why "git prune --expire=never" does not simply
> > exit immediately. Isn't there by definition nothing useful to do?
>
> I think the answer to the question is "no", but if I have to guess
> why such a low-hanging fruit optimization possibility has not been
> exploited so far is because it does not optimize a useful case,
> perhaps? IOW, falling into "if it hurts, then don't do it" category?
I think it would come up any time you run "git gc", if you've set
gc.pruneExpire to "never". And then it wastes time running a full object
walk (which is 30+ seconds for linux.git) even though it won't do
anything useful.
Curiously, git-gc is sprinkled with "if (prune_expire)" logic, including
skipping the call to git-prune entirely. But that only kicks in if you
run "git gc --no-prune". If "never" is truly the same thing (and I
cannot off the top of my head think of a reason that it isn't), then
this:
diff --git a/builtin/gc.c b/builtin/gc.c
index f3942188a6..5118535a4d 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -610,6 +610,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
if (prune_expire && parse_expiry_date(prune_expire, &dummy))
die(_("failed to parse prune expiry value %s"), prune_expire);
+ if (prune_expire && !dummy)
+ prune_expire = NULL; /* "never" same as --no-prune */
if (aggressive) {
strvec_push(&repack, "-f");
would bring the two cases in sync (I tried to minimize the diff for
illustration; probably some light refactoring would be in order).
I guess nobody cared because it is not that common to set pruneExpire to
"never". We did something like that at GitHub, but we always drove
repack and prune through our own scripts, not through git-gc. I don't
have access to those scripts anymore, but I'm pretty sure they just
skipped calling git-prune entirely for this case.
So yeah, I think it may just be a curiosity that nobody noticed and
bothered to optimize it. I am tempted to work the above into a proper
patch. We even do something similar already for the reflog expiration
variables.
-Peff
next prev parent reply other threads:[~2023-05-13 0:11 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-11 23:20 [PATCH v3 0/2] pack-objects: introduce `pack.extraRecentObjectsHook` Taylor Blau
2023-05-11 23:20 ` [PATCH v3 1/2] reachable.c: extract `obj_is_recent()` Taylor Blau
2023-05-11 23:20 ` [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook` Taylor Blau
2023-05-12 4:58 ` Jeff King
2023-05-15 20:15 ` Taylor Blau
2023-05-12 21:24 ` Jeff King
2023-05-12 21:36 ` Taylor Blau
2023-05-12 21:46 ` Jeff King
2023-05-12 21:45 ` Jeff King
2023-05-12 22:01 ` Jeff King
2023-05-12 23:21 ` Junio C Hamano
2023-05-13 0:11 ` Jeff King [this message]
2023-05-13 0:11 ` Jeff King
2023-05-15 20:49 ` Taylor Blau
2023-05-15 20:38 ` Taylor Blau
2023-05-11 23:23 ` [PATCH v3 0/2] pack-objects: introduce `pack.extraRecentObjectsHook` Taylor Blau
2023-05-11 23:39 ` Junio C Hamano
2023-05-11 23:48 ` Taylor Blau
2023-05-16 0:23 ` [PATCH v4 0/2] gc: introduce `gc.recentObjectsHook` Taylor Blau
2023-05-16 0:24 ` [PATCH v4 1/2] reachable.c: extract `obj_is_recent()` Taylor Blau
2023-05-16 0:24 ` [PATCH v4 2/2] gc: introduce `gc.recentObjectsHook` Taylor Blau
2023-05-24 23:21 ` Glen Choo
2023-06-07 22:56 ` Taylor Blau
2023-06-07 22:58 ` [PATCH v5 0/2] " Taylor Blau
2023-06-07 22:58 ` [PATCH v5 1/2] reachable.c: extract `obj_is_recent()` Taylor Blau
2023-06-07 22:58 ` [PATCH v5 2/2] gc: introduce `gc.recentObjectsHook` Taylor Blau
2023-06-09 23:33 ` Glen Choo
2023-06-12 21:14 ` 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=20230513001117.GA2974285@coredump.intra.peff.net \
--to=peff@peff.net \
--cc=chris.torek@gmail.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=me@ttaylorr.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).