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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 13, 2015, 15:47 UTC
Message-ID: <CAPig+cTug2Q3v1K5r76fhJ6OQY9V1e6MbiXQBGQJD51TCOGW=A@mail.gmail.com>
In-Reply-To: <1439479731-16018-1-git-send-email-gitter.spiros@gmail.com>

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

Show 8 quoted lines

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

Show 14 quoted lines

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

Show 6 quoted lines

> +                               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.

Show 10 quoted lines

> +       }
> +
>         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