Re: [RFC/WIP PATCH 06/11] remote.h: add get_remote_capabilities, request_capabilities

[Stefan Beller <sbeller@google.com>]

On Tue, May 26, 2015 at 11:45 PM, Jeff King <peff@peff.net> wrote:
> On Tue, May 26, 2015 at 03:01:10PM -0700, Stefan Beller wrote:
>
>> +void get_remote_capabilities(int in, char *src_buf, size_t src_len)
>> +{
>> +     struct strbuf capabilities_string = STRBUF_INIT;
>> +     for (;;) {
>> +             int len;
>> +             char *line = packet_buffer;
>> +             const char *arg;
>> +
>> +             len = packet_read(in, &src_buf, &src_len,
>> +                               packet_buffer, sizeof(packet_buffer),
>> +                               PACKET_READ_GENTLE_ON_EOF |
>> +                               PACKET_READ_CHOMP_NEWLINE);
>> +             if (len < 0)
>> +                     die_initial_contact(0);
>> +
>> +             if (!len)
>> +                     break;
>> +
>> +             if (len > 4 && skip_prefix(line, "ERR ", &arg))
>> +                     die("remote error: %s", arg);
>> +
>> +             if (starts_with(line, "capability:")) {
>> +                     strbuf_addstr(&capabilities_string, line + strlen("capability:"));
>> +                     strbuf_addch(&capabilities_string, ' ');
>> +             }
>> +     }
>
> I think this is the reverse case of next_capabilities in the upload-pack
> side, so I'll make the reverse suggestion. :) Would it make things nicer
> if both v1 and v2 parsed the capabilities into a string_list?

Ok, I'll do that. Though this makes future enhancements a bit uneasy.
Say we want to transport a message by the server admins, this might be
the right place to do.

    if (starts_with("message"))
        fprintf(stderr, ....

Of course we can later add this in the future, but it would break older
clients (clients as of this patch series). That's why I like the idea of
adding a prefix here. Maybe just a "c:" as an abbreviation for capability.
But now making it short and concise makes it painful in the future when
we want to transport anything else apart from capabilities in this phase
of the protocol exchange. Of course we could have a capability "server-message"
indicating that after the capabilities we'll have a dedicated message to be
displayed to the user.

Yeah well that should do.

I'll just parse in a string_list for now.

>
>> +     free(server_capabilities);
>> +     server_capabilities = xmalloc(capabilities_string.len + 1);
>> +     server_capabilities = strbuf_detach(&capabilities_string, NULL);
>
> Is this xmalloc line left over? The strbuf_detach will write over it.

No, I wasn't reading the fine documentation and assuming things I
should not.

>
>> +     strbuf_release(&capabilities_string);
>
> No need to release if we just detached.
>
>> +int request_capabilities(int out)
>> +{
>> +     fprintf(stderr, "request_capabilities\n");
>
> Debug cruft, I presume.
>
>> +     // todo: send our capabilities
>> +     packet_write(out, "capability:foo");
>
> No C99 comments. But I think this is just a placeholder. :)
>
> -Peff