{"thread":{"id":"28310","subject":"[RFC/PATCH git] http: avoid empty error messages for some curl errors","startedAt":"2011-09-05T22:22:02Z","lastAt":"2011-09-06T22:49:50Z","messageCount":6,"participants":["Jonathan Nieder","Daniel Stenberg","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"174891","messageId":"20110905222202.GA32071@elie","threadId":"28310","inReplyTo":null,"subject":"[RFC/PATCH git] http: avoid empty error messages for some curl errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-09-05T22:22:02Z","receivedAt":"2011-09-05T22:22:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When asked to fetch over SSL without a valid\n/etc/ssl/certs/ca-certificates.crt file, \"git fetch\" writes\n\n\terror:  while accessing https://github.com/torvalds/linux.git/info/refs\n\nwhich is a little disconcerting.  Better to fall back to\ncurl_easy_strerror(result) when the error string is empty, like the\ncurl utility does:\n\n\terror: Problem with the SSL CA cert (path? access rights?) while\n\taccessing https://github.com/torvalds/linux.git/info/refs\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nHi,\n\nI ran into this error today because this machine has no certs enabled.\nI'm not thrilled with the following patch because the error string\nbuffer is not cleared very often so it seems possible for it to be\nnot empty but stale at the relevant moment.  I would be happier if we\ncould rely on libcurl always filling the error buffer on errors.\n\nWhat do you think?\nJonathan\n\n http.c |    7 ++++++-\n 1 files changed, 6 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex a59cac45..dec3c60a 100644\n--- a/http.c\n+++ b/http.c\n@@ -851,8 +851,13 @@ static int http_request(const char *url, void *result, int target, int options)\n \t\t\t\tinit_curl_http_auth(slot->curl);\n \t\t\t\tret = HTTP_REAUTH;\n \t\t\t}\n-\t\t} else\n+\t\t} else {\n+\t\t\tif (!curl_errorstr[0])\n+\t\t\t\tstrlcpy(curl_errorstr,\n+\t\t\t\t\tcurl_easy_strerror(results.curl_result),\n+\t\t\t\t\tsizeof(curl_errorstr));\n \t\t\tret = HTTP_ERROR;\n+\t\t}\n \t} else {\n \t\terror(\"Unable to start HTTP request for %s\", url);\n \t\tret = HTTP_START_FAILED;\n-- \n1.7.6\n"},{"id":"174892","messageId":"20110905222934.GA20084@elie","threadId":"28310","inReplyTo":"20110905222202.GA32071@elie","subject":"[PATCH] http: remove extra newline in error message","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-09-05T22:29:34Z","receivedAt":"2011-09-05T22:29:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"There is no need for a blank line between the detailed error message\nand the later \"fatal: HTTP request failed\" notice.  Keep the newline\nwritten by error() itself and eliminate the extra one.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\n> I ran into this error today\n\nHere's a patch for a simpler buglet noticed at the same time.\n\n http.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/http.c b/http.c\nindex dec3c60a..fb3465f5 100644\n--- a/http.c\n+++ b/http.c\n@@ -913,7 +913,7 @@ int http_error(const char *url, int ret)\n {\n \t/* http_request has already handled HTTP_START_FAILED. */\n \tif (ret != HTTP_START_FAILED)\n-\t\terror(\"%s while accessing %s\\n\", curl_errorstr, url);\n+\t\terror(\"%s while accessing %s\", curl_errorstr, url);\n \n \treturn ret;\n }\n-- \n1.7.6\n"},{"id":"174922","messageId":"alpine.DEB.2.00.1109061023010.3841@tvnag.unkk.fr","threadId":"28310","inReplyTo":"20110905222202.GA32071@elie","subject":"Re: [RFC/PATCH git] http: avoid empty error messages for some curl errors","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2011-09-06T12:20:00Z","receivedAt":"2011-09-06T12:20:00Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Mon, 5 Sep 2011, Jonathan Nieder wrote:\n\n> +\t\t\tif (!curl_errorstr[0])\n> +\t\t\t\tstrlcpy(curl_errorstr,\n> +\t\t\t\t\tcurl_easy_strerror(results.curl_result),\n> +\t\t\t\t\tsizeof(curl_errorstr));\n\n(as libcurl hacker)\n\nFor the record, in libcurl we work on providing \"extra\" information in the \nerror buffer when there is additional info to provide that would help. In some \ncases we deem there isn't (or we just to provide any) and then the generic \nerror message is good enough and could indeed be used like this...\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"174941","messageId":"7v62l5a9ke.fsf@alter.siamese.dyndns.org","threadId":"28310","inReplyTo":"alpine.DEB.2.00.1109061023010.3841@tvnag.unkk.fr","subject":"Re: [RFC/PATCH git] http: avoid empty error messages for some curl errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-06T17:02:09Z","receivedAt":"2011-09-06T17:02:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Stenberg <daniel@haxx.se> writes:\n\n> On Mon, 5 Sep 2011, Jonathan Nieder wrote:\n>\n>> +\t\t\tif (!curl_errorstr[0])\n>> +\t\t\t\tstrlcpy(curl_errorstr,\n>> +\t\t\t\t\tcurl_easy_strerror(results.curl_result),\n>> +\t\t\t\t\tsizeof(curl_errorstr));\n>\n> (as libcurl hacker)\n>\n> For the record, in libcurl we work on providing \"extra\" information in\n> the error buffer when there is additional info to provide that would\n> help. In some cases we deem there isn't (or we just to provide any)\n> and then the generic error message is good enough and could indeed be\n> used like this...\n\nSorry if I am a bit slow but are you saying:\n\n (1) we provide \"extra\" but your patch is not using it which is bad?\n (2) the above is Ok but there are better ways to do it?\n (3) something else?\n"},{"id":"174944","messageId":"alpine.DEB.2.00.1109061957120.29735@tvnag.unkk.fr","threadId":"28310","inReplyTo":"7v62l5a9ke.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC/PATCH git] http: avoid empty error messages for some curl errors","fromName":"Daniel Stenberg","fromEmail":"daniel@haxx.se","sentAt":"2011-09-06T18:00:51Z","receivedAt":"2011-09-06T18:00:51Z","isPatch":true,"sender":{"key":"daniel@haxx.se","avatar":"https://gravatar.com/avatar/69fdca87edd17cee21ca2e79fc2ff671d644603c3dc27167430f3cd3dbab7ba8?d=mp&s=160"},"body":"On Tue, 6 Sep 2011, Junio C Hamano wrote:\n\n>>\n>> For the record, in libcurl we work on providing \"extra\" information in\n>> the error buffer when there is additional info to provide that would\n>> help. In some cases we deem there isn't (or we just to provide any)\n>> and then the generic error message is good enough and could indeed be\n>> used like this...\n>\n> Sorry if I am a bit slow but are you saying:\n>\n> (1) we provide \"extra\" but your patch is not using it which is bad?\n> (2) the above is Ok but there are better ways to do it?\n> (3) something else?\n\nSorry for being unclear.\n\nI was trying to explain why the proposed patch makes sense and that it will \ncontinue to make sense even if in future libcurl versions it would start \nreturning error messages for errors where currently it returns none.\n\nThus, I am in favour of the general idea of the patch - I have no comment for \nthe exact implementation as I haven't checked the details.\n\n-- \n\n  / daniel.haxx.se\n"},{"id":"174969","messageId":"7vehzt70c1.fsf@alter.siamese.dyndns.org","threadId":"28310","inReplyTo":"20110905222202.GA32071@elie","subject":"Re: [RFC/PATCH git] http: avoid empty error messages for some curl errors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-09-06T22:49:50Z","receivedAt":"2011-09-06T22:49:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> When asked to fetch over SSL without a valid\n> /etc/ssl/certs/ca-certificates.crt file, \"git fetch\" writes\n>\n> \terror:  while accessing https://github.com/torvalds/linux.git/info/refs\n>\n> which is a little disconcerting.  Better to fall back to\n> curl_easy_strerror(result) when the error string is empty, like the\n> curl utility does:\n>\n> \terror: Problem with the SSL CA cert (path? access rights?) while\n> \taccessing https://github.com/torvalds/linux.git/info/refs\n>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> ---\n> Hi,\n>\n> I ran into this error today because this machine has no certs enabled.\n> I'm not thrilled with the following patch because the error string\n> buffer is not cleared very often so it seems possible for it to be\n> not empty but stale at the relevant moment.  I would be happier if we\n> could rely on libcurl always filling the error buffer on errors.\n\nWill queue both patches; thanks.\n"}]}