On 2020.03.26 14:11, Emily Shaffer wrote:
> On Fri, Mar 20, 2020 at 03:00:45PM -0700, Jonathan Tan wrote:
> > With 50033772d5 ("connected: verify promisor-ness of partial clone",
> > 2020-01-30), the fast path (checking promisor packs) in
> > check_connected() now passes a subset of the slow path (rev-list) - if
> > all objects to be checked are found in promisor packs, both the fast
> > path and the slow path will pass; otherwise, the fast path will
> > definitely not pass. This means that we can always attempt the fast path
> > whenever we need to do the slow path.
> >
> > The fast path is currently guarded by a flag; therefore, remove that
> > flag. Also, make the fast path fallback to the slow path - if the fast
> > path fails, the failing OID and all remaining OIDs will be passed to
> > rev-list.
>
> It looks like a pretty simple change. I had one probably-biased
> complaint about gotos below, otherwise it looks reasonable to me.
[snip]
> > diff --git a/connected.c b/connected.c
> > index 7e9bd1bc62..846f2e4eef 100644
> > --- a/connected.c
> > +++ b/connected.c
> > @@ -52,7 +52,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> > strbuf_release(&idx_file);
> > }
> >
> > - if (opt->check_refs_are_promisor_objects_only) {
> > + if (has_promisor_remote()) {
> > /*
> > * For partial clones, we don't want to have to do a regular
> > * connectivity check because we have to enumerate and exclude
> > @@ -71,13 +71,18 @@ int check_connected(oid_iterate_fn fn, void *cb_data,
> > if (find_pack_entry_one(oid.hash, p))
> > goto promisor_pack_found;
> > }
> > - return 1;
> > + /*
> > + * 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 (!fn(cb_data, &oid));
> > return 0;
> > }
> >
> > +no_promisor_pack_found:
>
> Having a look at the final structure of the loop with these gotos, I'm a
> little confused. Could be this isn't C-idiomatic but I think the code
> could be easier to read with helpers instead of gotos. I realize it's
> longer but I have a hard time understanding that your gotos are used to
> double-continue or double-break; nested loops tend to make me want to
> use helpers. But - I'm a lowly barely-reformed C++ developer, so what do
> I know ;)
>
> int oid_in_promisor(oid) {
> for (p = get_all_packs(the_repository); p; p = p->next) {
> if (!p->pack_promisor)
> continue;
> if (find_pack_entry_one(oid.hash, p)
> return 1;
> }
> }
>
> int all_oids_in_promisors(oid, fn, cb_data)
> {
> do {
> if (! oid_in_promisor(oid))
> return 0;
> } while (!fn(cb_data, &oid));
> return 1;
> }
>
> int check_connected(...)
> {
> ...
> if (has_promisor_remote()) {
> if (all_oids_in_promisors(oid, fn, cb_data))
> return 0;
> if (opt->shallow_file) {
> ...
> }
I like this version better as well.
