Re: Git 2.26 fetches many times more objects than it should, wasting gigabytes

[Jonathan Tan <jonathantanmy@google.com>]

> On Wed, Apr 22, 2020 at 06:30:11AM -0400, Jeff King wrote:
> 
> > So it really just seems like v2 does not try hard enough. I think the
> > culprit is the MAX_IN_VAIN setting. If I do this:
> > 
> > diff --git a/fetch-pack.c b/fetch-pack.c
> > index 1734a573b0..016a413d49 100644
> > --- a/fetch-pack.c
> > +++ b/fetch-pack.c
> > @@ -46,7 +46,7 @@ static struct strbuf fsck_msg_types = STRBUF_INIT;
> >   * After sending this many "have"s if we do not get any new ACK , we
> >   * give up traversing our history.
> >   */
> > -#define MAX_IN_VAIN 256
> > +#define MAX_IN_VAIN 20000
> >  
> >  static int multi_ack, use_sideband;
> >  /* Allow specifying sha1 if it is a ref tip. */
> > 
> > then I get that same 48k objects, 23MB fetch that v0 does.
> 
> I don't quite think that's the solution, though. Both old and new are
> supposed to be respecting MAX_IN_VAIN. So it's not at all clear to me
> why it restricts the number of haves we'll send in v2, but not in v0.
> 
> Maybe somebody more familiar with the negotiation code can comment
> further.

Thanks for the reproduction recipe (in [1]) and your analysis. I took a
look, and it's because the check for in_vain is done differently. In v0:

  if (got_continue && MAX_IN_VAIN < in_vain) {

reflecting the documentation in pack-protocol.txt:

  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.

(Note that both the code and the documentation call it "continue", but
the code also correctly handles multi_ack_detailed, which instructs the
server to send "ACK common" and "ACK ready" in lieu of "ACK continue".)

When debugging, I noticed that in_vain was increasing far in excess of
MAX_IN_VAIN, but because got_continue was false, the client did not give
up.

But in v2:

  if (!haves_added || *in_vain >= MAX_IN_VAIN) {

("haves_added" is irrelevant to this discussion. It is another
termination condition - when we have run out of "have"s to send.)

So there is no check that "continue" was sent. We probably should change
v2 to match v0. I can start writing a patch unless someone else would
like to take a further look at it.

[1] https://lore.kernel.org/git/20200422095702.GA475060@coredump.intra.peff.net/