Re: [PATCH 1/2] fetch-pack: in protocol v2, in_vain only after ACK
From: Jonathan Nieder <hidden>
Date: 2020-04-26 00:28:07
Hi, Jonathan Tan wrote:
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 <redacted> --- fetch-pack.c | 13 +++++++++---- t/t5500-fetch-pack.sh | 19 +++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-)
Makes a lot of sense. Good find. [...]
quoted hunk ↗ jump to hunk
--- a/fetch-pack.c +++ b/fetch-pack.c
[...]
quoted hunk ↗ jump to hunk
@@ -1513,6 +1517,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args, break; case 1: in_vain = 0; + seen_ack = 1;
not about this patch: can these return values from process_acks be made into an enum with named enumerators? That would make what's happening in the call site more obvious.
quoted hunk ↗ jump to hunk
/* fallthrough */ default: state = FETCH_SEND_REQUEST;diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh index baa1a99f45..fcc5cc8ab0 100755 --- a/t/t5500-fetch-pack.sh +++ b/t/t5500-fetch-pack.sh@@ -385,6 +385,25 @@ 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 && + cp trace /tmp/x &&
Leftover debugging line?
+ test_i18ngrep "Total 3 " trace
Clever. In some sense this is a fragile test, since the server could change how it reports progress some day. Would it make sense (perhaps as a followup patch) for this to use a trace2 log instead? For example, if we turn on tracing in the server, then since 9ed8790282 (pack-objects: write objects packed to trace2, 2019-04-11) it will report how many objects were in the pack it wrote. After removing the "cp trace /tmp/x" line, Reviewed-by: Jonathan Nieder <redacted> Thanks.