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

> On Wed, Oct 22, 2014 at 10:11:40AM -0700, Junio C Hamano wrote:
>> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>>
>> > On di, 2014-10-21 at 10:56 -0700, Junio C Hamano wrote:
>> >> 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?
>>
>> 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.

> Thanks for the explanation, that makes it quite clear that this approach
> is wrong. The patch below (apologies for the formatting, I'm not quite
> sure how to use format-patch in this situation) does it differently: by
> buffering upload-pack's output until it has read all the input, the new
> test still succeeds and again 'make test' passes.

> @@ -384,15 +385,19 @@ static int get_common_commits(void)
>                         if (multi_ack == 2 && got_common
>                             && !got_other && ok_to_give_up()) {
>                                 sent_ready = 1;
> -                               packet_write(1, "ACK %s ready\n", last_hex);
> +                               packet_buf_write(&resp_buf, "ACK %s ready\n", last_hex);
>                         }
>                         if (have_obj.nr == 0 || multi_ack)
> -                               packet_write(1, "NAK\n");
> +                               packet_buf_write(&resp_buf, "NAK\n");