Re: [PATCH 1/2] http: allow selection of proxy authentication method
- From
- Knut Franke <k.franke@science-computing.de>
- Date
- Oct 30, 2015, 18:01 UTC
- Message-ID
- <20151030180129.GA6425@science-computing.de>
- In-Reply-To
- <xmqqsi4vkkcf.fsf@gitster.mtv.corp.google.com>
Junio C Hamano wrote:
Show 25 quoted lines
> > + if (http_proxy_authmethod) {
> > + int i;
> > + for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {
> > + if (!strcmp(http_proxy_authmethod, http_proxy_authmethods[i].name)) {
> > + curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> > + http_proxy_authmethods[i].curlauth_param);
> > + break;
> > + }
> > + }
> > + if (i == ARRAY_SIZE(http_proxy_authmethods)) {
> > + warning("unsupported proxy authentication method %s: using default",
> > + http_proxy_authmethod);
> > + }
> > + }
> > +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> > + else
> > + curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> > +#endif
> > +}
>
> This patch should take what 1c2dbf20 (http: support curl < 7.10.7,
> 2015-02-03) wanted to do into account. Having the configuration
> variable or the environment variable defined by itself, while
> running a Git built with old cURL, shouldn't trigger any warning,
> but the entire function should perhaps be ifdefed out or something?Maybe add a #define LIBCURL_CAN_HANDLE_PROXY_AUTH to clarify this, like we do with LIBCURL_CAN_HANDLE_AUTH_ANY? If so, should this be a separate patch? With the current (scattered) version dependencies, it took me a while to realize that if the function is ifdefed out for LIBCURL_VERSION_NUM < 0x070a07, we don't need to worry about default behavior in case LIBCURL_CAN_HANDLE_AUTH_ANY is not defined. (on the other hand, looking at other curl-version-ifdefs, the define for AUTH_ANY is the exception)
Show 6 quoted lines
> >> +static void copy_from_env(const char **var, const char *envname)
> >> +{
> >> + const char *val = getenv(envname);
> >> + if (val)
> >> + *var = xstrdup(val);
> >> +}[...]
> I primarily was > wishing that its name more clearly conveyed that it sets the > variable from the environment _only if_ the environment variable > exists, and otherwise it does not clobber.
How about env_override? Not perfect, but probably better. I don't think squeezing in more information (maybe_env_override, override_from_env_var) would help.
> The implementation of the helper seems to assume that the variable > must not be pointing at a free-able piece of memory when it is > called
In fact, if http.proxyAuthMethod (btw, I agree with Eric about capitalization) is set, I do call it with *var pointing to free-able memory. Will fix this.
FWIW, set_from_env() has the same pre-condition, which doesn't seem to be satisfied in all cases (namely when overwriting variables previously set by git_config_string()); not to mention missing free()s in http_cleanup.
Otherwise, I'll make the suggested fixes. Thanks.
Cheers, Knut
-- Vorstandsvorsitzender/Chairman of the board of management: Gerd-Lothar Leonhart Vorstand/Board of Management: Dr. Bernd Finkbeiner, Dr. Arno Steitz Vorsitzender des Aufsichtsrats/ Chairman of the Supervisory Board: Philippe Miltin Sitz/Registered Office: Tuebingen Registergericht/Registration Court: Stuttgart Registernummer/Commercial Register No.: HRB 382196