Re: [PATCH v3 2/2] builtin/pack-objects.c: introduce `pack.recentObjectsHook`

From: Jeff King <peff@peff.net>
Date: May 13, 2023, 00:11 UTC
Message-ID: <20230513001117.GA2974285@coredump.intra.peff.net>
In-Reply-To: <xmqq4johnnjr.fsf@gitster.g>

On Fri, May 12, 2023 at 04:21:44PM -0700, Junio C Hamano wrote:

Show 9 quoted lines

> 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