Re: [PATCH] fetch-pack: don't resend known-common refs in find_common

From: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Date: Oct 22, 2014, 07:41 UTC
Message-ID: <1413963706.11656.5.camel@seahawk>
In-Reply-To: <xmqqd29l1f3p.fsf@gitster.dls.corp.google.com>

On di, 2014-10-21 at 10:56 -0700, Junio C Hamano wrote:

Show 16 quoted lines

> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > By not clearing the request buffer in stateless-rpc mode, fetch-pack
> > would keep sending already known-common commits, leading to ever bigger
> > http requests, eventually getting too large for git-http-backend to
> > handle properly without filling up the pipe buffer in inflate_request.
> > ---
> > I'm still not quite sure whether this is the right thing to do, but make
> > test still passes :) The new testcase demonstrates the problem, when
> > running t5551 with EXPENSIVE, this test will hang without the patch to
> > fetch-pack.c and succeed otherwise.
> 
> IIUC, because "stateless" is just that, i.e. the server-end does not
> keep track of what is already known, not telling what is known to be
> common in each request would fundamentally break the protocol.  Am I
> mistaken?

That sounds plausible, but why then does the fetch complete with this line removed, and why does 'make test' still pass? I tried to understand the protocol, but the documentation has TODO's in some critical places :)

And if that's true, it means the inflate_request / upload-pack interaction should be fixed, so more than 64k (current linux pipe buffer size) of uncompressed data is supported. I see two options:

* Turning that interaction into a more cooperative process, with a
  select/poll loop
* Make upload-pack buffer its entire response when run in stateless_rpc
  mode until it has consumed all of the request

The latter sounds easier to do, but not being very familiar with the protocol, I may have missed something obvious.

-- 
Dennis Kaarsemaker
http://www.kaarsemaker.net