Re: [PATCH 4/5] gc: don't run "reflog expire" when keeping reflogs
- From
Jeff King <peff@peff.net>
- Date
- Mar 14, 2019, 00:40 UTC
- Message-ID
- <20190314004006.GI31968@sigill.intra.peff.net>
- In-Reply-To
- <20190313235439.30439-5-avarab@gmail.com>
On Thu, Mar 14, 2019 at 12:54:38AM +0100, Ævar Arnfjörð Bjarmason wrote:
Show 7 quoted lines
> 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