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

On Thu, Jun 4, 2015 at 6:09 AM, Jeff King <peff@peff.net> wrote:

Currently the distinction which protocol to speak is made here (in get_refs_via_connect), which may be a bit late. Though I updating the git protocol only first would also be feasible.

So for the next git protocol

 - get_refs_via_connect first asks for the capabilities and gets an answer from
   upload-pack-2. Now what?

 - we could have a callback in struct transport, which must be set
accordingly by
   fetch in step 4 (it turns out to be a git URL. transport.c ...)
   This callback is called with each pkt-line such as

        void parse-capability(char *line, struct
*transport_capabilities, void *cdata);

The line would contain the pkt-line, while the transport_capabilities would be a struct similar as in "[RFCv2 06/16] remote.h: add new struct for options", where the fetch implementation must select the right bits. Looking at fetch-pack.{c,h} we only expose one do-it-all method there, so we currently don't have file wide easily accessible variables, but rather all in a struct fetch_pack_args, which carries important information for the selection process such as verbosity or desired options. This is why a void* comes in handy as well. (It will be easy later to adapt that to the sending side as well).

Instead of a full grown line by line callback we could also just collect all the capabilities first in a string list and then only call back once into a

    void select_capabilities(struct string_list *available, struct
string_list *selected);

I think I'd find this second approach more handy as there are subtle details you'd miss in the first approach. Looking at fetch-pack.c, do_fetch_pack (line 790), we have one selection (no_done) conditioned on another (multi_ack_detailed), so having the full list there makes the code easier.

This second approach however might not be as future proof as the first, because we store all received capabilities (which may grow large in the future) and not throw unknowns away immediately.

I tend to rather implement the second one (easier to read/maintain trumps a maybe-performance-problem-in-the-future).

This performance-problem-in-the-future could be mitigated easily by having a preselection in transport.c get_capabilities, which ignores any capabilities not white listed there (harder to maintain though, as we have a more than one spot where to put a list)

By writing this mail I realized another thing. I have had the patch
    "[RFCv2 09/16] remote.h: add get_remote_capabilities, request_capabilities"
which has request_capabilities just translating from a struct
containing some bits
into a sequence of pkt-lines containing the actual protocol
capabilities. Maybe we
should not have that in the connect file, but rather as proposed in
this email, the
high level command directly selects the strings to put back on the
wire. (By having
"struct string_list *selected" as part of the select_capabilities arguments.)
then the request_capabilities in connect.c would be dumbed down to just:

    void request_capabilities(int out, struct string_list *list)
    {
        struct string_list_item *item;
        for_each_string_list_item(item, list) {
             packet_write(out, item->string);
        }
        packet_flush(out);
    }

I think that would be reasonable?

Okay