Hi Jonathan,
Thanks for taking a look at this.
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 :)
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,
