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

[Patrick Steinhardt <ps@pks.im>]

On Wed, Jun 24, 2026 at 09:27:56AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > 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.

You're right, this is a result of the refactor. Previously we had it
like this:

    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;
    }

But that made us correctly skip to the next object. Now though we have
to check for `if (!err) return 0;` in the refactored code. Makes me
wonder whether the logic would be easier to follow like this:

diff --git a/connected.c b/connected.c
index b557ff5db9..b5a9b0543d 100644
--- a/connected.c
+++ b/connected.c
@@ -13,8 +13,10 @@
 
 static int promised_object_cb(const struct object_id *oid UNUSED,
 			      struct object_info *oi UNUSED,
-			      void *payload UNUSED)
+			      void *payload)
 {
+	bool *found = payload;
+	*found = true;
 	return 1;
 }
 
@@ -45,11 +47,13 @@ static int check_connected_promisor(oid_iterate_fn fn,
 
 	odb_reprepare(the_repository->objects);
 	do {
+		bool found = false;
+
 		opts.prefix = *oid;
 
 		err = odb_for_each_object_ext(the_repository->objects,
 					      NULL, promised_object_cb,
-					      NULL, &opts);
+					      &found, &opts);
 		if (err < 0)
 			return err;
 
@@ -57,7 +61,7 @@ static int check_connected_promisor(oid_iterate_fn fn,
 		 * We have found an object that is not part of a promisor pack,
 		 * and thus we cannot skip the full connectivity check.
 		 */
-		if (err > 0)
+		if (!found)
 			return 0;
 	} while ((*oid = fn(cb_data)) != NULL);
 

It's also a bit concerning that this doesn't cause any tests to fail.
I'll try to figure out whether I can add one.

Thanks!

Patrick