| Message ID | eb18faea2d00d9b09d3fcc79646cc7556023c258.1588031728.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Protocol v2 in_vain fixes | expand |
Jonathan Tan wrote: > Signed-off-by: Jonathan Tan <jonathantanmy@google.com> > --- > fetch-pack.c | 13 +++++++++---- > t/t5500-fetch-pack.sh | 18 ++++++++++++++++++ > 2 files changed, 27 insertions(+), 4 deletions(-) Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Hi Jonathan, On Mon, 27 Apr 2020, Jonathan Tan wrote: > diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh > index baa1a99f45..961cd6beec 100755 > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -385,6 +385,24 @@ 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 This just failed in one of the Pipelines I monitor: https://github.com/git-for-windows/git-sdk-64/runs/648253955?check_suite_focus=true The short of it is: -- snip -- [...] packet: sideband< \2Enumerating objects: 4, done. packet: sideband< \2Counting objects: 25% (1/4)\15Counting objects: 50% (2/4)\15Counting objects: 75% (3/4)\15Counting objects: 100% (4/4)\15Counting obj packet: sideband< \2ects: 100% (4/4), done.Compressing objects: 50% (1/2)\15Compressing objects: 100% (2/2)\15Compressing objects: 100% (2/2), done.T packet: sideband< \2otal 3 (delta 0), reused 0 (delta 0), pack-reused 0 packet: sideband< PACK ... packet: upload-pack> 0000 packet: sideband< 0000 ++ return 1 error: last command exited with $?=1 t/t5500-fetch-pack.sh:388: error: not ok 43 - 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 -- snap -- In other words, that `test_i18ngrep` assumes incorrectly that "Total 3" will be found in the trace, but the sideband is totally allowed to cut through it and deliver it in different packets. This makes t5500 flaky. Ciao, Johannes > +' > + > test_expect_success 'fetch in shallow repo unreachable shallow objects' ' > ( > git clone --bare --branch B --single-branch "file://$(pwd)/." no-reflog && > -- > 2.26.2.303.gf8c07b1a785-goog > > >
diff --git a/fetch-pack.c b/fetch-pack.c index 45547a621e..76691dc6c0 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 */ @@ -1465,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; @@ -1521,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; @@ -1534,6 +1538,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, break; case COMMON_FOUND: in_vain = 0; + seen_ack = 1; /* fallthrough */ case NO_COMMON_FOUND: state = FETCH_SEND_REQUEST; diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index baa1a99f45..961cd6beec 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -385,6 +385,24 @@ 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 'fetch in shallow repo unreachable shallow objects' ' ( git clone --bare --branch B --single-branch "file://$(pwd)/." no-reflog &&
When fetching, Git stops negotiation when it has sent at least MAX_IN_VAIN (which is 256) "have" lines without having any of them ACK-ed. But this is supposed to trigger only after the first ACK, as pack-protocol.txt says: However, the 256 limit *only* turns on in the canonical client implementation if we have received at least one "ACK %s continue" during a prior round. This helps to ensure that at least one common ancestor is found before we give up entirely. The code path for protocol v0 observes this, but not protocol v2, resulting in shorter negotiation rounds but significantly larger packfiles. Teach the code path for protocol v2 to check this criterion only after at least one ACK was received. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- fetch-pack.c | 13 +++++++++---- t/t5500-fetch-pack.sh | 18 ++++++++++++++++++ 2 files changed, 27 insertions(+), 4 deletions(-)