Re: [PATCH 6/6] pack-objects: reuse on-disk deltas for thin "have" objects

[Jeff King <peff@peff.net>]

On Mon, Aug 20, 2018 at 02:03:53PM -0700, Junio C Hamano wrote:

> > So taking all of those options into account, what I ended up
> > with is a separate list of "external bases" that are not
> > part of the main packing list. Each delta entry that points
> > to an external base has a single-bit flag to do so; we have a
> > little breathing room in the bitfield section of
> > object_entry.
> >
> > This lets us limit the change primarily to the oe_delta()
> > and oe_set_delta_ext() functions. And as a bonus, most of
> > the rest of the code does not consider these dummy entries
> > at all, saving both runtime CPU and code complexity.
> 
> Tricky ;-)
> 
> I wonder if we can move the preferred base objects that we are not
> going to send also off of the "main packing list" to this new
> mechanism?

That gets trickier. Obviously we don't need to actually send them, so
they're ignored during the write phase. But we do put them into the
delta-search window along with the other objects, and we care about
things like their size and type (which our dummy objects do not even
have; they really are just placeholders for the oids).

So I'd guess that we'd end up with three arrays of entries:

 - objects we want to send, with full details including delta bases

 - preferred base objects with _some_ details

 - dummy thin objects for reused deltas

The trick is that I don't know which details fall under the "some" in
the second class. That could be extracted from a careful reading of the
code, but it seems like it has a high chance of regression (and I'm not
sure there's a huge upside, given that the code is already written
as-is).

I also worried about the same thing with these new reused dummy objects.
but I think by now I'd have shaken out any problems in production use.

> > +static struct bitmap_index *bitmap_git;
> > ...
> > +static int thin = 0;
> 
> Please trust what BSS will do to your static vars.

Heh. I copied the non-static declaration, but didn't think about
shortening it. Looks like there were one or two minor comments, so I'll
do a re-roll that addresses them.

-Peff