Re: [DO NOT APPLY PATCH 4/4] index-pack: optionally skip duplicate packfile entries

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 21, 2013, 23:20 UTC
Message-ID: <xmqqob8qskbc.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20130821205555.GD28165@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 10 quoted lines

> When we are building the pack index, we can notice that
> there are duplicate objects, pick one "winner" instance, and
> mention the object only once in the index (mapped to the
> winner's offset).
>
> This has the effect that the duplicate packfile entries are
> never found by lookup. The data still exists in the
> packfile, though, and can be used as a delta base if delta
> base offsets are used. If delta refs are used, then it is
> possible that some deltas may be broken.

I do not understand the last bit. If two copies of an object exist but you have only one slot for the object in the index, and another object names it as its base with ref-delta, then reconstituting it should work just fine---whichever representation of the base object is recorded in the .idx, that first needs to be reconstituted before the delta is applied to it, and both copies should yield identical contents for the delta base object, no?

In any case, ejecting one from the pack .idx would not help in the presense of either broken or malicious packer that reuses delta too aggressively. Suppose you have objects A and B and somehow manage to create a cycle of deltas, A names B as its delta base and B names A as its delta base. The packer may notice its mistake and then add another copy of A as a base object. The pack contains two copies of A (one is delta based on B, the other is full) and B (delta against A).

If B refers to the copy of A that is delta against B using ofs-delta, fixing the .idx file will have no effect. read_packed_sha1(B) will read the delta data of B, finds the offset to start reading the data for A which was excised from the .idx and unless that codepath is updated to consult revindex (i.e. you mark one copy of A in the .pack as bad, and when B refers to that bad copy of A via ofs-delta, you check the offset against revindex to get the object name of A and go to the good copy of A), you will never finish reading B because reading the bad copy of A will lead you to first reconstitute B.

> I think this line of "fixing" should probably be scrapped.

I tend to agree.