Re: [PATCH] gc: do not warn about too many loose objects

From: Elijah Newren <newren@gmail.com>
Date: Jul 16, 2018, 21:09 UTC
Message-ID: <CABPp-BEpCF9FE7eJwZWjY+bMsjDQnnDaSrHO+e3DtDDsR-=7Hg@mail.gmail.com>
In-Reply-To: <20180716203847.GE25189@sigill.intra.peff.net>

On Mon, Jul 16, 2018 at 1:38 PM, Jeff King <peff@peff.net> wrote:

Show 25 quoted lines

> On Mon, Jul 16, 2018 at 01:16:45PM -0700, Elijah Newren wrote:
>
>> >> The basic problem here, at least for us, is that gc has enough
>> >> information to know it could expunge some objects, but because of how
>> >> it is structured in terms of several substeps (reflog expiration,
>> >> repack, prune), the information is lost between the steps and it
>> >> instead writes them out as unreachable objects.  If we could prune (or
>> >> avoid exploding) loose objects that are only reachable from reflog
>> >> entries that we are expiring, then the problem goes away for us.  (I
>> >> totally understand that other repos may have enough unreachable
>> >> objects for other reasons that Peff's suggestion to just pack up
>> >> unreachable objects is still a really good idea.  But on its own, it
>> >> seems like a waste since it's packing stuff that we know we could just
>> >> expunge.)
>> >
>> > No, we should have expunged everything that could be during the "repack"
>> > and "prune" steps. We feed the expiration time to repack, so that it
>> > knows to drop objects entirely instead of exploding them loose.
>>
>> Um, except it doesn't actually do that.  The testcase I provided shows
>> that it leaves around 10000 objects that are totally deletable and
>> were only previously referenced by reflog entries -- entries that gc
>> removed without removing the corresponding objects.
>
> What's your definition of "totally deletable"?

The point of gc is to: expire reflogs, repack referenced objects, and delete loose objects that (1) are no longer referenced and (2) are "old enough".

The "old enough" bit is the special part here. Before the gc, those objects were referenced (only by old reflog entries) and were found in a pack. git currently writes those objects out to disk and gives them the age of the packfile they are contained in, which I think is the source of the bug. We have a reference for those objects from the reflogs, know they aren't referenced anywhere else, so those objects to me are the age of those reflog entries: 90 days. As such, they are "old enough" and should be deleted.

With big repos, it's easy to get into situations where there are well more than 10000 objects satisfying these conditions. In fact, it's not all that rare that the repo has far more loose objects after a git gc than before.

I never got around to fixing it properly, though, because 'git prune' is such a handy workaround that for now. Having people nuke all their loose objects is a bit risky, but the only other workaround people had was to re-clone (waiting the requisite 20+ minutes for the repo to download) and throw away their old clone. (Though some people even went that route, IIRC.)

> If the pack they were in is less than 2 weeks old, then they are
> unreachable but "fresh", and are intentionally retained. If it was older
> than 2 weeks, they should have been deleted.

That's what's currently implemented, yes, but as above I think that logic is bad.

Show 5 quoted lines

> If you don't like that policy, you can set gc.pruneExpire to something
> lower (including "now", but watch out, as that can race with other
> operations!). But I don't think that's an implementation short-coming.
> It's intentional to keep those objects. It's just that the format
> they're in is inefficient and confuses later gc runs.

I'm aware of pruneExpire, but I think it's the wrong way to handle this.

I totally agree with your general plan to put unreferenced loose objects into a pack. However, I don't think these objects should be part of that pack; they should just be deleted instead.