Re: [PATCH v2] Allow use of TLS 1.3

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

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?

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 ;-)

Thanks.