[RFC/PATCH git] http: avoid empty error messages for some curl errors

[RFC/PATCH git] http: avoid empty error messages for some curl errors

[Jonathan Nieder]

When asked to fetch over SSL without a valid
/etc/ssl/certs/ca-certificates.crt file, "git fetch" writes

	error:  while accessing https://github.com/torvalds/linux.git/info/refs

which is a little disconcerting.  Better to fall back to
curl_easy_strerror(result) when the error string is empty, like the
curl utility does:

	error: Problem with the SSL CA cert (path? access rights?) while
	accessing https://github.com/torvalds/linux.git/info/refs

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

I ran into this error today because this machine has no certs enabled.
I'm not thrilled with the following patch because the error string
buffer is not cleared very often so it seems possible for it to be
not empty but stale at the relevant moment.  I would be happier if we
could rely on libcurl always filling the error buffer on errors.

What do you think?
Jonathan

 http.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index a59cac45..dec3c60a 100644
--- a/http.c
+++ b/http.c
@@ -851,8 +851,13 @@ static int http_request(const char *url, void *result, int target, int options)
 				init_curl_http_auth(slot->curl);
 				ret = HTTP_REAUTH;
 			}
-		} else
+		} else {
+			if (!curl_errorstr[0])
+				strlcpy(curl_errorstr,
+					curl_easy_strerror(results.curl_result),
+					sizeof(curl_errorstr));
 			ret = HTTP_ERROR;
+		}
 	} else {
 		error("Unable to start HTTP request for %s", url);
 		ret = HTTP_START_FAILED;
-- 
1.7.6

[PATCH] http: remove extra newline in error message

[Jonathan Nieder]

There is no need for a blank line between the detailed error message
and the later "fatal: HTTP request failed" notice.  Keep the newline
written by error() itself and eliminate the extra one.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
> I ran into this error today

Here's a patch for a simpler buglet noticed at the same time.

 http.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index dec3c60a..fb3465f5 100644
--- a/http.c
+++ b/http.c
@@ -913,7 +913,7 @@ int http_error(const char *url, int ret)
 {
 	/* http_request has already handled HTTP_START_FAILED. */
 	if (ret != HTTP_START_FAILED)
-		error("%s while accessing %s\n", curl_errorstr, url);
+		error("%s while accessing %s", curl_errorstr, url);
 
 	return ret;
 }
-- 
1.7.6

Re: [RFC/PATCH git] http: avoid empty error messages for some curl errors

[Daniel Stenberg]

On Mon, 5 Sep 2011, Jonathan Nieder wrote:

> +			if (!curl_errorstr[0])
> +				strlcpy(curl_errorstr,
> +					curl_easy_strerror(results.curl_result),
> +					sizeof(curl_errorstr));

(as libcurl hacker)

For the record, in libcurl we work on providing "extra" information in the 
error buffer when there is additional info to provide that would help. In some 
cases we deem there isn't (or we just to provide any) and then the generic 
error message is good enough and could indeed be used like this...

-- 

  / daniel.haxx.se

Re: [RFC/PATCH git] http: avoid empty error messages for some curl errors

[Junio C Hamano]

Daniel Stenberg <daniel@haxx.se> writes:

> On Mon, 5 Sep 2011, Jonathan Nieder wrote:
>
>> +			if (!curl_errorstr[0])
>> +				strlcpy(curl_errorstr,
>> +					curl_easy_strerror(results.curl_result),
>> +					sizeof(curl_errorstr));
>
> (as libcurl hacker)
>
> For the record, in libcurl we work on providing "extra" information in
> the error buffer when there is additional info to provide that would
> help. In some cases we deem there isn't (or we just to provide any)
> and then the generic error message is good enough and could indeed be
> used like this...

Sorry if I am a bit slow but are you saying:

 (1) we provide "extra" but your patch is not using it which is bad?
 (2) the above is Ok but there are better ways to do it?
 (3) something else?

Re: [RFC/PATCH git] http: avoid empty error messages for some curl errors

[Daniel Stenberg]

On Tue, 6 Sep 2011, Junio C Hamano wrote:

>>
>> For the record, in libcurl we work on providing "extra" information in
>> the error buffer when there is additional info to provide that would
>> help. In some cases we deem there isn't (or we just to provide any)
>> and then the generic error message is good enough and could indeed be
>> used like this...
>
> Sorry if I am a bit slow but are you saying:
>
> (1) we provide "extra" but your patch is not using it which is bad?
> (2) the above is Ok but there are better ways to do it?
> (3) something else?

Sorry for being unclear.

I was trying to explain why the proposed patch makes sense and that it will 
continue to make sense even if in future libcurl versions it would start 
returning error messages for errors where currently it returns none.

Thus, I am in favour of the general idea of the patch - I have no comment for 
the exact implementation as I haven't checked the details.

-- 

  / daniel.haxx.se

Re: [RFC/PATCH git] http: avoid empty error messages for some curl errors

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> When asked to fetch over SSL without a valid
> /etc/ssl/certs/ca-certificates.crt file, "git fetch" writes
>
> 	error:  while accessing https://github.com/torvalds/linux.git/info/refs
>
> which is a little disconcerting.  Better to fall back to
> curl_easy_strerror(result) when the error string is empty, like the
> curl utility does:
>
> 	error: Problem with the SSL CA cert (path? access rights?) while
> 	accessing https://github.com/torvalds/linux.git/info/refs
>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> Hi,
>
> I ran into this error today because this machine has no certs enabled.
> I'm not thrilled with the following patch because the error string
> buffer is not cleared very often so it seems possible for it to be
> not empty but stale at the relevant moment.  I would be happier if we
> could rely on libcurl always filling the error buffer on errors.

Will queue both patches; thanks.