Re: [RFC PATCH 2/7] fetch-pack: allow NULL negotiator->known_common

> 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 information about
>> common objects and will have a NULL known_common. Teach fetch-pack to
>> allow this.
>
> Hmph, both the default and the skipping negotiator seem to put NULL
> in known_common and add_tip when its next() method is called.  Also
> they clear known_common to NULL after add_tip is called even once.
>
> So, how have we survived so far without this patch to "allow this
> (i.e.  known_common method to be NULL)"?  Is there something that
> makes sure a negotiator never gets called from this function after
> its .next or .add_tip method is called?
>
> Puzzled.  Or is this merely an optimization?  If so, it's not like
> the change "allows this", but it starts to take advantage of it in
> some way.
>
> 	... goes and looks at mark_complete_and_common_ref()
>
> The function seems to have an unconditional call to ->known_common(),
> so anybody passing a negotiator whose known_common is NULL would
> already be segfaulting, so this does not appear to be an optimization
> but necessary to keep the code from crashing.  I cannot quite tell
> if it is avoiding unnecessary work, or sweeping crashes under the
> rug, though.  
>
> Is the untold assumption that mark_complete_and_common_ref() will
> never be called after either mark_tips() or find_common() have been
> called?