diff options
author | Junio C Hamano <gitster@pobox.com> | 2020-05-01 23:40:00 +0300 |
---|---|---|
committer | Junio C Hamano <gitster@pobox.com> | 2020-05-01 23:40:00 +0300 |
commit | 0b07eecf6ed9334f09d6624732a4af2da03e38eb (patch) | |
tree | dc9a051dc91ba996bae50b24e6e0169a1d339375 | |
parent | 2c42fb76531f4565b5434e46102e6d85a0861738 (diff) | |
parent | 2f0a093dd640e0dad0b261dae2427f2541b5426c (diff) |
Merge branch 'jt/v2-fetch-nego-fix'
The upload-pack protocol v2 gave up too early before finding a
common ancestor, resulting in a wasteful fetch from a fork of a
project. This has been corrected to match the behaviour of v0
protocol.
* jt/v2-fetch-nego-fix:
fetch-pack: in protocol v2, reset in_vain upon ACK
fetch-pack: in protocol v2, in_vain only after ACK
fetch-pack: return enum from process_acks()
-rw-r--r-- | fetch-pack.c | 50 | ||||
-rwxr-xr-x | t/t5500-fetch-pack.sh | 48 |
2 files changed, 86 insertions, 12 deletions
diff --git a/fetch-pack.c b/fetch-pack.c index 8e98b3d4a5..f73a2ce6cb 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1143,6 +1143,7 @@ static void add_common(struct strbuf *req_buf, struct oidset *common) } static int add_haves(struct fetch_negotiator *negotiator, + int seen_ack, struct strbuf *req_buf, int *haves_to_send, int *in_vain) { @@ -1157,7 +1158,7 @@ static int add_haves(struct fetch_negotiator *negotiator, } *in_vain += haves_added; - if (!haves_added || *in_vain >= MAX_IN_VAIN) { + if (!haves_added || (seen_ack && *in_vain >= MAX_IN_VAIN)) { /* Send Done */ packet_buf_write(req_buf, "done\n"); ret = 1; @@ -1173,7 +1174,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, struct fetch_pack_args *args, const struct ref *wants, struct oidset *common, int *haves_to_send, int *in_vain, - int sideband_all) + int sideband_all, int seen_ack) { int ret = 0; struct strbuf req_buf = STRBUF_INIT; @@ -1230,7 +1231,8 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out, add_common(&req_buf, common); /* Add initial haves */ - ret = add_haves(negotiator, &req_buf, haves_to_send, in_vain); + ret = add_haves(negotiator, seen_ack, &req_buf, + haves_to_send, in_vain); } /* Send request */ @@ -1268,9 +1270,29 @@ static int process_section_header(struct packet_reader *reader, return ret; } -static int process_acks(struct fetch_negotiator *negotiator, - struct packet_reader *reader, - struct oidset *common) +enum common_found { + /* + * No commit was found to be possessed by both the client and the + * server, and "ready" was not received. + */ + NO_COMMON_FOUND, + + /* + * At least one commit was found to be possessed by both the client and + * the server, and "ready" was not received. + */ + COMMON_FOUND, + + /* + * "ready" was received, indicating that the server is ready to send + * the packfile without any further negotiation. + */ + READY +}; + +static enum common_found process_acks(struct fetch_negotiator *negotiator, + struct packet_reader *reader, + struct oidset *common) { /* received */ int received_ready = 0; @@ -1285,6 +1307,7 @@ static int process_acks(struct fetch_negotiator *negotiator, if (skip_prefix(reader->line, "ACK ", &arg)) { struct object_id oid; + received_ack = 1; if (!get_oid_hex(arg, &oid)) { struct commit *commit; oidset_insert(common, &oid); @@ -1319,8 +1342,8 @@ static int process_acks(struct fetch_negotiator *negotiator, if (!received_ready && reader->status != PACKET_READ_FLUSH) die(_("expected no other sections to be sent after no 'ready'")); - /* return 0 if no common, 1 if there are common, or 2 if ready */ - return received_ready ? 2 : (received_ack ? 1 : 0); + return received_ready ? READY : + (received_ack ? COMMON_FOUND : NO_COMMON_FOUND); } static void receive_shallow_info(struct fetch_pack_args *args, @@ -1444,6 +1467,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, int haves_to_send = INITIAL_FLUSH; struct fetch_negotiator negotiator_alloc; struct fetch_negotiator *negotiator; + int seen_ack = 0; if (args->no_dependents) { negotiator = NULL; @@ -1500,7 +1524,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, if (send_fetch_request(negotiator, fd[1], args, ref, &common, &haves_to_send, &in_vain, - reader.use_sideband)) + reader.use_sideband, + seen_ack)) state = FETCH_GET_PACK; else state = FETCH_PROCESS_ACKS; @@ -1508,13 +1533,14 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, case FETCH_PROCESS_ACKS: /* Process ACKs/NAKs */ switch (process_acks(negotiator, &reader, &common)) { - case 2: + case READY: state = FETCH_GET_PACK; break; - case 1: + case COMMON_FOUND: in_vain = 0; + seen_ack = 1; /* fallthrough */ - default: + case NO_COMMON_FOUND: state = FETCH_SEND_REQUEST; break; } diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index baa1a99f45..52dd1a688c 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -385,6 +385,54 @@ test_expect_success 'clone shallow with packed refs' ' test_cmp count8.expected count8.actual ' +test_expect_success 'in_vain not triggered before first ACK' ' + rm -rf myserver myclient trace && + git init myserver && + test_commit -C myserver foo && + git clone "file://$(pwd)/myserver" myclient && + + # MAX_IN_VAIN is 256. Because of batching, the client will send 496 + # (16+32+64+128+256) commits, not 256, before giving up. So create 496 + # irrelevant commits. + test_commit_bulk -C myclient 496 && + + # The new commit that the client wants to fetch. + test_commit -C myserver bar && + + GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin && + test_i18ngrep "Total 3 " trace +' + +test_expect_success 'in_vain resetted upon ACK' ' + rm -rf myserver myclient trace && + git init myserver && + + # Linked list of commits on master. The first is common; the rest are + # not. + test_commit -C myserver first_master_commit && + git clone "file://$(pwd)/myserver" myclient && + test_commit_bulk -C myclient 255 && + + # Another linked list of commits on anotherbranch with no connection to + # master. The first is common; the rest are not. + git -C myserver checkout --orphan anotherbranch && + test_commit -C myserver first_anotherbranch_commit && + git -C myclient fetch origin anotherbranch:refs/heads/anotherbranch && + git -C myclient checkout anotherbranch && + test_commit_bulk -C myclient 255 && + + # The new commit that the client wants to fetch. + git -C myserver checkout master && + test_commit -C myserver to_fetch && + + # The client will send (as "have"s) all 256 commits in anotherbranch + # first. The 256th commit is common between the client and the server, + # and should reset in_vain. This allows negotiation to continue until + # the client reports that first_anotherbranch_commit is common. + GIT_TRACE_PACKET="$(pwd)/trace" git -C myclient fetch --progress origin master && + test_i18ngrep "Total 3 " trace +' + test_expect_success 'fetch in shallow repo unreachable shallow objects' ' ( git clone --bare --branch B --single-branch "file://$(pwd)/." no-reflog && |