Junio C Hamano <gitster@xxxxxxxxx> writes: > Jonathan Tan <jonathantanmy@xxxxxxxxxx> 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? Shot in the dark. Perhaps clearing of .add_tip and .known_common in the .next method was done to catch a wrong calling sequence where mark_complete_and_common_ref() gets called after mark_tips() and/or find_common() have by forcing the code to segfault? If so, this patch removes the safety and we may want to add an equivalent safety logic. Perhaps by adding a state field in the negotiator instance to record that mark_tips() and/or find_common() have been used and call a BUG() if mark_complete_and_common_ref() gets called after that, if enforcing such an invariant was the original reason why these fields were cleared.
