"Force Charlie via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes: > From: Force Charlie <charlieio@xxxxxxxxxxx> > > Signed-off-by: Force Charlie <charlieio@xxxxxxxxxxx> > --- > http.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/http.c b/http.c > index 3dc8c560d6..99cb04faba 100644 > --- a/http.c > +++ b/http.c > @@ -48,6 +48,7 @@ char curl_errorstr[CURL_ERROR_SIZE]; > > static int curl_ssl_verify = -1; > static int curl_ssl_try; > +static int curl_http_version = 11; Is there any reason that we need to have this variable's value to be "int"? I _think_ in this patch, the variable is used to choose between the default and "HTTP/2", and I do not think the updated code can choose any other new value that may be supported by an even newer cURL library without further update, i.e. we'd need a variant of "if the configuration asks HTTP/2 then use CURLOPT_HTTP_VERSION with CURL_HTTP_VERSION_2" for the new choice. So I'd think it would not add much value to force end users use a rather cryptic "20" (vs "11") to choose between "2" and "1.1". Why not use spell it out, e.g. using the official name of the protocol "HTTP/2" (vs "HTTP/1.1"), with a "const char *" instead? The new configuration variable and the possible values it can take must be documented, of course. I think it would make the description far less embarrassing if we say "HTTP/2" etc. rather than "20", "11", etc. > @@ -284,6 +285,10 @@ static void process_curl_messages(void) > > static int http_options(const char *var, const char *value, void *cb) > { > + if (!strcmp("http.version",var)) { > + curl_http_version=git_config_int(var,value); STYLE. Missing SP after comma, and around assignment. > + return 0; > + } > if (!strcmp("http.sslverify", var)) { > curl_ssl_verify = git_config_bool(var, value); > return 0; > @@ -806,6 +811,13 @@ static CURL *get_curl_handle(void) > curl_easy_setopt(result, CURLOPT_SSL_VERIFYHOST, 2); > } > > +#if LIBCURL_VERSION_NUM >= 0x073100 > + if(curl_http_version == 20){ STYLE. Missing SP before opening paren and after closing paren. > + /* CURL Enable HTTP2*/ STYLE. Missing SP before closing asterisk-slash. > + curl_easy_setopt(result, CURLOPT_HTTP_VERSION,CURL_HTTP_VERSION_2); > + } > +#endif Shouldn't this block also handle the other values, e.g. "11"? I _think_ the curl_http_version variable (be it an deci-int, or a const char *) should be initialized to a value that you can use to notice that the configuration did not specify any, and then this part should become more like if (curl_http_version && !get_curl_http_version_opt(curl_http_version, &opt)) curl_easy_setopt(result, CURL_HTTP_VERSION, opt); with a helper function like this: static int get_curl_http_version_opt(const char *version_string, long *opt) { int i; static struct { const char *name; lnog opt_token; } choice[] = { { "HTTP/1.1", CURL_HTTP_VERSION_1_1 }, { "HTTP/2", CURL_HTTP_VERSION_2 }, }; for (i = 0; i < ARRAY_SIZE(choice); i++) { if (!strcmp(version_string, choice[i].name)) { *opt = choice[i].opt_token; return 0; } } return -1; /* not found */ } which would make it trivial to support new values later. > #if LIBCURL_VERSION_NUM >= 0x070907 > curl_easy_setopt(result, CURLOPT_NETRC, CURL_NETRC_OPTIONAL); > #endif