{"thread":{"id":"41898","subject":"[PATCH 2/2] http.c: implements the GIT_CURL_DEBUG environment variable","startedAt":"2016-04-01T10:44:41Z","lastAt":"2016-04-06T06:16:50Z","messageCount":14,"participants":["Elia Pinto","Torsten Bögershausen","Ramsay Jones","Junio C Hamano","Eric Sunshine","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"282444","messageId":"1459507482-36678-1-git-send-email-gitter.spiros@gmail.com","threadId":"41898","inReplyTo":null,"subject":"[PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-01T10:44:41Z","receivedAt":"2016-04-01T10:44:41Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Implements the GIT_CURL_DEBUG environment variable to allow a greater\ndegree of detail of GIT_CURL_VERBOSE, in particular the complete\ntransport header and all the data payload exchanged.\nIt might be useful if a particular situation could require a more\nthorough debugging analysis.\n\nSigned-off-by: Elia Pinto \n---\n imap-send.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n 1 file changed, 97 insertions(+), 2 deletions(-)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 4d3b773..cf79e7f 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf *server,\n }\n \n #ifdef USE_CURL_FOR_IMAP_SEND\n+\n+static\n+void curl_dump(const char *text,\n+\t FILE * stream, unsigned char *ptr, size_t size, char nohex)\n+{\n+\tsize_t i;\n+\tsize_t c;\n+\n+\tunsigned int width = 0x10;\n+\n+\tif (nohex)\n+\t\t/* without the hex output, we can fit more on screen */\n+\t\twidth = 0x40;\n+\n+\tfprintf(stream, \"%s, %10.10ld bytes (0x%8.8lx)\\n\",\n+\t\ttext, (long)size, (long)size);\n+\n+\tfor (i = 0; i < size; i += width) {\n+\n+\t\tfprintf(stream, \"%4.4lx: \", (long)i);\n+\n+\t\tif (!nohex) {\n+\t\t\t/* hex not disabled, show it */\n+\t\t\tfor (c = 0; c < width; c++)\n+\t\t\t\tif (i + c < size)\n+\t\t\t\t\tfprintf(stream, \"%02x \", ptr[i + c]);\n+\t\t\t\telse\n+\t\t\t\t\tfputs(\" \", stream);\n+\t\t}\n+\n+\t\tfor (c = 0; (c < width) && (i + c < size); c++) {\n+\t\t\t/* check for 0D0A; if found, skip past and start a new line of output */\n+\t\t\tif (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D\n+\t\t\t && ptr[i + c + 1] == 0x0A) {\n+\t\t\t\ti += (c + 2 - width);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tfprintf(stream, \"%c\",\n+\t\t\t\t(ptr[i + c] >= 0x20)\n+\t\t\t\t&& (ptr[i + c] < 0x80) ? ptr[i + c] : '.');\n+\t\t\t/* check again for 0D0A, to avoid an extra \\n if it's at width */\n+\t\t\tif (nohex && (i + c + 2 < size)\n+\t\t\t && ptr[i + c + 1] == 0x0D\n+\t\t\t && ptr[i + c + 2] == 0x0A) {\n+\t\t\t\ti += (c + 3 - width);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tfputc('\\n', stream);\t/* newline */\n+\t}\n+\tfflush(stream);\n+}\n+\n+static\n+int curl_trace(CURL * handle, curl_infotype type,\n+\t char *data, size_t size, void *userp)\n+{\n+\tconst char *text;\n+\t(void)handle;\t\t/* prevent compiler warning */\n+\n+\tswitch (type) {\n+\tcase CURLINFO_TEXT:\n+\t\tfprintf(stderr, \"== Info: %s\", data);\n+\tdefault:\t\t/* in case a new one is introduced to shock us */\n+\t\treturn 0;\n+\n+\tcase CURLINFO_HEADER_OUT:\n+\t\ttext = \"=> Send header\";\n+\t\tbreak;\n+\tcase CURLINFO_DATA_OUT:\n+\t\ttext = \"=> Send data\";\n+\t\tbreak;\n+\tcase CURLINFO_SSL_DATA_OUT:\n+\t\ttext = \"=> Send SSL data\";\n+\t\tbreak;\n+\tcase CURLINFO_HEADER_IN:\n+\t\ttext = \"<= Recv header\";\n+\t\tbreak;\n+\tcase CURLINFO_DATA_IN:\n+\t\ttext = \"<= Recv data\";\n+\t\tbreak;\n+\tcase CURLINFO_SSL_DATA_IN:\n+\t\ttext = \"<= Recv SSL data\";\n+\t\tbreak;\n+\t}\n+\n+\tcurl_dump(text, stderr, (unsigned char *)data, size, 1);\n+\treturn 0;\n+}\n+\n static CURL *setup_curl(struct imap_server_conf *srvc)\n {\n \tCURL *curl;\n@@ -1442,8 +1532,13 @@ static CURL *setup_curl(struct imap_server_conf *srvc)\n \n \tcurl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);\n \n-\tif (0 < verbosity || getenv(\"GIT_CURL_VERBOSE\"))\n-\t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n+\tif (0 < verbosity )\n+\t\tif (getenv(\"GIT_CURL_DEBUG\")) {\n+\t\t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1);\n+\t\t\tcurl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);\n+\t\t} else if (getenv(\"GIT_CURL_VERBOSE\"))\n+\t\t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1);\n+\n \n \treturn curl;\n }\n-- \n2.7.0.416.gbf6b42c.dirty\n"},{"id":"282443","messageId":"1459507482-36678-2-git-send-email-gitter.spiros@gmail.com","threadId":"41898","inReplyTo":"1459507482-36678-1-git-send-email-gitter.spiros@gmail.com","subject":"[PATCH 2/2] http.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-01T10:44:42Z","receivedAt":"2016-04-01T10:44:42Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Implements the GIT_CURL_DEBUG environment variable to allow a greater\ndegree of detail of GIT_CURL_VERBOSE, in particular the complete\ntransport header and all the data payload exchanged.\nIt might be useful if a particular situation could require a more\nthorough debugging analysis.\n\nSigned-off-by: Elia Pinto \n---\n http.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 96 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex dfc53c1..079779d 100644\n--- a/http.c\n+++ b/http.c\n@@ -437,6 +437,97 @@ static void set_curl_keepalive(CURL *c)\n }\n #endif\n \n+\n+static\n+void curl_dump(const char *text,\n+\t FILE * stream, unsigned char *ptr, size_t size, char nohex)\n+{\n+\tsize_t i;\n+\tsize_t c;\n+\n+\tunsigned int width = 0x10;\n+\n+\tif (nohex)\n+\t\t/* without the hex output, we can fit more on screen */\n+\t\twidth = 0x40;\n+\n+\tfprintf(stream, \"%s, %10.10ld bytes (0x%8.8lx)\\n\",\n+\t\ttext, (long)size, (long)size);\n+\n+\tfor (i = 0; i < size; i += width) {\n+\n+\t\tfprintf(stream, \"%4.4lx: \", (long)i);\n+\n+\t\tif (!nohex) {\n+\t\t\t/* hex not disabled, show it */\n+\t\t\tfor (c = 0; c < width; c++)\n+\t\t\t\tif (i + c < size)\n+\t\t\t\t\tfprintf(stream, \"%02x \", ptr[i + c]);\n+\t\t\t\telse\n+\t\t\t\t\tfputs(\" \", stream);\n+\t\t}\n+\n+\t\tfor (c = 0; (c < width) && (i + c < size); c++) {\n+\t\t\t/* check for 0D0A; if found, skip past and start a new line of output */\n+\t\t\tif (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D\n+\t\t\t && ptr[i + c + 1] == 0x0A) {\n+\t\t\t\ti += (c + 2 - width);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tfprintf(stream, \"%c\",\n+\t\t\t\t(ptr[i + c] >= 0x20)\n+\t\t\t\t&& (ptr[i + c] < 0x80) ? ptr[i + c] : '.');\n+\t\t\t/* check again for 0D0A, to avoid an extra \\n if it's at width */\n+\t\t\tif (nohex && (i + c + 2 < size)\n+\t\t\t && ptr[i + c + 1] == 0x0D\n+\t\t\t && ptr[i + c + 2] == 0x0A) {\n+\t\t\t\ti += (c + 3 - width);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tfputc('\\n', stream);\t/* newline */\n+\t}\n+\tfflush(stream);\n+}\n+\n+static\n+int curl_trace(CURL * handle, curl_infotype type,\n+\t char *data, size_t size, void *userp)\n+{\n+\tconst char *text;\n+\t(void)handle;\t\t/* prevent compiler warning */\n+\n+\tswitch (type) {\n+\tcase CURLINFO_TEXT:\n+\t\tfprintf(stderr, \"== Info: %s\", data);\n+\tdefault:\t\t/* in case a new one is introduced to shock us */\n+\t\treturn 0;\n+\n+\tcase CURLINFO_HEADER_OUT:\n+\t\ttext = \"=> Send header\";\n+\t\tbreak;\n+\tcase CURLINFO_DATA_OUT:\n+\t\ttext = \"=> Send data\";\n+\t\tbreak;\n+\tcase CURLINFO_SSL_DATA_OUT:\n+\t\ttext = \"=> Send SSL data\";\n+\t\tbreak;\n+\tcase CURLINFO_HEADER_IN:\n+\t\ttext = \"<= Recv header\";\n+\t\tbreak;\n+\tcase CURLINFO_DATA_IN:\n+\t\ttext = \"<= Recv data\";\n+\t\tbreak;\n+\tcase CURLINFO_SSL_DATA_IN:\n+\t\ttext = \"<= Recv SSL data\";\n+\t\tbreak;\n+\t}\n+\n+\tcurl_dump(text, stderr, (unsigned char *)data, size, 1);\n+\treturn 0;\n+}\n+\n+\n static CURL *get_curl_handle(void)\n {\n \tCURL *result = curl_easy_init();\n@@ -532,7 +623,11 @@ static CURL *get_curl_handle(void)\n \t\t\t\"your curl version is too old (>= 7.19.4)\");\n #endif\n \n-\tif (getenv(\"GIT_CURL_VERBOSE\"))\n+\tif (getenv(\"GIT_CURL_DEBUG\")) {\n+\t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n+\t\tcurl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);\n+\t\tcurl_easy_setopt(result, CURLOPT_DEBUGDATA, NULL);\n+\t} else if (getenv(\"GIT_CURL_VERBOSE\"))\n \t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n \n \tcurl_easy_setopt(result, CURLOPT_USERAGENT,\n-- \n2.7.0.416.gbf6b42c.dirty\n"},{"id":"282450","messageId":"56FE5F1C.2040103@web.de","threadId":"41898","inReplyTo":"1459507482-36678-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2016-04-01T11:44:28Z","receivedAt":"2016-04-01T11:44:28Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 01.04.16 12:44, Elia Pinto wrote:\n> Implements the GIT_CURL_DEBUG environment variable to allow a greater\n> degree of detail of GIT_CURL_VERBOSE, in particular the complete\n> transport header and all the data payload exchanged.\n> It might be useful if a particular situation could require a more\n> thorough debugging analysis.\n> \n> Signed-off-by: Elia Pinto \n> ---\n> imap-send.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 97 insertions(+), 2 deletions(-)\n> \n> diff --git a/imap-send.c b/imap-send.c\n> index 4d3b773..cf79e7f 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n> @@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf *server,\n> }\n> \n> #ifdef USE_CURL_FOR_IMAP_SEND\n> +\n> +static\n> +void curl_dump(const char *text,\n> +\t FILE * stream, unsigned char *ptr, size_t size, char nohex)\nStyle: FILE *stream\n> +{\n> +\tsize_t i;\n> +\tsize_t c;\nc is typically a character.\n'j' may be better, unless you find a longer,\nand more descriptive name.\n\n> +\n> +\tunsigned int width = 0x10;\n> +\n> +\tif (nohex)\n> +\t\t/* without the hex output, we can fit more on screen */\n> +\t\twidth = 0x40;\n> +\n> +\tfprintf(stream, \"%s, %10.10ld bytes (0x%8.8lx)\\n\",\n> +\t\ttext, (long)size, (long)size);\n> +\n> +\tfor (i = 0; i < size; i += width) {\n> +\n> +\t\tfprintf(stream, \"%4.4lx: \", (long)i);\n> +\n> +\t\tif (!nohex) {\n> +\t\t\t/* hex not disabled, show it */\n> +\t\t\tfor (c = 0; c < width; c++)\n> +\t\t\t\tif (i + c < size)\n> +\t\t\t\t\tfprintf(stream, \"%02x \", ptr[i + c]);\n> +\t\t\t\telse\n> +\t\t\t\t\tfputs(\" \", stream);\n> +\t\t}\n> +\n> +\t\tfor (c = 0; (c < width) && (i + c < size); c++) {\n> +\t\t\t/* check for 0D0A; if found, skip past and start a new line of output */\nShould we write '\\r' and '\\n' instead for hex codes ?\nand CRLF instead of 0D0A ? (Yes the RFC use hexcodes, but we write readable C-code)\n"},{"id":"282465","messageId":"56FE8C25.8020607@ramsayjones.plus.com","threadId":"41898","inReplyTo":"1459507482-36678-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-04-01T14:56:37Z","receivedAt":"2016-04-01T14:56:37Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 01/04/16 11:44, Elia Pinto wrote:\n> Implements the GIT_CURL_DEBUG environment variable to allow a greater\n> degree of detail of GIT_CURL_VERBOSE, in particular the complete\n> transport header and all the data payload exchanged.\n> It might be useful if a particular situation could require a more\n> thorough debugging analysis.\n> \n> Signed-off-by: Elia Pinto \n> ---\n> imap-send.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n> 1 file changed, 97 insertions(+), 2 deletions(-)\n> \n> diff --git a/imap-send.c b/imap-send.c\n> index 4d3b773..cf79e7f 100644\n> --- a/imap-send.c\n> +++ b/imap-send.c\n[snip]\n\n> @@ -1442,8 +1532,13 @@ static CURL *setup_curl(struct imap_server_conf *srvc)\n> \n> \tcurl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);\n> \n> -\tif (0 < verbosity || getenv(\"GIT_CURL_VERBOSE\"))\n> -\t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n> +\tif (0 < verbosity )\n\npreviously it was sufficient to set GIT_CURL_VERBOSE, now I have to\nset verbosity too?\n\n[Does it matter that you change \"1L\" to \"1\" in the curl_easy_setopt()\ncall? In http.c (line 567) it also uses \"1\", but ...]\n\n> +\t\tif (getenv(\"GIT_CURL_DEBUG\")) {\n> +\t\t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1);\n> +\t\t\tcurl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);\n> +\t\t} else if (getenv(\"GIT_CURL_VERBOSE\"))\n> +\t\t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1);\n> +\n> \n> \treturn curl;\n> }\n> \n\nI would have expected something like:\n\nif (0 < verbosity || getenv(\"GIT_CURL_VERBOSE\")) {\n\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n\tif (getenv(\"GIT_CURL_DEBUG\"))\n\t\tcurl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);\n}\n\nHope That Helps.\n\nATB,\nRamsay Jones\n"},{"id":"282466","messageId":"56FE8DC3.5080304@ramsayjones.plus.com","threadId":"41898","inReplyTo":"1459507482-36678-2-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH 2/2] http.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-04-01T15:03:31Z","receivedAt":"2016-04-01T15:03:31Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 01/04/16 11:44, Elia Pinto wrote:\n> Implements the GIT_CURL_DEBUG environment variable to allow a greater\n> degree of detail of GIT_CURL_VERBOSE, in particular the complete\n> transport header and all the data payload exchanged.\n> It might be useful if a particular situation could require a more\n> thorough debugging analysis.\n> \n> Signed-off-by: Elia Pinto \n> ---\n> http.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> 1 file changed, 96 insertions(+), 1 deletion(-)\n> \n> diff --git a/http.c b/http.c\n> index dfc53c1..079779d 100644\n> --- a/http.c\n> +++ b/http.c\n[snip]\n\n> @@ -532,7 +623,11 @@ static CURL *get_curl_handle(void)\n> \t\t\t\"your curl version is too old (>= 7.19.4)\");\n> #endif\n> \n> -\tif (getenv(\"GIT_CURL_VERBOSE\"))\n> +\tif (getenv(\"GIT_CURL_DEBUG\")) {\n> +\t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n> +\t\tcurl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);\n> +\t\tcurl_easy_setopt(result, CURLOPT_DEBUGDATA, NULL);\n> +\t} else if (getenv(\"GIT_CURL_VERBOSE\"))\n> \t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n> \n> \tcurl_easy_setopt(result, CURLOPT_USERAGENT,\n> \n\nAgain, maybe something like:\n\nif (getenv(\"GIT_CURL_VERBOSE\")) {\n\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n\tif (getenv(\"GIT_CURL_DEBUG\"))\n\t\tcurl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);\n}\n\nAlthough that does make GIT_CURL_DEBUG subordinate to GIT_CURL_VERBOSE.\nSo, that may not be desired ...\n\nATB,\nRamsay Jones\n"},{"id":"282469","messageId":"xmqqtwjljq97.fsf@gitster.mtv.corp.google.com","threadId":"41898","inReplyTo":"1459507482-36678-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-01T15:35:48Z","receivedAt":"2016-04-01T15:35:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n> Implements the GIT_CURL_DEBUG environment variable to allow a greater\n> degree of detail of GIT_CURL_VERBOSE, in particular the complete\n> transport header and all the data payload exchanged.\n> It might be useful if a particular situation could require a more\n> thorough debugging analysis.\n\nMy impression is that using GIT_TRACE_* is the more mainstream\ntrend, and it may be beneficial to work any new debugging aid like\nthis one to fit within that mechanism.\n\nI am not saying new GIT_*_DEBUG is wrong. I just wanted to make\nsure you have considered doing this as a new trace in GIT_TRACE_*\nfamily and rejected that apporach with a very good reason, in\nwhich case that rationale deserves to be in the log message.\n"},{"id":"282503","messageId":"CAPig+cRrmdiz4fHAX3xg6CinSOPBv207RZLaacRBkoDNHLaPwA@mail.gmail.com","threadId":"41898","inReplyTo":"1459507482-36678-1-git-send-email-gitter.spiros@gmail.com","subject":"Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-01T20:25:34Z","receivedAt":"2016-04-01T20:25:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Apr 1, 2016 at 6:44 AM, Elia Pinto wrote:\n> Implements the GIT_CURL_DEBUG environment variable to allow a greater\n> degree of detail of GIT_CURL_VERBOSE, in particular the complete\n> transport header and all the data payload exchanged.\n> It might be useful if a particular situation could require a more\n> thorough debugging analysis.\n\nIn addition to review comments by others, why are the new curl_dump()\nand curl_trace() functions duplicated in both patches rather than\nfactored out to a shared implementation?\n\n> Signed-off-by: Elia Pinto \n> ---\n> diff --git a/imap-send.c b/imap-send.c\n> @@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf *server,\n> }\n>\n> #ifdef USE_CURL_FOR_IMAP_SEND\n> +\n> +static\n> +void curl_dump(const char *text,\n> + FILE * stream, unsigned char *ptr, size_t size, char nohex)\n> +{\n> + size_t i;\n> + size_t c;\n> +\n> + unsigned int width = 0x10;\n> +\n> + if (nohex)\n> + /* without the hex output, we can fit more on screen */\n> + width = 0x40;\n> +\n> + fprintf(stream, \"%s, %10.10ld bytes (0x%8.8lx)\\n\",\n> + text, (long)size, (long)size);\n> +\n> + for (i = 0; i < size; i += width) {\n> +\n> + fprintf(stream, \"%4.4lx: \", (long)i);\n> +\n> + if (!nohex) {\n> + /* hex not disabled, show it */\n> + for (c = 0; c < width; c++)\n> + if (i + c < size)\n> + fprintf(stream, \"%02x \", ptr[i + c]);\n> + else\n> + fputs(\" \", stream);\n> + }\n> +\n> + for (c = 0; (c < width) && (i + c < size); c++) {\n> + /* check for 0D0A; if found, skip past and start a new line of output */\n> + if (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D\n> + && ptr[i + c + 1] == 0x0A) {\n> + i += (c + 2 - width);\n> + break;\n> + }\n> + fprintf(stream, \"%c\",\n> + (ptr[i + c] >= 0x20)\n> + && (ptr[i + c] < 0x80) ? ptr[i + c] : '.');\n> + /* check again for 0D0A, to avoid an extra \\n if it's at width */\n> + if (nohex && (i + c + 2 < size)\n> + && ptr[i + c + 1] == 0x0D\n> + && ptr[i + c + 2] == 0x0A) {\n> + i += (c + 3 - width);\n> + break;\n> + }\n> + }\n> + fputc('\\n', stream); /* newline */\n> + }\n> + fflush(stream);\n> +}\n> +\n> +static\n> +int curl_trace(CURL * handle, curl_infotype type,\n> + char *data, size_t size, void *userp)\n> +{\n> + const char *text;\n> + (void)handle; /* prevent compiler warning */\n> +\n> + switch (type) {\n> + case CURLINFO_TEXT:\n> + fprintf(stderr, \"== Info: %s\", data);\n> + default: /* in case a new one is introduced to shock us */\n> + return 0;\n> +\n> + case CURLINFO_HEADER_OUT:\n> + text = \"=> Send header\";\n> + break;\n> + case CURLINFO_DATA_OUT:\n> + text = \"=> Send data\";\n> + break;\n> + case CURLINFO_SSL_DATA_OUT:\n> + text = \"=> Send SSL data\";\n> + break;\n> + case CURLINFO_HEADER_IN:\n> + text = \"<= Recv header\";\n> + break;\n> + case CURLINFO_DATA_IN:\n> + text = \"<= Recv data\";\n> + break;\n> + case CURLINFO_SSL_DATA_IN:\n> + text = \"<= Recv SSL data\";\n> + break;\n> + }\n> +\n> + curl_dump(text, stderr, (unsigned char *)data, size, 1);\n> + return 0;\n> +}\n"},{"id":"282610","messageId":"CA+EOSBkSG6SLOgQeFuBSupYW1nc6UcHd=iCFcGhs265NBd77vw@mail.gmail.com","threadId":"41898","inReplyTo":"56FE8DC3.5080304@ramsayjones.plus.com","subject":"Re: [PATCH 2/2] http.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-04T12:41:27Z","receivedAt":"2016-04-04T12:41:27Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2016-04-01 17:03 GMT+02:00 Ramsay Jones :\n>\n>\n> On 01/04/16 11:44, Elia Pinto wrote:\n>> Implements the GIT_CURL_DEBUG environment variable to allow a greater\n>> degree of detail of GIT_CURL_VERBOSE, in particular the complete\n>> transport header and all the data payload exchanged.\n>> It might be useful if a particular situation could require a more\n>> thorough debugging analysis.\n>>\n>> Signed-off-by: Elia Pinto \n>> ---\n>> http.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n>> 1 file changed, 96 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/http.c b/http.c\n>> index dfc53c1..079779d 100644\n>> --- a/http.c\n>> +++ b/http.c\n> [snip]\n>\n>> @@ -532,7 +623,11 @@ static CURL *get_curl_handle(void)\n>> \"your curl version is too old (>= 7.19.4)\");\n>> #endif\n>>\n>> - if (getenv(\"GIT_CURL_VERBOSE\"))\n>> + if (getenv(\"GIT_CURL_DEBUG\")) {\n>> + curl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n>> + curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);\n>> + curl_easy_setopt(result, CURLOPT_DEBUGDATA, NULL);\n>> + } else if (getenv(\"GIT_CURL_VERBOSE\"))\n>> curl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n>>\n>> curl_easy_setopt(result, CURLOPT_USERAGENT,\n>>\n>\n> Again, maybe something like:\n>\n> if (getenv(\"GIT_CURL_VERBOSE\")) {\n> curl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n> if (getenv(\"GIT_CURL_DEBUG\"))\n> curl_easy_setopt(result, CURLOPT_DEBUGFUNCTION, curl_trace);\n> }\n>\n> Although that does make GIT_CURL_DEBUG subordinate to GIT_CURL_VERBOSE.\n> So, that may not be desired ...\n\nThank you. But actually it is not a desirable change, for me almost, I\nprefer that the two definitions are independent. And it is true the\nopposite: if it is defined the curl DEBUG flag then it is implicitly\ndefined the curl VERBOSE flag, because it is a prerequisite of the\nDEBUG functionality.\n\nThanks in any case for the review.\n\nBest\n>\n> ATB,\n> Ramsay Jones\n>\n>\n"},{"id":"282612","messageId":"CA+EOSBn6HT1AwqcTxW+8p31ZnUV_-1qjip_CnnFKCXwiSyUfiQ@mail.gmail.com","threadId":"41898","inReplyTo":"56FE8C25.8020607@ramsayjones.plus.com","subject":"Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-04T12:45:48Z","receivedAt":"2016-04-04T12:45:48Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2016-04-01 16:56 GMT+02:00 Ramsay Jones :\n>\n>\n> On 01/04/16 11:44, Elia Pinto wrote:\n>> Implements the GIT_CURL_DEBUG environment variable to allow a greater\n>> degree of detail of GIT_CURL_VERBOSE, in particular the complete\n>> transport header and all the data payload exchanged.\n>> It might be useful if a particular situation could require a more\n>> thorough debugging analysis.\n>>\n>> Signed-off-by: Elia Pinto \n>> ---\n>> imap-send.c | 99 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--\n>> 1 file changed, 97 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/imap-send.c b/imap-send.c\n>> index 4d3b773..cf79e7f 100644\n>> --- a/imap-send.c\n>> +++ b/imap-send.c\n> [snip]\n>\n>> @@ -1442,8 +1532,13 @@ static CURL *setup_curl(struct imap_server_conf *srvc)\n>>\n>> curl_easy_setopt(curl, CURLOPT_UPLOAD, 1L);\n>>\n>> - if (0 < verbosity || getenv(\"GIT_CURL_VERBOSE\"))\n>> - curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n>> + if (0 < verbosity )\nIt was already so in the previous code, I have not changed it. If it\nis a desirable change it would take another patch\n>\n> previously it was sufficient to set GIT_CURL_VERBOSE, now I have to\n> set verbosity too?\n\n>\n> [Does it matter that you change \"1L\" to \"1\" in the curl_easy_setopt()\n> call? In http.c (line 567) it also uses \"1\", but ...]\n>\n>> + if (getenv(\"GIT_CURL_DEBUG\")) {\n>> + curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);\n>> + curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);\n>> + } else if (getenv(\"GIT_CURL_VERBOSE\"))\n>> + curl_easy_setopt(curl, CURLOPT_VERBOSE, 1);\n>> +\nOK.\n>>\n>> return curl;\n>> }\n>>\n>\n> I would have expected something like:\n>\n> if (0 < verbosity || getenv(\"GIT_CURL_VERBOSE\")) {\n> curl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n> if (getenv(\"GIT_CURL_DEBUG\"))\n> curl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);\n> }\n>\nNo. Thank you. But actually it is not a desirable change\n> Hope That Helps.\n>\n> ATB,\n> Ramsay Jones\n>\n>\n>\n"},{"id":"282616","messageId":"CA+EOSBkY-Tsz3ZAfK3uAXJsrE585cOhyxjsU4FhgDMFC-ypkUg@mail.gmail.com","threadId":"41898","inReplyTo":"xmqqtwjljq97.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-04T16:08:38Z","receivedAt":"2016-04-04T16:08:38Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2016-04-01 17:35 GMT+02:00 Junio C Hamano :\n> Elia Pinto writes:\n>\n>> Implements the GIT_CURL_DEBUG environment variable to allow a greater\n>> degree of detail of GIT_CURL_VERBOSE, in particular the complete\n>> transport header and all the data payload exchanged.\n>> It might be useful if a particular situation could require a more\n>> thorough debugging analysis.\n>\n> My impression is that using GIT_TRACE_* is the more mainstream\n> trend, and it may be beneficial to work any new debugging aid like\n> this one to fit within that mechanism.\n\nI thought about it, and I agree with you. The idea could be\n\n- Call the variable GIT_TRACE_CURL_DEBUG instead\n- Add the new GIT_TRACE_CURL_VERBOSE variable, keeping\nGIT_CURL_VERBOSE for compatibility\n- Documenting these GIT_TRACE_CURL_XXX variables (GIT_CURL_VERBOSE\nit is not even documented i think)\n- perhaps use the git trace api in doing these new patches\n\nLook reasonable? It seems reasonable? I'd like your own opinion\n\nThank you for your suggestion\n>\n> I am not saying new GIT_*_DEBUG is wrong. I just wanted to make\n> sure you have considered doing this as a new trace in GIT_TRACE_*\n> family and rejected that apporach with a very good reason, in\n> which case that rationale deserves to be in the log message.\n"},{"id":"282620","messageId":"xmqqr3el5nen.fsf@gitster.mtv.corp.google.com","threadId":"41898","inReplyTo":"CA+EOSBkY-Tsz3ZAfK3uAXJsrE585cOhyxjsU4FhgDMFC-ypkUg@mail.gmail.com","subject":"Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-04T16:50:08Z","receivedAt":"2016-04-04T16:50:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n>> My impression is that using GIT_TRACE_* is the more mainstream\n>> trend, and it may be beneficial to work any new debugging aid like\n>> this one to fit within that mechanism.\n>\n> I thought about it, and I agree with you. The idea could be\n>\n> - Call the variable GIT_TRACE_CURL_DEBUG instead\n\nI think GIT_TRACE_CURL_DEBUG is overly verbose; tracing by\ndefinition is a debugging aid.\n\n> - Add the new GIT_TRACE_CURL_VERBOSE variable, keeping\n> GIT_CURL_VERBOSE for compatibility\n\nI do not care too deeply either way.\n\n - GIT_CURL_VERBOSE can stay the same as-is and show its output to\n whatever output channel it spits things out.\n\n - Or it can be a synonym for GIT_TRACE_CURL=2 (as I understand that\n the VERBOSE output goes to the standard error stream)\n\nIf you want tracing as debugging aid and existing CURL_VERBOSE\northogonal, it would probably make more sense to go the former\nroute, not linking this new \"DEBUG\" thing with the existing\n\"VERBOSE\" thing.\n\n> - Documenting these GIT_TRACE_CURL_XXX variables (GIT_CURL_VERBOSE\n> it is not even documented i think)\n\nIf we decide to leave them untangled, this is not necessary.\n\n> - perhaps use the git trace api in doing these new patches\n>\n> Look reasonable? It seems reasonable? I'd like your own opinion\n\nNot really sensible as long as you have that \"perhaps\" in the list.\n\nSomething that does not use the trace API shouldn't pretend to by\nusing GIT_TRACE_* names.\n\nGIT_TRACE_CURL could be your new thing and would decide where to\nshow its output by using the GIT_TRACE_* api.\n"},{"id":"282694","messageId":"CA+EOSBmL8oHo_wWUzyBq=4Z7fFoFj5Gnf7UveOyiNMS3yhH8Uw@mail.gmail.com","threadId":"41898","inReplyTo":"CAPig+cRrmdiz4fHAX3xg6CinSOPBv207RZLaacRBkoDNHLaPwA@mail.gmail.com","subject":"Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-05T10:21:03Z","receivedAt":"2016-04-05T10:21:03Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"2016-04-01 22:25 GMT+02:00 Eric Sunshine :\n> On Fri, Apr 1, 2016 at 6:44 AM, Elia Pinto wrote:\n>> Implements the GIT_CURL_DEBUG environment variable to allow a greater\n>> degree of detail of GIT_CURL_VERBOSE, in particular the complete\n>> transport header and all the data payload exchanged.\n>> It might be useful if a particular situation could require a more\n>> thorough debugging analysis.\n>\n> In addition to review comments by others, why are the new curl_dump()\n> and curl_trace() functions duplicated in both patches rather than\n> factored out to a shared implementation?\nIt's right. Do you think i can use some existing file or should I\ncreate a new object file ?\n\nThank you very much\n>\n>> Signed-off-by: Elia Pinto \n>> ---\n>> diff --git a/imap-send.c b/imap-send.c\n>> @@ -1395,6 +1395,96 @@ static int append_msgs_to_imap(struct imap_server_conf *server,\n>> }\n>>\n>> #ifdef USE_CURL_FOR_IMAP_SEND\n>> +\n>> +static\n>> +void curl_dump(const char *text,\n>> + FILE * stream, unsigned char *ptr, size_t size, char nohex)\n>> +{\n>> + size_t i;\n>> + size_t c;\n>> +\n>> + unsigned int width = 0x10;\n>> +\n>> + if (nohex)\n>> + /* without the hex output, we can fit more on screen */\n>> + width = 0x40;\n>> +\n>> + fprintf(stream, \"%s, %10.10ld bytes (0x%8.8lx)\\n\",\n>> + text, (long)size, (long)size);\n>> +\n>> + for (i = 0; i < size; i += width) {\n>> +\n>> + fprintf(stream, \"%4.4lx: \", (long)i);\n>> +\n>> + if (!nohex) {\n>> + /* hex not disabled, show it */\n>> + for (c = 0; c < width; c++)\n>> + if (i + c < size)\n>> + fprintf(stream, \"%02x \", ptr[i + c]);\n>> + else\n>> + fputs(\" \", stream);\n>> + }\n>> +\n>> + for (c = 0; (c < width) && (i + c < size); c++) {\n>> + /* check for 0D0A; if found, skip past and start a new line of output */\n>> + if (nohex && (i + c + 1 < size) && ptr[i + c] == 0x0D\n>> + && ptr[i + c + 1] == 0x0A) {\n>> + i += (c + 2 - width);\n>> + break;\n>> + }\n>> + fprintf(stream, \"%c\",\n>> + (ptr[i + c] >= 0x20)\n>> + && (ptr[i + c] < 0x80) ? ptr[i + c] : '.');\n>> + /* check again for 0D0A, to avoid an extra \\n if it's at width */\n>> + if (nohex && (i + c + 2 < size)\n>> + && ptr[i + c + 1] == 0x0D\n>> + && ptr[i + c + 2] == 0x0A) {\n>> + i += (c + 3 - width);\n>> + break;\n>> + }\n>> + }\n>> + fputc('\\n', stream); /* newline */\n>> + }\n>> + fflush(stream);\n>> +}\n>> +\n>> +static\n>> +int curl_trace(CURL * handle, curl_infotype type,\n>> + char *data, size_t size, void *userp)\n>> +{\n>> + const char *text;\n>> + (void)handle; /* prevent compiler warning */\n>> +\n>> + switch (type) {\n>> + case CURLINFO_TEXT:\n>> + fprintf(stderr, \"== Info: %s\", data);\n>> + default: /* in case a new one is introduced to shock us */\n>> + return 0;\n>> +\n>> + case CURLINFO_HEADER_OUT:\n>> + text = \"=> Send header\";\n>> + break;\n>> + case CURLINFO_DATA_OUT:\n>> + text = \"=> Send data\";\n>> + break;\n>> + case CURLINFO_SSL_DATA_OUT:\n>> + text = \"=> Send SSL data\";\n>> + break;\n>> + case CURLINFO_HEADER_IN:\n>> + text = \"<= Recv header\";\n>> + break;\n>> + case CURLINFO_DATA_IN:\n>> + text = \"<= Recv data\";\n>> + break;\n>> + case CURLINFO_SSL_DATA_IN:\n>> + text = \"<= Recv SSL data\";\n>> + break;\n>> + }\n>> +\n>> + curl_dump(text, stderr, (unsigned char *)data, size, 1);\n>> + return 0;\n>> +}\n"},{"id":"282743","messageId":"CAPig+cSR6CQ6Q=TOaTgqwC0nOkB_khDLevWDPZsOwKbGrjptKQ@mail.gmail.com","threadId":"41898","inReplyTo":"CA+EOSBmL8oHo_wWUzyBq=4Z7fFoFj5Gnf7UveOyiNMS3yhH8Uw@mail.gmail.com","subject":"Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-04-06T05:53:35Z","receivedAt":"2016-04-06T05:53:35Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Apr 5, 2016 at 6:21 AM, Elia Pinto wrote:\n> 2016-04-01 22:25 GMT+02:00 Eric Sunshine :\n>> In addition to review comments by others, why are the new curl_dump()\n>> and curl_trace() functions duplicated in both patches rather than\n>> factored out to a shared implementation?\n>\n> It's right. Do you think i can use some existing file or should I\n> create a new object file ?\n\nPeff or Junio would be more qualified to answer, but perhaps the\nshared implementation could go in http.c?\n"},{"id":"282747","messageId":"20160406061650.GA18208@sigill.intra.peff.net","threadId":"41898","inReplyTo":"CAPig+cSR6CQ6Q=TOaTgqwC0nOkB_khDLevWDPZsOwKbGrjptKQ@mail.gmail.com","subject":"Re: [PATCH 1/2] imap-send.c: implements the GIT_CURL_DEBUG environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-06T06:16:50Z","receivedAt":"2016-04-06T06:16:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 06, 2016 at 01:53:35AM -0400, Eric Sunshine wrote:\n\n> On Tue, Apr 5, 2016 at 6:21 AM, Elia Pinto wrote:\n> > 2016-04-01 22:25 GMT+02:00 Eric Sunshine :\n> >> In addition to review comments by others, why are the new curl_dump()\n> >> and curl_trace() functions duplicated in both patches rather than\n> >> factored out to a shared implementation?\n> >\n> > It's right. Do you think i can use some existing file or should I\n> > create a new object file ?\n> \n> Peff or Junio would be more qualified to answer, but perhaps the\n> shared implementation could go in http.c?\n\nYeah. If it is linking with curl it must be in one of the objects that\ngets compiled only when NO_CURL is not set. Which is currently http.c,\nhttp-walker.c, and remote-curl.c. Of the three, the only one that makes\nsense is http.c.\n\nA new file would be OK (provided it gets added to the OBJECTS line in\nMakefile:1921) but I think it fits well in http.c, which is essentially\nour bucket of curl-related wrappers.\n\n-Peff\n"}]}