Re: [PATCH 2/2] repack: avoid loosening promisor pack objects in partial clones

From: Jeff King <peff@peff.net>
Date: Apr 15, 2021, 09:03 UTC
Message-ID: <YHgBTPGTmDUBMGA9@coredump.intra.peff.net>
In-Reply-To: <xmqqo8egurx5.fsf@gitster.g>

On Wed, Apr 14, 2021 at 08:51:02PM -0700, Junio C Hamano wrote:

Show 23 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >> When `-A` and `-d` are used together, besides packing all objects (-A)
> >> and removing redundant packs (-d), it also unpack all unreachable
> >> objects and deletes them by calling `git pruned-packed`.
> >
> > I still think of these objects as not unreachable, even though I know
> > that pack-objects calls them that (the argument is called
> > --unpack-unreachable). So I would say "it also loosens all objects that
> > were previously packed but did not go into the new pack", but perhaps
> > this is OK too.
> 
> Hmph, that is puzzling.  I understand that the operation about
> 
>  (1) finding all the objects that are still reachable and send them
>      into a newly created pack, and
> 
>  (2) among the objects that were previously in the packs, eject
>      those that weren't made into the new pack with the previous
>      point.
> 
> Where did I get it wrong?  If all the reachable ones are dealt with
> with the first point, what is leftover is not reachable, no?

Right. I think your understanding is correct, and the commit message is a bit confused. Normally after we eject loose objects, they'd stay there (a follow-up git-gc may run git-prune and delete them, though if they were recent enough not to just drop completely during the repack, then git-prune would likewise leave them be). Talking about prune-packed here is misleading, because it usually has nothing to do with these objects.

What makes the partial-clone situation under discussion interesting is that the objects _are_ reachable. They are excluded from the new pack because we put them in a separate promisor pack. But we erroneously turn them loose, rather than realizing that they were excluded for a different reason.

So the fundamental bug is that we turn them loose at all. What makes the bug trickier to see is that when we run prune-packed afterwards, we then clean up the evidence of the bug (so it looks more like a performance problem than a correctness one).

-Peff