[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.