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

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

[friel]

From: Friel <friel@openai.com>

index-pack accepts repeated object IDs by default. However,
--check-self-contained-and-connected also enables strict index
validation, which rejects them. A clone therefore rejects a pack
containing duplicate objects even when all of its objects are connected.

By default, shallow and filtered clones do not request this pack-local
check and already accept duplicate objects. Ordinary full clones reject
the same pack because they enable strict index validation.

Git's upload-pack normally uses pack-objects to select each reachable
object once before writing a response. A server can instead construct
that response by streaming entries from existing packs. When those
packs overlap, the same object can appear more than once.

Avoiding duplicates requires the producers to coordinate their object
selection or track object IDs across all input packs. A duplicate can
also be used as a delta base, so removing it can require buffering and
rewriting the response. Doing that work at request time gives up the
memory and latency benefits of streaming existing packs.

Add pack.allowDuplicateObjects so a client can accept these packs during
its initial fetch:

    git clone -c pack.allowDuplicateObjects <repository>

Add --[no-]allow-duplicate-objects to override the configuration for
index-pack. Preserve the default rejection introduced by 68be2fea50
(receive-pack, fetch-pack: reject bogus pack that records objects twice,
2011-11-16). An explicit --strict or --verify remains strict regardless
of the configuration and cannot be combined with
--allow-duplicate-objects. Object validation, connectivity checks, and
delta resolution remain unchanged.

Signed-off-by: Friel <friel@openai.com>
---
Applies on top of tb/pack-with-duplicates.

 Documentation/config/pack.adoc    |  11 ++
 Documentation/git-index-pack.adoc |  12 ++-
 builtin/index-pack.c              |  41 ++++++-
 t/t5308-pack-detect-duplicates.sh | 171 ++++++++++++++++++++++++++++++
 t/t5309-pack-delta-cycles.sh      |  10 ++
 5 files changed, 241 insertions(+), 4 deletions(-)

diff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc
index 22384c2d2f..2229878abe 100644
--- a/Documentation/config/pack.adoc
+++ b/Documentation/config/pack.adoc
@@ -39,6 +39,17 @@ is set to "multi", reuse parts of just the bitmapped packfile. This
 can reduce memory and CPU usage to serve fetches, but might result in
 sending a slightly larger pack. Defaults to true.
 
+pack.allowDuplicateObjects::
+	Allow linkgit:git-index-pack[1] to accept a pack containing
+	multiple copies of the same object while checking that the pack
+	is self-contained and connected. For example,
+	`git clone -c pack.allowDuplicateObjects <repository>` can accept
+	a pack generated from overlapping existing packs. Object and
+	connectivity checks are preserved. Explicit `--strict` and
+	`--verify` continue to reject duplicate objects.
+	`--no-allow-duplicate-objects` overrides this setting.
+	Defaults to `false`.
+
 pack.island::
 	An extended regular expression configuring a set of delta
 	islands. See "DELTA ISLANDS" in linkgit:git-pack-objects[1]
diff --git a/Documentation/git-index-pack.adoc b/Documentation/git-index-pack.adoc
index 18036953c0..1cb11ff898 100644
--- a/Documentation/git-index-pack.adoc
+++ b/Documentation/git-index-pack.adoc
@@ -11,7 +11,9 @@ SYNOPSIS
 [verse]
 'git index-pack' [-v] [-o <index-file>] [--[no-]rev-index] <pack-file>
 'git index-pack' --stdin [--fix-thin] [--keep] [-v] [-o <index-file>]
-		  [--[no-]rev-index] [<pack-file>]
+		  [--[no-]rev-index]
+		  [--[no-]allow-duplicate-objects]
+		  [<pack-file>]
 
 
 DESCRIPTION
@@ -97,6 +99,14 @@ default and "Indexing objects" when `--stdin` is specified.
 --check-self-contained-and-connected::
 	Die if the pack contains broken links. For internal use only.
 
+--allow-duplicate-objects::
+--no-allow-duplicate-objects::
+	Allow or reject multiple copies of the same object while checking
+	that the pack is self-contained and connected. The default is
+	controlled by `pack.allowDuplicateObjects`. The command-line
+	option overrides the configuration. `--allow-duplicate-objects`
+	cannot be combined with `--strict` or `--verify`.
+
 --fsck-objects[=<msg-id>=<severity>...]::
 	Die if the pack contains broken objects, but unlike `--strict`, don't
 	choke on broken links. If the pack contains a tree pointing to a
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index bc86925ad0..4ffcbb6c28 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -33,7 +33,7 @@
 #include "strvec.h"
 
 static const char index_pack_usage[] =
-"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-id>=<severity>...]] [--fsck-objects[=<msg-id>=<severity>...]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
+"git index-pack [-v] [-o <index-file>] [--keep | --keep=<msg>] [--[no-]rev-index] [--verify] [--strict[=<msg-id>=<severity>...]] [--[no-]allow-duplicate-objects] [--fsck-objects[=<msg-id>=<severity>...]] (<pack-file> | --stdin [--fix-thin] [<pack-file>])";
 
 struct object_entry {
 	struct pack_idx_entry idx;
@@ -135,6 +135,11 @@ static int nr_threads;
 
 static int from_stdin;
 static int strict;
+static enum {
+	DUPLICATE_OBJECTS_REJECT = 0,
+	DUPLICATE_OBJECTS_ALLOW_CONFIG,
+	DUPLICATE_OBJECTS_ALLOW_OPTION,
+} allow_duplicate_objects;
 static int do_fsck_object;
 static struct fsck_options fsck_options;
 static int verbose;
@@ -1673,6 +1678,12 @@ static int git_index_pack_config(const char *k, const char *v,
 			die(_("bad pack.indexVersion=%"PRIu32), opts->version);
 		return 0;
 	}
+	if (!strcmp(k, "pack.allowduplicateobjects")) {
+		allow_duplicate_objects = git_config_bool(k, v) ?
+			DUPLICATE_OBJECTS_ALLOW_CONFIG :
+			DUPLICATE_OBJECTS_REJECT;
+		return 0;
+	}
 	if (!strcmp(k, "pack.threads")) {
 		nr_threads = git_config_int(k, v, ctx->kvi);
 		if (nr_threads < 0)
@@ -1889,6 +1900,7 @@ int cmd_index_pack(int argc,
 		   struct repository *repo UNUSED)
 {
 	int i, fix_thin_pack = 0, verify = 0, stat_only = 0, rev_index;
+	int write_idx_strict;
 	const char *curr_index;
 	char *curr_rev_index = NULL;
 	const char *index_name = NULL, *pack_name = NULL, *rev_index_name = NULL;
@@ -1941,8 +1953,11 @@ int cmd_index_pack(int argc,
 				strict = 1;
 				do_fsck_object = 1;
 				fsck_set_msg_types(&fsck_options, arg);
+			} else if (!strcmp(arg, "--allow-duplicate-objects")) {
+				allow_duplicate_objects = DUPLICATE_OBJECTS_ALLOW_OPTION;
+			} else if (!strcmp(arg, "--no-allow-duplicate-objects")) {
+				allow_duplicate_objects = DUPLICATE_OBJECTS_REJECT;
 			} else if (!strcmp(arg, "--check-self-contained-and-connected")) {
-				strict = 1;
 				check_self_contained_and_connected = 1;
 			} else if (skip_to_optional_arg(arg, "--fsck-objects", &arg)) {
 				do_fsck_object = 1;
@@ -2022,6 +2037,26 @@ int cmd_index_pack(int argc,
 		usage(index_pack_usage);
 	if (fix_thin_pack && !from_stdin)
 		die(_("the option '%s' requires '%s'"), "--fix-thin", "--stdin");
+
+	/*
+	 * Connectivity checks require strict object traversal, but may allow
+	 * duplicate entries in the pack index.
+	 */
+	write_idx_strict = strict;
+	if (check_self_contained_and_connected) {
+		strict = 1;
+		if (allow_duplicate_objects == DUPLICATE_OBJECTS_REJECT)
+			write_idx_strict = 1;
+	}
+
+	if (write_idx_strict &&
+	    allow_duplicate_objects == DUPLICATE_OBJECTS_ALLOW_OPTION)
+		die(_("options '%s' and '%s' cannot be used together"),
+		    "--allow-duplicate-objects", "--strict");
+	if (verify &&
+	    allow_duplicate_objects == DUPLICATE_OBJECTS_ALLOW_OPTION)
+		die(_("options '%s' and '%s' cannot be used together"),
+		    "--allow-duplicate-objects", "--verify");
 	if (promisor_msg && pack_name)
 		die(_("--promisor cannot be used with a pack name"));
 	if (from_stdin && !startup_info->have_repository)
@@ -2055,7 +2090,7 @@ int cmd_index_pack(int argc,
 		read_idx_option(&opts, index_name);
 		opts.flags |= WRITE_IDX_VERIFY | WRITE_IDX_STRICT;
 	}
-	if (strict)
+	if (write_idx_strict)
 		opts.flags |= WRITE_IDX_STRICT;
 
 	if (HAVE_THREADS && !nr_threads) {
diff --git a/t/t5308-pack-detect-duplicates.sh b/t/t5308-pack-detect-duplicates.sh
index c6273a1aeb..95c81fa7b4 100755
--- a/t/t5308-pack-detect-duplicates.sh
+++ b/t/t5308-pack-detect-duplicates.sh
@@ -139,4 +139,175 @@ test_expect_success 'index-pack can reject packs with duplicates' '
 	test_expect_code 1 git cat-file -e $LO_SHA1
 '
 
+test_expect_success 'connectivity check rejects duplicate objects by default' '
+	clear_packs &&
+	create_pack dups.pack 2 &&
+	test_must_fail git index-pack \
+		--check-self-contained-and-connected --stdin <dups.pack &&
+	test_expect_code 1 git cat-file -e $LO_SHA1
+'
+
+test_expect_success 'connectivity check can allow duplicate objects' '
+	clear_packs &&
+	create_pack dups.pack 2 &&
+	git index-pack --check-self-contained-and-connected \
+		--allow-duplicate-objects --stdin <dups.pack &&
+	git cat-file -e "$LO_SHA1" &&
+	git cat-file -e "$HI_SHA1"
+'
+
+test_expect_success 'configuration allows duplicates with connectivity checks' '
+	clear_packs &&
+	create_pack dups.pack 2 &&
+	git -c pack.allowDuplicateObjects index-pack \
+		--check-self-contained-and-connected --stdin <dups.pack &&
+	git cat-file -e "$LO_SHA1" &&
+	git cat-file -e "$HI_SHA1"
+'
+
+test_expect_success 'command-line allowance overrides false configuration' '
+	clear_packs &&
+	create_pack dups.pack 2 &&
+	git -c pack.allowDuplicateObjects=false index-pack \
+		--check-self-contained-and-connected \
+		--allow-duplicate-objects --stdin <dups.pack &&
+	git cat-file -e "$LO_SHA1" &&
+	git cat-file -e "$HI_SHA1"
+'
+
+test_expect_success 'command-line rejection overrides true configuration' '
+	clear_packs &&
+	create_pack dups.pack 2 &&
+	test_must_fail git -c pack.allowDuplicateObjects=true index-pack \
+		--check-self-contained-and-connected \
+		--no-allow-duplicate-objects --stdin <dups.pack 2>err &&
+	test_grep "appears twice in the pack" err
+'
+
+test_expect_success 'configured allowance does not relax explicit strict mode' '
+	clear_packs &&
+	create_pack dups.pack 2 &&
+	test_must_fail git -c pack.allowDuplicateObjects=true index-pack \
+		--strict --stdin <dups.pack 2>err &&
+	test_grep "appears twice in the pack" err
+'
+
+test_expect_success 'explicit strict mode cannot allow duplicate objects' '
+	clear_packs &&
+	create_pack dups.pack 2 &&
+	test_must_fail git index-pack --strict --allow-duplicate-objects \
+		--stdin <dups.pack 2>err &&
+	test_grep "cannot be used together" err
+'
+
+test_expect_success 'configured allowance does not relax verification' '
+	clear_packs &&
+	create_pack dups.pack 2 &&
+	git index-pack --check-self-contained-and-connected \
+		--allow-duplicate-objects --stdin <dups.pack &&
+	test_must_fail git -c pack.allowDuplicateObjects=true index-pack \
+		--verify .git/objects/pack/pack-*.pack 2>err &&
+	test_grep "appears twice in the pack" err
+'
+
+test_expect_success 'verify cannot allow duplicate objects' '
+	test_must_fail git index-pack --verify --allow-duplicate-objects \
+		.git/objects/pack/pack-*.pack 2>err &&
+	test_grep "cannot be used together" err
+'
+
+test_expect_success 'configured allowance preserves connectivity checks' '
+	clear_packs &&
+	tree=$(git mktree </dev/null) &&
+	commit=$(echo message | git commit-tree "$tree") &&
+	{
+		pack_header 2 &&
+		pack_obj $commit &&
+		pack_obj $commit
+	} >not-self-contained.pack &&
+	pack_trailer not-self-contained.pack &&
+	rm .git/objects/$(test_oid_to_path $commit) &&
+	test_expect_code 1 git -c pack.allowDuplicateObjects index-pack \
+		--check-self-contained-and-connected \
+		--stdin <not-self-contained.pack &&
+	test "$(git cat-file -t $commit)" = commit
+'
+
+test_expect_success 'allowing duplicates preserves object checks' '
+	clear_packs &&
+	tree=$(git mktree </dev/null) &&
+	cat >bad-commit <<-EOF &&
+	tree $tree
+	author A U Thor 1234567890 +0000
+	committer C O Mitter <committer@example.com> 1234567890 +0000
+
+	message
+	EOF
+	commit=$(git hash-object --literally -t commit -w --stdin \
+		<bad-commit) &&
+	{
+		pack_header 2 &&
+		pack_obj $commit &&
+		pack_obj $commit
+	} >bad-objects.pack &&
+	pack_trailer bad-objects.pack &&
+	rm .git/objects/$(test_oid_to_path $commit) &&
+	test_must_fail git index-pack \
+		--check-self-contained-and-connected --fsck-objects \
+		--allow-duplicate-objects --stdin <bad-objects.pack 2>err &&
+	test_grep "missingEmail" err
+'
+
+test_expect_success 'set up upload-pack that serves duplicate objects' '
+	test_commit clone-duplicates &&
+	commit=$(git rev-parse HEAD) &&
+	tree=$(git rev-parse HEAD^{tree}) &&
+	blob=$(git rev-parse HEAD:clone-duplicates.t) &&
+	{
+		pack_header 4 &&
+		pack_obj "$commit" &&
+		pack_obj "$tree" &&
+		pack_obj "$blob" &&
+		pack_obj "$blob"
+	} >clone-duplicates.pack &&
+	pack_trailer clone-duplicates.pack &&
+	write_script .git/duplicate-pack-hook <<-EOF
+	cat >/dev/null &&
+	cat "$TRASH_DIRECTORY/clone-duplicates.pack"
+	EOF
+'
+
+test_expect_success 'clone rejects duplicate objects by default' '
+	test_must_fail git clone --no-local \
+		-u "git -c uploadpack.packObjectsHook=./duplicate-pack-hook upload-pack" \
+		. clone-reject 2>err &&
+	test_grep "appears twice in the pack" err
+'
+
+test_expect_success 'shallow clone already accepts duplicate objects' '
+	git clone --no-local --depth=1 \
+		-u "git -c uploadpack.packObjectsHook=./duplicate-pack-hook upload-pack" \
+		. clone-shallow &&
+	test "$(git -C clone-shallow rev-parse HEAD)" = "$commit" &&
+	git -C clone-shallow fsck --full
+'
+
+test_expect_success 'filtered clone already accepts duplicate objects' '
+	git clone --no-local --filter=blob:none \
+		-u "git -c uploadpack.allowFilter=true -c uploadpack.packObjectsHook=./duplicate-pack-hook upload-pack" \
+		. clone-filter &&
+	test "$(git -C clone-filter config remote.origin.partialclonefilter)" = blob:none &&
+	test "$(git -C clone-filter rev-parse HEAD)" = "$commit" &&
+	git -C clone-filter fsck --full
+'
+
+test_expect_success 'clone -c pack.allowDuplicateObjects accepts duplicate objects' '
+	git clone --no-local -c pack.allowDuplicateObjects \
+		-u "git -c uploadpack.packObjectsHook=./duplicate-pack-hook upload-pack" \
+		. clone-allow &&
+	test "$(git -C clone-allow config --bool pack.allowDuplicateObjects)" = true &&
+	test "$(git -C clone-allow rev-parse HEAD)" = "$commit" &&
+	git -C clone-allow fsck --full
+'
+
 test_done
diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh
index f613950e38..fb5ed81ca3 100755
--- a/t/t5309-pack-delta-cycles.sh
+++ b/t/t5309-pack-delta-cycles.sh
@@ -217,6 +217,16 @@ test_expect_success 'failover after a tail into a three-object delta cycle' '
 	check_blob "$T" tail
 '
 
+test_expect_success 'configured connectivity check recovers duplicate delta bases' '
+	clear_packs &&
+	git -c pack.allowDuplicateObjects index-pack --fix-thin \
+		--check-self-contained-and-connected \
+		--stdin <recoverable-1.pack &&
+	printf "\7\0" >expect.duplicate-base &&
+	check_blob "$A" expect.duplicate-base &&
+	git cat-file -e "$B"
+'
+
 test_expect_success 'index-pack works with thin pack A->B->C with B on disk' '
 	git init server &&
 	(

base-commit: fd2739b159d075cd4c6fa69b2cd876ba1caa5c88

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

[Taylor Blau]

On Mon, Jul 27, 2026 at 09:25:32PM -0700, friel@openai.com wrote:
> Git's upload-pack normally uses pack-objects to select each reachable
> object once before writing a response. A server can instead construct
> that response by streaming entries from existing packs. When those
> packs overlap, the same object can appear more than once.
>
> Avoiding duplicates requires the producers to coordinate their object
> selection or track object IDs across all input packs. A duplicate can
> also be used as a delta base, so removing it can require buffering and
> rewriting the response. Doing that work at request time gives up the
> memory and latency benefits of streaming existing packs.

Right. An out-of-tree implementation of upload-pack may choose to stitch
multiple individual packs together by concatenating them, trading some
pack generation time for a pack which may contain duplicate objects.

While this series is primarily motivated by that use-case, I suspect
that there are optimizations we could make within Git's implementation
of upload-pack that would take advantage of environments where clients
are prepared to accept packs that contain duplicate objects.

That's not a goal of this patch, of course, but something to keep in
mind as others review this.

> Applies on top of tb/pack-with-duplicates.
>
>  Documentation/config/pack.adoc    |  11 ++
>  Documentation/git-index-pack.adoc |  12 ++-
>  builtin/index-pack.c              |  41 ++++++-
>  t/t5308-pack-detect-duplicates.sh | 171 ++++++++++++++++++++++++++++++
>  t/t5309-pack-delta-cycles.sh      |  10 ++
>  5 files changed, 241 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/config/pack.adoc b/Documentation/config/pack.adoc
> index 22384c2d2f..2229878abe 100644
> --- a/Documentation/config/pack.adoc
> +++ b/Documentation/config/pack.adoc
> @@ -39,6 +39,17 @@ is set to "multi", reuse parts of just the bitmapped packfile. This
>  can reduce memory and CPU usage to serve fetches, but might result in
>  sending a slightly larger pack. Defaults to true.
>
> +pack.allowDuplicateObjects::
> +	Allow linkgit:git-index-pack[1] to accept a pack containing
> +	multiple copies of the same object while checking that the pack
> +	is self-contained and connected. For example,
> +	`git clone -c pack.allowDuplicateObjects <repository>` can accept
> +	a pack generated from overlapping existing packs. Object and
> +	connectivity checks are preserved. Explicit `--strict` and
> +	`--verify` continue to reject duplicate objects.
> +	`--no-allow-duplicate-objects` overrides this setting.
> +	Defaults to `false`.
> +

A couple of brief thoughts here:

 - Is "while checking that the pack is self-contained and connected"
   true in all cases? Certainly if we give the option
   '--check-self-contained-any-connected' to 'index-pack'. But if
   a user invokes "git -c pack.allowDuplicateObjects index-pack ...",
   we will not bother to perform the same checks.

 - The "For example [...]" may be unnecessary here. I don't have a
   strong feeling here either way, but it feels somewhat specific to
   'git-clone(1)' so perhaps belongs there instead?

 - "Explicit `--strict` and `--verify` [...]" and the following
   sentence. I think that this means to suggest that `--strict` and
   `--verify` both continue to behave as-is, but setting this
   configuration option allows them to conditionally accept
   otherwise-good packs that happen to contain duplicate objects.

   I wonder if these couple of sentences may be combined like: "When
   `true`, linkgit:git-index-pack[1] will accept otherwise-valid packs
   containing duplicate objects under `--strict` or `--verify`." But
   reading further, I don't think that that's actually what this option
   does. More below.

>  pack.island::
>  	An extended regular expression configuring a set of delta
>  	islands. See "DELTA ISLANDS" in linkgit:git-pack-objects[1]
> diff --git a/Documentation/git-index-pack.adoc b/Documentation/git-index-pack.adoc
> index 18036953c0..1cb11ff898 100644
> --- a/Documentation/git-index-pack.adoc
> +++ b/Documentation/git-index-pack.adoc
> @@ -11,7 +11,9 @@ SYNOPSIS
>  [verse]
>  'git index-pack' [-v] [-o <index-file>] [--[no-]rev-index] <pack-file>
>  'git index-pack' --stdin [--fix-thin] [--keep] [-v] [-o <index-file>]
> -		  [--[no-]rev-index] [<pack-file>]
> +		  [--[no-]rev-index]
> +		  [--[no-]allow-duplicate-objects]
> +		  [<pack-file>]

Not the fault of this patch, but the synopsis and usage string
(`index_pack_usage`) do not agree, hence the 'index-pack' entry in
t/t0450/adoc-help-mismatches. So putting this on a new line is OK, but I
think it's fine to keep this and "[<pack-file>]" on the same line as
"[--[no-]rev-index]" in the pre-image of this patch.

>  DESCRIPTION
> @@ -97,6 +99,14 @@ default and "Indexing objects" when `--stdin` is specified.
>  --check-self-contained-and-connected::
>  	Die if the pack contains broken links. For internal use only.
>
> +--allow-duplicate-objects::
> +--no-allow-duplicate-objects::
> +	Allow or reject multiple copies of the same object while checking
> +	that the pack is self-contained and connected. The default is
> +	controlled by `pack.allowDuplicateObjects`. The command-line
> +	option overrides the configuration. `--allow-duplicate-objects`
> +	cannot be combined with `--strict` or `--verify`.

Hmm. This suggests something other than what I gathered when reading the
corresponding git-config(1) entry.

Are there cases where we would want want to allow duplicate object,s but
retain the other "--strict" behavior of dying when the pack contains
broken objects, or links off to objects that we don't have? I would
imagine that 'git clone' would want to do just this. I imagine that such
a use-case would expect that even if we are cloning from a source that
is known to produce packs with duplicate objects we would still want to
verify that none of the objects it references are missing, etc.

I think that suggests something more along the lines of having this
option opt you out of this specific portion of "--strict"'s behavior, as
in "git index-pack --strict --allow-duplicate-objects". I may be missing
something here.

> @@ -135,6 +135,11 @@ static int nr_threads;
>
>  static int from_stdin;
>  static int strict;
> +static enum {
> +	DUPLICATE_OBJECTS_REJECT = 0,
> +	DUPLICATE_OBJECTS_ALLOW_CONFIG,
> +	DUPLICATE_OBJECTS_ALLOW_OPTION,
> +} allow_duplicate_objects;

I was initially a little surprised to see a new enum value here for what
I imagined would be a true/false value. But looking at the diff below, I
think that this is to silently ignore a "true" value for the config
option 'pack.allowDuplicateObjects' in the presence of "--strict".

So I think that this tri-state is fine in that sense. But I imagine that
much of this goes away if we take this option to instead carve out one
specific behavior of --strict instead of being incompatible with it
entirely.

> +	if (write_idx_strict &&
> +	    allow_duplicate_objects == DUPLICATE_OBJECTS_ALLOW_OPTION)
> +		die(_("options '%s' and '%s' cannot be used together"),
> +		    "--allow-duplicate-objects", "--strict");
> +	if (verify &&
> +	    allow_duplicate_objects == DUPLICATE_OBJECTS_ALLOW_OPTION)
> +		die(_("options '%s' and '%s' cannot be used together"),
> +		    "--allow-duplicate-objects", "--verify");

If you end up keeping the existing meaning and need to declare this
incompatible with write_idx_strict and verify, there is a helper for
this case:

    die_for_incompatible_opt2(allow_duplicate_objects == DUPLICATE_OBJECTS_ALLOW_OPTION,
                              "--allow-duplicate-objects",
                              write_idx_strict, "--strict");

    die_for_incompatible_opt2(allow_duplicate_objects == DUPLICATE_OBJECTS_ALLOW_OPTION,
                              "--allow-duplicate-objects",
                              verify, "--verify");

Alternatively, since writing "allow_duplicate_objects == DUPLICATE_OBJECTS_ALLOW_OPTION"
is kind of a mouthful, you could instead write this abomination:

    if (allow_duplicate_objects == DUPLICATE_OBJECTS_ALLOW_OPTION) {
        die_for_incompatible_opt2(1, "--allow-duplicate-objects",
                                  write_idx_strict, "--strict");
        die_for_incompatible_opt2(1, "--allow-duplicate-objects",
                                  verify, "--verify");
    }

;-)

> @@ -2055,7 +2090,7 @@ int cmd_index_pack(int argc,
>  		read_idx_option(&opts, index_name);
>  		opts.flags |= WRITE_IDX_VERIFY | WRITE_IDX_STRICT;
>  	}
> -	if (strict)
> +	if (write_idx_strict)
>  		opts.flags |= WRITE_IDX_STRICT;
>
>  	if (HAVE_THREADS && !nr_threads) {

OK. Since we aren't treating this as a carve-out, we don't have any
further changes in pack-write.c. Makes sense, though I am curious about
your thoughts on whether the alternate interface makes more or less
sense.

> diff --git a/t/t5308-pack-detect-duplicates.sh b/t/t5308-pack-detect-duplicates.sh
> index c6273a1aeb..95c81fa7b4 100755
> --- a/t/t5308-pack-detect-duplicates.sh
> +++ b/t/t5308-pack-detect-duplicates.sh

I haven't read the tests carefully (under the assumption that they may
change substantively if the meaning of "--allow-duplicate-objects" is
altered). But from skimming, I wonder if there is some room to shrink
the number of tests.

When working with an agent, I typically ask it to implement the minimal
number of tests, along with a prompt that it must demonstrate that those
tests still exercise all interesting behavior. Often I will repeat this
a number of times until I am similarly convinced.

Thanks,
Taylor

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

[Junio C Hamano]

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.

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.

Thanks.

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

[Taylor Blau]

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/