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

[Patrick Steinhardt <ps@pks.im>]

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