Re: [PATCH 2/6] fetch-pack: add partial clone refiltering

[Jonathan Tan <jonathantanmy@google.com>]

Robert Coup <robert@coup.net.nz> writes:
> On Fri, 4 Feb 2022 at 18:02, Jonathan Tan <jonathantanmy@google.com> 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).