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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

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

Hmph, but then everybody would know that it is now interacting with
upload-pack-2; I think it probably is a better way to go.

In any case, all codepaths other than what actually runs exec()
should not be basing their decision on the program names---that is
what you added transport_version field for, and they should look
at that field if they want to switch behaviour between protocol
versions anyway, so I think we can live with either way.

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

Hmm, that's an unfortunate interaction.  If you wrote a new protocol
driver that talks v2, it may be natural to say

	git fetch --upload-pack=a.out --transport-version=2

when you want to test it, and we do not want to invoke a.out-2 in
that case.

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

At least shouldn't you be able to tell if we are using compiled-in
default or user specified value (whether configuration/command line)?

Does the code that initialise data->options.{upload,receive}pack
fields with the compiled-in default values know what protocol
version is going to be used at that point?  I think that is where
the appending of "-2" should be done; in other words, I think
addition of "-2" should be done _ONLY_ for compiled-in default
values, and it should be done not just for exec() but should be
visible to everybody, including those who are debugging and inspect
data->options.uploadpack field.  What we spawn must match what is
stored there.