| Message ID | 8499d5268e49b6b823b0b9312d77e41e311d9a75.1587775989.git.jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Protocol v2 in_vain fixes | expand |
Jonathan Tan wrote: > --- a/t/t5500-fetch-pack.sh > +++ b/t/t5500-fetch-pack.sh > @@ -404,6 +404,36 @@ test_expect_success 'in_vain not triggered before first ACK' ' > 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 Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> Thanks. As with the other patch, trace2 output from the server might be more stable. Do these tests pass with protocol v0 as well? (Forgive my laziness in not checking yet.)
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com> > > Thanks. > > As with the other patch, trace2 output from the server might be more > stable. > > Do these tests pass with protocol v0 as well? (Forgive my laziness in > not checking yet.) Thanks for your review. Yes they pass with protocol v0 too (checked by passing "-c protocol.version=0" when fetching).
diff --git a/fetch-pack.c b/fetch-pack.c index 7d15c7af81..8551c07ed9 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1287,6 +1287,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); diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index fcc5cc8ab0..98e1442cbf 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh @@ -404,6 +404,36 @@ test_expect_success 'in_vain not triggered before first ACK' ' 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 &&
In the function process_acks() in fetch-pack.c, the variable received_ack is meant to track that an ACK was received, but it was never set. This results in negotiation terminating prematurely through the in_vain counter, when the counter should have been reset upon every ACK. Therefore, reset the in_vain counter upon every ACK. Helped-by: Jonathan Nieder <jrnieder@gmail.com> Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- fetch-pack.c | 1 + t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+)