{"thread":{"id":"42091","subject":"[PATCHv3 2/3] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable","startedAt":"2016-04-20T16:28:22Z","lastAt":"2016-04-20T20:45:00Z","messageCount":10,"participants":["Elia Pinto","Junio C Hamano","Ramsay Jones","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"283963","messageId":"20160420162825.62380-1-gitter.spiros@gmail.com","threadId":"42091","inReplyTo":null,"subject":"[PATCHv3 0/3] Implements the GIT_TRACE_CURL environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-20T16:28:22Z","receivedAt":"2016-04-20T16:28:22Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"\nThis is the third version but in reality is the complete rewriting of the patches discussed here\n(here called V1)\n\n$gmane/290520\n$gmane/290521\n\n*Changes from V2\n($gmane/291868)\n\n- fix garbage comment in http.c (i am very sorry for that)\n- add final '.' to the commit message for imap-send.c and to other commit messages\n- typofix double ; in http.c\n- merge the nice cleanup and code refactoring of Ramsay Jones (Thank you very much !!)\n- squash the previous commit 2/4\n\n*Changes from V1\n\n- introduced GIT_TRACE_CURL variable with its documentation\n- changed the name of the temporary variable \"i\" in \"w\" in the helper routine\n- used the c escape sequences instead of the hex equivalent\n- dropped the previous GIT_DEBUG_CURL env var\n- curl_dump and curl_trace factored out to a shared implementation\nin http.c\n\nElia Pinto (3):\n git.txt: document the new GIT_TRACE_CURL environment variable\n imap-send.c: introduce the GIT_TRACE_CURL enviroment variable\n http.c: implements the GIT_TRACE_CURL environment variable\n\n Documentation/git.txt | 8 ++++\n http.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 6 +++\n imap-send.c | 6 +++\n 4 files changed, 120 insertions(+), 1 deletion(-)\n\n-- \n2.8.1.383.g31b84cc\n"},{"id":"283964","messageId":"20160420162825.62380-2-gitter.spiros@gmail.com","threadId":"42091","inReplyTo":"20160420162825.62380-1-gitter.spiros@gmail.com","subject":"[PATCHv3 1/3] git.txt: document the new GIT_TRACE_CURL environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-20T16:28:23Z","receivedAt":"2016-04-20T16:28:23Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Describe the purpose of the GIT_TRACE_CURL environment variable.\n\nHelped-by: Torsten Bögershausen \nHelped-by: Ramsay Jones \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nHelped-by: Jeff King \nSigned-off-by: Elia Pinto \n---\n Documentation/git.txt | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 8afe349..958db0f 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -1075,6 +1075,14 @@ of clones and fetches.\n \tcloning of shallow repositories.\n \tSee 'GIT_TRACE' for available trace output options.\n \n+'GIT_TRACE_CURL'::\n+\tEnables a curl full trace dump of all incoming and outgoing data,\n+\tincluding descriptive information, of the git transport protocol.\n+\tThis is similar to doing curl --trace-ascii on the command line.\n+\tThis option overrides setting the GIT_CURL_VERBOSE environment\n+\tvariable.\n+\tSee 'GIT_TRACE' for available trace output options.\n+\n 'GIT_LITERAL_PATHSPECS'::\n \tSetting this variable to `1` will cause Git to treat all\n \tpathspecs literally, rather than as glob patterns. For example,\n-- \n2.8.1.383.g31b84cc\n"},{"id":"283962","messageId":"20160420162825.62380-3-gitter.spiros@gmail.com","threadId":"42091","inReplyTo":"20160420162825.62380-1-gitter.spiros@gmail.com","subject":"[PATCHv3 2/3] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-20T16:28:24Z","receivedAt":"2016-04-20T16:28:24Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Permit the use of the GIT_TRACE_CURL environment variable calling\nthe curl_trace and curl_dump http.c helper routine.\n\nHelped-by: Torsten Bögershausen \nHelped-by: Ramsay Jones \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nHelped-by: Jeff King \nSigned-off-by: Elia Pinto \n---\n imap-send.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 938c691..61c6787 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1444,6 +1444,12 @@ static CURL *setup_curl(struct imap_server_conf *srvc)\n \tif (0 < verbosity || getenv(\"GIT_CURL_VERBOSE\"))\n \t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n \n+\tif (curl_trace_want()) {\n+\t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n+\t\tcurl_easy_setopt(curl, CURLOPT_DEBUGFUNCTION, curl_trace);\n+\t\tcurl_easy_setopt(curl, CURLOPT_DEBUGDATA, NULL);\n+\t}\n+\n \treturn curl;\n }\n \n-- \n2.8.1.383.g31b84cc\n"},{"id":"283965","messageId":"20160420162825.62380-4-gitter.spiros@gmail.com","threadId":"42091","inReplyTo":"20160420162825.62380-1-gitter.spiros@gmail.com","subject":"[PATCHv3 3/3] http.c: implements the GIT_TRACE_CURL environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-20T16:28:25Z","receivedAt":"2016-04-20T16:28:25Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Implements the GIT_TRACE_CURL environment variable to allow a\ngreater degree of detail of GIT_CURL_VERBOSE, in particular\nthe complete transport header and all the data payload exchanged.\nIt might be useful if a particular situation could require a more\nthorough debugging analysis.\n\nHelped-by: Torsten Bögershausen \nHelped-by: Ramsay Jones \nHelped-by: Junio C Hamano \nHelped-by: Eric Sunshine \nHelped-by: Jeff King \nSigned-off-by: Elia Pinto \n---\n http.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 6 ++++\n 2 files changed, 106 insertions(+), 1 deletion(-)\n\ndiff --git a/http.c b/http.c\nindex 4304b80..507c386 100644\n--- a/http.c\n+++ b/http.c\n@@ -11,6 +11,7 @@\n #include \"gettext.h\"\n #include \"transport.h\"\n \n+static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n #if LIBCURL_VERSION_NUM >= 0x070a08\n long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;\n #else\n@@ -464,6 +465,100 @@ static void set_curl_keepalive(CURL *c)\n }\n #endif\n \n+\n+void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)\n+{\n+\tsize_t i;\n+\tsize_t w;\n+\tstruct strbuf out = STRBUF_INIT;\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+\tstrbuf_addf(&out, \"%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\tstrbuf_addf(&out, \"%4.4lx: \", (long)i);\n+\n+\t\tif (!nohex) {\n+\t\t\t/* hex not disabled, show it */\n+\t\t\tfor (w = 0; w < width; w++)\n+\t\t\t\tif (i + w < size)\n+\t\t\t\t\tstrbuf_addf(&out, \"%02x \", ptr[i + w]);\n+\t\t\t\telse\n+\t\t\t\t\tstrbuf_addf(&out,\" \");\n+\t\t}\n+\n+\t\tfor (w = 0; (w < width) && (i + w < size); w++) {\n+\t\t\t/* check for 0D0A; if found, skip past and start a new line of output */\n+\t\t\tif (nohex && (i + w + 1 < size) && ptr[i + w] == '\\r'\n+\t\t\t && ptr[i + w + 1] == '\\n') {\n+\t\t\t\ti += (w + 2 - width);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tstrbuf_addch(&out, (ptr[i + w] >= 0x20)\n+\t\t\t\t&& (ptr[i + w] < 0x80) ? ptr[i + w] : '.');\n+\t\t\t/* check again for 0D0A, to avoid an extra \\n if it's at width */\n+\t\t\tif (nohex && (i + w + 2 < size)\n+\t\t\t && ptr[i + w + 1] == '\\r'\n+\t\t\t && ptr[i + w + 2] == '\\n') {\n+\t\t\t\ti += (w + 3 - width);\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t}\n+\t\tstrbuf_addch(&out, '\\n');\n+\t\ttrace_strbuf(&trace_curl, &out);\n+\t\tstrbuf_release(&out);\n+\t}\n+}\n+\n+int curl_trace_want(void)\n+{\n+\treturn trace_want(&trace_curl);\n+}\n+\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\ttrace_printf_key(&trace_curl, \"== 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, (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@@ -563,7 +658,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 (curl_trace_want()) {\n+\t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1L);\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,\ndiff --git a/http.h b/http.h\nindex 4ef4bbd..e452f6a 100644\n--- a/http.h\n+++ b/http.h\n@@ -224,4 +224,10 @@ extern int finish_http_object_request(struct http_object_request *freq);\n extern void abort_http_object_request(struct http_object_request *freq);\n extern void release_http_object_request(struct http_object_request *freq);\n \n+/* Debug callback and helper routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */\n+int curl_trace_want(void);\n+int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp);\n+void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex);\n+\n+\n #endif /* HTTP_H */\n-- \n2.8.1.383.g31b84cc\n"},{"id":"283977","messageId":"xmqq4maww2bk.fsf@gitster.mtv.corp.google.com","threadId":"42091","inReplyTo":"20160420162825.62380-1-gitter.spiros@gmail.com","subject":"Re: [PATCHv3 0/3] Implements the GIT_TRACE_CURL environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-20T18:41:03Z","receivedAt":"2016-04-20T18:41:03Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n> Elia Pinto (3):\n> git.txt: document the new GIT_TRACE_CURL environment variable\n> imap-send.c: introduce the GIT_TRACE_CURL enviroment variable\n> http.c: implements the GIT_TRACE_CURL environment variable\n\nI think 2 & 3 need to be swapped; otherwise 2 would refer to\nyet-to-be-invented curl_trace() function, and break the build\nwithout 3, no?\n\nStrictly speaking 1 should come at the end for the same reason, as\nsetting GIT_TRACE_CURL after seeing that commit would not give users\nanything new.\n\nOther than that, I didn't find anything blatantly wrong ;-). Will\nnitpick individual patches later but I expect that it would be\nsufficient to locally tweak while queuing without rerolling.\n\nThanks.\n"},{"id":"283978","messageId":"xmqqzisoun5n.fsf@gitster.mtv.corp.google.com","threadId":"42091","inReplyTo":"20160420162825.62380-4-gitter.spiros@gmail.com","subject":"Re: [PATCHv3 3/3] http.c: implements the GIT_TRACE_CURL environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-20T18:53:56Z","receivedAt":"2016-04-20T18:53:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n> Implements the GIT_TRACE_CURL environment variable to allow a\n\ns/Implements/Implement/; speak as if you are giving an order to the\ncodebase to \"be like so\".\n\n> greater degree of detail of GIT_CURL_VERBOSE, in particular\n> the complete 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> Helped-by: Torsten Bögershausen \n> Helped-by: Ramsay Jones \n> Helped-by: Junio C Hamano \n> Helped-by: Eric Sunshine \n> Helped-by: Jeff King \n> Signed-off-by: Elia Pinto \n> ---\n> http.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n> http.h | 6 ++++\n> 2 files changed, 106 insertions(+), 1 deletion(-)\n>\n> diff --git a/http.c b/http.c\n> index 4304b80..507c386 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -11,6 +11,7 @@\n> #include \"gettext.h\"\n> #include \"transport.h\"\n> \n> +static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n> #if LIBCURL_VERSION_NUM >= 0x070a08\n> long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;\n> #else\n> @@ -464,6 +465,100 @@ static void set_curl_keepalive(CURL *c)\n> }\n> #endif\n> \n> +\n\nNo need for this extra blank line.\n\n> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)\n> +{\n> +\tsize_t i;\n> +\tsize_t w;\n\nIs it better to narrow the scope of 'w' to the \"for (i)\" loop?\n\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\n\nNo need for this extra blank line.\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> +\tstrbuf_addf(&out, \"%s, %10.10ld bytes (0x%8.8lx)\\n\",\n> +\t\ttext, (long)size, (long)size);\n\nInconsistent indentation that uses only two HT, when existing\nlines and other new lines in thsi patch align with HT with SP\nto match \"(\" on the previous line.\n\n> +\n> +\tfor (i = 0; i < size; i += width) {\n> +\n> +\t\tstrbuf_addf(&out, \"%4.4lx: \", (long)i);\n> +\n> +\t\tif (!nohex) {\n> +\t\t\t/* hex not disabled, show it */\n\nUnlike the previous \"without the hex, we can fit more\", this comment\nis probably adds more noise than value.\n\n> +\t\t\tfor (w = 0; w < width; w++)\n> +\t\t\t\tif (i + w < size)\n> +\t\t\t\t\tstrbuf_addf(&out, \"%02x \", ptr[i + w]);\n> +\t\t\t\telse\n> +\t\t\t\t\tstrbuf_addf(&out,\" \");\n> +\t\t}\n> +\n> +\t\tfor (w = 0; (w < width) && (i + w < size); w++) {\n> +\t\t\t/* check for 0D0A; if found, skip past and start a new line of output */\n> +\t\t\tif (nohex && (i + w + 1 < size) && ptr[i + w] == '\\r'\n> +\t\t\t && ptr[i + w + 1] == '\\n') {\n> +\t\t\t\ti += (w + 2 - width);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t\tstrbuf_addch(&out, (ptr[i + w] >= 0x20)\n> +\t\t\t\t&& (ptr[i + w] < 0x80) ? ptr[i + w] : '.');\n\nLikewise.\n\n> +\t\t\t/* check again for 0D0A, to avoid an extra \\n if it's at width */\n> +\t\t\tif (nohex && (i + w + 2 < size)\n> +\t\t\t && ptr[i + w + 1] == '\\r'\n> +\t\t\t && ptr[i + w + 2] == '\\n') {\n> +\t\t\t\ti += (w + 3 - width);\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n\nThis may only be the matter of taste, but I somehow found this \"we\npretend to go width bytes at a time, unless there is a line-break in\nwhich case we may fold before we hit width bytes\" and need for\ncompensating with \"w+3-width\" here unnecessarily convoluted. I\nwonder if the code becomes clearer if insterad you said \"we go one\nline at a time, but we may fold the line if it is wider than width\nbytes\"?\n\n> +\t\tstrbuf_addch(&out, '\\n');\n> +\t\ttrace_strbuf(&trace_curl, &out);\n> +\t\tstrbuf_release(&out);\n> +\t}\n> +}\n> +\n> +int curl_trace_want(void)\n> +{\n> +\treturn trace_want(&trace_curl);\n> +}\n> +\n> +int curl_trace(CURL *handle, curl_infotype type,\n> +\t char *data, size_t size, void *userp)\n\nInconsistent indentation.\n\n> +{\n> +\tconst char *text;\n> +\t(void)handle;\t\t/* prevent compiler warning */\n\nWhat compiler warning? Usually unused parameter (not unused\nvariable) is not something compilers warn against.\n\n> +\tswitch (type) {\n> +\tcase CURLINFO_TEXT:\n> +\t\ttrace_printf_key(&trace_curl, \"== Info: %s\", data);\n> +\tdefault:\t\t/* in case a new one is introduced to shock us */\n\nThe comment bothers me.\n\nWhat is the longer term plan for this function? Do we expect to\nignore some type of data, or do we expect to show all new type of\ndata? If the former, \"we ignore unknown types by default\" is fine,\nand if the latter, it probably makes more sense to show with\ntext=\"unknown type\"?\n\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, (unsigned char *)data, size, 1);\n> +\treturn 0;\n> +}\n"},{"id":"283988","messageId":"5717DD63.6050002@ramsayjones.plus.com","threadId":"42091","inReplyTo":"xmqq4maww2bk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 0/3] Implements the GIT_TRACE_CURL environment variable","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-04-20T19:49:55Z","receivedAt":"2016-04-20T19:49:55Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 20/04/16 19:41, Junio C Hamano wrote:\n> Elia Pinto writes:\n> \n>> Elia Pinto (3):\n>> git.txt: document the new GIT_TRACE_CURL environment variable\n>> imap-send.c: introduce the GIT_TRACE_CURL enviroment variable\n>> http.c: implements the GIT_TRACE_CURL environment variable\n> \n> I think 2 & 3 need to be swapped; otherwise 2 would refer to\n> yet-to-be-invented curl_trace() function, and break the build\n> without 3, no?\n> \n> Strictly speaking 1 should come at the end for the same reason, as\n> setting GIT_TRACE_CURL after seeing that commit would not give users\n> anything new.\n\nYep, I was just about to send an email saying that the patches should\nbe in the exact opposite order! (ie. 1->3 and 3->1) That is *if* you\nwant to keep them as a series. I would squash them into one patch ...\n\n> Other than that, I didn't find anything blatantly wrong ;-). Will\n> nitpick individual patches later but I expect that it would be\n> sufficient to locally tweak while queuing without rerolling.\n\nI have one small issue ... \n\nATB,\nRamsay Jones\n"},{"id":"283991","messageId":"5717DEEC.4000903@ramsayjones.plus.com","threadId":"42091","inReplyTo":"xmqqzisoun5n.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv3 3/3] http.c: implements the GIT_TRACE_CURL environment variable","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-04-20T19:56:28Z","receivedAt":"2016-04-20T19:56:28Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 20/04/16 19:53, Junio C Hamano wrote:\n> Elia Pinto writes:\n> \n>> Implements the GIT_TRACE_CURL environment variable to allow a\n> \n> s/Implements/Implement/; speak as if you are giving an order to the\n> codebase to \"be like so\".\n> \n>> greater degree of detail of GIT_CURL_VERBOSE, in particular\n>> the complete 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>> Helped-by: Torsten Bögershausen \n>> Helped-by: Ramsay Jones \n>> Helped-by: Junio C Hamano \n>> Helped-by: Eric Sunshine \n>> Helped-by: Jeff King \n>> Signed-off-by: Elia Pinto \n>> ---\n>> http.c | 101 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n>> http.h | 6 ++++\n>> 2 files changed, 106 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/http.c b/http.c\n>> index 4304b80..507c386 100644\n>> --- a/http.c\n>> +++ b/http.c\n>> @@ -11,6 +11,7 @@\n>> #include \"gettext.h\"\n>> #include \"transport.h\"\n>> \n>> +static struct trace_key trace_curl = TRACE_KEY_INIT(CURL);\n>> #if LIBCURL_VERSION_NUM >= 0x070a08\n>> long int git_curl_ipresolve = CURL_IPRESOLVE_WHATEVER;\n>> #else\n>> @@ -464,6 +465,100 @@ static void set_curl_keepalive(CURL *c)\n>> }\n>> #endif\n>> \n>> +\n> \n> No need for this extra blank line.\n> \n>> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)\n>> +{\n>> +\tsize_t i;\n>> +\tsize_t w;\n\nAs I said in a previous email, curl_dump() should be marked\nstatic here (and remove the declaration from http.h). Unless,\nof course, there are plans for future use/calls being added?\n\nATB,\nRamsay Jones\n"},{"id":"283993","messageId":"CAPc5daWhDToqoeeGrhzjVuDgNu1ar0RvF10iKpH-61yj=z6fVQ@mail.gmail.com","threadId":"42091","inReplyTo":"5717DEEC.4000903@ramsayjones.plus.com","subject":"Re: [PATCHv3 3/3] http.c: implements the GIT_TRACE_CURL environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-20T20:07:54Z","receivedAt":"2016-04-20T20:07:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, Apr 20, 2016 at 12:56 PM, Ramsay Jones\n wrote:\n>>> +void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex)\n>>> +{\n>>> + size_t i;\n>>> + size_t w;\n>\n> As I said in a previous email, curl_dump() should be marked\n> static here (and remove the declaration from http.h). Unless,\n> of course, there are plans for future use/calls being added?\n\nGood point. Thanks.\n"},{"id":"283997","messageId":"20160420204500.GB14904@sigill.intra.peff.net","threadId":"42091","inReplyTo":"5717DD63.6050002@ramsayjones.plus.com","subject":"Re: [PATCHv3 0/3] Implements the GIT_TRACE_CURL environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-20T20:45:00Z","receivedAt":"2016-04-20T20:45:00Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 20, 2016 at 08:49:55PM +0100, Ramsay Jones wrote:\n\n> > Strictly speaking 1 should come at the end for the same reason, as\n> > setting GIT_TRACE_CURL after seeing that commit would not give users\n> > anything new.\n> \n> Yep, I was just about to send an email saying that the patches should\n> be in the exact opposite order! (ie. 1->3 and 3->1) That is *if* you\n> want to keep them as a series. I would squash them into one patch ...\n\nI also wondered about simply squashing them. IMHO it does not help to\nsplit documentation from the addition of a feature. It is not as if we\nwill take one over the other, and by putting them in the same patch you\ndo not have to justify one without the other.\n\n> > Other than that, I didn't find anything blatantly wrong ;-). Will\n> > nitpick individual patches later but I expect that it would be\n> > sufficient to locally tweak while queuing without rerolling.\n> \n> I have one small issue ... \n\nOverall I'm pleased at the concept, though I find the output a little\nfunny in places.\n\nMost of the \"Send/Recv SSL data\" chunks are just line noise. Do people\nactually care about seeing them? I can conceive of a case where you are\ndebugging SSL-level stuff, but I feel like you might do better using\nopenssl s_client to do so, and not git. Should we stick to more\nHTTP-level debugging?\n\nFor the actual data packets, the first line gets treated differently\nthan the rest, and you get:\n\n16:33:38.164068 http.c:515 => Send header, 0000000167 bytes (0x000000a7)\n0000: GET /git/git/info/refs?service=git-upload-pack HTTP/1.1\n16:33:38.164070 http.c:515 0039: Host: github.com\n16:33:38.164072 http.c:515 004b: User-Agent: git/2.8.1.220.g9816fc6\n...\n\nfor instance. Would it be saner to break the \"Send header...\" bit and\nthe first data line into separate trace outputs, and end up with\nsomething more like:\n\n16:33:38.164068 http.c:515 => Send header, 0000000167 bytes (0x000000a7)\n16:33:38.164069 http.c:515 0000: GET /git/git/info/refs?service=git-upload-pack HTTP/1.1\n16:33:38.164070 http.c:515 0039: Host: github.com\n16:33:38.164072 http.c:515 004b: User-Agent: git/2.8.1.220.g9816fc6\n\nOr it might even be nice to prefix each line to indicate it is about\nsending a header. That would make it much easier to grep for just\nparticular \n\nIt might even be nice to prefix _all_ of the lines with some state\ninformation, like \"send header\". That's more verbose, but makes it much\neasier to pick out snippets with line-oriented tools like grep. I often\nfind myself doing that kind of thing, either to inspect a subset of the\noutput, or because I want to be able to pull out things like request\ncontent verbatim so I can replay it.\n\nOne of my complaints with GIT_CURL_VERBOSE is that it puts your\ncredentials into the debugging output. Since it looks like we're\nparsing through the data anyway, I wonder if we could auto-censor\nAuthorization headers by default (and then possibly output them if an\nextra variable is given). That would make it safe to ask people to show\nthe output of GIT_CURL_TRACE on the list without having to explain\nfurther.\n\n-Peff\n"}]}