On Sat, Mar 24, 2018 at 1:55 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Loganaden Velvindron <lo...@hackers.mu> writes:
>
>> Subject: Re: [PATCH v2] Allow use of TLS 1.3
>
> Let's retitle it to something like
>
> Subject: [PATCH v2] http: allow use of TLS 1.3
>
>> Add a tlsv1.3 option to http.sslVersion in addition to the existing
>> tlsv1.[012] options. libcurl has supported this since 7.52.0.
>
> Good.
>
>>
>> Done during IETF 101 Hackathon
>
> I am on the fence wrt the value of this line, especially because I
> would strongly suspect that this version is not what you wrote and
> tested during your Hackathon. Even if it were, would it give value
> to future "git log" readers by supplying extra context?
>
Will remove this line.
>> Signed-off-by: Loganaden Velvindron <lo...@hackers.mu>
>> ---
>> Documentation/config.txt | 2 +-
>> http.c | 3 +++
>> 2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index ce9102cea..b18cb9104 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -1957,7 +1957,7 @@ http.sslVersion::
>> - tlsv1.0
>> - tlsv1.1
>> - tlsv1.2
>> -
>> + - tlsv1.3
>> +
>
> Before this change, the block that shows the list of versions had
> one blank line before and after it. Now we lost the blank line
> after the block. Is it intended? Possibilities that come to my
> mind as a reviewer are:
>
> A. There is no difference in the rendered output if we have zero
> blank line (i.e. with the patch), or one blank line (i.e. before
> the patch applied).
>
> A.1) the submitter made this change on purpose, because it will
> make the source shorter without affecting the output, as a
> "clean-up while at it" change.
>
> A.2) this was an accidental change, which did not break the
> output merely because the submitter was lucky.
>
> B. The rendered output changes due to the lack of the blank line.
>
> B.1) And it changes in a good way. The submitter made this
> change on purpose.
>
> B.2) And it changes in a bad way, but the submitter did not
> notice it.
>
> Please do not make reviewers wonder. Either avoid making
> unnecessary changes (e.g. you could have just added a new line with
> tlsv1.3 on it without touching the blank line), or make the change
> and explain why you made that change that is not essential for the
> purpose of adding tls1.3 which is the main focus of this patch.
Alright.
>
>> Can be overridden by the `GIT_SSL_VERSION` environment variable.
>> To force git to use libcurl's default ssl version and ignore any
>> diff --git a/http.c b/http.c
>> index a5bd5d62c..25eb84c11 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -62,6 +62,9 @@ static struct {
>> { "tlsv1.1", CURL_SSLVERSION_TLSv1_1 },
>> { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
>> #endif
>> +#ifdef CURL_SSLVERSION_TLSv1_3
>> + { "tlsv1.3", CURL_SSLVERSION_TLSv1_3 }
>> +#endif
>> };
>
> It seems to me that
>
> https://github.com/curl/curl/blob/master/include/curl/curl.h#L1956
>
> tells me that this #ifdef would not work. Did you test it with the
> "test not version but feature" change you made at the last minute?
I compiled it.
>
> I know it is not your fault but is Ævar's, but you're responsible
> for double-checking what you are told on the internet ;-)
Yes, my fault, not Ævar Arnfjörð Bjarmason .
>
> Thanks.
