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

From: Stefan Beller <sbeller@google.com>
Date: Jun 2, 2015, 22:09 UTC
Message-ID: <CAGZ79kbnX_kyuvj73PGcO7OBOj7CfdouARrqNWEkCnUfdN=DqQ@mail.gmail.com>
In-Reply-To: <xmqq8uc123n5.fsf@gitster.dls.corp.google.com>

On Tue, Jun 2, 2015 at 2:37 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 37 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> 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;
>
>         char *to_free = NULL;
>
>> +     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;
>> +     }
>
> Hmph, so everybody else thinks it is interacting with 'upload-pack',
> and this is the only function that knows it is actually talking with
> 'upload-pack-2'?

Yes.

Show 5 quoted lines

>
> I am wondering why there isn't a separate helper function that
> munges data->options.{uploadpack,receivepack} fields based on
> the value of transport_version that is called _before_ this function
> is called.

That makes sense.

>
> Also, how does this interact with the name of the program the end
> user can specify via "fetch --upload-pack=<program name>" option?

You'd specify --upload-pack=foo-frotz and --transport-version=2 and it would look for foo-frotz-2 instead.

The problem IMHO is we have quite a few places where the upload-pack binary path can be configured. Either as a command line option or as a repository configuration.

And the way we're currently architecting the next protocol, the version is encoded in the file name, which makes sense (an old binary will not accept a new protocol), so what should happen when

* there is a repository configuration "upload-pack-custom" for upload-pack
   for historic reasons. When just switching to a new version, you would need
   to add a "upload-pack-custom-2" binary on the server side anyway

* additionally to the configured value you want to play around with the new
  protocol, so would you rather just say "--transport-version=2" or also need
  to have some sort of "--upload-pack=upload-pack-custom-another-path"
  involved? It's easy to forget the second option I believe.

* the user specifies
"--upload-pack=custom-upload-pack-which-talks-version1" and
 "--transport-version=2" together. This will fail, but at which stage do we
  want to fail?

All these questions lead me to think it's maybe better to make the rest of Git unaware of the added "-${version}" string and pretend we would be talking to upload-pack instead.