On Thu, Aug 11, 2016 at 02:57:51AM -0400, Jeff King wrote: > > 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. Here it is. It ended up needing a few preparatory patches. [1/4]: provide an initializer for "struct object_info" [2/4]: sha1_file: make packed_object_info public [3/4]: pack-objects: break delta cycles before delta-search phase [4/4]: pack-objects: use mru list when iterating over packs I had originally intended to include an extra patch to handle the --depth limits better. But after writing it, I'm not sure it's actually a good idea. I'll post it as an addendum with more discussion. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html