In negotiation using protocol v2, fetch-pack sometimes does not make full use of the information obtained in the ref advertisement: specifically, that if the server advertises a commit that the client also has, the client never needs to inform the server that it has the commit's parents, since it can just tell the server that it has the advertised commit and it knows that the server can and will infer the rest. This is because, in do_fetch_pack_v2(), rev_list_insert_ref_oid() is invoked before everything_local(). This means that if we have a commit that is both our ref and their ref, it would be enqueued by rev_list_insert_ref_oid() as SEEN, and since it is thus already SEEN, everything_local() would not enqueue it. If everything_local() were invoked first, as it is in do_fetch_pack() for protocol v0, then everything_local() would enqueue it with COMMON_REF | SEEN. The addition of COMMON_REF ensures that its parents are not sent as "have" lines. Change the order in do_fetch_pack_v2() to be consistent with do_fetch_pack(), and to avoid sending unnecessary "have" lines. Signed-off-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> --- fetch-pack.c | 6 +++--- t/t5500-fetch-pack.sh | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 3 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 09f5c83c4..114207b8e 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1391,9 +1391,6 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, for_each_ref(clear_marks, NULL); marked = 1; - for_each_ref(rev_list_insert_ref_oid, NULL); - for_each_cached_alternate(insert_one_alternate_object); - /* Filter 'ref' by 'sought' and those that aren't local */ mark_complete_and_common_ref(args, &ref); filter_refs(args, &ref, sought, nr_sought); @@ -1401,6 +1398,9 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, state = FETCH_DONE; else state = FETCH_SEND_REQUEST; + + for_each_ref(rev_list_insert_ref_oid, NULL); + for_each_cached_alternate(insert_one_alternate_object); break; case FETCH_SEND_REQUEST: if (send_fetch_request(fd[1], args, ref, &common, diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index d4f435155..026ba9c9e 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -755,6 +755,45 @@ test_expect_success 'fetching deepen' ' ) ' +test_expect_success 'use ref advertisement to prune "have" lines sent' ' + rm -rf server client && + git init server && + test_commit -C server both_have_1 && + git -C server tag -d both_have_1 && + test_commit -C server both_have_2 && + + # In this test, the ref name that only the server has is a prefix of all + # other refs. This is because in protocol v2, the client sends + # "ref-prefix" to limit the ref advertisement. Naming the ref "bo" means + # that "ref-prefix refs/tags/bo*" is sent, resulting in the client also + # knowing about refs/tags/both_have_2, just as it would when it uses + # protocol v0. + git clone server client && + test_commit -C server bo && + test_commit -C client client_has && + + # In both protocol v0 and v2, ensure that the parent of both_have_2 is + # not sent as a "have" line. The client should know that the server has + # both_have_2, so it only needs to inform the server that it has + # both_have_2, and the server can infer the rest. + + rm -f trace && + cp -r client clientv0 && + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv0 \ + fetch origin bo && + grep "have $(git -C client rev-parse client_has)" trace && + grep "have $(git -C client rev-parse both_have_2)" trace && + ! grep "have $(git -C client rev-parse both_have_2^)" trace && + + rm -f trace && + cp -r client clientv2 && + GIT_TRACE_PACKET="$(pwd)/trace" git -C clientv2 -c protocol.version=2 \ + fetch origin bo && + grep "have $(git -C client rev-parse client_has)" trace && + grep "have $(git -C client rev-parse both_have_2)" trace && + ! grep "have $(git -C client rev-parse both_have_2^)" trace +' + test_expect_success 'filtering by size' ' rm -rf server client && test_create_repo server && -- 2.17.0.768.g1526ddbba1.dirty
