2015-08-12 17:16 GMT+02:00 Junio C Hamano <gitster@xxxxxxxxx>:
> Elia Pinto <gitter.spiros@xxxxxxxxx> writes:
>
>> diff --git a/http.c b/http.c
>> index e9c6fdd..1504005 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -37,6 +37,8 @@ static int curl_ssl_verify = -1;
>> static int curl_ssl_try;
>> static const char *ssl_cert;
>> static const char *ssl_cipherlist;
>> +static const char *ssl_version;
>> +static long sslversion = CURL_SSLVERSION_DEFAULT;
>
> I think you shouldn't have to initialize this variable.
> See below.
>
>> init_curl_http_auth(result);
>>
>> +
>
> An unnecessary double blank?
I will fix this and the typo in the next patch.
>
>> + if (getenv("GIT_SSL_VERSION"))
>> + ssl_version = getenv("GIT_SSL_VERSION");
>> + if (ssl_version != NULL && *ssl_version) {
>> + if (!strcmp(ssl_version,"tlsv1")) {
>> + sslversion = CURL_SSLVERSION_TLSv1;
>> + } else if (!strcmp(ssl_version,"sslv2")) {
>> + sslversion = CURL_SSLVERSION_SSLv2;
>> + } else if (!strcmp(ssl_version,"sslv3")) {
>> + sslversion = CURL_SSLVERSION_SSLv3;
>> +#if LIBCURL_VERSION_NUM >= 0x072200
>> + } else if (!strcmp(ssl_version,"tlsv1.0")) {
>> + sslversion = CURL_SSLVERSION_TLSv1_0;
>> + } else if (!strcmp(ssl_version,"tlsv1.1")) {
>> + sslversion = CURL_SSLVERSION_TLSv1_1;
>> + } else if (!strcmp(ssl_version,"tlsv1.2")) {
>> + sslversion = CURL_SSLVERSION_TLSv1_2;
>> +#endif
>> + } else {
>> + warning("unsupported ssl version %s : using default",
>> + ssl_version);
>> + }
>> + }
>> + curl_easy_setopt(result, CURLOPT_SSLVERSION,
>> + sslversion);
>
> A few problems:
>
> - Why do we even have to call this when sslversion is not given by
> the end user, either from the environment or from the config?
> Wouldn't we use the default version if we do not make this call?
Right. I will fix.
>
> - It is true that 0x072200 is described as introducing these three
> in [*1*] but the structure is maintenance burden waiting to
> happen. If you #if/#endif based on defined(CURL_SSLVERSION_$v),
> it will be obvious to other people how to add their future
> favourite versions in their patches. Also it may be read better
> if you separated the logic and the code by using a table like
> this:
>
> static struct {
> const char *name;
> long ssl_version;
> } sslversions[] = {
> { "tlsv1", CURL_SSLVERSION_TLSv1 },
> { "sslv2", CURL_SSLVERSION_SSLv2 },
> ...
> #ifdef CURL_SSLVERSION_TLSv1_0
> { "tlsv1.0", CURL_SSLVERSION_TLSv1_0 },
> #endif
> ...,
> { NULL }
> };
>
unfortunately they are enum constant (not #defined ) and I don't know
a smart way in C to make this change with the preprocessor. Would you
have any idea?
>
>
>> if (getenv("GIT_SSL_CIPHER_LIST"))
>> ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
>> -
>
> This blank removal is understandable (i.e. group related things
> together).
>
>> if (ssl_cipherlist != NULL && *ssl_cipherlist)
>> curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
>> ssl_cipherlist);
>> -
>
> This is not.
I will fix.
Thank you very much
>
>> if (ssl_cert != NULL)
>> curl_easy_setopt(result, CURLOPT_SSLCERT, ssl_cert);
>> if (has_cert_password())
>
> [References]
>
> *1* https://github.com/bagder/curl/blob/master/docs/libcurl/symbols-in-versions
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html