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, 17:16 UTC
Message-ID: <xmqqzionfafj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160808165127.fvhnkcfsj4vif7iu@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 12 quoted lines

>> 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.
>
> The dedicated cycle-detection will lose that warning, too (it doesn't
> touch that code, but it's effectively checking the same thing earlier).
>
> I agree it's unfortunate to lose. On the other hand, it is being lost
> because we are correctly handling the cycles, and there is nothing to
> warn about. So it ceases to be a problem, and starts being a normal,
> acceptable thing.

That unfortunately is beside the point. The existing cycle breaking was meant to be a last ditch effort to avoid producing a broken pack (i.e. a suboptimal pack without cycle is better than unusable pack with delta cycle), while letting us know that we found a case where the remainder of the pack building machinery does not function well without it (so that we know we broke something when we tweaked the machinery without intending to break it). Squelching the warnings feels similar to "we see too many valgrind warnings, so let's stop running valgrind"; I was hoping there would be a solution more like "instead of not running, let's teach valgrind this and that codepath is OK".