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 <jonathantanmy@xxxxxxxxxx> > --- > fetch-pack.c | 13 +++++++++---- > t/t5500-fetch-pack.sh | 19 +++++++++++++++++++ > 2 files changed, 28 insertions(+), 4 deletions(-) Makes a lot of sense. Good find. [...] > --- a/fetch-pack.c > +++ b/fetch-pack.c [...] > @@ -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. > /* 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 <jrnieder@xxxxxxxxx> Thanks.
