Re: [RFC/WIP PATCH 00/11] Protocol version 2, again!

From: Stefan Beller <sbeller@google.com>
Date: Jun 1, 2015, 17:49 UTC
Message-ID: <CAGZ79kZxFnkneixquUijd2yfKBh6+XnYiYzCh8L9Mkourh64Fw@mail.gmail.com>
In-Reply-To: <20150527070838.GA6873@peff.net>

On Wed, May 27, 2015 at 12:08 AM, Jeff King <peff@peff.net> wrote:

Show 13 quoted lines

> On Wed, May 27, 2015 at 02:18:18AM -0400, Jeff King wrote:
>
>> > The new protocol works just like the old protocol, except for
>> > the capabilities negotiation being before any exchange of real data.
>>
>> I like this approach. [...]
>
> So now I've read through all the patches. I still like it. :)
>
> There's a lot of work to be done still, but I think this is a great
> start. Thanks for getting the ball rolling.
>
> -Peff

Thanks for the reviews. I think I got them all covered by now and I am pretty happy about the upload-pack part for now.

However the client side with builtin/fetch, builtin/fetch-pack, fetch-pack
is a bit of a mystery to me, as I cannot fully grasp the difference between
 * connect.{h,c}
 * remote.{h.c}
 * transport.{h.c}
there. All of it seems to be doing network related stuff, but I have trouble
getting the big picture. I am assuming all of these 3 are rather a low level,
used like a library, though there must be even more hierarchy in there,
connect is most low level judging from the header file and used by
the other two.
transport.h seems to provide the most toplevel library stuff as it includes
remote.h in its header?

The problem I am currently trying to tackle, is passing the options through all the layers early on. so in a few places we have code like

    switch (version) {
    case 2: /* first talk about capabilities, then get the heads */
        get_remote_capabilities(data->fd[0], NULL, 0);
        select_capabilities();
        request_capabilities(data->fd[1]);
        /* fall through */
    case 1:
        get_remote_heads(data->fd[0], NULL, 0, &refs,
                 for_push ? REF_NORMAL : 0,
                 &data->extra_have,
                 &data->shallow);
        break;
    default:
        die("BUG: Transport version %d not supported", version);
        break;
    }

and the issue I am concerned about is the select_capabilities as well as the request_capabilities function here. The select_capabilities functionality is currently residing in the high level parts of the code as it both depends on the advertised server capabilities and on the user input (--use-frotz or config options), so the capability selection is done in fetchpack.c

So there are 2 routes to go: Either we leave the select_capabilities in the upper layers (near the actual high level command, fetch, fetchpack) or we put it into the transport layer and just passing in a struct what the user desires. And when the users desire doesn't meet the server capabilities we die deep down in the transport layer.

Any advice on how to proceed there welcome!

Thanks, Stefan