2015-08-13 17:47 GMT+02:00 Eric Sunshine <sunshine@xxxxxxxxxxxxxx>:
> On Thu, Aug 13, 2015 at 11:28 AM, Elia Pinto <gitter.spiros@xxxxxxxxx> wrote:
>> Teach git about a new option, "http.sslVersion", which permits one to
>> specify the SSL version to use when negotiating SSL connections. The
>> setting can be overridden by the GIT_SSL_VERSION environment
>> variable.
>>
>> Signed-off-by: Elia Pinto <gitter.spiros@xxxxxxxxx>
>> ---
>> This is the third version of the patch. The changes compared to the previous version are:
>
> Looks better. A few comments below...
>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index c97c648..6e9359c 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -364,9 +381,22 @@ static CURL *get_curl_handle(void)
>> if (http_proactive_auth)
>> init_curl_http_auth(result);
>>
>> + if (getenv("GIT_SSL_VERSION"))
>> + ssl_version = getenv("GIT_SSL_VERSION");
>> + if (ssl_version != NULL && *ssl_version) {
>> + int i;
>> + for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {
>> + if (sslversions[i].name != NULL && *sslversions[i].name && !strcmp(ssl_version,sslversions[i].name)) {
>
> This sort of loop is normally either handled by indexing up to a limit
> (ARRAY_SIZE, in this case) or by iterating until hitting a sentinel
> (NULL, in this case). It is redundant to use both, as this code does.
I do not think. sslversions[i].name can be null, see how the structure
is initialized. No ?
The other your observations written below are ok for me, but i will
wait for your answer on this before you send another revision. Thank
you very much.
> The former (using ARRAY_SIZE) is typically employed when you know the
> number of items upfront, such as when the item list is local and
> compiled in; the latter (NULL sentinel) is typically used when
> receiving an item list as an argument to a function where you don't
> know the item count upfront (and the item count is not passed to the
> function as a separate argument).
>
> In this case, the item list is local and its size is known to the
> compiler, so that suggests using ARRAY_SIZE, and dropping the NULL
> sentinel.
>
> Style aside: This 'if' statement is very wide and likely should be
> wrapped over multiple lines (trying to keep the code within an
> 80-column limit).
ok.
>
>> + curl_easy_setopt(result, CURLOPT_SSLVERSION,
>> + sslversions[i].ssl_version);
>> + break;
>> + }
>> + if ( i == ARRAY_SIZE(sslversions) ) warning("unsupported ssl version %s: using default",
>> + ssl_version);
>
> Style:
> Drop spaces inside 'if' parentheses.
> Place warning() on its own line.
ok.
>
>> + }
>> +
>> if (getenv("GIT_SSL_CIPHER_LIST"))
>> ssl_cipherlist = getenv("GIT_SSL_CIPHER_LIST");
>> -
>> if (ssl_cipherlist != NULL && *ssl_cipherlist)
>> curl_easy_setopt(result, CURLOPT_SSL_CIPHER_LIST,
>> ssl_cipherlist);
>> --
>> 2.5.0.234.gefc8a62.dirty
--
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