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

[Robert Coup <robert@coup.net.nz>]

Hi Jonathan,

Thanks for taking a look at this.

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 :)


diff --git a/fetch-pack.c b/fetch-pack.c
index dd67044165..870bfba267 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -1125,15 +1125,16 @@ static struct ref *do_fetch_pack(struct
fetch_pack_args *args,
        negotiator = &negotiator_alloc;
        if (is_refiltering) {
                fetch_negotiator_init_noop(negotiator);
+               filter_refs(args, &ref, sought, nr_sought);
        } else {
                fetch_negotiator_init(r, negotiator);
-       }

-       mark_complete_and_common_ref(negotiator, args, &ref);
-       filter_refs(args, &ref, sought, nr_sought);
-       if (!is_refiltering && everything_local(args, &ref)) {
-               packet_flush(fd[1]);
-               goto all_done;
+               mark_complete_and_common_ref(negotiator, args, &ref);
+               filter_refs(args, &ref, sought, nr_sought);
+               if (everything_local(args, &ref)) {
+                       packet_flush(fd[1]);
+                       goto all_done;
+               }
        }
        if (find_common(negotiator, args, fd, &oid, ref) < 0)
                if (!args->keep_pack)
@@ -1615,13 +1616,18 @@ static struct ref *do_fetch_pack_v2(struct
fetch_pack_args *args,
                        if (args->depth > 0 || args->deepen_since ||
args->deepen_not)
                                args->deepen = 1;

-                       /* Filter 'ref' by 'sought' and those that
aren't local */
-                       mark_complete_and_common_ref(negotiator, args, &ref);
-                       filter_refs(args, &ref, sought, nr_sought);
-                       if (!args->refilter && everything_local(args, &ref))
-                               state = FETCH_DONE;
-                       else
+                       if (args->refilter) {
+                               filter_refs(args, &ref, sought, nr_sought);
                                state = FETCH_SEND_REQUEST;
+                       } else {
+                               /* Filter 'ref' by 'sought' and those
that aren't local */
+
mark_complete_and_common_ref(negotiator, args, &ref);
+                               filter_refs(args, &ref, sought, nr_sought);
+                               if (everything_local(args, &ref))
+                                       state = FETCH_DONE;
+                               else
+                                       state = FETCH_SEND_REQUEST;
+                       }

                        mark_tips(negotiator, args->negotiation_tips);
                        for_each_cached_alternate(negotiator,