Re: [RFCv2 10/16] transport: connect_setup appends protocol version number
- From
Stefan Beller <sbeller@google.com>
- Date
- Jun 2, 2015, 18:04 UTC
- Message-ID
- <CAGZ79kYg0W7biSXV1VH8NKcG=YNpotvbB7GD5-JAg8hEq0LOuw@mail.gmail.com>
- In-Reply-To
- <CACsJy8Dped78Db0Pb455-dxha4aaQnSWDN_TwRpe9miVmwHcjg@mail.gmail.com>
On Tue, Jun 2, 2015 at 2:58 AM, Duy Nguyen <pclouds@gmail.com> wrote:
Show 40 quoted lines
> 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.