Robert Coup <robert@xxxxxxxxxxx> writes: > On Fri, 4 Feb 2022 at 18:02, Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote: > > > > The approach that I would have expected is to not call > > mark_complete_and_common_ref(), filter_refs(), everything_local(), and > > find_common(), but your approach here is to ensure that > > mark_complete_and_common_ref() and find_common() do not do anything. > > v0: find_common() definitely still does something: during refiltering it skips > checking the local object db, but it's still responsible for sending > the "wants". > > filter_refs() is necessary under v0 & v2 so the remote refs all get marked as > matched, otherwise we end up erroring after the transfer with > "error: no such remote ref refs/heads/main" etc. > > > Comparing the two approaches, the advantage of yours is that we only > > need to make the change once to support both protocol v0 and v2 > > (although the change looks more substantial than just skipping function > > calls), but it makes the code more difficult to read in that we now have > > function calls that do nothing. What do you think about my approach? > > My original approach was to leave the negotiator in place, and just conditional > around it in do_fetch_pack_v2 — this worked ok for protocol v2 but the v0 > implementation didn't work. After that I switched to forcing noop in [1/6] > which made both implementations match (& tidier imo). > > To make the test pass and skip those calls I need a patch like the below > — filter_refs() is still required during refiltering for the ref-matching. To me > this looks more complicated, but I'm happy to defer to your thinking. > > Thanks, > > Rob :) Thanks for investigating this; looks like I was perhaps too optimistic in thinking that the code could be reorganized in the way I'm suggesting. I think that it's worth checking if we could refactor the parts that are needed with --refilter out of filter_refs() and find_common() (and in doing so, make these functions do only what their names imply). I don't have time to do so right now, but I don't want to unnecessarily block this patch set either, so I'll say that ideally another reviewer (or you) would do that investigation, but I have no objection to merging this patch set in this state (other than the change of the argument name, perhaps to "--repair", and associated documentation).
