Re: [PATCH] connected: always use partial clone optimization

From: Jeff King <peff@peff.net>
Date: Mar 30, 2020, 13:37 UTC
Message-ID: <20200330133714.GA2410648@coredump.intra.peff.net>
In-Reply-To: <20200326211156.GA37946@google.com>

On Thu, Mar 26, 2020 at 02:11:56PM -0700, Emily Shaffer wrote:

Show 35 quoted lines

> 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 agree the current code is a bit hard to follow, and I do like the separation you gave here. But there's something subtle going on in the current code with the fn() callback. It lets us iterate forward over the list, but we can never seek back to the beginning to reset. Yet we have multiple loops calling fn().

This works in the current code because we do something like:

  fn(&oid);
  do {
    /* check for oid in promisor packs; if not, jump to rev-list below */
  } while(!fn(&oid));

  start_command(&rev_list);
  do {
    /* feed oid to rev-list */
  } while(!fn(&oid));

The two subtle things are:

 - we keep the cursor in the same spot after breaking out of the
   promisor loop, so that we don't feed any promisor objects to rev-list

 - by using the same local object_id buffer, when we break out of the
   promisor-checking loop, our do-while will pick up where we left off,
   and not miss checking that first object

I'd worry that splitting the two loops across separate functions would make it easier for that second thing to be forgotten. On the other hand, maybe pulling out some of the details into loops would make the dependency more clear (because there'd be less intervening cruft).

-Peff