Re: [PATCH v3 1/2] pack-objects: break delta cycles before delta-search phase

From: Jeff King <peff@peff.net>
Date: Aug 11, 2016, 06:57 UTC
Message-ID: <20160811065751.p64bi3sngbeotwc3@sigill.intra.peff.net>
In-Reply-To: <20160811050252.g3iusy7bp3j6tzte@sigill.intra.peff.net>

On Thu, Aug 11, 2016 at 01:02:52AM -0400, Jeff King wrote:

Show 11 quoted lines

> > > + *   2. Updating our size; check_object() will have filled in the size of our
> > > + *      delta, but a non-delta object needs it true size.
> > 
> > Excellent point.
> 
> I was not clever enough to think of it; the pack-objects code is filled
> with nice assertions (Thanks, Nico!) that help out when you are stupid. :)
> 
> One thing to be careful of is that there are more things this
> drop_reused_delta() should be doing. But I looked through the rest of
> check_object() and could not find anything else.

Argh, I spoke too soon.

It is true that the size lookup is the only part of check_object() we skip if we are reusing the delta. But what I didn't notice is that when we choose to reuse a delta, we overwrite entry->type (the real type!) with the in_pack_type (OFS_DELTA, etc). We need to undo that so that later stages see the real type.

I'm not sure how my existing tests worked (I confirmed that they do indeed break the delta). It may be that only some code paths actually care about the real type. But when playing with the depth-limit (which uses the same drop_reused_delta() helper), I managed to make some pretty broken packs.

So please disregard the v4 patch I just sent; I haven't triggered it, but I imagine it has the same problem, and I just didn't manage to trigger it.

I'll clean that up and send out a new series.

-Peff