{"thread":{"id":"42395","subject":"[PATCH v6 2/2] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable","startedAt":"2016-05-20T10:37:06Z","lastAt":"2016-05-26T04:51:05Z","messageCount":5,"participants":["Elia Pinto","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":6,"patchTotal":2},"messages":[{"id":"287080","messageId":"20160520103708.38308-1-gitter.spiros@gmail.com","threadId":"42395","inReplyTo":null,"subject":"[PATCH v6 0/2] Implement the GIT_TRACE_CURL environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-05-20T10:37:06Z","receivedAt":"2016-05-20T10:37:06Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"This is the sixty 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 V5\n($gmane/293236)\n\n- don't export curl_trace anymore. Define it static\n- fix a minor cleanup (style) in setup_curl_trace\n- and, finally, i rewrote completely curl_dump, separating it\ninto two functions (one for http header and one for the http data),\nhoping for a coherent implementation with curl --ascii-trace output\nbut easier to read than the previous implementation that\nused a hard-to-read double-loop.\n\n \n*Changes from V4\n($gmane/292867)\n\n- add a better abstraction with the routine setup_curl_trace\n- curl_dump : drop the noex parameter, define nopriv boolean as int\n- use decimal constant where appropiate\n- fix multi-line comment\n- redo the authorization header skip with a replace of possible sensitive data.\n We prefer to print only:\n 09:00:53.238330 http.c:534 => Send header: Authorization: \n intested of\n 09:00:53.238330 http.c:534 => Send header: Authorization: Basic(o other scheme) \n as it was done in the original proposed suggestion by Jeff King.\n This is because i think it's better not to print even the authorization scheme.\n We add also the (previously missing) proxy-authorization case\n- curl_dump: fix strbuf memory leak\n\nas suggested by Jeff King\n($gmane/292891)\n($gmane/292892)\n\nIn this series i keep the original curl_dump parsing code, even though it is\nobjectively difficult to read. This is because the same code is used internally by curl\nto do \"ascii-trace\" and is also reported in the libcurl code examples and test.\nI think this may make maintenance of code easier in the future (libcurl\nnew dev, new features and so on)\n\nOf course if the maintainer (or other) believes it is really necessary\nto rewrite the above code to accept the patches i will do.\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\n\n\n\n\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 | 123 +++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 2 +\n imap-send.c | 1 +\n 4 files changed, 132 insertions(+), 2 deletions(-)\n\n-- \n2.8.2.435.g7c6234f.dirty\n"},{"id":"287079","messageId":"20160520103708.38308-2-gitter.spiros@gmail.com","threadId":"42395","inReplyTo":"20160520103708.38308-1-gitter.spiros@gmail.com","subject":"[PATCH v6 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-05-20T10:37:07Z","receivedAt":"2016-05-20T10:37:07Z","isPatch":true,"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 | 124 +++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 2 +\n 3 files changed, 132 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex dd6dbf7..a46a356 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -1077,6 +1077,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 df6dd01..ba32bac 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@@ -477,6 +478,125 @@ static void set_curl_keepalive(CURL *c)\n }\n #endif\n \n+static void curl_dump_header(const char *text, unsigned char *ptr, size_t size, int nopriv_header)\n+{\n+\tstruct strbuf out = STRBUF_INIT;\n+\tconst char *header;\n+\tstruct strbuf **header_list, **ptr_list;\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+\tstrbuf_add(&out,ptr,size);\n+\theader_list = strbuf_split_max(&out, '\\n', 0);\n+\n+\tfor (ptr_list = header_list; *ptr_list; ptr_list++) {\n+\t/*\n+\t * if we are called with nopriv_header substitute a dummy value\n+\t * in the Authorization or Proxy-Authorization http header if\n+\t * present.\n+\t */\n+\tif (nopriv_header &&\n+\t\t(skip_prefix((*ptr_list)->buf , \"Authorization:\", &header)\n+\t\t|| skip_prefix((*ptr_list)->buf , \"Proxy-Authorization:\", &header))) { \n+\t\t/* The first token is the type, which is OK to log */\n+\t\twhile (isspace(*header))\n+\t\t\theader++;\n+\t\twhile (*header && !isspace(*header))\n+\t\t\theader++;\n+\t\t/* Everything else is opaque and possibly sensitive */\n+\t\tstrbuf_setlen((*ptr_list), header - (*ptr_list)->buf );\n+\t\tstrbuf_addstr((*ptr_list), \" \");\n+\t}\n+\tstrbuf_insert((*ptr_list), 0, text, strlen(text));\n+\tstrbuf_insert((*ptr_list), strlen(text), \": \", 2);\n+\tstrbuf_rtrim((*ptr_list));\n+\tstrbuf_addch((*ptr_list), '\\n');\n+\ttrace_strbuf(&trace_curl, (*ptr_list));\n+\t}\n+\tstrbuf_list_free(header_list);\n+\tstrbuf_release(&out);\n+}\n+static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)\n+{\n+\tsize_t i;\n+\tstruct strbuf out = STRBUF_INIT;\n+\tunsigned int width = 80;\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+\n+\tfor (i = 0; i < size; i += width) {\n+\t\tsize_t w;\n+\n+\t\tstrbuf_reset(&out);\n+\t\tstrbuf_addf(&out, \"%s: \", text);\n+\t\tfor (w = 0; (w < width) && (i + w < size); w++) {\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}\n+\t\tstrbuf_addch(&out, '\\n');\n+\t\ttrace_strbuf(&trace_curl, &out);\n+\t}\n+\tstrbuf_release(&out);\n+}\n+\n+static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp)\n+{\n+\tconst char *text;\n+\tint nopriv_header = 0;\t/*\n+\t\t\t\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_header = 1;\n+\t\tcurl_dump_header(text, (unsigned char *)data, size, nopriv_header);\n+\t\tbreak;\n+\tcase CURLINFO_DATA_OUT:\n+\t\ttext = \"=> Send data\";\n+\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\tbreak;\n+\tcase CURLINFO_SSL_DATA_OUT:\n+\t\ttext = \"=> Send SSL data\";\n+\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\tbreak;\n+\tcase CURLINFO_HEADER_IN:\n+\t\ttext = \"<= Recv header\";\n+\t\tnopriv_header = 0;\n+\t\tcurl_dump_header(text, (unsigned char *)data, size, nopriv_header);\n+\t\tbreak;\n+\tcase CURLINFO_DATA_IN:\n+\t\ttext = \"<= Recv data\";\n+\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\tbreak;\n+\tcase CURLINFO_SSL_DATA_IN:\n+\t\ttext = \"<= Recv SSL data\";\n+\t\tcurl_dump_data(text, (unsigned char *)data, size);\n+\t\tbreak;\n+\t}\n+\treturn 0;\n+}\n+\n+void setup_curl_trace(CURL *handle)\n+{\n+\tif (!trace_want(&trace_curl)) \n+\t\treturn;\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+}\n+\n+\n static CURL *get_curl_handle(void)\n {\n \tCURL *result = curl_easy_init();\n@@ -575,9 +695,9 @@ static CURL *get_curl_handle(void)\n \t\twarning(\"protocol restrictions not applied to curl redirects because\\n\"\n \t\t\t\"your curl version is too old (>= 7.19.4)\");\n #endif\n-\n \tif (getenv(\"GIT_CURL_VERBOSE\"))\n-\t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1);\n+\t\tcurl_easy_setopt(result, CURLOPT_VERBOSE, 1L);\n+\tsetup_curl_trace(result);\n \n \tcurl_easy_setopt(result, CURLOPT_USERAGENT,\n \t\tuser_agent ? user_agent : git_user_agent());\ndiff --git a/http.h b/http.h\nindex 36f558b..5ab9d9c 100644\n--- a/http.h\n+++ b/http.h\n@@ -225,4 +225,6 @@ 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+/* setup routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */\n+void setup_curl_trace(CURL *handle);\n #endif /* HTTP_H */\n-- \n2.8.2.435.g7c6234f.dirty\n"},{"id":"287078","messageId":"20160520103708.38308-3-gitter.spiros@gmail.com","threadId":"42395","inReplyTo":"20160520103708.38308-1-gitter.spiros@gmail.com","subject":"[PATCH v6 2/2] imap-send.c: introduce the GIT_TRACE_CURL enviroment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-05-20T10:37:08Z","receivedAt":"2016-05-20T10:37:08Z","isPatch":true,"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 setup_curl_trace 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 | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/imap-send.c b/imap-send.c\nindex 938c691..50377c5 100644\n--- a/imap-send.c\n+++ b/imap-send.c\n@@ -1443,6 +1443,7 @@ static CURL *setup_curl(struct imap_server_conf *srvc)\n \n \tif (0 < verbosity || getenv(\"GIT_CURL_VERBOSE\"))\n \t\tcurl_easy_setopt(curl, CURLOPT_VERBOSE, 1L);\n+\tsetup_curl_trace(curl);\n \n \treturn curl;\n }\n-- \n2.8.2.435.g7c6234f.dirty\n"},{"id":"287128","messageId":"xmqqlh34cvmd.fsf@gitster.mtv.corp.google.com","threadId":"42395","inReplyTo":"20160520103708.38308-2-gitter.spiros@gmail.com","subject":"Re: [PATCH v6 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-20T20:39:06Z","receivedAt":"2016-05-20T20:39:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n> diff --git a/http.c b/http.c\n> index df6dd01..ba32bac 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> @@ -477,6 +478,125 @@ static void set_curl_keepalive(CURL *c)\n> }\n> #endif\n> \n> +static void curl_dump_header(const char *text, unsigned char *ptr, size_t size, int nopriv_header)\n> +{\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tconst char *header;\n> +\tstruct strbuf **header_list, **ptr_list;\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> +\tstrbuf_add(&out,ptr,size);\n> +\theader_list = strbuf_split_max(&out, '\\n', 0);\n> +\n> +\tfor (ptr_list = header_list; *ptr_list; ptr_list++) {\n> +\t/*\n> +\t * if we are called with nopriv_header substitute a dummy value\n> +\t * in the Authorization or Proxy-Authorization http header if\n> +\t * present.\n> +\t */\n> +\tif (nopriv_header &&\n> +\t\t(skip_prefix((*ptr_list)->buf , \"Authorization:\", &header)\n> +\t\t|| skip_prefix((*ptr_list)->buf , \"Proxy-Authorization:\", &header))) { \n> +\t\t/* The first token is the type, which is OK to log */\n> +\t\twhile (isspace(*header))\n> +\t\t\theader++;\n> +\t\twhile (*header && !isspace(*header))\n> +\t\t\theader++;\n> +\t\t/* Everything else is opaque and possibly sensitive */\n> +\t\tstrbuf_setlen((*ptr_list), header - (*ptr_list)->buf );\n> +\t\tstrbuf_addstr((*ptr_list), \" \");\n> +\t}\n> +\tstrbuf_insert((*ptr_list), 0, text, strlen(text));\n> +\tstrbuf_insert((*ptr_list), strlen(text), \": \", 2);\n> +\tstrbuf_rtrim((*ptr_list));\n> +\tstrbuf_addch((*ptr_list), '\\n');\n> +\ttrace_strbuf(&trace_curl, (*ptr_list));\n\nThis funny indentation makes me wonder why you didn't make it into a\nhelper function. If it would require too many parameters, I could\nunderstand the aversion, as it would end up looking like:\n\n\tfor (header = headers; *header; header++) {\n \tif (hide_sensitive_header)\n\t\t\tredact_sensitive_header(header, &too, &many, &other,\n\t\t\t ¶ms, &you,\n \t\t\t\t\t&need, &to, &pass);\n\t\tstrbuf_insert(*header, 0, text, strlen(text));\n strbuf_insert(*header, strlen(text), \": \", 2);\n ...\n trace_strbuf(&trace_curl, *header);\n\t}\n\nbut I think redact_sensitive_header() helper would only need to take\na single strbuf, from the look of your actual implementation.\n\nYes, I am also suggesting to rename header_list and ptr_list to\nheaders and header, respectively. header_list may be an OK name (as\nit is \"a list, each element of which is a header\"), but ptr_list is\na poor name--\"a pointer into a list\" is a much less interesting thing\nfor the reader of the code to learn from the name than it represents\n\"one header\".\n\nAnd your \"const char *header\" does not have to be here (it would be\nan implementation detail of redact_sensitive_header() function), so\nsuch a renaming would not cause conflicts in variable names.\n\n> +\t}\n> +\tstrbuf_list_free(header_list);\n> +\tstrbuf_release(&out);\n> +}\n> +static void curl_dump_data(const char *text, unsigned char *ptr, size_t size)\n\nA blank line, between the end of previous function body and the\nbegining of this function, is missing.\n\n> +{\n> +\tsize_t i;\n> +\tstruct strbuf out = STRBUF_INIT;\n> +\tunsigned int width = 80;\n\nIn a few places Git limits the width of the output, like using function\nname in hunk header lines and drawing diffstat in \"diff --stat\", we\ndo default to limit the total width to 80 display columns.\n\nGiven that this routine prefixes each and every line with a short\nheading like \"=> Send header: \" that costs at around 15-20 columns,\nand the loop we see below only counts the true payload without\ncounting the heading, you would probably want to reduce this\nsomewhat so that the whole thing would fit within 80 columns.\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> +\n> +\tfor (i = 0; i < size; i += width) {\n> +\t\tsize_t w;\n> +\n> +\t\tstrbuf_reset(&out);\n> +\t\tstrbuf_addf(&out, \"%s: \", text);\n> +\t\tfor (w = 0; (w < width) && (i + w < size); w++) {\n> +\t\t\tstrbuf_addch(&out, (ptr[i + w] >= 0x20)\n> +\t\t\t\t&& (ptr[i + w] < 0x80) ? ptr[i + w] : '.');\n\nPlease think twice to make sure a long expression that has to span\nmultiple lines is cut at a sensible place. This cuts at the worst\nplace where the syntactic element that binds strongest sits in the\nexpression.\n\nThe weakest binding is the comma between two parameters given to\naddch(), so let's cut it there:\n\n for (w = 0; (w < width) && (i + w < size); w++) {\n unsigned char ch = ptr[i + w];\n strbuf_addch(&out,\n (0x20 <= ch && ch < 0x80) ? ch : '.');\n }\n\nIf you find the second line still too long, then cut it before '?', i.e.\n\n strbuf_addch(&out,\n (0x20 <= ch && ch < 0x80) \n ? ch : '.');\n\nor alternatively:\n\n\n strbuf_addch(&out, ((0x20 <= ch && ch < 0x80) \n ? ch : '.'));\n\n> +static int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp)\n> +{\n> +\tconst char *text;\n> +\tint nopriv_header = 0;\t/*\n> +\t\t\t\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_header = 1;\n> +\t\tcurl_dump_header(text, (unsigned char *)data, size, nopriv_header);\n> +\tcase CURLINFO_HEADER_IN:\n> +\t\ttext = \"<= Recv header\";\n> +\t\tnopriv_header = 0;\n> +\t\tcurl_dump_header(text, (unsigned char *)data, size, nopriv_header);\n> +\t\tbreak;\n\nI do not think an extra variable nopriv_header gives us any\nreadability advantage. If you looked at these two calls to\ncurl_dump_header() and its parameters alone, you cannot tell\nwhich one is sensitive.\n\nWouldn't it be easier to read if they were like this?\n\n static int curl_trace(..., char *data_, ...)\n {\n enum { NO_FILTER = 0, DO_FILTER = 1 };\n unsigned char *data = (unsigned char *) data_;\n\n curl_dump_header(\"Send header\", data, size, DO_FILTER);\n curl_dump_header(\"Recv header\", data, size, NO_FILTER);\n"},{"id":"287551","messageId":"20160526045105.GC6756@sigill.intra.peff.net","threadId":"42395","inReplyTo":"xmqqlh34cvmd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v6 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-26T04:51:05Z","receivedAt":"2016-05-26T04:51:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 20, 2016 at 01:39:06PM -0700, Junio C Hamano wrote:\n\n> > +{\n> > +\tsize_t i;\n> > +\tstruct strbuf out = STRBUF_INIT;\n> > +\tunsigned int width = 80;\n> \n> In a few places Git limits the width of the output, like using function\n> name in hunk header lines and drawing diffstat in \"diff --stat\", we\n> do default to limit the total width to 80 display columns.\n> \n> Given that this routine prefixes each and every line with a short\n> heading like \"=> Send header: \" that costs at around 15-20 columns,\n> and the loop we see below only counts the true payload without\n> counting the heading, you would probably want to reduce this\n> somewhat so that the whole thing would fit within 80 columns.\n\nI think that may be a losing battle. Remember that we are getting the\nusual trace header on top of this, which I think leaves only about 20\ncharacters for actual data on each line.\n\nI kind of doubt people will manually read the body data. In my\nexperience, debugging git-over-http you want either:\n\n 1. Just the request/response headers to see what was said.\n\n 2. A complete dump of each body with no other formatting (e.g., so you\n can replay it with curl).\n\nThis trace output gives you (1). You can in theory generate (2) from it\nwith a perl snippet, but the non-printing characters have been\nirreversibly transformed.\n\nSo I dunno. I do not mind having the body stuff there for completeness,\nbut I doubt I'd use it myself. And I don't care much either way about\nhow long its lines are.\n\n-Peff\n"}]}