{"thread":{"id":"42168","subject":"[PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable","startedAt":"2016-04-28T11:57:46Z","lastAt":"2016-04-28T18:05:31Z","messageCount":10,"participants":["Elia Pinto","Jeff King","Stefan Beller","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"284789","messageId":"20160428115748.37177-1-gitter.spiros@gmail.com","threadId":"42168","inReplyTo":null,"subject":"[PATCHv4 0/2] Implement the GIT_TRACE_CURL environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-28T11:57:46Z","receivedAt":"2016-04-28T11:57:46Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"This is the fourth 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 V3\n($gmane/292040)\n\n- add missing static to curl_dump\n- reorder the patch order\n- tried to fix all (but i am not sure) the problems reported by Julio ($gmane/292055)\n- * squash the documentation with the http.c commit.\n * in the trace prefix each line to indicate it is about sending a header, and drop the \n initial hex count\n * auto-censor Authorization headers by default \n\n as suggested by Jeff King ($gmane/292074)\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\nElia Pinto (2):\n http.c: implement the GIT_TRACE_CURL environment variable\n imap-send.c: introduce the GIT_TRACE_CURL enviroment variable\n\n Documentation/git.txt | 8 ++++\n http.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 4 ++\n imap-send.c | 6 +++\n 4 files changed, 126 insertions(+), 1 deletion(-)\n\n-- \n2.8.1.487.gf8c3767.dirty\n"},{"id":"284788","messageId":"20160428115748.37177-2-gitter.spiros@gmail.com","threadId":"42168","inReplyTo":"20160428115748.37177-1-gitter.spiros@gmail.com","subject":"[PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-28T11:57:47Z","receivedAt":"2016-04-28T11:57:47Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Implement 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. Document the new GIT_TRACE_CURL\nenvironment 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 http.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 4 ++\n 3 files changed, 120 insertions(+), 1 deletion(-)\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,\ndiff --git a/http.c b/http.c\nindex 985b995..5c2c729 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@@ -478,6 +479,108 @@ static void set_curl_keepalive(CURL *c)\n }\n #endif\n \n+static void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex, char nopriv)\n+{\n+\tsize_t i;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tunsigned int width = 0x10;\n+\n+\t/* without the hex output, we can fit more on screen */\n+\tif (nohex) width = 0x50;\n+\n+\tstrbuf_addf(&out, \"%s, %10.10ld bytes (0x%8.8lx)\\n\",\n+\t\ttext, (long)size, (long)size);\n+\ttrace_strbuf(&trace_curl, &out);\n+\tstrbuf_reset(&out);\n+\n+\tfor (i = 0; i < size; i += width) {\n+\t\tsize_t w;\n+\t\tstrbuf_addf(&out, \"%s: \", text);\n+\t\tif (!nohex) {\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+\t\tfor (w = 0; (w < width) && (i + w < size); w++) {\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\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\t/* if we are called with nopriv we skip the Authorization field if present\n+\t\t * and print a blank line\n+\t\t*/\n+\t\tif ( nopriv && strstr(out.buf, \"Authorization:\"))\n+\t\t\tstrbuf_reset(&out);\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, char *data, size_t size, void *userp)\n+{\n+\tconst char *text;\n+\t(void)handle;\t\t/* prevent compiler unused parameter warning if checked */\n+\t(void)userp;\t\t/* prevent compiler unused parameter warning if checked */\n+\tchar nopriv = 0;\t/* default: there are no sensitive data\n+\t\t\t\t * in the trace to be skipped\n+\t\t\t\t*/\n+\n+\tswitch (type) {\n+\tcase CURLINFO_TEXT:\n+\t\ttrace_printf_key(&trace_curl, \"== Info: %s\", data);\n+\tdefault:\t\t/* we ignore unknown types by default */\n+\t\treturn 0;\n+\n+\tcase CURLINFO_HEADER_OUT:\n+\t\ttext = \"=> Send header\";\n+\t\tnopriv = 1;\n+\t\tbreak;\n+\tcase CURLINFO_DATA_OUT:\n+\t\ttext = \"=> Send data\";\n+\t\tnopriv = 0;\n+\t\tbreak;\n+\tcase CURLINFO_SSL_DATA_OUT:\n+\t\ttext = \"=> Send SSL data\";\n+\t\tnopriv = 0;\n+\t\tbreak;\n+\tcase CURLINFO_HEADER_IN:\n+\t\ttext = \"<= Recv header\";\n+\t\tnopriv = 0;\n+\t\tbreak;\n+\tcase CURLINFO_DATA_IN:\n+\t\ttext = \"<= Recv data\";\n+\t\tnopriv = 0;\n+\t\tbreak;\n+\tcase CURLINFO_SSL_DATA_IN:\n+\t\ttext = \"<= Recv SSL data\";\n+\t\tnopriv = 0;\n+\t\tbreak;\n+\t}\n+\n+\tcurl_dump(text, (unsigned char *)data, size, 1, nopriv);\n+\treturn 0;\n+}\n+\n+\n static CURL *get_curl_handle(void)\n {\n \tCURL *result = curl_easy_init();\n@@ -577,7 +680,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 36f558b..1ee80e5 100644\n--- a/http.h\n+++ b/http.h\n@@ -225,4 +225,8 @@ 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+\n #endif /* HTTP_H */\n-- \n2.8.1.487.gf8c3767.dirty\n"},{"id":"284790","messageId":"20160428115748.37177-3-gitter.spiros@gmail.com","threadId":"42168","inReplyTo":"20160428115748.37177-1-gitter.spiros@gmail.com","subject":"[PATCHv4 2/2] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-04-28T11:57:48Z","receivedAt":"2016-04-28T11:57:48Z","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.487.gf8c3767.dirty\n"},{"id":"284813","messageId":"20160428144710.GD25364@sigill.intra.peff.net","threadId":"42168","inReplyTo":"20160428115748.37177-2-gitter.spiros@gmail.com","subject":"Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-28T14:47:10Z","receivedAt":"2016-04-28T14:47:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 28, 2016 at 11:57:47AM +0000, Elia Pinto wrote:\n\n> +static void curl_dump(const char *text, unsigned char *ptr, size_t size, char nohex, char nopriv)\n\nWe usually use \"int\" for our boolean flags. Space savings don't matter\noutside of a struct (and if they did, you should be using a single flags\nfield), and this way the user does not have to guess whether the \"char\"\nis significant.\n\nIt looks like we never pass anything but \"1\" for nohex. Can we drop this\nparameter entirely? But see below...\n\n> +{\n> +\tsize_t i;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tunsigned int width = 0x10;\n> +\n> +\t/* without the hex output, we can fit more on screen */\n> +\tif (nohex) width = 0x50;\n\nMaybe it is just me, but I think this is more readable using decimal\nconstants. I mind it less in checking ASCII values like 0x20, but here I\nthink just saying \"80\" is more customary.\n\n> +\tfor (i = 0; i < size; i += width) {\n> +\t\tsize_t w;\n> +\t\tstrbuf_addf(&out, \"%s: \", text);\n\nI really like this new format. Doing:\n\n GIT_TRACE_CURL=1 git ... 2>&1 | grep '=> Send header: '\n\nis very readable.\n\nHowever, I did run into an interesting case. The output looks like:\n\n 10:24:04.540803 http.c:527 => Send header: Host: github.com\n 10:24:04.540809 http.c:527 => Send header: x\n 10:24:04.540811 http.c:527 => Send header: User-Agent: git/2.8.1.341.g2caf4c9.dirty\n\nWhat's that weird \"x\" line?\n\nIt turns out that the line before it is:\n\n Authorization: Basic some-really-long-opaque-token-that-ends-in-x\n\nSince we break at a newline _or_ at the width, that gets broken onto the\nfollowing line. The Authorization line hits the code below to suppress\nthe output.\n\nSo not only do I find the breaking of the line hard to read, but it\nmeans we may leak data from the Authorization line that got broken into\nthe next chunk (here it was only one character, but with a sufficiently\nlong header, it could be real data).\n\nSo I think we probably want to _just_ break at newlines, however long\nthey are.\n\nBut that probably isn't a good idea for binary data. So I'd suggest that\nsending/receiving headers break on newlines, and actual body data should\nrespect the width field (we may still have line-oriented data in the\nbody which would be easier to read without line-breaking, but if you are\ndebugging that you are better off with GIT_TRACE_PACKET anyway).\n\n> +\t\t for (w = 0; (w < width) && (i + w < size); w++) {\n> +\t\t\t if (nohex && (i + w + 1 < size) && ptr[i + w] == '\\r'\n> +\t\t\t\t&& ptr[i + w + 1] == '\\n') {\n> +\t\t\t\t i += (w + 2 - width);\n> +\t\t\t\t break;\n> +\t\t\t }\n\nThis loop puzzled me for a bit. When we end early due to a newline, we\nsubtract out the width here. I guess that's to accomodate the \"i +=\nwidth\" that the outer for-loop is going to do.\n\nIf you follow my suggestion above to split the code paths for\nline-oriented and fixed-width data, then this all gets much simpler.\n\n> +\t\t/* if we are called with nopriv we skip the Authorization field if present\n> +\t\t * and print a blank line\n> +\t\t*/\n> +\t\tif ( nopriv && strstr(out.buf, \"Authorization:\"))\n> +\t\t\tstrbuf_reset(&out);\n\nStyle: multi-line comments should look like:\n\n /*\n * the comment\n * goes here\n */\n\nand there should be no whitespace after the opening \"(\".\n\nRemoving the field entirely may be a bit confusing when you're\ndebugging. Instead, perhaps we can just redact the interesting bits,\nlike:\n\ndiff --git a/http.c b/http.c\nindex 8ab0adc..30e8858 100644\n--- a/http.c\n+++ b/http.c\n@@ -481,7 +481,11 @@ static void curl_dump(const char *text, unsigned char *ptr, size_t size, char no\n \n \tfor (i = 0; i < size; i += width) {\n \t\tsize_t w;\n+\t\tsize_t prefix_len;\n+\t\tconst char *header;\n+\n \t\tstrbuf_addf(&out, \"%s: \", text);\n+\t\tprefix_len = out.len;\n \t\tif (!nohex) {\n \t\t\tfor (w = 0; w < width; w++)\n \t\t\t\tif (i + w < size)\n@@ -507,8 +511,17 @@ static void curl_dump(const char *text, unsigned char *ptr, size_t size, char no\n \t\t/* if we are called with nopriv we skip the Authorization field if present\n \t\t * and print a blank line\n \t\t*/\n-\t\tif ( nopriv && strstr(out.buf, \"Authorization:\"))\n-\t\t\tstrbuf_reset(&out);\n+\t\tif (nopriv &&\n+\t\t skip_prefix(out.buf + prefix_len, \"Authorization:\", &header)) {\n+\t\t\t/* The first token is the type, which is OK to log */\n+\t\t\twhile (isspace(*header))\n+\t\t\t\theader++;\n+\t\t\twhile (*header && !isspace(*header))\n+\t\t\t\theader++;\n+\t\t\t/* Everything else is opaque and possibly sensitive */\n+\t\t\tstrbuf_setlen(&out, header - out.buf);\n+\t\t\tstrbuf_addstr(&out, \" \");\n+\t\t}\n \n \t\tstrbuf_addch(&out, '\\n');\n \t\ttrace_strbuf(&trace_curl, &out);\n\nThat tells the viewer that we did in fact send the header (which is\nuseful to know), and which type it used.\n\n> +\t\tstrbuf_addch(&out, '\\n');\n> +\t\ttrace_strbuf(&trace_curl, &out);\n> +\t\tstrbuf_release(&out);\n> +\t}\n> +}\n\nThis is the only strbuf_release() in the function, and it's inside the\nloop. Yet we use the strbuf to print the initial line (and do a reset()\nafter). So if the field we get is 0 bytes, we'll leak the strbuf memory\nused by the initial line.\n\nI don't know if that's possible with curl or not. But just in case, we\ncould structure the loop more like:\n\n ... output initial line ...\n for (i = 0; i < size; i += width) {\n\tstrbuf_reset(&out);\n\t... output data line ...\n }\n strbuf_release(&out);\n\nThat has the added bonus that we do not have to reallocate for each\niteration of the loop (we just reset the length back to zero each time,\nand then free the memory at the very end).\n\n-Peff\n"},{"id":"284814","messageId":"20160428145524.GE25364@sigill.intra.peff.net","threadId":"42168","inReplyTo":"20160428115748.37177-3-gitter.spiros@gmail.com","subject":"Re: [PATCHv4 2/2] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-28T14:55:24Z","receivedAt":"2016-04-28T14:55:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 28, 2016 at 11:57:48AM +0000, Elia Pinto wrote:\n\n> diff --git a/imap-send.c b/imap-send.c\n> index 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\nIn the only other caller of curl_trace_want(), we repeat these exact\nsame lines (and really, what else would one do with that flag besides\nenable curl_trace?).\n\nPerhaps a better abstraction would be:\n\n int setup_curl_trace(CURL *handle)\n {\n\tif (!trace_want(&trace_curl))\n\t\treturn 0;\n\tcurl_easy_setopt(handle, CURLOPT_VERBOSE, 1L);\n\tcurl_easy_setopt(handle, CURLOPT_DEBUGFUNCTION, curl_trace);\n\tcurl_easy_setopt(handle, CURLOPT_DEBUGDATA, NULL);\n\treturn 1;\n }\n\nYou could even get rid of the return value, too. We do not use it here,\nbut just let GIT_TRACE_CURL naturally override GIT_CURL_VERBOSE by\nsetting the DEBUGFUNCTION. In the other caller, we do:\n\n if (curl_trace_want()) {\n ... set up handle ...\n } else {\n ... check and setup up GIT_CURL_VERBOSE ...\n }\n\nbut we can do the else block regardless. It is a noop if tracing is set\nup.\n\n-Peff\n"},{"id":"284847","messageId":"CAGZ79kYe-WsAJj3xkr9YkHZbHqQ9rhKV80+K0Bx3SZsB85+tHA@mail.gmail.com","threadId":"42168","inReplyTo":"20160428115748.37177-2-gitter.spiros@gmail.com","subject":"Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-28T17:26:06Z","receivedAt":"2016-04-28T17:26:06Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 28, 2016 at 4:57 AM, Elia Pinto wrote:\n> Implement the GIT_TRACE_CURL environment variable to allow a\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. Document the new GIT_TRACE_CURL\n> environment variable.\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> Documentation/git.txt | 8 ++++\n> http.c | 109 +++++++++++++++++++++++++++++++++++++++++++++++++-\n> http.h | 4 ++\n> 3 files changed, 120 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git.txt b/Documentation/git.txt\n> index 8afe349..958db0f 100644\n> --- a/Documentation/git.txt\n> +++ b/Documentation/git.txt\n> @@ -1075,6 +1075,14 @@ of clones and fetches.\n> cloning of shallow repositories.\n> See 'GIT_TRACE' for available trace output options.\n>\n> +'GIT_TRACE_CURL'::\n> + Enables a curl full trace dump of all incoming and outgoing data,\n> + including descriptive information, of the git transport protocol.\n> + This is similar to doing curl --trace-ascii on the command line.\n> + This option overrides setting the GIT_CURL_VERBOSE environment\n> + variable.\n\nHow does it overwrite the GIT_CURL_VERBOSE variable?\nAfter a quick `grep -r GIT_CURL_VERBOSE`, I notice 2 things:\n\n* apparently GIT_CURL_VERBOSE is used as a boolean,\n so I presume we assume True for GIT_CURL_VERBOSE, but\n extend it?\n* GIT_CURL_VERBOSE is not documented at all. (It is mentioned in\n the release notes for 2.3.0, not sure if that counts as documentation)\n As you know the area, care to send a documentation patch for\n GIT_CURL_VERBOSE?\n\nI am trying to understand how much more information I get by using\nGIT_TRACE_CURL instead of GIT_CURL_VERBOSE.\n\nGIT_TRACE_CURL follows the standard of GIT_TRACE_$subsystem, so I\nguess that will be the encouraged way of debugging and GIT_CURL_VERBOSE\nwill not be encouraged to the user?\n"},{"id":"284848","messageId":"xmqqfuu563gg.fsf@gitster.mtv.corp.google.com","threadId":"42168","inReplyTo":"20160428144710.GD25364@sigill.intra.peff.net","subject":"Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-28T17:35:59Z","receivedAt":"2016-04-28T17:35:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> +\t\t for (w = 0; (w < width) && (i + w < size); w++) {\n>> +\t\t\t if (nohex && (i + w + 1 < size) && ptr[i + w] == '\\r'\n>> +\t\t\t\t&& ptr[i + w + 1] == '\\n') {\n>> +\t\t\t\t i += (w + 2 - width);\n>> +\t\t\t\t break;\n>> +\t\t\t }\n>\n> This loop puzzled me for a bit. When we end early due to a newline, we\n> subtract out the width here. I guess that's to accomodate the \"i +=\n> width\" that the outer for-loop is going to do.\n\nI think I essentially said the same thing on the previous round and\nI thought I suggested to restructure the loop to primarily aim to\nsplit at line-end (instead of the above which primarily aims to\nsplit at width but line-end may cause a premature split).\n"},{"id":"284851","messageId":"20160428174423.GA32486@sigill.intra.peff.net","threadId":"42168","inReplyTo":"CAGZ79kYe-WsAJj3xkr9YkHZbHqQ9rhKV80+K0Bx3SZsB85+tHA@mail.gmail.com","subject":"Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-28T17:44:24Z","receivedAt":"2016-04-28T17:44:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 28, 2016 at 10:26:06AM -0700, Stefan Beller wrote:\n\n> > +'GIT_TRACE_CURL'::\n> > + Enables a curl full trace dump of all incoming and outgoing data,\n> > + including descriptive information, of the git transport protocol.\n> > + This is similar to doing curl --trace-ascii on the command line.\n> > + This option overrides setting the GIT_CURL_VERBOSE environment\n> > + variable.\n> \n> How does it overwrite the GIT_CURL_VERBOSE variable?\n\nYou can't use both, as they are both triggered using the CURLOPT_VERBOSE\noption of curl. The main difference is that with GIT_CURL_VERBOSE, we\nrely on curl to print the information to stderr. With GIT_CURL_TRACE, we\ndo the printing ourselves (so we can tweak the output format, send it to\nplaces other than stderr, etc).\n\n> After a quick `grep -r GIT_CURL_VERBOSE`, I notice 2 things:\n> \n> * apparently GIT_CURL_VERBOSE is used as a boolean,\n> so I presume we assume True for GIT_CURL_VERBOSE, but\n> extend it?\n\nIt's not a boolean. If the variable exists at all, we turn on verbose\noutput (so I guess you can consider it a boolean, but we do not parse\nits contents as boolean; GIT_CURL_VERBOSE=false does not do what you\nmight think).\n\n> * GIT_CURL_VERBOSE is not documented at all. (It is mentioned in\n> the release notes for 2.3.0, not sure if that counts as documentation)\n> As you know the area, care to send a documentation patch for\n> GIT_CURL_VERBOSE?\n\nI think there is no need for GIT_CURL_VERBOSE once we have\nGIT_TRACE_CURL. The latter is more flexible and matches the GIT_TRACE_*\ninterface we use elsewhere.\n\nSo I think we should consider GIT_CURL_VERBOSE deprecated (though I do\nnot mind keeping it for old-timers since it is literally one line of\ncode).\n\n-Peff\n"},{"id":"284852","messageId":"CAGZ79kaqDa5Bo2EQ-1VO6H5UpLun7emWPTmgoBAEeNaWJBaEnw@mail.gmail.com","threadId":"42168","inReplyTo":"20160428174423.GA32486@sigill.intra.peff.net","subject":"Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-04-28T17:48:38Z","receivedAt":"2016-04-28T17:48:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 28, 2016 at 10:44 AM, Jeff King wrote:\n> On Thu, Apr 28, 2016 at 10:26:06AM -0700, Stefan Beller wrote:\n>\n>> > +'GIT_TRACE_CURL'::\n>> > + Enables a curl full trace dump of all incoming and outgoing data,\n>> > + including descriptive information, of the git transport protocol.\n>> > + This is similar to doing curl --trace-ascii on the command line.\n>> > + This option overrides setting the GIT_CURL_VERBOSE environment\n>> > + variable.\n>>\n>> How does it overwrite the GIT_CURL_VERBOSE variable?\n>\n> You can't use both, as they are both triggered using the CURLOPT_VERBOSE\n> option of curl. The main difference is that with GIT_CURL_VERBOSE, we\n> rely on curl to print the information to stderr. With GIT_CURL_TRACE, we\n> do the printing ourselves (so we can tweak the output format, send it to\n> places other than stderr, etc).\n\nWell that's the information I'd rather find in the documentation\nthan in a mailing list archive ;)\n\n>\n>> After a quick `grep -r GIT_CURL_VERBOSE`, I notice 2 things:\n>>\n>> * apparently GIT_CURL_VERBOSE is used as a boolean,\n>> so I presume we assume True for GIT_CURL_VERBOSE, but\n>> extend it?\n>\n> It's not a boolean. If the variable exists at all, we turn on verbose\n> output (so I guess you can consider it a boolean, but we do not parse\n> its contents as boolean; GIT_CURL_VERBOSE=false does not do what you\n> might think).\n>\n>> * GIT_CURL_VERBOSE is not documented at all. (It is mentioned in\n>> the release notes for 2.3.0, not sure if that counts as documentation)\n>> As you know the area, care to send a documentation patch for\n>> GIT_CURL_VERBOSE?\n>\n> I think there is no need for GIT_CURL_VERBOSE once we have\n> GIT_TRACE_CURL. The latter is more flexible and matches the GIT_TRACE_*\n> interface we use elsewhere.\n>\n> So I think we should consider GIT_CURL_VERBOSE deprecated (though I do\n> not mind keeping it for old-timers since it is literally one line of\n> code).\n\nI see, so by this patch there is no need to document\nGIT_CURL_VERBOSE any more?\n\n>\n> -Peff\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"284854","messageId":"20160428180531.GA578@sigill.intra.peff.net","threadId":"42168","inReplyTo":"CAGZ79kaqDa5Bo2EQ-1VO6H5UpLun7emWPTmgoBAEeNaWJBaEnw@mail.gmail.com","subject":"Re: [PATCHv4 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-28T18:05:31Z","receivedAt":"2016-04-28T18:05:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Apr 28, 2016 at 10:48:38AM -0700, Stefan Beller wrote:\n\n> >> How does it overwrite the GIT_CURL_VERBOSE variable?\n> >\n> > You can't use both, as they are both triggered using the CURLOPT_VERBOSE\n> > option of curl. The main difference is that with GIT_CURL_VERBOSE, we\n> > rely on curl to print the information to stderr. With GIT_CURL_TRACE, we\n> > do the printing ourselves (so we can tweak the output format, send it to\n> > places other than stderr, etc).\n> \n> Well that's the information I'd rather find in the documentation\n> than in a mailing list archive ;)\n\nSure, but I'm not sure what of part of that you want to put in the\ndocumentation. I was just explaining why the implementation constrains\nus to overriding, and there really aren't any other sane options.\n\nI don't think we want to get into defining the exact set of information,\nwhich is not up to us anyway. We're just relaying whatever curl gives\nus.\n\nIMHO, we do not even need to mention GIT_CURL_VERBOSE here at all. That\nis an undocumented interface that can hopefully just be forgotten over\ntime.\n\n> > So I think we should consider GIT_CURL_VERBOSE deprecated (though I do\n> > not mind keeping it for old-timers since it is literally one line of\n> > code).\n> \n> I see, so by this patch there is no need to document\n> GIT_CURL_VERBOSE any more?\n\nExactly.\n\n-Peff\n"}]}