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

From: Junio C Hamano <[email-masked]>
Date: Wed, 22 Oct 2014 10:11:40 -0700

Dennis Kaarsemaker <[email-masked]> writes:

> On di, 2014-10-21 at 10:56 -0700, Junio C Hamano wrote:
>> Dennis Kaarsemaker <[email-masked]> 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?

The fetch-pack program tries to help the upload-pack program(s)
running on the other end find what nodes in the graph both
repositories have in common by sending what the repository on its
end has.  Some commits may not be known by the other side (e.g. your
new commits that haven't been pushed there that are made on a branch
forked from the common history), and some others may be known
(i.e. you drilled down the history from the tips of your refs and
reached a commit that you fetched from the common history
previously).  The latter are ACKed by the upload-pack process and
are remembered to be re-sent to the _next_ incarnation of the
upload-pack process when stateless RPC is in use.

With your patch, you stop telling the upload-pack process what these
two programs already found to be common in their exchange.  After
the first exchange, fetch-pack and upload-pack may have noticed that
both ends have version 2.0, but because you do not convey that fact
to the other side, the new incarnation of upload-pack may end up
deciding that the version 1.9 is the newest common commit between
the two, and sending commits between 1.9 and 2.0.

If you imagine an extreme case, it would be easy to see why "the
fetch completes" and "make test passes" are not sufficient to say
anything about this change.  Even if you break the protocol in in a
way different from your patch, by not sending any "have", such a
butchered "fetch-pack" will become "fetch everything from scratch",
aka "clone".  The end result will still have correct history and
"fetch completes" would be true.

But I'd prefer deferring a more detailed analysis/explanation to
Shawn, as stateless RPC is his creation.