[PATCH] pack-objects: handle missing objects when excluding promisor objects

[PATCH] pack-objects: handle missing objects when excluding promisor objects

[Patrick Steinhardt]

In a partial clone Git tries to maintain two different sets of objects:
promisor objects should be those that we have received from the remote,
and any such object may not be fully connected. Locally created objects
on the other hand should be stored as non-promisor objects. The goal is
that promisor objects should never reference non-promisor objects as the
"promisor" property is contagious, where a promisor implicitly means
that any of its referenced objects are promisor objects, too.

So when git-index-pack(1) indexes a packfile from a promisor remote, and
that packfile has objects that refer to non-promisor objects, then it
tries to reinstate that partition by executing `git pack-objects
--exclude-promisor-objects-best-effort`. That command packs local,
non-promisor objects that become reachable from a promisor object into a
new promisor pack. This best-effort mode tries to exclude promisor
objects by using an `is_not_in_promisor_pack_obj()` callback for the
rev-walk: for every object that we see, we check whether it's part of a
promisor pack. And if it is, we exclude that object.

But in a partial clone we may fail to find some of the objects during
the object walk. The callback only stops us from walking into the
parents of a commit that lives in a promisor pack, but the commit itself
is still yielded, and `traverse_commit_list()` then queues its root
tree. And in a repository that was cloned with a "tree:0" filter that
tree does not exist, so `is_not_in_promisor_pack_obj()` fails to read it
and Git would `BUG()`:

    BUG: builtin/pack-objects.c:5088: should_include_obj should only be called on existing objects
    error: pack-objects died of signal 6
    fatal: could not finish pack-objects to repack local links
    fatal: index-pack failed

This bug can trigger in situations where we have e.g. a combination of
shallow and partial clones, and that bug has existed ever since we
introduced the logic in c08589efdc5 (index-pack: repack local links into
promisor packs, 2024-11-01).

But more recently, in d43b9acb05b (setup: unify setup of shallow file,
2026-07-07) we have introduced another case where it can occur. With
this change, Git now propagates the "--shallow-file" option to child
processes via GIT_SHALLOW_FILE. But when unshallowing or deepening a
repository we'd pass "--shallow-file=" to `git index-pack --promisor`,
and thus we'd now also propagate that environment variable to `git
pack-objects --exclude-promisor-objects-best-effort`. And as that asks
it to ignore the shallow boundary we may eventually try to read beyond
the shallow boundary and hit the missing tree of a promisor commit.

While we could stop propagating the environment variable to the child
process that would not help us to fix the first case. And besides, all
other global Git options also propagate to child processes in the same
way, so it feels wrong to behave differently here.

Instead, we can simply observe the fact that a missing object cannot be
a local non-promisor object by definition, and hence there is nothing
for us to pack in that case. So instead of calling `BUG()`, we should
rather just skip such objects. And coincidentally, that's also what
`repack_local_links()` (which invokes git-pack-objects(1) with
"--exclude-promisor-objects-best-effort") already does.

Reported-by: Daniel Zoghalchali <daniel@latchkey.dev>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
Hi,

this patch fixes the issue reported in [1]. Thanks!

Patrick

[1]: <CAJwQ=KQynymg4CJOLkyebACYVQa+LhXB2UifniTASkLyArWejA@mail.gmail.com>
---
 builtin/pack-objects.c   |  3 ++-
 t/t5616-partial-clone.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ca3a891dfbc..d6caed16d97 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -5085,7 +5085,8 @@ static int is_not_in_promisor_pack_obj(struct object *obj, void *data UNUSED)
 	};
 
 	if (odb_read_object_info_extended(the_repository->objects, &obj->oid, &info, 0))
-		BUG("should_include_obj should only be called on existing objects");
+		/* Missing; assume it is a promisor object */
+		return 0;
 	return source_info.source->type != ODB_SOURCE_PACKED || !source_info.u.packed.pack->pack_promisor;
 }
 
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 7cc6f58a31f..97a72e161ce 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -747,6 +747,61 @@ test_expect_success 'after fetching descendants of non-promisor commits, gc work
 	git -C partial gc --prune=now
 '
 
+for opt in --unshallow --deepen=1
+do
+	test_expect_success "filtered fetch $opt of a moved ref repacks local links" '
+		test_when_finished "rm -rf source target" &&
+
+		git init source &&
+		test_commit_bulk -C source 4 &&
+		git -C source config set uploadpack.allowFilter true &&
+		git -C source config set uploadpack.allowAnySHA1InWant true &&
+		COMMIT=$(git -C source rev-parse main~1) &&
+
+		git init target &&
+		git -C target remote add origin "file://$(pwd)/source" &&
+		git -C target fetch --no-tags --depth=1 origin "+$COMMIT:refs/remotes/origin/main" &&
+
+		# The fetched pack contains the new tip, whose parent is the
+		# local non-promisor shallow tip, so index-pack repacks the
+		# latter into a promisor pack. The pack-objects process doing
+		# so inherits the shallow file passed to index-pack and thus
+		# walks into the promisor commits below the former boundary,
+		# whose trees do not exist.
+		git -C target fetch --no-tags --filter=tree:0 "$opt" origin +refs/heads/main:refs/remotes/origin/main &&
+		git -C target rev-parse --verify refs/remotes/origin/main >actual &&
+		git -C source rev-parse --verify refs/heads/main >expect &&
+		test_cmp expect actual &&
+		git -C target fsck
+	'
+done
+
+test_expect_success 'repacking local links tolerates missing trees of promisor commits' '
+	test_when_finished "rm -rf source target" &&
+
+	git init source &&
+	test_commit_bulk -C source 4 &&
+	git -C source config set uploadpack.allowFilter true &&
+	git -C source config set uploadpack.allowAnySHA1InWant true &&
+	COMMIT=$(git -C source rev-parse main~1) &&
+
+	# Fetch a single commit as a non-promisor object, then unshallow it
+	# with a filter that omits trees. The commits below the former
+	# shallow boundary are now promisor objects whose trees are missing.
+	git init target &&
+	git -C target remote add origin "file://$(pwd)/source" &&
+	git -C target fetch --no-tags --depth=1 origin "+$COMMIT:refs/remotes/origin/main" &&
+	git -C target fetch --no-tags --filter=tree:0 --unshallow origin "+$COMMIT:refs/remotes/origin/main" &&
+	test_path_is_missing target/.git/shallow &&
+
+	# Fetching a descendant of the local non-promisor commit causes
+	# index-pack to repack that commit into a promisor pack. The revision
+	# walk must not die when it reaches the missing trees of the promisor
+	# commits below it.
+	git -C target fetch --no-tags --filter=tree:0 origin +refs/heads/main:refs/remotes/origin/main &&
+	git -C target rev-parse --verify refs/remotes/origin/main &&
+	git -C target fsck
+'
 
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd

---
base-commit: 7e34d8174dcda3435d1404c9a30bef46afe6c13e
change-id: 20261026-b4-pks-pack-objects-repack-links-with-missing-objects-ba49defedef1

Re: [PATCH] pack-objects: handle missing objects kkkjwhen excluding promisor objects

[Mark C. Chu-Carroll]

On Tue, 27 Oct 2026 08:00:28 +0100, Patrick Steinhardt <ps@pks.im> wrote:
> In a partial clone Git tries to maintain two different sets of objects:
> promisor objects should be those that we have received from the remote,
> and any such object may not be fully connected. Locally created objects
> on the other hand should be stored as non-promisor objects. The goal is
> that promisor objects should never reference non-promisor objects as the
> "promisor" property is contagious, where a promisor implicitly means
> that any of its referenced objects are promisor objects, too.

Wordsmithing nitpick: this reads awkwardly. You start by saying there 
are two different kinds of things, and then introduce one of them,
followed by a comma, followed by something which _isn't_ the second 
type of thing.

Maybe:
In a partial clone, Git tries to maintain two different sets of objects:
promisor objects (which are those we have recieved from a remote, and may 
not be fully connected), and non-promisor objects (including any locally
created objects). The goal is that .... 

Re: [PATCH] pack-objects: handle missing objects kkkjwhen excluding promisor objects

[Patrick Steinhardt]

On Tue, Oct 27, 2026 at 12:02:11PM -0400, Mark C. Chu-Carroll wrote:
> 
> On Tue, 27 Oct 2026 08:00:28 +0100, Patrick Steinhardt <ps@pks.im> wrote:
> > In a partial clone Git tries to maintain two different sets of objects:
> > promisor objects should be those that we have received from the remote,
> > and any such object may not be fully connected. Locally created objects
> > on the other hand should be stored as non-promisor objects. The goal is
> > that promisor objects should never reference non-promisor objects as the
> > "promisor" property is contagious, where a promisor implicitly means
> > that any of its referenced objects are promisor objects, too.
> 
> Wordsmithing nitpick: this reads awkwardly. You start by saying there 
> are two different kinds of things, and then introduce one of them,
> followed by a comma, followed by something which _isn't_ the second 
> type of thing.
> 
> Maybe:
> In a partial clone, Git tries to maintain two different sets of objects:
> promisor objects (which are those we have recieved from a remote, and may 
> not be fully connected), and non-promisor objects (including any locally
> created objects). The goal is that .... 

That reads better indeed. Queued locally for now, I'll wait a bit longer
before sending this out. Thanks!

Patrick

Re: [PATCH] pack-objects: handle missing objects when excluding promisor objects

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> this patch fixes the issue reported in [1]. Thanks!
>  builtin/pack-objects.c   |  3 ++-
>  t/t5616-partial-clone.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index ca3a891dfbc..d6caed16d97 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -5085,7 +5085,8 @@ static int is_not_in_promisor_pack_obj(struct object *obj, void *data UNUSED)
>  	};
>  
>  	if (odb_read_object_info_extended(the_repository->objects, &obj->oid, &info, 0))
> -		BUG("should_include_obj should only be called on existing objects");
> +		/* Missing; assume it is a promisor object */
> +		return 0;

It is not entirely the fault of this patch, but the code relies on
.fetch_if_missing being set to 0 elsewhere.  While that is not wrong
per se, shouldn't this call to odb_read_object_info_extended() pass
the OBJECT_INFO_SKIP_FETCH_OBJECT bit explicitly for clarity?

>  	return source_info.source->type != ODB_SOURCE_PACKED || !source_info.u.packed.pack->pack_promisor;
>  }

Thanks.

Re: [PATCH] pack-objects: handle missing objects when excluding promisor objects

[Patrick Steinhardt]

On Wed, Oct 28, 2026 at 12:37:02AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > this patch fixes the issue reported in [1]. Thanks!
> >  builtin/pack-objects.c   |  3 ++-
> >  t/t5616-partial-clone.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 57 insertions(+), 1 deletion(-)
> >
> > diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> > index ca3a891dfbc..d6caed16d97 100644
> > --- a/builtin/pack-objects.c
> > +++ b/builtin/pack-objects.c
> > @@ -5085,7 +5085,8 @@ static int is_not_in_promisor_pack_obj(struct object *obj, void *data UNUSED)
> >  	};
> >  
> >  	if (odb_read_object_info_extended(the_repository->objects, &obj->oid, &info, 0))
> > -		BUG("should_include_obj should only be called on existing objects");
> > +		/* Missing; assume it is a promisor object */
> > +		return 0;
> 
> It is not entirely the fault of this patch, but the code relies on
> .fetch_if_missing being set to 0 elsewhere.  While that is not wrong
> per se, shouldn't this call to odb_read_object_info_extended() pass
> the OBJECT_INFO_SKIP_FETCH_OBJECT bit explicitly for clarity?

Yeah. This is fine in practice because we always unset
`fetch_if_missing` with "--exclude-promisor-objects[-best-effort]", but
adding the flag explicitly for clarity feels sensible to me.

Patrick