{"thread":{"id":"42200","subject":"[PATCHv5 0/2] Implement the GIT_TRACE_CURL environment variable","startedAt":"2016-05-02T14:28:11Z","lastAt":"2016-05-02T19:25:54Z","messageCount":9,"participants":["Elia Pinto","Ramsay Jones","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"285158","messageId":"20160502142813.50868-1-gitter.spiros@gmail.com","threadId":"42200","inReplyTo":null,"subject":"[PATCHv5 0/2] Implement the GIT_TRACE_CURL environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-05-02T14:28:11Z","receivedAt":"2016-05-02T14:28:11Z","isPatch":false,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"This is the fifth 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 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\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 | 115 +++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 4 ++\n imap-send.c | 1 +\n 4 files changed, 126 insertions(+), 2 deletions(-)\n\n-- \n2.8.2.435.ga07a3e0\n"},{"id":"285159","messageId":"20160502142813.50868-2-gitter.spiros@gmail.com","threadId":"42200","inReplyTo":"20160502142813.50868-1-gitter.spiros@gmail.com","subject":"[PATCHv5 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-05-02T14:28:12Z","receivedAt":"2016-05-02T14:28:12Z","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 | 115 +++++++++++++++++++++++++++++++++++++++++++++++++-\n http.h | 4 ++\n 3 files changed, 125 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex 34ff007..5e59576 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -1076,6 +1076,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..5e2bc19 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,116 @@ 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, int nopriv)\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+\t\tsize_t prefix_len;\n+\t\tconst char *header;\n+\n+\t\tstrbuf_reset(&out);\n+\t\tstrbuf_addf(&out, \"%s: \", text);\n+\t\tprefix_len = out.len;\n+\t\tfor (w = 0; (w < width) && (i + w < size); w++) {\n+\t\t\tif ((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 ((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+\n+\t\t/*\n+\t\t * if we are called with nopriv substitute a dummy value\n+\t\t * in the Authorization or Proxy-Authorization http header if\n+\t\t * present.\n+\t\t */\n+\t\tif (nopriv &&\n+\t\t\t(skip_prefix(out.buf + prefix_len, \"Authorization:\", &header)\n+\t\t\t|| skip_prefix(out.buf + prefix_len, \"Proxy-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\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+\t\tstrbuf_addch(&out, '\\n');\n+\t\ttrace_strbuf(&trace_curl, &out);\n+\t}\n+\tstrbuf_release(&out);\n+}\n+\n+void setup_curl_trace(CURL *handle)\n+{\n+\tif (!trace_want(&trace_curl)) return;\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+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+\tint nopriv = 0;\t\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 = 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, nopriv);\n+\treturn 0;\n+}\n+\n+\n static CURL *get_curl_handle(void)\n {\n \tCURL *result = curl_easy_init();\n@@ -576,9 +687,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..cd186a4 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 setup routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */\n+void setup_curl_trace(CURL *handle);\n+int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp);\n+\n #endif /* HTTP_H */\n-- \n2.8.2.435.ga07a3e0\n"},{"id":"285160","messageId":"20160502142813.50868-3-gitter.spiros@gmail.com","threadId":"42200","inReplyTo":"20160502142813.50868-1-gitter.spiros@gmail.com","subject":"[PATCHv5 2/2] imap-send.c: introduce the GIT_TRACE_CURL environment variable","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2016-05-02T14:28:13Z","receivedAt":"2016-05-02T14:28:13Z","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 | 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.ga07a3e0\n"},{"id":"285164","messageId":"57278877.50907@ramsayjones.plus.com","threadId":"42200","inReplyTo":"20160502142813.50868-2-gitter.spiros@gmail.com","subject":"Re: [PATCHv5 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-05-02T17:03:51Z","receivedAt":"2016-05-02T17:03:51Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 02/05/16 15:28, Elia Pinto wrote:\n\n[snip]\n\n> diff --git a/http.h b/http.h\n> index 36f558b..cd186a4 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 setup routine for curl_easy_setopt CURLOPT_DEBUGFUNCTION */\n> +void setup_curl_trace(CURL *handle);\n> +int curl_trace(CURL *handle, curl_infotype type, char *data, size_t size, void *userp);\n\nGiven that you have wrapped the use of the debug callback into the\nsetup_curl_trace() routine (good), do you still need to export the\ncurl_trace() function?\n\nI would make that static, so that only setup_curl_trace() needs to\nbe a public symbol. (I would also re-order the function definitions,\nso that setup_curl_trace() comes after curl_trace(), but that is a\nminor point).\n\nATB,\nRamsay Jones\n"},{"id":"285180","messageId":"20160502181347.GB8439@sigill.intra.peff.net","threadId":"42200","inReplyTo":"20160502142813.50868-1-gitter.spiros@gmail.com","subject":"Re: [PATCHv5 0/2] Implement the GIT_TRACE_CURL environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-02T18:13:47Z","receivedAt":"2016-05-02T18:13:47Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 02, 2016 at 02:28:11PM +0000, Elia Pinto wrote:\n\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\nI'm not sure I agree. If you're debugging curl's auth selection, that's\nomitting an important piece of data. And unlike the actual credential, I\ndon't think it's particularly secret (and in many cases can be deduced\nfrom the \"WWW-Authenticate\" header the server sends, coupled with curl's\ncode).\n\n> We add also the (previously missing) proxy-authorization case\n\nGood catch.\n\n> In this series i keep the original curl_dump parsing code, even though it is \n> objectively difficult to read. This is because the same code is used internally by curl \n> to do \"ascii-trace\" and is also reported in the libcurl code examples and test. \n> I think this may make maintenance of code easier in the future (libcurl \n> new dev, new features and so on) \n\nI don't agree with this line of reasoning. The code in question is\npurely about how we format the output buffer, not about parsing what\ncurl gives us. We _should_ be diverging if we prefer a different output\nformat. And I don't think it's a question just of readability (though I\ndo agree the existing one is hard to read); it also foils the redaction\nof the authorization header.\n\n-Peff\n"},{"id":"285181","messageId":"20160502181533.GC8439@sigill.intra.peff.net","threadId":"42200","inReplyTo":"20160502142813.50868-2-gitter.spiros@gmail.com","subject":"Re: [PATCHv5 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-02T18:15:33Z","receivedAt":"2016-05-02T18:15:33Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 02, 2016 at 02:28:12PM +0000, Elia Pinto wrote:\n\n> +\t\tif (nopriv &&\n> +\t\t\t(skip_prefix(out.buf + prefix_len, \"Authorization:\", &header)\n> +\t\t\t|| skip_prefix(out.buf + prefix_len, \"Proxy-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\t/* Everything else is opaque and possibly sensitive */\n\nThe first comment there is now misleading, as we _don't_ keep the first\ntoken (though as mentioned elsewhere, I think we should).\n\n> +void setup_curl_trace(CURL *handle)\n> +{\n> +\tif (!trace_want(&trace_curl)) return;\n\nStyle: we always put conditional bodies on a new line, even when they\nare trivial like this.\n\n-Peff\n"},{"id":"285191","messageId":"xmqq60uwthfh.fsf@gitster.mtv.corp.google.com","threadId":"42200","inReplyTo":"20160502142813.50868-2-gitter.spiros@gmail.com","subject":"Re: [PATCHv5 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-02T18:59:14Z","receivedAt":"2016-05-02T18:59:14Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n> +static void curl_dump(const char *text, unsigned char *ptr, size_t size, int nopriv)\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\t...\n> +\t\tfor (w = 0; (w < width) && (i + w < size); w++) {\n> +\t\t\tif ((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 ((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...\n> +\t}\n> +\tstrbuf_release(&out);\n> +}\n\nThere is no change in this hard-to-read double-loop since the\nprevious round?\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\nI really do not want to see these casts. Unused parameters are\nperfectly normal in a codebase with callback functions, no? I do\nnot think these are the first occurrences of unused parameters in\nour codebase, and I do not think we have such cast to void to them.\nWhy add this ugliness only to here?\n"},{"id":"285194","messageId":"xmqqvb2ws23h.fsf@gitster.mtv.corp.google.com","threadId":"42200","inReplyTo":"20160502142813.50868-2-gitter.spiros@gmail.com","subject":"Re: [PATCHv5 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-02T19:15:46Z","receivedAt":"2016-05-02T19:15:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\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> +\tint nopriv = 0;\t\t/*\n\nThis decl-after-statment breaks our build.\n"},{"id":"285195","messageId":"20160502192554.GA13492@sigill.intra.peff.net","threadId":"42200","inReplyTo":"xmqq60uwthfh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv5 1/2] http.c: implement the GIT_TRACE_CURL environment variable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-02T19:25:54Z","receivedAt":"2016-05-02T19:25:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 02, 2016 at 11:59:14AM -0700, Junio C Hamano wrote:\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> \n> I really do not want to see these casts. Unused parameters are\n> perfectly normal in a codebase with callback functions, no? I do\n> not think these are the first occurrences of unused parameters in\n> our codebase, and I do not think we have such cast to void to them.\n> Why add this ugliness only to here?\n\nYeah, it is pointless to use -Wunused-parameter in our code base, as it\nturns up over 1200 hits (though some are repeats from include files).\nMost are callbacks, but some also look like compat functions. But\nclearly it's going to be a false positive any time the interface to the\nfunction is dictated by something other than the function body.\n\nI generally don't mind quieting false positive warnings if we think it\nmight provide an opportunity for cleanup (i.e., parameters that really\n_can_ go away). But I don't think gcc provides a great way to do it.\n\nYou can mark individual parameters as unused, like:\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex ba51cfd..71b4f7b 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -331,7 +331,7 @@ extern char *gitdirname(char *);\n #endif\n \n #ifndef has_dos_drive_prefix\n-static inline int git_has_dos_drive_prefix(const char *path)\n+static inline int git_has_dos_drive_prefix(const char *path __attribute__((unused)))\n {\n \treturn 0;\n }\n@@ -339,7 +339,7 @@ static inline int git_has_dos_drive_prefix(const char *path)\n #endif\n \n #ifndef skip_dos_drive_prefix\n-static inline int git_skip_dos_drive_prefix(char **path)\n+static inline int git_skip_dos_drive_prefix(char **path __attribute__((unused)))\n {\n \treturn 0;\n }\n\nThat's not too bad in a single-argument function, but for callbacks it\ncan get pretty tedious:\n\ndiff --git a/wt-status.c b/wt-status.c\nindex 1ea2ebe..7f00981 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -1300,8 +1300,11 @@ struct grab_1st_switch_cbdata {\n \tunsigned char nsha1[20];\n };\n \n-static int grab_1st_switch(unsigned char *osha1, unsigned char *nsha1,\n-\t\t\t const char *email, unsigned long timestamp, int tz,\n+static int grab_1st_switch(unsigned char *osha1 __attribute__((unused)),\n+\t\t\t unsigned char *nsha1,\n+\t\t\t const char *email __attribute__((unused)),\n+\t\t\t unsigned long timestamp __attribute__((unused)),\n+\t\t\t int tz __attribute__((unused)),\n \t\t\t const char *message, void *cb_data)\n {\n \tstruct grab_1st_switch_cbdata *cb = cb_data;\n\nIt would be much nicer if we had some way of declaring the whole\n_function_ as having an interface dictated elsewhere. Then it becomes a\nsingle attribute per callback function, and actually tells the human\nreader something useful: not \"I happen to not need these variables right\nnow\", but \"my interface is specified elsewhere and not up for debate\".\n\nBut AFAIK, there's no such attribute.\n\n-Peff\n"}]}