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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 14, 2015, 17:57 UTC
Message-ID: <CAPig+cSUR4CEbg3HobZPHyWfGFoGzctP7BbTswg8vyXjGmma6Q@mail.gmail.com>
In-Reply-To: <1439570135-17324-1-git-send-email-gitter.spiros@gmail.com>

On Fri, Aug 14, 2015 at 12:35 PM, Elia Pinto <gitter.spiros@gmail.com> wrote:

Show 11 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 fourth revision of the patch. Changes from previous
>
> - Used only ARRAY_SIZE for walking sslversions (Eric, Junio)
> - Fixed some problems of style: spurious blanks in if stm, wrapped warning

Thanks, this is looking better. A few very minor style-related comments below (most or all of which were mentioned previously, I think)...

Show 11 quoted lines

> diff --git a/http.c b/http.c
> index e9c6fdd..83118b5 100644
> --- a/http.c
> +++ b/http.c
> @@ -364,9 +380,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) {

In this codebase, it is customary to omit the "!= NULL", so:

    if (ssl_version && *ssl_version) {

> +               int i;
> +               for ( i = 0; i < ARRAY_SIZE(sslversions); i++ ) {

Style: Drop space after open '(' and before close ')' in 'for' loop:

    for (i = 0; i < ARRAY_SIZE(sslversions); i++) {

> +                       if (!strcmp(ssl_version,sslversions[i].name)) {

Style: Insert space after comma.

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

warning() call should be on its own line.

Style: Insert space after comma.

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