On Tue, Apr 11, 2017 at 4:06 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Tue, Apr 11, 2017 at 12:20:50PM +0300, Sergey Ryazanov wrote:
>> diff --git a/http.c b/http.c
>> index 96d84bb..8be75b2 100644
>> --- a/http.c
>> +++ b/http.c
>> @@ -836,8 +836,14 @@ static CURL *get_curl_handle(void)
>> }
>> }
>>
>> - if (curl_http_proxy) {
>> - curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
>> + if (curl_http_proxy && curl_http_proxy[0] == '\0') {
>> + /*
>> + * Handle case with the empty http.proxy value here to keep
>> + * common code clean.
>> + * NB: empty option disables proxying at all.
>> + */
>> + curl_easy_setopt(result, CURLOPT_PROXY, "");
>> + } else if (curl_http_proxy) {
>
> This seems pretty reasonable. You could bump this under the existing "if
> (curl_http_proxy)" condition, but that would add an extra level of
> indentation to the rest of the parsing.
>
Because of this additional indentation I decide to add new if () block.
> One thing I do wonder, though: can credential_from_url() ever return a
> NULL host field for other inputs, and what should we do on those inputs?
>
> For example, if I set the value to just "https://"; with no hostname,
> then I think we'll end up with a NULL proxy_auth.host field. And we'll
> feed that NULL to CURLOPT_PROXY, which will cause it to use the
> defaults.
>
> I don't know _what_ "https://"; should do. It's clearly bogus. But
> telling curl to use the defaults seems funny. In that sense, your
> original was much better (we'd feed it to curl, which would be free to
> complain).
>
I thought about this situation too. IMHO the best solution here is to
check host after credential_from_url() call. If host is NULL then we
should show warning message and exit. Then user could fix its
configuration.
Since I in any case will send v3 with grammar fixes then I could add
new patch to the series. This new patch will check host for NULL and
print warning message. Are you Ok with such solution?
--
Sergey