Re: [RFC PATCH] index-pack: optionally allow duplicate objects

[Taylor Blau <ttaylorr@openai.com>]

On Tue, Jul 28, 2026 at 06:41:34PM -0700, Junio C Hamano wrote:
> friel@openai.com writes:
>
> > Signed-off-by: Friel <friel@openai.com>
> > ---
> > Applies on top of tb/pack-with-duplicates.
>
> I am really reluctant to take us in this direction.  The last time I
> had a deep discussion on this was with Shawn Pearce (so those who
> knew him can tell how long ago that was), and the essence of his
> suggestion was that allowing malformed or invalid packfiles is a
> slippery slope.  They complicate everything, from delta cycle
> detection to ensuring that repository data stays healthy.

I share the concern, but I am not sure to what extent historical
discussions should decide our stance on whether or not we want to
provide support for packs containing duplicate objects.

A lot of this discussion pre-dates my involvement with the project, but
the closest thread I could find on the topic with Shawn was from back in
[1]. AFAICT the main concern at the time was that binary searching the
pack would not handle duplicates well.

Your comment here around delta cycle detection is something that came up
in the series I wrote (upon which this patch is based) trying to harden
Git's handling of packs containing duplicate objects. The main thing I
found when writing that series that Git did *not* already handle well
was navigating cycles during delta resolution, in particular with
REF_DELTAs. One of the patches in that series teaches Git how to handle
this case in a way that (a) doesn't adversely impact the performance of
delta resolution in packs that do *not* contain duplicates, and (b) is
not especially complex.

My sense is that many of the things the project was concerned with at
the time have since been hardened, and that Git is more well-equipped to
deal with duplicate object-containing packs than we may give it credit
for.

A couple of thoughts on why I think this direction is useful:

 - I think that Peff makes a good point in [2], which effectively boils
   down to, "OK, maybe these packs are buggy, but if they contain the
   sole copy of an object we care about, we must be able to read them."
   As I understand it, that is effectively why we disambiguate between
   "index-pack" and "index-pack --strict".

   I think a version of this patch that carves out duplicates as OK even
   in "--strict" mode results in better behavior for packs that contain
   duplicate objects (whereas before we had to drop all of the
   additional checks that "--strict" requires in order to index such a
   pack).

 - There are genuinely useful scenarios by which a sender may wish to
   consolidate two or more packs together by appending (as opposed to
   repacking) them, in a way that is extremely cheap to do.

   Friel may have a better example here, but a useful mental model for
   me has been: if an upload-pack implementation knows that it can serve
   a request by sending the objects from some known subset of all packs,
   it may make sense to simply combine those packs by appending them
   rather than generating a new pack with the union of their objects.

   That trade-off is useful IMHO for serving fetches and clones for very
   large and fast-moving repositories where repacking may not always be
   able to keep up.

I am not proposing that we make packs containing duplicate objects the
norm. But mine and Friel's approach here is to first demonstrate (via
'tb/pack-with-duplicates') that Git has good handling for packs
containing duplicate objects, and subsequently (via this patch) to make
it possible to index such packs.

If we can find useful ways to combine the ideas above with Git's in-tree
implementation of upload-pack, one could imagine that Git itself may
eventually send packs containing duplicate copies of some object(s)
behind a capability. In other words, for clients that know how to
process such a pack, the server may wish to ask the client to do just
that in the name of saving some CPU cycles necessary to generate a pack
that doesn't have any duplicate objects.

> Changes that help us detect such a broken pack as early as possible
> and prevent it from entering your repository are very much welcome.
> Changes that accept such a broken pack as if nothing were wrong, not
> so much.

I agree that the RFC as written makes the exception look broader than it
should.

A reroll should make `--allow-duplicate-objects` carve out only the
duplicate-OID part of `--strict`, rather than make the two options
incompatible. `index-pack` should still recompute every object ID,
resolve every delta, perform the usual object and link checks, and check
connectivity when requested. Only `WRITE_IDX_STRICT`'s requirement that
each OID have one physical entry would be relaxed. The default should,
IMHO, remain that we reject such packs.

But I would note that having packs containing duplicate objects is not a
new repository state for Git. Non-strict `index-pack` accepts duplicate
entries today, and shallow and filtered clones can store the same pack.
My series in 'tb/pack-with-duplicates' attempts to fix the known-broken
assumptions in reverse indexes, delta resolution, MIDX verification, and
bitmap reuse because those packs can already exist.

If duplicate entries are to be forbidden entirely in order for a pack to
be considered valid, then I think we should reject them in every
`index-pack` mode, including the shallow and filtered clone paths, and
diagnose existing packs in `fsck`. Otherwise, a default-off opt-in for a
known producer is a cleaner way IMHO to express that duplicate objects
are OK, rather than relying on whether or not we performed a
shallow/partial clone (and thus did not invoke index-pack with
"--strict").

I think that narrower version is worth pursuing.

Thanks,
Taylor

[1]: https://lore.kernel.org/git/CAJo=hJs3mM7=LcOop-WD=bipA=Wx-7MDh6ObQwFUE38tjurvcw@mail.gmail.com/
[2]: https://lore.kernel.org/git/20140830131649.GA26833@peff.net/