Re: [RFCv2 10/16] transport: connect_setup appends protocol version number
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 2, 2015, 22:27 UTC
- Message-ID
- <xmqqa8whzqxr.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <CAGZ79kbnX_kyuvj73PGcO7OBOj7CfdouARrqNWEkCnUfdN=DqQ@mail.gmail.com>
Stefan Beller <sbeller@google.com> writes:
Show 12 quoted lines
>> 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.
Show 5 quoted lines
>> 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.