> 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 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. > 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.
