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

From: Stefan Beller <sbeller@google.com>
Date: May 29, 2015, 19:39 UTC
Message-ID: <CAGZ79kbm5FCjMH-vV+_ByNvAdOD2idVtd8-bizEJpZpdE9cGjQ@mail.gmail.com>
In-Reply-To: <20150527064521.GD885@peff.net>

On Tue, May 26, 2015 at 11:45 PM, Jeff King <peff@peff.net> wrote:

Show 32 quoted lines

> 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.

Show 6 quoted lines

>
>> +     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.

Show 17 quoted lines

>
>> +     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