> > 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? [snip] > > 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? Ah...yes, if I remember correctly, that was my original intention when I set them to NULL. > 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. Sounds good. As I said in my reply to your query on patch 1, we might not need to set NULL anymore, but if we do, I'll do this.
