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

[Jonathan Tan <jonathantanmy@google.com>]

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> >> Sounds good.  Jonathan?  I've squashed Josh'es Reviewed-by, but I
> >> will refrain from merging it to 'next' just yet to see if you too
> >> like the proposed code structure.
> >
> > I think that this is a local enough concern that going either way won't
> > paint us into a corner, so if what's in
> > jt/connectivity-check-optim-in-partial-clone is OK, I prefer using that
> > to reduce churn.
> 
> If you do not think their improvement is not much of improvement,
> then please say so.

Yes, I don't think that their improvement is much of an improvement. If
we were to split up the logic into functions, one of the functions would
need to be documented as "Return true if all objects returned by 'fn'
exist in promisor packs. If one of them does not, return false; the ID
of the failing object is then stored in 'oid', and 'fn' can be used to
obtain all untested objects." Peff said something similar in [1].

So I think it's better to inline the logic, even if we have to use
"goto", so that we can see all of this in one place.

[1] https://lore.kernel.org/git/20200330133714.GA2410648@coredump.intra.peff.net/

> On the other hand, if you also believe that the
> resulting code is better, adopting the improvement (in other words,
> help from other people) and get a better version of the code queued
> before it hits 'next' is not a "churn".  Leaving a chance to make
> the code into a shape that you think better is a waste and risking
> the chance to forget improving it with a follow-up patch.

I agree.