Re: [PATCH v2] Allow use of TLS 1.3

> Loganaden Velvindron <logan@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?
>

>> Signed-off-by: Loganaden Velvindron <logan@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.

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