Re: [PATCH v2 7/7] pack-objects: use mru list when iterating over packs

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 8, 2016, 16:28 UTC
Message-ID: <xmqq8tw7gr82.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160808145042.uwrk2m6jq3m4li37@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 21 quoted lines

> Here's a list of approaches I think we can use to fix this:
>
>   1. squelch the warning and ignore it. The downside here, besides not
>      warning the user about true in-pack cycles, is that we have no
>      opportunity to actually find a new delta (because we realize the
>      problem only after the delta-compression phase).
>
>      My test repository is a bad packing of all of the forks of
>      torvalds/linux, with 3600 packs. I'm happy to share it if anybody
>      wants to see it, but note that it is 11GB.
>
>      The space overhead of the resulting pack in this case is ~3.2%
>      (versus a pack generated by the original code, using the static
>      pack order).  Which is really not that bad, but I'm sure there are
>      more pathological cases (i.e., there were on the order of hundreds
>      or maybe thousands of cycles that needed broken, out of about 15
>      million total objects; but one could imagine the worst case as
>      nr_of_objects/2).
> ...
>
>     So I dunno. I really like the MRU approach if we can salvage it.

I think I share the same feeling. As long as the chance is small enough that the pack reordering creates a new cycle, the resulting pack would not become too bloated by the last-ditch cycle breaking code and finding a replacement delta instead of inflating it may not be worth the trouble.

It worries me a lot to lose the warning unconditionally, though. That's the (only) coal-mine canary that lets us notice a problem when we actually start hitting that last-ditch cycle breaking too often.