Re: [PATCH 2/2] repack -ad: prune the list of shallow commits

From: Jeff King <peff@peff.net>
Date: Jul 18, 2018, 17:45 UTC
Message-ID: <20180718174505.GA3084@sigill.intra.peff.net>
In-Reply-To: <CACsJy8DW8=FoRyEwAy48S76q0gxQbrS3emHou7QDhHqzwJRu+g@mail.gmail.com>

On Wed, Jul 18, 2018 at 07:31:40PM +0200, Duy Nguyen wrote:

Show 19 quoted lines

> > Sort of an aside to the patch under discussion, but I think it may make
> > sense for prune_shallow() to take a callback function for determining
> > whether a commit is reachable.
> >
> > I have an old patch that teaches git-prune to lazily do the reachability
> > check, since in many cases "git repack" will have just packed all of the
> > loose objects. But it just occurred to me that this patch is totally
> > broken with respect to prune_shallow(), because it would not set the
> > SEEN flag (I've literally been running with it for years, which goes to
> > show how often I use the shallow feature).
> >
> > And if we were to have repack do a prune_shallow(), it may want to use a
> > different method than traversing and marking each object SEEN.
> 
> All of this sounds good. The only thing I'd like to do a bit
> differently is to pass the reachable commits in prune_shallow() as a
> commit-slab instead of taking a callback function. I'll refactor this
> code, move prune_shallow() to a separate command prune-shallow and do
> the locking thing.

I think using a slab is much nicer than the current global-struct flags. But it's not as flexible as a callback, for two reasons:

  - in the lazy case, the caller might not even have loaded the slab
    yet. On the other hand, it might be sufficient to just be broad
    there, and just always pre-populate the slab when
    is_repository_shallow(), before we even call into prune_shallow().
    If we have _any_ entries in the shallow file, we'd need to compute
    reachability.

  - it precludes any optimizations that compute partial reachability.
    Asking "is XYZ reachable" is a much easier question to answer than
    "show me the full reachability graph." At the least, it lets you
    stop the graph traversal early. And with generation numbers, it can
    even avoid traversing down unproductive segments of the graph.

I think it's OK to switch to a slab for now if you don't want to do the callback thing. But I think a callback is probably long-term the right thing (and I actually think it may be _less_ work to implement, since then prune would probably be OK sticking with the global struct flags).

-Peff