Re: [PATCH 4/5] gc: don't run "reflog expire" when keeping reflogs

[Jeff King <peff@peff.net>]

On Thu, Mar 14, 2019 at 12:54:38AM +0100, Ævar Arnfjörð Bjarmason wrote:

> Don't redundantly run "git reflog expire --all" when gc.reflogExpire
> and gc.reflogExpireUnreachable are set to "never".
> 
> I'm being careful to not repeat the issue fixed in
> 8ab5aa4bd8 ("parseopt: handle malformed --expire arguments more
> nicely", 2018-04-21). We'll die early if the config variables are set
> to invalid values.

I think the general sentiment here makes sense, that reflog expiration
should be a noop if we're set to "never" anyway.

It does feel a little funny for "gc" to be peeking into the internals of
how "reflog" will make its decision about how to run, though. I doubt
those rules are likely to change, but if they did, there'd be very
little to remind somebody working on reflog that they need to change the
matching rules here.

Is our problem running "git reflog" at all, or is it that "git reflog"
does too much work even when it's been told "never"? If the latter,
could we just have it exit early as soon as it's figured out the prune
expiry dates it's using?

I.e., something like:

diff --git a/builtin/reflog.c b/builtin/reflog.c
index 4d3430900d..aab221f315 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -594,6 +594,15 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
 			break;
 	}
 
+	/*
+	 * If we're not expiring anything and not dropping stale entries,
+	 * there's no point in even opening the reflogs, since we're guaranteed
+	 * to do nothing.
+	 */
+	if (!cb.cmd.expire_total && !cb.cmd.expire_unreachable &&
+	    !cb.cmd.stalefix)
+		return 0;
+
 	/*
 	 * We can trust the commits and objects reachable from refs
 	 * even in older repository.  We cannot trust what's reachable

Seeing "--stale-fix" again reminded me: that may be the "oops, we can
spend tons of CPU walking history" bit that I mentioned in the other
part of the thread. But I don't think git-gc would ever run with that
option.

-Peff