Re: [RFC PATCH 1/7] fetch-pack: allow NULL negotiator->add_tip

From: Jonathan Tan <jonathantanmy@google.com>
Date: Aug 7, 2020, 20:53 UTC
Message-ID: <20200807205308.2732007-1-jonathantanmy@google.com>
In-Reply-To: <xmqqo8noooni.fsf@gitster.c.googlers.com>

Show 9 quoted lines

> Jonathan Tan <jonathantanmy@google.com> writes:
> 
> > In a subsequent patch, a null fetch negotiator will be introduced. This
> > negotiator, among other things, will not need any tip information and
> > will have a NULL add_tip.
> 
> After reading the original code that the patch touches, I know what
> "a NULL add_tip" is, but it would have been nicer to readers if you
> said "callback" or "method" or "function".

OK.

Show 10 quoted lines

> With or without the "if add_tip method is set to NULL, return
> without doing anything" optimization, it does make sense to refactor
> two existing places that do identical things.  
> 
> If all the current negotiators never have NULL in the .add_tip
> field, however, I'd suspect that it would make it easier to
> understand if this step is explained as refactoring duplicated code
> into a single helper _without_ the early-return for optimization,
> and then the step that introduces the negotiator that does want to
> lack add_tip method to add the early return.

This makes sense.

> After all, if the null
> negotiator added a pointer to a no-op function to its .add_tip field,
> things would still work correctly without the early-return in the
> new helper function, no?

Right now no, because in a partial clone, I also want to avoid any iterations over refs since whenever refs are iterated, their targets are checked for existence (see the call to ref_resolves_to_object() in packed_ref_iterator_advance() in packed-refs.c). However after looking at the code again, I see that for_each_rawref() exists, which might do what I want (coupled with our own way of deref-fing tags in fetch-pack). I'll take another look and if this works, I'll just avoid the whole null method thing.