Re: [PATCH v3] http: add support for specifying the SSL version

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 13, 2015, 16:11 UTC
Message-ID: <CAPig+cSC2a07RYioQ4+sw=pujFW8=sv_d5vv=XiayuSg7FBcHw@mail.gmail.com>
In-Reply-To: <CA+EOSBkSkvvBQDpxL_ygj+2haMk1U7T00-Xmxn8iyXcnV6RN5Q@mail.gmail.com>

On Thu, Aug 13, 2015 at 11:58 AM, Elia Pinto <gitter.spiros@gmail.com> wrote:

Show 33 quoted lines

> 2015-08-13 17:47 GMT+02:00 Eric Sunshine <sunshine@sunshineco.com>:
>> On Thu, Aug 13, 2015 at 11:28 AM, Elia Pinto <gitter.spiros@gmail.com> 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@gmail.com>
>>> ---
>>> 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 initialization:

    static struct {
       const char *name;
       long ssl_version;
       } sslversions[] = {
           { "sslv2", CURL_SSLVERSION_SSLv2 },
           ...
           { "tlsv1.2", CURL_SSLVERSION_TLSv1_2 },
           { NULL }
    };

terminates the list with a NULL sentinel entry, which does indeed set sslversions[i].name to NULL. When you know the item count ahead of time (as you do in this case), this sort of end-of-list sentinel is redundant, and complicates the code unnecessarily. For instance, the 'sslversions[i].name != NULL' expression in the 'if':

    if (sslversions[i].name != NULL && *sslversions[i].name ...

is an unwanted complication. In fact, the '*sslversions[i].name' expression is also unnecessary.