Re: [PATCH 2/2] index-pack: prefetch missing REF_DELTA bases

[Jeff King <peff@peff.net>]

On Thu, May 16, 2019 at 11:26:46AM -0700, Jonathan Tan wrote:

> > Pretty unlikely, but should we put some kind of circuit-breaker into the
> > client to ensure this?
> 
> I thought of this - such a server could, but it seems to me that it
> would be similar to a server streaming random bytes to us without
> stopping (which is already possible).

True. I was thinking mainly of the infinite-redirection protections we
put in place for https. But I agree that in general, since we don't have
inherent limits on the size of workloads, that servers can already troll
clients pretty hard in a variety of ways.

So I could go either way, though I do think it makes sense for on-demand
fetches for partial clones to avoid asking for thin packs as a general
principle. As a matter of fact, should partial clones _always_ avoid
asking for thin packs?  That would make this issue go away entirely.

Sometimes it would be more efficient (we do not have to get an extra
base object just to resolve the delta we needed) but sometimes worse (if
we did actually have the base, it's a win). Whether it's a win would
depend on the "hit" rate, and I suspect that is heavily dependent on
workload characteristics (what kind of filtering is in use, are we
topping up in a non-partial way, etc).

> > Off the top of my head, I am pretty sure your assumption holds for all
> > versions of Git that support delta-base-offset[1]. But that feels a lot
> > less certain to me. I could imagine an alternate server implementation,
> > for example, that is gluing together packs and does not try hard to
> > order the base before the delta, which would require it to use REF_DELTA
> > instead of OFS_DELTA.
> 
> A cursory glance makes me think that REF_DELTA against a base object
> also in the pack is already correctly handled. Right before the
> invocation of conclude_pack() (which calls fix_unresolved_deltas(), the
> function I modified), resolve_deltas() is invoked. The latter invokes
> resolve_base() (directly or through threaded_second_pass()) which
> invokes find_unresolved_deltas(), which invokes
> find_unresolved_deltas_1(), which seems to handle both REF_DELTA and
> OFS_DELTA.
> 
> Snipping the rest as I don't think we need to solve those if we can
> handle REF_DELTA being against an object in a pack, but let me know if
> you think that some of the points there still need to be addressed.

Right, REF_DELTA is definitely correctly handled currently, and I don't
think that would break with your patch. It's just that your patch would
introduce a bunch of extra traffic as we request bases separately that
are already in the pack.

-Peff