Re: [PATCH v3 3/4] http: handle proxy proactive authentication

From: Jeff King <peff@peff.net>
Date: Mar 6, 2012, 10:09 UTC
Message-ID: <20120306100947.GA710@sigill.intra.peff.net>
In-Reply-To: <4F55EDF3.2030201@seap.minhap.es>

On Tue, Mar 06, 2012 at 11:58:59AM +0100, Nelson Benitez Leon wrote:

Show 10 quoted lines

> > Also, when turning it back into a URL to hand to curl, should we be
> > percent-encoding the items we put in? If my password has an "@" in it,
> > wouldn't we generate a bogus URL? Although looking at how the http auth
> > code handles this, we set CURLOPT_USERPWD directly. Should you be
> > setting CURLOPT_PROXYUSERPWD instead of munging the proxy URL?
> 
> Ok, but it seems is CURLOPT_PROXYUSERNAME and CURLOPT_PROXYPASSWORD what
> we need here as per documentation[1]
> 
> [1] http://curl.haxx.se/libcurl/c/curl_easy_setopt.html#CURLOPTPROXYUSERNAME

Yes, the split CURLOPT_USERNAME and CURLOPT_PASSWORD interface is much better (it allows the username to contain a colon). But it was not introduced until curl 7.19.3, and we support older versions.

We could do an #if on LIBCURL_VERSION_NUM to use the new form when it's available, but nobody has complained so far.

Show 8 quoted lines

> >> +			free ((void *)curl_http_proxy);
> > 
> > Please don't cast to void. This is C, not C++, and casts to void
> > pointers are implicit.  They can never help, and might cover up an
> > actual type error (e.g., casting a non-pointer type).
> 
> Ok, will remove it, I copy/paste it from the http code and I must admit
> I didn't understand why this was needed.

Ah. I grepped for the spot you copied. The cast is to get rid of the "const" on curl_http_proxy. But if it's a pointer to allocated memory, it should not be declared const in the first place. Unfortunately, fixing this means casting in the call to git_config_string (which for some reason takes a pointer-to-const-pointer, even though the value it puts in will always be allocated by xstrdup). Or fixing git_config_string, but that cascades to require fixing in lots of other places. Ugh.

-Peff