Re: [RFCv2 10/16] transport: connect_setup appends protocol version number

[Stefan Beller <sbeller@google.com>]

On Tue, Jun 2, 2015 at 2:58 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Jun 2, 2015 at 7:02 AM, Stefan Beller <sbeller@google.com> wrote:
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>>
>> Notes:
>>     name it to_free
>>
>>  transport.c | 17 +++++++++++++++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/transport.c b/transport.c
>> index 651f0ac..b49fc60 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -496,15 +496,28 @@ static int set_git_option(struct git_transport_options *opts,
>>  static int connect_setup(struct transport *transport, int for_push, int verbose)
>>  {
>>         struct git_transport_data *data = transport->data;
>> +       const char *remote_program;
>> +       char *to_free = 0;
>>
>>         if (data->conn)
>>                 return 0;
>>
>> +       remote_program = (for_push ? data->options.receivepack
>> +                                  : data->options.uploadpack);
>> +
>> +       if (transport->smart_options->transport_version >= 2) {
>> +               to_free = xmalloc(strlen(remote_program) + 12);
>> +               sprintf(to_free, "%s-%d", remote_program,
>> +                       transport->smart_options->transport_version);
>> +               remote_program = to_free;
>> +       }
>> +
>
> It looks to me that the caller should pass "upload-pack-2" here in
> data->options.uploadpack already. We should not need to manipulate the
> uploadpack's program name. Not sure how complicated it would be
> though.
>

I tried that before as it seemed to be the better approach to me. But
there are multiple
occasions where you can overwrite the "upload-pack" string. It can be
a repository
option or globally configured or coming from a command line argument.

And keeping track of all these places and both passing around the
version number as
well as the binary name seemed cumbersome to me when seeing my implementation.
This way we only have a very small change and you can tell the version
number from
the name, which is an advantage (the version is fixed and will not be
negotiable, so you
need some way to tell the protocol version and the name seems to be
the obvious choice).

So if there are no strong arguments for doing it the other way, I'd
like to keep it this way.