[PATCH 0/3] connected: search promisor objects generically

[PATCH 0/3] connected: search promisor objects generically

[Patrick Steinhardt]

Hi,

this patch series refactors "connected.c" so that we search for promisor
objects in a generic way instead of reaching into internal of the object
database. As a result, the connectivity checks will work properly in
repos that don't use packfiles in the first place.

The series is built on top of 8d96f09e92 (Merge branch
'js/objects-larger-than-4gb-on-windows', 2026-06-19) with
ps/odb-source-packed at 1bba3c035d (odb/source-packed: drop pointer to
"files" parent source, 2026-06-17) merged into it.

Thanks!

Patrick

---
Patrick Steinhardt (3):
      odb/source-packed: extract logic to skip certain packs
      odb/source-packed: support flags when iterating an object prefix
      connected: search promisor objects generically

 connected.c         | 39 +++++++++++++++++++++++++--------------
 odb/source-packed.c | 50 +++++++++++++++++++++++++++++++++++++-------------
 2 files changed, 62 insertions(+), 27 deletions(-)


---
base-commit: 4a8e7a446f41435e157131162dfe901eca9250fe
change-id: 20260612-pks-connected-generic-promisor-checks-2933bff3028d

[PATCH 1/3] odb/source-packed: extract logic to skip certain packs

[Patrick Steinhardt]

The caller can pass flags that allow them to filter out specific kinds
of objects when iterating objects via `odb_for_each_object()`. This only
works for "normal" iteration though, as we `BUG()` when the user passes
flags and specifies an object prefix.

This limitation will be lifted in the next commit. Prepare for this by
extracting the logic that skips certain kinds of packs so that we can
easily reuse it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-packed.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 42c28fba0e..3afc4bf01f 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -126,6 +126,22 @@ static int match_hash(unsigned len, const unsigned char *a, const unsigned char
 	return 1;
 }
 
+static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_flags flags)
+{
+	if ((flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
+		return true;
+	if ((flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&
+	    !p->pack_promisor)
+		return true;
+	if ((flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&
+	    p->pack_keep_in_core)
+		return true;
+	if ((flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&
+	    p->pack_keep)
+		return true;
+	return false;
+}
+
 static int for_each_prefixed_object_in_midx(
 	struct odb_source_packed *store,
 	struct multi_pack_index *m,
@@ -306,17 +322,9 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 	for (e = packfile_store_get_packs(packed); e; e = e->next) {
 		struct packed_git *p = e->pack;
 
-		if ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
-			continue;
-		if ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&
-		    !p->pack_promisor)
-			continue;
-		if ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&
-		    p->pack_keep_in_core)
-			continue;
-		if ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&
-		    p->pack_keep)
+		if (should_exclude_pack(p, opts->flags))
 			continue;
+
 		if (open_pack_index(p)) {
 			pack_errors = 1;
 			continue;

-- 
2.55.0.rc1.745.g43192e7977.dirty

Re: [PATCH 1/3] odb/source-packed: extract logic to skip certain packs

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The caller can pass flags that allow them to filter out specific kinds
> of objects when iterating objects via `odb_for_each_object()`. This only
> works for "normal" iteration though, as we `BUG()` when the user passes
> flags and specifies an object prefix.
>
> This limitation will be lifted in the next commit. Prepare for this by
> extracting the logic that skips certain kinds of packs so that we can
> easily reuse it.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source-packed.c | 28 ++++++++++++++++++----------
>  1 file changed, 18 insertions(+), 10 deletions(-)

Quite straight-forward creation of a simple helper function.

> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 42c28fba0e..3afc4bf01f 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -126,6 +126,22 @@ static int match_hash(unsigned len, const unsigned char *a, const unsigned char
>  	return 1;
>  }
>  
> +static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_flags flags)
> +{
> +	if ((flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
> +		return true;
> +	if ((flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&
> +	    !p->pack_promisor)
> +		return true;
> +	if ((flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&
> +	    p->pack_keep_in_core)
> +		return true;
> +	if ((flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&
> +	    p->pack_keep)
> +		return true;
> +	return false;
> +}
> +
>  static int for_each_prefixed_object_in_midx(
>  	struct odb_source_packed *store,
>  	struct multi_pack_index *m,
> @@ -306,17 +322,9 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
>  	for (e = packfile_store_get_packs(packed); e; e = e->next) {
>  		struct packed_git *p = e->pack;
>  
> -		if ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
> -			continue;
> -		if ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&
> -		    !p->pack_promisor)
> -			continue;
> -		if ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&
> -		    p->pack_keep_in_core)
> -			continue;
> -		if ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&
> -		    p->pack_keep)
> +		if (should_exclude_pack(p, opts->flags))
>  			continue;
> +
>  		if (open_pack_index(p)) {
>  			pack_errors = 1;
>  			continue;

[PATCH 2/3] odb/source-packed: support flags when iterating an object prefix

[Patrick Steinhardt]

Callers of `odb_for_each_object()` can specify an optional object name
prefix so that we only yield objects that match it. This is incompatible
though with passing flags at the same time, as we don't yet know to
handle them.

Loosen this restriction by calling `should_exclude_pack()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-packed.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 3afc4bf01f..6f31f0ff94 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -148,6 +148,7 @@ static int for_each_prefixed_object_in_midx(
 	const struct odb_for_each_object_options *opts,
 	struct odb_source_packed_for_each_object_wrapper_data *data)
 {
+	bool pack_errors = false;
 	int ret;
 
 	for (; m; m = m->base_midx) {
@@ -171,6 +172,20 @@ static int for_each_prefixed_object_in_midx(
 			const struct object_id *current = NULL;
 			struct object_id oid;
 
+			if (opts->flags) {
+				uint32_t pack_id = nth_midxed_pack_int_id(m, i);
+				struct packed_git *pack;
+
+				if (prepare_midx_pack(m, pack_id)) {
+					pack_errors = true;
+					continue;
+				}
+
+				pack = nth_midxed_pack(m, pack_id);
+				if (should_exclude_pack(pack, opts->flags))
+					continue;
+			}
+
 			current = nth_midxed_object_oid(&oid, m, i);
 
 			if (!match_hash(len, opts->prefix->hash, current->hash))
@@ -198,6 +213,8 @@ static int for_each_prefixed_object_in_midx(
 	ret = 0;
 
 out:
+	if (!ret && pack_errors)
+		ret = -1;
 	return ret;
 }
 
@@ -260,9 +277,6 @@ static int odb_source_packed_for_each_prefixed_object(
 	bool pack_errors = false;
 	int ret;
 
-	if (opts->flags)
-		BUG("flags unsupported");
-
 	store->skip_mru_updates = true;
 
 	m = get_multi_pack_index(store);
@@ -275,6 +289,8 @@ static int odb_source_packed_for_each_prefixed_object(
 	for (e = packfile_store_get_packs(store); e; e = e->next) {
 		if (e->pack->multi_pack_index)
 			continue;
+		if (should_exclude_pack(e->pack, opts->flags))
+			continue;
 
 		if (open_pack_index(e->pack)) {
 			pack_errors = true;

-- 
2.55.0.rc1.745.g43192e7977.dirty

[PATCH 3/3] connected: search promisor objects generically

[Patrick Steinhardt]

When performing connectivity checks we have to figure out whether any of
the new objects are promisor objects, as we cannot assume full
connectivity if so.

This check is performed by iterating through all packfiles in the
repository and searching each of them for the given object. Of course,
this mechanism is quite specific to implementation details of the object
database, as we assume that it uses packfiles in the first place.

Refactor the logic so that we instead use `odb_for_each_object_ext()`
with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`
flag. This will yield all objects that have the exact object name and
that are part of a promisor pack in a generic way.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 connected.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/connected.c b/connected.c
index 7e26976832..9a666f0cdf 100644
--- a/connected.c
+++ b/connected.c
@@ -11,6 +11,13 @@
 #include "packfile.h"
 #include "promisor-remote.h"
 
+static int promised_object_cb(const struct object_id *oid UNUSED,
+			      struct object_info *oi UNUSED,
+			      void *payload UNUSED)
+{
+	return 1;
+}
+
 /*
  * If we feed all the commits we want to verify to this command
  *
@@ -46,6 +53,11 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	}
 
 	if (repo_has_promisor_remote(the_repository)) {
+		struct odb_for_each_object_options opts = {
+			.flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,
+			.prefix_hex_len = the_repository->hash_algo->hexsz,
+		};
+
 		/*
 		 * For partial clones, we don't want to have to do a regular
 		 * connectivity check because we have to enumerate and exclude
@@ -54,31 +66,30 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 		 * object is a promisor object. Instead, just make sure we
 		 * received, in a promisor packfile, the objects pointed to by
 		 * each wanted ref.
-		 *
-		 * Before checking for promisor packs, be sure we have the
-		 * latest pack-files loaded into memory.
 		 */
-		odb_reprepare(the_repository->objects);
 		do {
-			struct packed_git *p;
-
-			repo_for_each_pack(the_repository, p) {
-				if (!p->pack_promisor)
-					continue;
-				if (find_pack_entry_one(oid, p))
-					goto promisor_pack_found;
+			opts.prefix = oid;
+
+			err = odb_for_each_object_ext(the_repository->objects,
+						      NULL, promised_object_cb,
+						      NULL, &opts);
+			if (err < 0)
+				break;
+			if (err > 0) {
+				err = 0;
+				continue;
 			}
+
 			/*
 			 * Fallback to rev-list with oid and the rest of the
 			 * object IDs provided by fn.
 			 */
 			goto no_promisor_pack_found;
-promisor_pack_found:
-			;
 		} while ((oid = fn(cb_data)) != NULL);
+
 		if (opt->err_fd)
 			close(opt->err_fd);
-		return 0;
+		return err;
 	}
 
 no_promisor_pack_found:

-- 
2.55.0.rc1.745.g43192e7977.dirty

Re: [PATCH 3/3] connected: search promisor objects generically

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When performing connectivity checks we have to figure out whether any of
> the new objects are promisor objects, as we cannot assume full
> connectivity if so.
>
> This check is performed by iterating through all packfiles in the
> repository and searching each of them for the given object. Of course,
> this mechanism is quite specific to implementation details of the object
> database, as we assume that it uses packfiles in the first place.
>
> Refactor the logic so that we instead use `odb_for_each_object_ext()`
> with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`
> flag. This will yield all objects that have the exact object name and
> that are part of a promisor pack in a generic way.
> ...


> -		 *
> -		 * Before checking for promisor packs, be sure we have the
> -		 * latest pack-files loaded into memory.
>  		 */
> -		odb_reprepare(the_repository->objects);

Hmph?

>  		do {
> -			struct packed_git *p;
> -
> -			repo_for_each_pack(the_repository, p) {
> -				if (!p->pack_promisor)
> -					continue;
> -				if (find_pack_entry_one(oid, p))
> -					goto promisor_pack_found;
> +			opts.prefix = oid;
> +
> +			err = odb_for_each_object_ext(the_repository->objects,
> +						      NULL, promised_object_cb,
> +						      NULL, &opts);
> +			if (err < 0)
> +				break;
> +			if (err > 0) {
> +				err = 0;
> +				continue;
>  			}

So we used to manually iterate and stop when we have a matching pack
entry, but now "stop when we find" is done by promisor_object_cb
callback that returns 1.

What is the reason why we no longer odb_(re)prepare() upfront before
going into the loop?  Would it make us miss a newly added promisor
packs?  We will fall back to rev-list for correctness, so it may not
matter, though.

> +
>  			/*
>  			 * Fallback to rev-list with oid and the rest of the
>  			 * object IDs provided by fn.
>  			 */
>  			goto no_promisor_pack_found;
> -promisor_pack_found:
> -			;
>  		} while ((oid = fn(cb_data)) != NULL);
> +
>  		if (opt->err_fd)
>  			close(opt->err_fd);
> -		return 0;
> +		return err;
>  	}
>  
>  no_promisor_pack_found:

Re: [PATCH 3/3] connected: search promisor objects generically

[Patrick Steinhardt]

On Mon, Jun 22, 2026 at 10:57:10AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > -		 *
> > -		 * Before checking for promisor packs, be sure we have the
> > -		 * latest pack-files loaded into memory.
> >  		 */
> > -		odb_reprepare(the_repository->objects);
> 
> Hmph?

Oh, I think I completely misread this as `odb_alternates_prepare()`,
which is something you typically see before loops like this. By using a
helper like `odb_for_each_object_ext()` we of course wouldn't have to
call that function anymore.

But this here is of course different, as this call would also cause us
to reload packfiles and loose objects.

> >  		do {
> > -			struct packed_git *p;
> > -
> > -			repo_for_each_pack(the_repository, p) {
> > -				if (!p->pack_promisor)
> > -					continue;
> > -				if (find_pack_entry_one(oid, p))
> > -					goto promisor_pack_found;
> > +			opts.prefix = oid;
> > +
> > +			err = odb_for_each_object_ext(the_repository->objects,
> > +						      NULL, promised_object_cb,
> > +						      NULL, &opts);
> > +			if (err < 0)
> > +				break;
> > +			if (err > 0) {
> > +				err = 0;
> > +				continue;
> >  			}
> 
> So we used to manually iterate and stop when we have a matching pack
> entry, but now "stop when we find" is done by promisor_object_cb
> callback that returns 1.
> 
> What is the reason why we no longer odb_(re)prepare() upfront before
> going into the loop?  Would it make us miss a newly added promisor
> packs?  We will fall back to rev-list for correctness, so it may not
> matter, though.

So yes, this is a bug.

Thanks!

Patrick

Re: [PATCH 3/3] connected: search promisor objects generically

[Christian Couder]

On Mon, Jun 22, 2026 at 10:50 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> When performing connectivity checks we have to figure out whether any of
> the new objects are promisor objects, as we cannot assume full
> connectivity if so.
>
> This check is performed by iterating through all packfiles in the
> repository and searching each of them for the given object. Of course,
> this mechanism is quite specific to implementation details of the object
> database, as we assume that it uses packfiles in the first place.
>
> Refactor the logic so that we instead use `odb_for_each_object_ext()`
> with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`
> flag. This will yield all objects that have the exact object name and
> that are part of a promisor pack in a generic way.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  connected.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/connected.c b/connected.c
> index 7e26976832..9a666f0cdf 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -11,6 +11,13 @@
>  #include "packfile.h"
>  #include "promisor-remote.h"
>
> +static int promised_object_cb(const struct object_id *oid UNUSED,
> +                             struct object_info *oi UNUSED,
> +                             void *payload UNUSED)
> +{
> +       return 1;
> +}
> +
>  /*
>   * If we feed all the commits we want to verify to this command
>   *
> @@ -46,6 +53,11 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>         }
>
>         if (repo_has_promisor_remote(the_repository)) {
> +               struct odb_for_each_object_options opts = {
> +                       .flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,
> +                       .prefix_hex_len = the_repository->hash_algo->hexsz,
> +               };
> +
>                 /*
>                  * For partial clones, we don't want to have to do a regular
>                  * connectivity check because we have to enumerate and exclude
> @@ -54,31 +66,30 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
>                  * object is a promisor object. Instead, just make sure we
>                  * received, in a promisor packfile, the objects pointed to by
>                  * each wanted ref.
> -                *
> -                * Before checking for promisor packs, be sure we have the
> -                * latest pack-files loaded into memory.
>                  */
> -               odb_reprepare(the_repository->objects);

Like Junio, I am not sure it's correct to remove the
`odb_reprepare(the_repository->objects)` call.

I think it was added for good reasons in b739d971 (connected.c:
reprepare packs for corner cases, 2020-03-13) and I am not sure
odb_for_each_object_ext() is performing something similar.

At least the commit message should mention this change and explain a
bit why the reasons the call was added are not valid anymore.

>                 do {
> -                       struct packed_git *p;
> -
> -                       repo_for_each_pack(the_repository, p) {
> -                               if (!p->pack_promisor)
> -                                       continue;
> -                               if (find_pack_entry_one(oid, p))
> -                                       goto promisor_pack_found;
> +                       opts.prefix = oid;
> +
> +                       err = odb_for_each_object_ext(the_repository->objects,
> +                                                     NULL, promised_object_cb,
> +                                                     NULL, &opts);
> +                       if (err < 0)
> +                               break;
> +                       if (err > 0) {
> +                               err = 0;
> +                               continue;
>                         }
> +
>                         /*
>                          * Fallback to rev-list with oid and the rest of the
>                          * object IDs provided by fn.
>                          */
>                         goto no_promisor_pack_found;
> -promisor_pack_found:
> -                       ;
>                 } while ((oid = fn(cb_data)) != NULL);
> +
>                 if (opt->err_fd)
>                         close(opt->err_fd);
> -               return 0;
> +               return err;
>         }
>
>  no_promisor_pack_found:

These changes are difficult to understand as there are a number of
`goto`, `break`, `return`, etc involved.

I think it comes in the first place from check_connected() doing too
many things, and adding a preparatory commit to refactor it would
help.

For example the preparatory commit could move a lot of code from
check_connected() to the following new functions:

/*
 * Returns:
 *   1  = all wanted OIDs found in promisor packs: connected, done.
 *   0  = at least one OID not found: caller must fall back to rev-list.
 *  <0  = error.
 * On the fallback (0) return, *oid is left pointing at the first
 * not-found OID so the rev-list path can resume the iteration.
 */
static int check_connected_promisor(oid_iterate_fn fn, void *cb_data,
                                     const struct object_id **oid);

/*
 * In a non-promisor repo, pass the first OID as `oid`.
 * Otherwise pass the first not-found OID resumed from
 * check_connected_promisor() as `oid`.
 */
static int check_connected_rev_list(oid_iterate_fn fn, void *cb_data,
                                     struct check_connected_options *opt,
                                     const struct object_id *oid);

Re: [PATCH 3/3] connected: search promisor objects generically

[Patrick Steinhardt]

On Tue, Jun 23, 2026 at 09:45:44AM +0200, Christian Couder wrote:
> On Mon, Jun 22, 2026 at 10:50 AM Patrick Steinhardt <ps@pks.im> wrote:
> > diff --git a/connected.c b/connected.c
> > index 7e26976832..9a666f0cdf 100644
> > --- a/connected.c
> > +++ b/connected.c
> > @@ -54,31 +66,30 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> >                  * object is a promisor object. Instead, just make sure we
> >                  * received, in a promisor packfile, the objects pointed to by
> >                  * each wanted ref.
> > -                *
> > -                * Before checking for promisor packs, be sure we have the
> > -                * latest pack-files loaded into memory.
> >                  */
> > -               odb_reprepare(the_repository->objects);
> 
> Like Junio, I am not sure it's correct to remove the
> `odb_reprepare(the_repository->objects)` call.
> 
> I think it was added for good reasons in b739d971 (connected.c:
> reprepare packs for corner cases, 2020-03-13) and I am not sure
> odb_for_each_object_ext() is performing something similar.
> 
> At least the commit message should mention this change and explain a
> bit why the reasons the call was added are not valid anymore.

Yeah, I think you're both correct. The only explanation I have is that I
might have repeatedly misread this as `odb_prepare_alternates()`, which
is something we often call before suck loops.

> >                 do {
> > -                       struct packed_git *p;
> > -
> > -                       repo_for_each_pack(the_repository, p) {
> > -                               if (!p->pack_promisor)
> > -                                       continue;
> > -                               if (find_pack_entry_one(oid, p))
> > -                                       goto promisor_pack_found;
> > +                       opts.prefix = oid;
> > +
> > +                       err = odb_for_each_object_ext(the_repository->objects,
> > +                                                     NULL, promised_object_cb,
> > +                                                     NULL, &opts);
> > +                       if (err < 0)
> > +                               break;
> > +                       if (err > 0) {
> > +                               err = 0;
> > +                               continue;
> >                         }
> > +
> >                         /*
> >                          * Fallback to rev-list with oid and the rest of the
> >                          * object IDs provided by fn.
> >                          */
> >                         goto no_promisor_pack_found;
> > -promisor_pack_found:
> > -                       ;
> >                 } while ((oid = fn(cb_data)) != NULL);
> > +
> >                 if (opt->err_fd)
> >                         close(opt->err_fd);
> > -               return 0;
> > +               return err;
> >         }
> >
> >  no_promisor_pack_found:
> 
> These changes are difficult to understand as there are a number of
> `goto`, `break`, `return`, etc involved.

Yeah, agreed. I had my issues understanding this logic, too.

> I think it comes in the first place from check_connected() doing too
> many things, and adding a preparatory commit to refactor it would
> help.
> 
> For example the preparatory commit could move a lot of code from
> check_connected() to the following new functions:

I'll give that a try, thanks!

Patrick

[PATCH v2 0/4] connected: search promisor objects generically

[Patrick Steinhardt]

Hi,

this patch series refactors "connected.c" so that we search for promisor
objects in a generic way instead of reaching into internal of the object
database. As a result, the connectivity checks will work properly in
repos that don't use packfiles in the first place.

The series is built on top of 8d96f09e92 (Merge branch
'js/objects-larger-than-4gb-on-windows', 2026-06-19) with
ps/odb-source-packed at 1bba3c035d (odb/source-packed: drop pointer to
"files" parent source, 2026-06-17) merged into it.

Changes in v2:
  - Fix the accidentally-dropped call to `odb_reprepare()`.
  - Add a preparatory commit that splits out `check_connected_promisor()`.
    I think also splitting out `check_connected_rev_list()` would only
    have diminishing returns, so I skipped that part.
  - Link to v1: https://patch.msgid.link/20260622-pks-connected-generic-promisor-checks-v1-0-25eba2698202@pks.im

Thanks!

Patrick

---
Patrick Steinhardt (4):
      odb/source-packed: extract logic to skip certain packs
      odb/source-packed: support flags when iterating an object prefix
      connected: split out promisor-based connectivity check
      connected: search promisor objects generically

 connected.c         | 95 ++++++++++++++++++++++++++++++++++-------------------
 odb/source-packed.c | 50 ++++++++++++++++++++--------
 2 files changed, 98 insertions(+), 47 deletions(-)

Range-diff versus v1:

1:  6ff1fc8d89 = 1:  a1a1af0fc6 odb/source-packed: extract logic to skip certain packs
2:  1022a1fdcc = 2:  bd81a9e478 odb/source-packed: support flags when iterating an object prefix
3:  102fab7df2 < -:  ---------- connected: search promisor objects generically
-:  ---------- > 3:  f39ef68c3e connected: split out promisor-based connectivity check
-:  ---------- > 4:  558f30a6f2 connected: search promisor objects generically

---
base-commit: 4a8e7a446f41435e157131162dfe901eca9250fe
change-id: 20260612-pks-connected-generic-promisor-checks-2933bff3028d

[PATCH v2 1/4] odb/source-packed: extract logic to skip certain packs

[Patrick Steinhardt]

The caller can pass flags that allow them to filter out specific kinds
of objects when iterating objects via `odb_for_each_object()`. This only
works for "normal" iteration though, as we `BUG()` when the user passes
flags and specifies an object prefix.

This limitation will be lifted in the next commit. Prepare for this by
extracting the logic that skips certain kinds of packs so that we can
easily reuse it.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-packed.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 42c28fba0e..3afc4bf01f 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -126,6 +126,22 @@ static int match_hash(unsigned len, const unsigned char *a, const unsigned char
 	return 1;
 }
 
+static bool should_exclude_pack(struct packed_git *p, enum odb_for_each_object_flags flags)
+{
+	if ((flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
+		return true;
+	if ((flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&
+	    !p->pack_promisor)
+		return true;
+	if ((flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&
+	    p->pack_keep_in_core)
+		return true;
+	if ((flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&
+	    p->pack_keep)
+		return true;
+	return false;
+}
+
 static int for_each_prefixed_object_in_midx(
 	struct odb_source_packed *store,
 	struct multi_pack_index *m,
@@ -306,17 +322,9 @@ static int odb_source_packed_for_each_object(struct odb_source *source,
 	for (e = packfile_store_get_packs(packed); e; e = e->next) {
 		struct packed_git *p = e->pack;
 
-		if ((opts->flags & ODB_FOR_EACH_OBJECT_LOCAL_ONLY) && !p->pack_local)
-			continue;
-		if ((opts->flags & ODB_FOR_EACH_OBJECT_PROMISOR_ONLY) &&
-		    !p->pack_promisor)
-			continue;
-		if ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_IN_CORE_KEPT_PACKS) &&
-		    p->pack_keep_in_core)
-			continue;
-		if ((opts->flags & ODB_FOR_EACH_OBJECT_SKIP_ON_DISK_KEPT_PACKS) &&
-		    p->pack_keep)
+		if (should_exclude_pack(p, opts->flags))
 			continue;
+
 		if (open_pack_index(p)) {
 			pack_errors = 1;
 			continue;

-- 
2.55.0.rc1.745.g43192e7977.dirty

[PATCH v2 2/4] odb/source-packed: support flags when iterating an object prefix

[Patrick Steinhardt]

Callers of `odb_for_each_object()` can specify an optional object name
prefix so that we only yield objects that match it. This is incompatible
though with passing flags at the same time, as we don't yet know to
handle them.

Loosen this restriction by calling `should_exclude_pack()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 odb/source-packed.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/odb/source-packed.c b/odb/source-packed.c
index 3afc4bf01f..6f31f0ff94 100644
--- a/odb/source-packed.c
+++ b/odb/source-packed.c
@@ -148,6 +148,7 @@ static int for_each_prefixed_object_in_midx(
 	const struct odb_for_each_object_options *opts,
 	struct odb_source_packed_for_each_object_wrapper_data *data)
 {
+	bool pack_errors = false;
 	int ret;
 
 	for (; m; m = m->base_midx) {
@@ -171,6 +172,20 @@ static int for_each_prefixed_object_in_midx(
 			const struct object_id *current = NULL;
 			struct object_id oid;
 
+			if (opts->flags) {
+				uint32_t pack_id = nth_midxed_pack_int_id(m, i);
+				struct packed_git *pack;
+
+				if (prepare_midx_pack(m, pack_id)) {
+					pack_errors = true;
+					continue;
+				}
+
+				pack = nth_midxed_pack(m, pack_id);
+				if (should_exclude_pack(pack, opts->flags))
+					continue;
+			}
+
 			current = nth_midxed_object_oid(&oid, m, i);
 
 			if (!match_hash(len, opts->prefix->hash, current->hash))
@@ -198,6 +213,8 @@ static int for_each_prefixed_object_in_midx(
 	ret = 0;
 
 out:
+	if (!ret && pack_errors)
+		ret = -1;
 	return ret;
 }
 
@@ -260,9 +277,6 @@ static int odb_source_packed_for_each_prefixed_object(
 	bool pack_errors = false;
 	int ret;
 
-	if (opts->flags)
-		BUG("flags unsupported");
-
 	store->skip_mru_updates = true;
 
 	m = get_multi_pack_index(store);
@@ -275,6 +289,8 @@ static int odb_source_packed_for_each_prefixed_object(
 	for (e = packfile_store_get_packs(store); e; e = e->next) {
 		if (e->pack->multi_pack_index)
 			continue;
+		if (should_exclude_pack(e->pack, opts->flags))
+			continue;
 
 		if (open_pack_index(e->pack)) {
 			pack_errors = true;

-- 
2.55.0.rc1.745.g43192e7977.dirty

Re: [PATCH v2 2/4] odb/source-packed: support flags when iterating an object prefix

[Christian Couder]

On Wed, Jun 24, 2026 at 12:37 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> Callers of `odb_for_each_object()` can specify an optional object name
> prefix so that we only yield objects that match it. This is incompatible
> though with passing flags at the same time, as we don't yet know to
> handle them.
>
> Loosen this restriction by calling `should_exclude_pack()`.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb/source-packed.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/odb/source-packed.c b/odb/source-packed.c
> index 3afc4bf01f..6f31f0ff94 100644
> --- a/odb/source-packed.c
> +++ b/odb/source-packed.c
> @@ -148,6 +148,7 @@ static int for_each_prefixed_object_in_midx(
>         const struct odb_for_each_object_options *opts,
>         struct odb_source_packed_for_each_object_wrapper_data *data)
>  {
> +       bool pack_errors = false;
>         int ret;
>
>         for (; m; m = m->base_midx) {
> @@ -171,6 +172,20 @@ static int for_each_prefixed_object_in_midx(
>                         const struct object_id *current = NULL;
>                         struct object_id oid;
>
> +                       if (opts->flags) {
> +                               uint32_t pack_id = nth_midxed_pack_int_id(m, i);
> +                               struct packed_git *pack;
> +
> +                               if (prepare_midx_pack(m, pack_id)) {
> +                                       pack_errors = true;
> +                                       continue;
> +                               }
> +
> +                               pack = nth_midxed_pack(m, pack_id);
> +                               if (should_exclude_pack(pack, opts->flags))
> +                                       continue;
> +                       }
> +
>                         current = nth_midxed_object_oid(&oid, m, i);
>
>                         if (!match_hash(len, opts->prefix->hash, current->hash))

It looks like this is:

                        if (!match_hash(len, opts->prefix->hash, current->hash))
                                break;

and I wonder if the `if (opts->flags) { ... }` block would be better
after that prefix check rather than before it.

Putting it after the prefix check would make sure we don't continue
when the prefix doesn't match.

[PATCH v2 3/4] connected: split out promisor-based connectivity check

[Patrick Steinhardt]

When performing a connectivity check in a partial clone we try to avoid
doing the connectivity check by checking whether all new tips are part
of a promisor pack. This makes use of the fact that we don't expect full
connectivity for promised objects anyway, so it's basically fine if
those objects are not fully connected.

The logic that handles this promisor-based check is somewhat hard to
read though as it uses nested loops and gotos. Pull it out into a
standalone function, which makes it a bit easier to reason about.

We'll also further simplify the function in the next commit.

Suggested-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 connected.c | 85 ++++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 51 insertions(+), 34 deletions(-)

diff --git a/connected.c b/connected.c
index 7e26976832..d2b334173f 100644
--- a/connected.c
+++ b/connected.c
@@ -11,6 +11,49 @@
 #include "packfile.h"
 #include "promisor-remote.h"
 
+/*
+ * For partial clones, we don't want to have to do a regular connectivity check
+ * because we have to enumerate and exclude all promisor objects (slow), and
+ * then the connectivity check itself becomes a no-op because in a partial
+ * clone every object is a promisor object. Instead, just make sure we
+ * received, in a promisor packfile, the objects pointed to by each wanted ref.
+ *
+ * Before checking for promisor packs, be sure we have the latest pack-files
+ * loaded into memory.
+ *
+ * Returns 1 when all object IDs have been found in promisor packs, in which
+ * case we're fully connected and thus done. Returns 0 when we have found
+ * objects in non-promisor packs, in which case we'll have to fall back to the
+ * rev-list-based connectivity checks. Returns a negative error code on error.
+ */
+static int check_connected_promisor(oid_iterate_fn fn,
+				    void *cb_data,
+				    const struct object_id **oid)
+{
+	odb_reprepare(the_repository->objects);
+	do {
+		struct packed_git *p;
+
+		repo_for_each_pack(the_repository, p) {
+			if (!p->pack_promisor)
+				continue;
+			if (find_pack_entry_one(*oid, p))
+				goto promisor_pack_found;
+		}
+
+		/*
+		 * We have found an object that is not part of a promisor pack,
+		 * and thus we cannot skip the full connectivity check.
+		 */
+		return 0;
+
+promisor_pack_found:
+		;
+	} while ((*oid = fn(cb_data)) != NULL);
+
+	return 1;
+}
+
 /*
  * If we feed all the commits we want to verify to this command
  *
@@ -46,42 +89,16 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
 	}
 
 	if (repo_has_promisor_remote(the_repository)) {
-		/*
-		 * For partial clones, we don't want to have to do a regular
-		 * connectivity check because we have to enumerate and exclude
-		 * all promisor objects (slow), and then the connectivity check
-		 * itself becomes a no-op because in a partial clone every
-		 * object is a promisor object. Instead, just make sure we
-		 * received, in a promisor packfile, the objects pointed to by
-		 * each wanted ref.
-		 *
-		 * Before checking for promisor packs, be sure we have the
-		 * latest pack-files loaded into memory.
-		 */
-		odb_reprepare(the_repository->objects);
-		do {
-			struct packed_git *p;
-
-			repo_for_each_pack(the_repository, p) {
-				if (!p->pack_promisor)
-					continue;
-				if (find_pack_entry_one(oid, p))
-					goto promisor_pack_found;
-			}
-			/*
-			 * Fallback to rev-list with oid and the rest of the
-			 * object IDs provided by fn.
-			 */
-			goto no_promisor_pack_found;
-promisor_pack_found:
-			;
-		} while ((oid = fn(cb_data)) != NULL);
-		if (opt->err_fd)
-			close(opt->err_fd);
-		return 0;
+		err = check_connected_promisor(fn, cb_data, &oid);
+		if (err) {
+			if (opt->err_fd)
+				close(opt->err_fd);
+			if (err > 0)
+				err = 0;
+			return err;
+		}
 	}
 
-no_promisor_pack_found:
 	if (opt->shallow_file) {
 		strvec_push(&rev_list.args, "--shallow-file");
 		strvec_push(&rev_list.args, opt->shallow_file);

-- 
2.55.0.rc1.745.g43192e7977.dirty

[PATCH v2 4/4] connected: search promisor objects generically

[Patrick Steinhardt]

When performing connectivity checks we have to figure out whether any of
the new objects are promisor objects, as we cannot assume full
connectivity if so.

This check is performed by iterating through all packfiles in the
repository and searching each of them for the given object. Of course,
this mechanism is quite specific to implementation details of the object
database, as we assume that it uses packfiles in the first place.

Refactor the logic so that we instead use `odb_for_each_object_ext()`
with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`
flag. This will yield all objects that have the exact object name and
that are part of a promisor pack in a generic way.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 connected.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/connected.c b/connected.c
index d2b334173f..b557ff5db9 100644
--- a/connected.c
+++ b/connected.c
@@ -11,6 +11,13 @@
 #include "packfile.h"
 #include "promisor-remote.h"
 
+static int promised_object_cb(const struct object_id *oid UNUSED,
+			      struct object_info *oi UNUSED,
+			      void *payload UNUSED)
+{
+	return 1;
+}
+
 /*
  * For partial clones, we don't want to have to do a regular connectivity check
  * because we have to enumerate and exclude all promisor objects (slow), and
@@ -30,25 +37,28 @@ static int check_connected_promisor(oid_iterate_fn fn,
 				    void *cb_data,
 				    const struct object_id **oid)
 {
+	struct odb_for_each_object_options opts = {
+		.flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,
+		.prefix_hex_len = the_repository->hash_algo->hexsz,
+	};
+	int err;
+
 	odb_reprepare(the_repository->objects);
 	do {
-		struct packed_git *p;
+		opts.prefix = *oid;
 
-		repo_for_each_pack(the_repository, p) {
-			if (!p->pack_promisor)
-				continue;
-			if (find_pack_entry_one(*oid, p))
-				goto promisor_pack_found;
-		}
+		err = odb_for_each_object_ext(the_repository->objects,
+					      NULL, promised_object_cb,
+					      NULL, &opts);
+		if (err < 0)
+			return err;
 
 		/*
 		 * We have found an object that is not part of a promisor pack,
 		 * and thus we cannot skip the full connectivity check.
 		 */
-		return 0;
-
-promisor_pack_found:
-		;
+		if (err > 0)
+			return 0;
 	} while ((*oid = fn(cb_data)) != NULL);
 
 	return 1;

-- 
2.55.0.rc1.745.g43192e7977.dirty

Re: [PATCH v2 4/4] connected: search promisor objects generically

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> When performing connectivity checks we have to figure out whether any of
> the new objects are promisor objects, as we cannot assume full
> connectivity if so.
>
> This check is performed by iterating through all packfiles in the
> repository and searching each of them for the given object. Of course,
> this mechanism is quite specific to implementation details of the object
> database, as we assume that it uses packfiles in the first place.
>
> Refactor the logic so that we instead use `odb_for_each_object_ext()`
> with an object prefix filter and the `ODB_FOR_EACH_OBJECT_PROMISOR_ONLY`
> flag. This will yield all objects that have the exact object name and
> that are part of a promisor pack in a generic way.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  connected.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/connected.c b/connected.c
> index d2b334173f..b557ff5db9 100644
> --- a/connected.c
> +++ b/connected.c
> @@ -11,6 +11,13 @@
>  #include "packfile.h"
>  #include "promisor-remote.h"
>  
> +static int promised_object_cb(const struct object_id *oid UNUSED,
> +			      struct object_info *oi UNUSED,
> +			      void *payload UNUSED)
> +{
> +	return 1;
> +}
> +
>  /*
>   * For partial clones, we don't want to have to do a regular connectivity check
>   * because we have to enumerate and exclude all promisor objects (slow), and
> @@ -30,25 +37,28 @@ static int check_connected_promisor(oid_iterate_fn fn,
>  				    void *cb_data,
>  				    const struct object_id **oid)
>  {
> +	struct odb_for_each_object_options opts = {
> +		.flags = ODB_FOR_EACH_OBJECT_PROMISOR_ONLY,
> +		.prefix_hex_len = the_repository->hash_algo->hexsz,
> +	};
> +	int err;
> +
>  	odb_reprepare(the_repository->objects);
>  	do {
> -		struct packed_git *p;
> +		opts.prefix = *oid;
>  
> -		repo_for_each_pack(the_repository, p) {
> -			if (!p->pack_promisor)
> -				continue;
> -			if (find_pack_entry_one(*oid, p))
> -				goto promisor_pack_found;
> -		}
> +		err = odb_for_each_object_ext(the_repository->objects,
> +					      NULL, promised_object_cb,
> +					      NULL, &opts);

promised_object_cb() returns 1 without any computation since we are
only interested in learning ODB_FOR_EACH_OBJECT_PROMISOR_ONLY finds
any such object.

odb_for_each_object_ext() returns 0 (if it iterates all the sources
to the end), but if its call to odb_source_for_each_object() yields
non-zero value, the returned value comes back as "err" here,
terminating the for-each iteration immediately.

odb_source_for_each_object() is implemented differently per the
source backend, but taking an example of "packfile" backend,
packfile_loose_for_each_object() ends up calling cb (wrapped in
packfile_store_for_each_object_wrapper_data) via
for_each_object_in_pack(), which stops immediately when cb returns
non-zero and the value returned from there is the value given by cb,
i.e., 1.  So we will have err==1 when we find any object.

> +		if (err < 0)
> +			return err;

And err presumably is 1 in such a case, so this does not trigger.

>  		/*
>  		 * We have found an object that is not part of a promisor pack,
>  		 * and thus we cannot skip the full connectivity check.
>  		 */
> -		return 0;
> -
> -promisor_pack_found:
> -		;
> +		if (err > 0)
> +			return 0;

And this does.

I may be misreading the patch, but as we return 0 from here, do we
cause the caller to fall back to full connectivity check?  The
caller, check_connected(), sees a zero returned from here.

>  	} while ((*oid = fn(cb_data)) != NULL);
>  
>  	return 1;