{"thread":{"id":"44568","subject":"[PATCH] difftool.c: mark a file-local symbol with static","startedAt":"2016-11-29T17:36:32Z","lastAt":"2017-01-26T18:26:12Z","messageCount":28,"participants":["Ramsay Jones","Johannes Schindelin","Junio C Hamano","Jeff King","David Aguilar","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"306547","messageId":"4ddad7ea-5ac8-20b2-da9e-5843c486878a@ramsayjones.plus.com","threadId":"44568","inReplyTo":null,"subject":"[PATCH] difftool.c: mark a file-local symbol with static","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-11-29T17:36:24Z","receivedAt":"2016-11-29T17:36:32Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\nSigned-off-by: Ramsay Jones \n---\n\nHi Johannes,\n\nIf you need to re-roll your 'js/difftool-builtin' branch, could\nyou please squash this into the relevant patch.\n\nThanks!\n\nAlso, due to a problem in my config.mak file on Linux (a commented\nout line that had a line continuation '\\', grrrrr!), gcc issued a\nwarning, thus:\n\n builtin/difftool.c: In function ‘run_dir_diff’:\n builtin/difftool.c:568:13: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n warning(\"\");\n ^\nI am not sure why -Wno-format-zero-length is set in DEVELOPER_CFLAGS,\nbut do you really need to space the output with an an 'empty'\n\"warning:\" line? (Just curious).\n\nATB,\nRamsay Jones\n\n builtin/difftool.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 3480920..830369c 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -170,7 +170,7 @@ struct path_entry {\n \tchar path[FLEX_ARRAY];\n };\n \n-int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)\n+static int path_entry_cmp(struct path_entry *a, struct path_entry *b, void *key)\n {\n \treturn strcmp(a->path, key ? key : b->path);\n }\n-- \n2.9.0\n"},{"id":"306585","messageId":"alpine.DEB.2.20.1611301204020.117539@virtualbox","threadId":"44568","inReplyTo":"4ddad7ea-5ac8-20b2-da9e-5843c486878a@ramsayjones.plus.com","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-11-30T11:07:51Z","receivedAt":"2016-11-30T11:08:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ramsay,\n\nOn Tue, 29 Nov 2016, Ramsay Jones wrote:\n\n> If you need to re-roll your 'js/difftool-builtin' branch, could\n> you please squash this into the relevant patch.\n\nFixed. Thanks!\n\n> Also, due to a problem in my config.mak file on Linux (a commented\n> out line that had a line continuation '\\', grrrrr!), gcc issued a\n> warning, thus:\n> \n> builtin/difftool.c: In function ‘run_dir_diff’:\n> builtin/difftool.c:568:13: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> warning(\"\");\n> ^\n> I am not sure why -Wno-format-zero-length is set in DEVELOPER_CFLAGS,\n> but do you really need to space the output with an an 'empty'\n> \"warning:\" line? (Just curious).\n\nThat `warning(\"\");` comes from a straight-forward port of this line (see\nhttps://github.com/git/git/blob/v2.11.0/git-difftool.perl#L425):\n\n\t$errmsg .= \"warning:\\n\";\n\nI could see two possible ways out:\n\n- warning(\"%s\", \"\"); (ugly!)\n\n- do away with the \"prefix every line with warning:\" convention and simply\n have a multi-line `warning(_(\"...\\n...\\n\"), ...)`\n\nWhat do you think?\nDscho"},{"id":"306599","messageId":"29abc89b-9ca5-930f-8e90-ca446ac2b96a@ramsayjones.plus.com","threadId":"44568","inReplyTo":"alpine.DEB.2.20.1611301204020.117539@virtualbox","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-11-30T19:57:18Z","receivedAt":"2016-11-30T19:59:00Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 30/11/16 11:07, Johannes Schindelin wrote:\n> Hi Ramsay,\n> \n> On Tue, 29 Nov 2016, Ramsay Jones wrote:\n>> Also, due to a problem in my config.mak file on Linux (a commented\n>> out line that had a line continuation '\\', grrrrr!), gcc issued a\n>> warning, thus:\n>>\n>> builtin/difftool.c: In function ‘run_dir_diff’:\n>> builtin/difftool.c:568:13: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n>> warning(\"\");\n>> ^\n>> I am not sure why -Wno-format-zero-length is set in DEVELOPER_CFLAGS,\n>> but do you really need to space the output with an an 'empty'\n>> \"warning:\" line? (Just curious).\n> \n> That `warning(\"\");` comes from a straight-forward port of this line (see\n> https://github.com/git/git/blob/v2.11.0/git-difftool.perl#L425):\n> \n> \t$errmsg .= \"warning:\\n\";\n\nAh, OK, so it is being used to 'space out' the (possibly multiple)\n'Both files modified' warning(s) followed by a 2-line warning\nsummary. Hmm, I am not sure the 'Working tree file has been left'\n(after each pair of files) part of the message is adding much ...\n\n> I could see two possible ways out:\n> \n> - warning(\"%s\", \"\"); (ugly!)\n> \n> - do away with the \"prefix every line with warning:\" convention and simply\n> have a multi-line `warning(_(\"...\\n...\\n\"), ...)`\n> \n> What do you think?\n\nI think, for now, it is probably best to do nothing. ;-)\n\nUntil you have replaced the 'legacy' difftool, it would be best\nnot to alter the output from the tool, so that the tests can be\nused unaltered on both. This could be addressed (if necessary)\nafter you complete your series.\n\n[I have fixed my config.mak file now, so I don't see the warning\nanymore! Having -Wno-format-zero-length in DEVELOPER_CFLAGS, or\nnot, is a separate matter.]\n\nATB,\nRamsay Jones\n\n"},{"id":"306601","messageId":"xmqqtwaod7ly.fsf@gitster.mtv.corp.google.com","threadId":"44568","inReplyTo":"29abc89b-9ca5-930f-8e90-ca446ac2b96a@ramsayjones.plus.com","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-30T20:40:25Z","receivedAt":"2016-11-30T20:40:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones writes:\n\n> [I have fixed my config.mak file now, so I don't see the warning\n> anymore! Having -Wno-format-zero-length in DEVELOPER_CFLAGS, or\n> not, is a separate matter.]\n\nI suspect that 658df95a4a (\"add DEVELOPER makefile knob to check for\nacknowledged warnings\", 2016-02-25) took it from me (namely, Make\nscript in my 'todo' branch). In turn, I added it to my set of flags\nin order to squelch this exact warning, so...\n\n\n"},{"id":"306613","messageId":"20161130212510.ihcmvig7jq44p3nx@sigill.intra.peff.net","threadId":"44568","inReplyTo":"xmqqtwaod7ly.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-30T21:25:10Z","receivedAt":"2016-11-30T21:25:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2016 at 12:40:25PM -0800, Junio C Hamano wrote:\n\n> Ramsay Jones writes:\n> \n> > [I have fixed my config.mak file now, so I don't see the warning\n> > anymore! Having -Wno-format-zero-length in DEVELOPER_CFLAGS, or\n> > not, is a separate matter.]\n> \n> I suspect that 658df95a4a (\"add DEVELOPER makefile knob to check for\n> acknowledged warnings\", 2016-02-25) took it from me (namely, Make\n> script in my 'todo' branch). In turn, I added it to my set of flags\n> in order to squelch this exact warning, so...\n\nFor anybody interested in the history, we started using this when\nstatus_printf() got the format attribute. Relevant patch and discussion:\n\n http://public-inbox.org/git/20130710002328.GC19423@sigill.intra.peff.net/T/#u\n\nWe went with disabling the warning because it really is wrong. It makes\nan assumption that calling a format function with an empty string\ndoesn't do anything, but that's only true of the stock printf functions.\nOur custom functions _do_ have a side effect in this case.\n\nThe other options are to have a special function for \"print a warning\n(or status) line with no content\". Or to teach those functions to handle\nnewlines specially. We've often discussed that you should be able to do:\n\n warning(\"foo\\nbar\");\n\nand have it print:\n\n warning: foo\n warning: bar\n\nThat's useful in itself, and would probably make cases like this easier\nto handle, too. But it's a pretty big change. Another option would be to\njust teach formatting functions to handle a single \"\\n\" as a synonym for\nthe empty string (or even detect trailing newlines and avoid appending\nour own in that case). That would mean you could do:\n\n warning(\"\\n\");\n\nto print a blank line. That's arguably more obvious about the intent to\na reader (I say arguably because the new behavior _is_ subtle if you\nhappen to know that warning() usually appends a newline).\n\nAnyway. Those are all options, but I don't think there is any problem\nwith sticking with warning(\"\") for now. It is not the first part of the\ncode that tickles the format-zero-length warning.\n\n-Peff\n"},{"id":"306630","messageId":"3e6a6685-19ec-4536-4a5f-3a56e30fb530@ramsayjones.plus.com","threadId":"44568","inReplyTo":"20161130212510.ihcmvig7jq44p3nx@sigill.intra.peff.net","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-11-30T22:37:40Z","receivedAt":"2016-11-30T22:37:48Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 30/11/16 21:25, Jeff King wrote:\n> On Wed, Nov 30, 2016 at 12:40:25PM -0800, Junio C Hamano wrote:\n> \n>> Ramsay Jones writes:\n>>\n>>> [I have fixed my config.mak file now, so I don't see the warning\n>>> anymore! Having -Wno-format-zero-length in DEVELOPER_CFLAGS, or\n>>> not, is a separate matter.]\n>>\n>> I suspect that 658df95a4a (\"add DEVELOPER makefile knob to check for\n>> acknowledged warnings\", 2016-02-25) took it from me (namely, Make\n>> script in my 'todo' branch). In turn, I added it to my set of flags\n>> in order to squelch this exact warning, so...\n> \n> For anybody interested in the history, we started using this when\n> status_printf() got the format attribute. Relevant patch and discussion:\n> \n> http://public-inbox.org/git/20130710002328.GC19423@sigill.intra.peff.net/T/#u\n\nAh, thank you! I've been trying to remember this for the last hour or so ...\n(I misremembered something about gettext, and went looking in the wrong\ndirection ;-) ).\n\n> We went with disabling the warning because it really is wrong. It makes\n> an assumption that calling a format function with an empty string\n> doesn't do anything, but that's only true of the stock printf functions.\n> Our custom functions _do_ have a side effect in this case.\n\nYes, I agree, gcc is making an unwarranted assumption.\n\n> The other options are to have a special function for \"print a warning\n> (or status) line with no content\". Or to teach those functions to handle\n> newlines specially. We've often discussed that you should be able to do:\n> \n> warning(\"foo\\nbar\");\n> \n> and have it print:\n> \n> warning: foo\n> warning: bar\n> \n> That's useful in itself, and would probably make cases like this easier\n> to handle, too. But it's a pretty big change. Another option would be to\n> just teach formatting functions to handle a single \"\\n\" as a synonym for\n> the empty string (or even detect trailing newlines and avoid appending\n> our own in that case). That would mean you could do:\n> \n> warning(\"\\n\");\n> \n> to print a blank line. That's arguably more obvious about the intent to\n> a reader (I say arguably because the new behavior _is_ subtle if you\n> happen to know that warning() usually appends a newline).\n\nYes, I remember the discussion now and agree that this could\ncause problems.\n\n> Anyway. Those are all options, but I don't think there is any problem\n> with sticking with warning(\"\") for now. It is not the first part of the\n> code that tickles the format-zero-length warning.\n\nHmm, well I have been building git for some time with the warning\nenabled without problem.\n\n[I can see a few examples of 'status_printf_ln(s, c, \"%s\", \"\");' in\ngit-grep output, so ...]\n\nI am not suggesting any change here, and was just curious why it\nseemed to be unnecessary now (I knew it was necessary at one time).\n\nATB,\nRamsay Jones\n\n"},{"id":"306632","messageId":"20161130231848.v5ge6otytim2t6d2@sigill.intra.peff.net","threadId":"44568","inReplyTo":"3e6a6685-19ec-4536-4a5f-3a56e30fb530@ramsayjones.plus.com","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-11-30T23:18:48Z","receivedAt":"2016-11-30T23:19:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 30, 2016 at 10:37:40PM +0000, Ramsay Jones wrote:\n\n> > Anyway. Those are all options, but I don't think there is any problem\n> > with sticking with warning(\"\") for now. It is not the first part of the\n> > code that tickles the format-zero-length warning.\n> \n> Hmm, well I have been building git for some time with the warning\n> enabled without problem.\n> \n> [I can see a few examples of 'status_printf_ln(s, c, \"%s\", \"\");' in\n> git-grep output, so ...]\n> \n> I am not suggesting any change here, and was just curious why it\n> seemed to be unnecessary now (I knew it was necessary at one time).\n\nI forgot, we ended up reversing course later and silencing them:\n\n http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/\n\nBy the rationale of that conversation, we should be doing:\n\n warning(\"%s\", \"\");\n\nhere.\n\n-Peff\n"},{"id":"306643","messageId":"xmqqinr4bkf4.fsf@gitster.mtv.corp.google.com","threadId":"44568","inReplyTo":"20161130231848.v5ge6otytim2t6d2@sigill.intra.peff.net","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-11-30T23:46:39Z","receivedAt":"2016-11-30T23:47:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I forgot, we ended up reversing course later and silencing them:\n>\n> http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/\n>\n> By the rationale of that conversation, we should be doing:\n>\n> warning(\"%s\", \"\");\n>\n> here.\n\nI forgot too. Thanks for digging up that thread.\n\n"},{"id":"306655","messageId":"59da5383-16a0-b327-75a8-b4c4ad7bd479@ramsayjones.plus.com","threadId":"44568","inReplyTo":"xmqqinr4bkf4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-12-01T01:18:35Z","receivedAt":"2016-12-01T01:18:42Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 30/11/16 23:46, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> I forgot, we ended up reversing course later and silencing them:\n>>\n>> http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/\n>>\n>> By the rationale of that conversation, we should be doing:\n>>\n>> warning(\"%s\", \"\");\n>>\n>> here.\n> \n> I forgot too. Thanks for digging up that thread.\n\nYes, I blamed wt-status.c:227 and came up with commit 7d7d68022\nas well.\n\nSo, by the same rationale, we should remove -Wno-format-zero-length\nfrom DEVELOPER_CFLAGS. yes?\n\nATB,\nRamsay Jones\n\n"},{"id":"306668","messageId":"20161201040234.3rnuttitneweedn5@sigill.intra.peff.net","threadId":"44568","inReplyTo":"59da5383-16a0-b327-75a8-b4c4ad7bd479@ramsayjones.plus.com","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T04:02:34Z","receivedAt":"2016-12-01T04:03:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 01, 2016 at 01:18:35AM +0000, Ramsay Jones wrote:\n\n> >> I forgot, we ended up reversing course later and silencing them:\n> >>\n> >> http://public-inbox.org/git/20140505052117.GC6569@sigill.intra.peff.net/\n> >>\n> >> By the rationale of that conversation, we should be doing:\n> >>\n> >> warning(\"%s\", \"\");\n> >>\n> >> here.\n> > \n> > I forgot too. Thanks for digging up that thread.\n> \n> Yes, I blamed wt-status.c:227 and came up with commit 7d7d68022\n> as well.\n> \n> So, by the same rationale, we should remove -Wno-format-zero-length\n> from DEVELOPER_CFLAGS. yes?\n\nI don't have a preference on which direction we go, but yes, right now\nwe are in an awkward middle ground. We should do one of:\n\n 1. Drop -Wno-format-zero-length from DEVELOPER_CFLAGS and make sure\n future patches to do not violate it.\n\n 2. Declare warning(\"\") as OK.\n\nI still think the warning is silly, but (1) has value in that it\nproduces the least surprise and annoyance to various people building\nGit.\n\n-Peff\n"},{"id":"306701","messageId":"xmqq60n3bjel.fsf@gitster.mtv.corp.google.com","threadId":"44568","inReplyTo":"20161201040234.3rnuttitneweedn5@sigill.intra.peff.net","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-01T18:20:50Z","receivedAt":"2016-12-01T18:20:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I don't have a preference on which direction we go, but yes, right now\n> we are in an awkward middle ground. We should do one of:\n>\n> 1. Drop -Wno-format-zero-length from DEVELOPER_CFLAGS and make sure\n> future patches to do not violate it.\n>\n> 2. Declare warning(\"\") as OK.\n>\n> I still think the warning is silly, but (1) has value in that it\n> produces the least surprise and annoyance to various people building\n> Git.\n\nWhat is most awkward is that \"make\", with no customization out of\nbox, suggests to use -Wall in CFLAGS and triggers the misguided\nwarning, and DEVELOPER_CFLAGS, partly because it adds -Werror to\nturn warnings (including this misguided one) into errors, disables\nit with -Wno-format-zero-length. As a result:\n\n - Casual builders that follow our suggestion get warnings.\n\n - Developers do not notice their new code may make the \"casual\n builder\" experience worse.\n\nThat is totally backwards. That is probably what you meant by \"the\nleast surprise and annoyance\"?\n\nI also still think that any_printf_like_function(\"%s\", \"\") looks\nsilly. I know that we've already started moving in that direction\nand we stopped at a place we do not want to be in, but perhaps it\nwas a mistake to move in that direction in the first place. I am\ntempted to say we should do something like the attached, but that\nmay not fly well, as I suspect that -Wno-format-zero-length may be a\nlot more exotic than the -Wall compiler option. An obvious second\nbest option would be to drop -Wall from the \"everybody\" CFLAGS and\nmove it to DEVELOPER_CFLAGS instead.\n\ndiff --git a/Makefile b/Makefile\nindex a379738db2..137c10e257 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -391,10 +391,9 @@ GIT-VERSION-FILE: FORCE\n \n # CFLAGS and LDFLAGS are for the users to override from the command line.\n \n-CFLAGS = -g -O2 -Wall\n+CFLAGS = -g -O2 -Wall -Wno-format-zero-length\n DEVELOPER_CFLAGS = -Werror \\\n \t-Wdeclaration-after-statement \\\n-\t-Wno-format-zero-length \\\n \t-Wold-style-definition \\\n \t-Woverflow \\\n \t-Wpointer-arith \\\n\n\n"},{"id":"306708","messageId":"20161201185056.eso5rhec7izlbywa@sigill.intra.peff.net","threadId":"44568","inReplyTo":"xmqq60n3bjel.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-12-01T18:50:57Z","receivedAt":"2016-12-01T18:51:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 01, 2016 at 10:20:50AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I don't have a preference on which direction we go, but yes, right now\n> > we are in an awkward middle ground. We should do one of:\n> >\n> > 1. Drop -Wno-format-zero-length from DEVELOPER_CFLAGS and make sure\n> > future patches to do not violate it.\n> >\n> > 2. Declare warning(\"\") as OK.\n> >\n> > I still think the warning is silly, but (1) has value in that it\n> > produces the least surprise and annoyance to various people building\n> > Git.\n> \n> What is most awkward is that \"make\", with no customization out of\n> box, suggests to use -Wall in CFLAGS and triggers the misguided\n> warning, and DEVELOPER_CFLAGS, partly because it adds -Werror to\n> turn warnings (including this misguided one) into errors, disables\n> it with -Wno-format-zero-length. As a result:\n> \n> - Casual builders that follow our suggestion get warnings.\n> \n> - Developers do not notice their new code may make the \"casual\n> builder\" experience worse.\n> \n> That is totally backwards. That is probably what you meant by \"the\n> least surprise and annoyance\"?\n\nYes, exactly. :)\n\nThe surprise and annoyance for (1) is that people who get caught up by\nthe warning while writing new patches say \"this warning is stupid, why\ndon't we disable it\". But that is a much smaller number of people to be\nannoyed.\n\n> I also still think that any_printf_like_function(\"%s\", \"\") looks\n> silly. I know that we've already started moving in that direction\n> and we stopped at a place we do not want to be in, but perhaps it\n> was a mistake to move in that direction in the first place. I am\n> tempted to say we should do something like the attached, but that\n> may not fly well, as I suspect that -Wno-format-zero-length may be a\n> lot more exotic than the -Wall compiler option.\n\nYeah, I think portability concerns are what caused us not to go down\nthat road in the first place. But I think it's also a mistake to put too\nmuch into CFLAGS, because somebody who wants to override one flag has to\nrepeat the rest. So, e.g., right now I might do:\n\n make CFLAGS=\"-O3 -Wall\"\n\nto bump the optimization level. But if our codebase relies on\n-Wno-format-zero-length being there, then I have to know to put it in\nthere, too.\n\nYou can work around that with an extra make variable, but that also\nmakes it harder to disable if your compiler doesn't like it.\n\n> An obvious second\n> best option would be to drop -Wall from the \"everybody\" CFLAGS and\n> move it to DEVELOPER_CFLAGS instead.\n\nYeah, though that doesn't help the example above.\n\nAs ugly as warning(\"%s\", \"\") is, I think it may be the thing that annoys\nthe smallest number of people.\n\n-Peff\n"},{"id":"309923","messageId":"20170122052608.tpr5pihfgafhoynj@gmail.com","threadId":"44568","inReplyTo":"20161201185056.eso5rhec7izlbywa@sigill.intra.peff.net","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2017-01-22T05:26:08Z","receivedAt":"2017-01-22T05:26:25Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Dec 01, 2016 at 01:50:57PM -0500, Jeff King wrote:\n> On Thu, Dec 01, 2016 at 10:20:50AM -0800, Junio C Hamano wrote:\n> > I also still think that any_printf_like_function(\"%s\", \"\") looks\n> > silly. I know that we've already started moving in that direction\n> > and we stopped at a place we do not want to be in, but perhaps it\n> > was a mistake to move in that direction in the first place. I am\n> > tempted to say we should do something like the attached, but that\n> > may not fly well, as I suspect that -Wno-format-zero-length may be a\n> > lot more exotic than the -Wall compiler option.\n> \n> Yeah, I think portability concerns are what caused us not to go down\n> that road in the first place.\n> makes it harder to disable if your compiler doesn't like it.\n> \n> > An obvious second\n> > best option would be to drop -Wall from the \"everybody\" CFLAGS and\n> > move it to DEVELOPER_CFLAGS instead.\n> \n> Yeah, though that doesn't help the example above.\n> \n> As ugly as warning(\"%s\", \"\") is, I think it may be the thing that annoys\n> the smallest number of people.\n> \n> -Peff\n\nHow about using warning(\" \") instead?\n\nFor difftool.c specifically, the following is a fine solution,\nand doesn't require that we change our warning flags just for\nthis one file.\n-- \nDavid\n\n--- 8< ---\nFrom 28bdc381202ced35399cfdf4899a019b0000c7a0 Mon Sep 17 00:00:00 2001\nFrom: David Aguilar \nDate: Sat, 21 Jan 2017 21:21:09 -0800\nSubject: [PATCH] difftool: avoid zero-length format string\n\nThe purpose of the warning(\"\") line is to add an empty line to\nimprove the readability of the message.\n\nUse warning(\" \") instead to avoid zero-length format string\nwarnings while retaining the desired behavior.\n\nSigned-off-by: David Aguilar \n---\n builtin/difftool.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 42ad9e804a..d9f8ada291 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\t\twarning(_(\"both files modified: '%s' and '%s'.\"),\n \t\t\t\t\twtdir.buf, rdir.buf);\n \t\t\t\twarning(_(\"working tree file has been left.\"));\n-\t\t\t\twarning(\"\");\n+\t\t\t\twarning(\" \");\n \t\t\t\terr = 1;\n \t\t\t} else if (unlink(wtdir.buf) ||\n \t\t\t\t copy_file(wtdir.buf, rdir.buf, st.st_mode))\n-- \n2.11.0.747.g28bdc38120\n\n"},{"id":"310090","messageId":"20170124142346.u3d7l6772mtkgpcf@sigill.intra.peff.net","threadId":"44568","inReplyTo":"20170122052608.tpr5pihfgafhoynj@gmail.com","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-24T14:23:46Z","receivedAt":"2017-01-24T14:30:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"mn Sat, Jan 21, 2017 at 09:26:08PM -0800, David Aguilar wrote:\n\n> > > An obvious second\n> > > best option would be to drop -Wall from the \"everybody\" CFLAGS and\n> > > move it to DEVELOPER_CFLAGS instead.\n> > \n> > Yeah, though that doesn't help the example above.\n> > \n> > As ugly as warning(\"%s\", \"\") is, I think it may be the thing that annoys\n> > the smallest number of people.\n> > \n> > -Peff\n> \n> How about using warning(\" \") instead?\n> \n> For difftool.c specifically, the following is a fine solution,\n> and doesn't require that we change our warning flags just for\n> this one file.\n\nI dunno. As ugly as the \"%s\" thing is in the source, at least it doesn't\nchange the output. Not that an extra space is the end of the world, but\nit seems like it's letting the problem escape from the source code.\n\nDo people still care about resolving this? -Wno-format-zero-length is in\nthe DEVELOPER options. It wasn't clear to me if that was sufficient, or\nif we're going to get a bunch of reports from people that need to be\ndirected to the right compiler options.\n\nThe problematic code just hit 'next', so I suppose we'll see soon (OTOH,\nthe real test is when it get shipped as part of a release).\n\n-Peff\n"},{"id":"310113","messageId":"xmqqlgu0ceia.fsf@gitster.mtv.corp.google.com","threadId":"44568","inReplyTo":"20170124142346.u3d7l6772mtkgpcf@sigill.intra.peff.net","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-24T21:52:13Z","receivedAt":"2017-01-24T21:52:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> > As ugly as warning(\"%s\", \"\") is, I think it may be the thing that annoys\n>> > the smallest number of people.\n>> > \n>> > -Peff\n>> \n>> How about using warning(\" \") instead?\n>> \n>> For difftool.c specifically, the following is a fine solution,\n>> and doesn't require that we change our warning flags just for\n>> this one file.\n>\n> I dunno. As ugly as the \"%s\" thing is in the source, at least it doesn't\n> change the output. Not that an extra space is the end of the world, but\n> it seems like it's letting the problem escape from the source code.\n>\n> Do people still care about resolving this? -Wno-format-zero-length is in\n> the DEVELOPER options. It wasn't clear to me if that was sufficient, or\n> if we're going to get a bunch of reports from people that need to be\n> directed to the right compiler options.\n\nI view both as ugly, but probably \"%s\", \"\" is lessor of the two\nevils.\n\nPerhaps\n\n\t#define JUST_SHOW_EMPTY_LINE \"%s\", \"\"\n\n\t\t...\n\t\twarning(JUST_SHOW_EMPTY_LINE);\n ...\n\nor something silly like that?\n\n"},{"id":"310119","messageId":"20170124230500.h3fasbvutjkkke5h@sigill.intra.peff.net","threadId":"44568","inReplyTo":"xmqqlgu0ceia.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-24T23:05:00Z","receivedAt":"2017-01-24T23:05:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 24, 2017 at 01:52:13PM -0800, Junio C Hamano wrote:\n\n> > I dunno. As ugly as the \"%s\" thing is in the source, at least it doesn't\n> > change the output. Not that an extra space is the end of the world, but\n> > it seems like it's letting the problem escape from the source code.\n> >\n> > Do people still care about resolving this? -Wno-format-zero-length is in\n> > the DEVELOPER options. It wasn't clear to me if that was sufficient, or\n> > if we're going to get a bunch of reports from people that need to be\n> > directed to the right compiler options.\n> \n> I view both as ugly, but probably \"%s\", \"\" is lessor of the two\n> evils.\n> \n> Perhaps\n> \n> \t#define JUST_SHOW_EMPTY_LINE \"%s\", \"\"\n> \n> \t\t...\n> \t\twarning(JUST_SHOW_EMPTY_LINE);\n> ...\n> \n> or something silly like that?\n\nGross, but at least it's self documenting. :)\n\nI guess a less horrible version of that is:\n\n static inline warning_blank_line(void)\n {\n\twarning(\"%s\", \"\");\n }\n\nWe'd potentially need a matching one for error(), but at last it avoids\nmacro trickery.\n\n-Peff\n"},{"id":"310150","messageId":"alpine.DEB.2.20.1701251135090.3469@virtualbox","threadId":"44568","inReplyTo":"20170124230500.h3fasbvutjkkke5h@sigill.intra.peff.net","subject":"Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-25T10:36:50Z","receivedAt":"2017-01-25T10:37:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Tue, 24 Jan 2017, Jeff King wrote:\n\n> On Tue, Jan 24, 2017 at 01:52:13PM -0800, Junio C Hamano wrote:\n> \n> > > I dunno. As ugly as the \"%s\" thing is in the source, at least it\n> > > doesn't change the output. Not that an extra space is the end of the\n> > > world, but it seems like it's letting the problem escape from the\n> > > source code.\n> > >\n> > > Do people still care about resolving this? -Wno-format-zero-length\n> > > is in the DEVELOPER options. It wasn't clear to me if that was\n> > > sufficient, or if we're going to get a bunch of reports from people\n> > > that need to be directed to the right compiler options.\n> > \n> > I view both as ugly, but probably \"%s\", \"\" is lessor of the two evils.\n> > \n> > Perhaps\n> > \n> > \t#define JUST_SHOW_EMPTY_LINE \"%s\", \"\"\n> > \n> > \t\t...\n> > \t\twarning(JUST_SHOW_EMPTY_LINE);\n> > ...\n> > \n> > or something silly like that?\n> \n> Gross, but at least it's self documenting. :)\n> \n> I guess a less horrible version of that is:\n> \n> static inline warning_blank_line(void)\n> {\n> \twarning(\"%s\", \"\");\n> }\n> \n> We'd potentially need a matching one for error(), but at last it avoids\n> macro trickery.\n\nI fail to see how this function, or this definition, makes the code better\nthan simply calling `warning(\"%s\", \"\");` and be done with it.\n\nCiao,\nJohannes\n"},{"id":"310175","messageId":"20170125183542.pe5qolexqqx6jhsi@sigill.intra.peff.net","threadId":"44568","inReplyTo":"alpine.DEB.2.20.1701251135090.3469@virtualbox","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-25T18:35:42Z","receivedAt":"2017-01-25T18:35:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 25, 2017 at 11:36:50AM +0100, Johannes Schindelin wrote:\n\n> > Gross, but at least it's self documenting. :)\n> > \n> > I guess a less horrible version of that is:\n> > \n> > static inline warning_blank_line(void)\n> > {\n> > \twarning(\"%s\", \"\");\n> > }\n> > \n> > We'd potentially need a matching one for error(), but at last it avoids\n> > macro trickery.\n> \n> I fail to see how this function, or this definition, makes the code better\n> than simply calling `warning(\"%s\", \"\");` and be done with it.\n\nThe only advantage is that it is self-documenting, so somebody does not\ncome through later and convert (\"%s\", \"\") back to (\"\"). We could also\nwrite a comment. But I am happy if we simply catch it in review (or\npreferably the person is clueful enough to read the output of git-blame\nand see why it is that way in the first place).\n\nSo maybe:\n\n-- >8 --\nSubject: [PATCH] difftool: hack around -Wzero-length-format warning\n\nBuilding with \"gcc -Wall\" will complain that the format in:\n\n warning(\"\")\n\nis empty. Which is true, but the warning is over-eager. We\nare calling the function for its side effect of printing\n\"warning:\", even with an empty string.\n\nOur DEVELOPER Makefile knob disables the warning, but not\neverybody uses it. Let's silence the warning in the code so\nthat nobody reports it or tries to \"fix\" it.\n\nSigned-off-by: Jeff King \n---\n builtin/difftool.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/difftool.c b/builtin/difftool.c\nindex 42ad9e804..b5e85ab07 100644\n--- a/builtin/difftool.c\n+++ b/builtin/difftool.c\n@@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n \t\t\t\twarning(_(\"both files modified: '%s' and '%s'.\"),\n \t\t\t\t\twtdir.buf, rdir.buf);\n \t\t\t\twarning(_(\"working tree file has been left.\"));\n-\t\t\t\twarning(\"\");\n+\t\t\t\twarning(\"%s\", \"\");\n \t\t\t\terr = 1;\n \t\t\t} else if (unlink(wtdir.buf) ||\n \t\t\t\t copy_file(wtdir.buf, rdir.buf, st.st_mode))\n-- \n2.11.0.840.gd37c5973a\n"},{"id":"310211","messageId":"xmqq7f5iakxw.fsf@gitster.mtv.corp.google.com","threadId":"44568","inReplyTo":"20170125183542.pe5qolexqqx6jhsi@sigill.intra.peff.net","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-25T21:28:27Z","receivedAt":"2017-01-25T21:29:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The only advantage is that it is self-documenting, so somebody does not\n> come through later and convert (\"%s\", \"\") back to (\"\"). We could also\n> write a comment. But I am happy if we simply catch it in review (or\n> preferably the person is clueful enough to read the output of git-blame\n> and see why it is that way in the first place).\n\nAnd the last sentence unfortunatly does not reflect reality. \n\nI would prefer something self-documenting, like your wrapper with a\ncomment. Then somebody who is looking at the implementation of\nwarning_blank_line() will not get tempted to turn \"%s\", \"\" into \"\"\nbecause of the comment. And somebody who is looking at the callsite\nof warning_blank_line() will think twice before suggesting to turn\nit into warning(\"\").\n\nThat does not make it unnecessary to review; we still need to catch\nthose who wants to add new calls to warning(\"\") without even knowing\nthe presence of warning_blank_line(), if the original codepath being\ntouched does not have any call to it.\n\n> So maybe:\n\nIn any case, the patch is a minimum effort band-aid that lets us\npunt on the whole issue for now, so I'll queue it as-is.\n\nThanks.\n\n\n> -- >8 --\n> Subject: [PATCH] difftool: hack around -Wzero-length-format warning\n>\n> Building with \"gcc -Wall\" will complain that the format in:\n>\n> warning(\"\")\n>\n> is empty. Which is true, but the warning is over-eager. We\n> are calling the function for its side effect of printing\n> \"warning:\", even with an empty string.\n>\n> Our DEVELOPER Makefile knob disables the warning, but not\n> everybody uses it. Let's silence the warning in the code so\n> that nobody reports it or tries to \"fix\" it.\n>\n> Signed-off-by: Jeff King \n> ---\n> builtin/difftool.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index 42ad9e804..b5e85ab07 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> \t\t\t\twarning(_(\"both files modified: '%s' and '%s'.\"),\n> \t\t\t\t\twtdir.buf, rdir.buf);\n> \t\t\t\twarning(_(\"working tree file has been left.\"));\n> -\t\t\t\twarning(\"\");\n> +\t\t\t\twarning(\"%s\", \"\");\n> \t\t\t\terr = 1;\n> \t\t\t} else if (unlink(wtdir.buf) ||\n> \t\t\t\t copy_file(wtdir.buf, rdir.buf, st.st_mode))\n"},{"id":"310221","messageId":"20170125220101.et67ebkumsqosaku@sigill.intra.peff.net","threadId":"44568","inReplyTo":"xmqq7f5iakxw.fsf@gitster.mtv.corp.google.com","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-25T22:01:01Z","receivedAt":"2017-01-25T22:01:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 25, 2017 at 01:28:27PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The only advantage is that it is self-documenting, so somebody does not\n> > come through later and convert (\"%s\", \"\") back to (\"\"). We could also\n> > write a comment. But I am happy if we simply catch it in review (or\n> > preferably the person is clueful enough to read the output of git-blame\n> > and see why it is that way in the first place).\n> \n> And the last sentence unfortunatly does not reflect reality. \n> \n> I would prefer something self-documenting, like your wrapper with a\n> comment. Then somebody who is looking at the implementation of\n> warning_blank_line() will not get tempted to turn \"%s\", \"\" into \"\"\n> because of the comment. And somebody who is looking at the callsite\n> of warning_blank_line() will think twice before suggesting to turn\n> it into warning(\"\").\n\nI am OK with it either way. I was mostly responding to Dscho's\ncomplaint, and I would just like to get this resolved so we never have\nto revisit it again. :)\n\n> In any case, the patch is a minimum effort band-aid that lets us\n> punt on the whole issue for now, so I'll queue it as-is.\n\nHere's one other option that I came across. Pragmas feel gross, but I\nthink it will behave as we want, and it doesn't require cooperation from\nthe callsites at all.\n\n-- >8 --\nSubject: [PATCH] disable -Wzero-length-format via #pragma\n\nBuilding with \"gcc -Wall\" will complain that the format in:\n\n warning(\"\")\n\nis empty. Which is true, but the warning is over-eager. We\nare calling the function for its side effect of printing\n\"warning:\", even with an empty string.\n\nWe disable this warning already with the DEVELOPER Makefile\nknob. But we can't unconditionally add -Wno-format-zero-length\nto the normal CFLAGS variable, because not all compilers will\nunderstand it. So we may get reports about the warning from\nnon-developer users who compile with our default of -Wall.\n\nInstead, we can disable the warning using a gcc-specific\n#pragma. This should be ignored by non-gcc compilers, and do\nwhat we want for gcc.\n\nI tested also with clang, which often implements gcc\ncompatible extensions like this. Clang does not generate the\nwarning in the first place, but also does not complain about\nour pragma.\n\nSigned-off-by: Jeff King \n---\n git-compat-util.h | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 325950426..a6558930d 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -413,6 +413,8 @@ extern int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2\n extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n extern void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n+#pragma GCC diagnostic ignored \"-Wformat-zero-length\"\n+\n #ifndef NO_OPENSSL\n #ifdef APPLE_COMMON_CRYPTO\n #include \"compat/apple-common-crypto.h\"\n-- \n2.11.0.840.gd37c5973a\n\n"},{"id":"310295","messageId":"546179e0-1d6e-86f7-00cf-e13218b76de1@kdbg.org","threadId":"44568","inReplyTo":"20170125220101.et67ebkumsqosaku@sigill.intra.peff.net","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-01-26T06:39:55Z","receivedAt":"2017-01-26T06:40:05Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 25.01.2017 um 23:01 schrieb Jeff King:\n> +#pragma GCC diagnostic ignored \"-Wformat-zero-length\"\n\nLast time I used #pragma GCC in a cross-platform project, it triggered \nan \"unknown pragma\" warning for MSVC. (It was the C++ compiler, I don't \nknow if the C compiler would also warn.) It would have to be spelled \nlike this:\n\n#pragma warning(disable: 4068) /* MSVC: unknown pragma */\n#pragma GCC diagnostic ignored \"-Wformat-zero-length\"\n\nDscho mentioned that he's compiling with MSVC. It would do him a favor.\n\n-- Hannes\n\n"},{"id":"310302","messageId":"alpine.DEB.2.20.1701261213060.3469@virtualbox","threadId":"44568","inReplyTo":"20170125183542.pe5qolexqqx6jhsi@sigill.intra.peff.net","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-26T11:16:10Z","receivedAt":"2017-01-26T11:16:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 25 Jan 2017, Jeff King wrote:\n\n> On Wed, Jan 25, 2017 at 11:36:50AM +0100, Johannes Schindelin wrote:\n> \n> > > Gross, but at least it's self documenting. :)\n> > > \n> > > I guess a less horrible version of that is:\n> > > \n> > > static inline warning_blank_line(void)\n> > > {\n> > > \twarning(\"%s\", \"\");\n> > > }\n> > > \n> > > We'd potentially need a matching one for error(), but at last it avoids\n> > > macro trickery.\n> > \n> > I fail to see how this function, or this definition, makes the code better\n> > than simply calling `warning(\"%s\", \"\");` and be done with it.\n> \n> The only advantage is that it is self-documenting, so somebody does not\n> come through later and convert (\"%s\", \"\") back to (\"\").\n\nWe could switch the DEVELOPER option on by default, when gcc or clang is\nused at least. Otherwise the DEVELOPER option (which I like very much)\nwould not be able to live up to its full potential.\n\nAnother thing we should consider: paying more attention to Continuous\nIntegration. At the moment, it happens quite frequently that `pu` builds\nand passes the test suite fine on Linux, but neither on Windows nor on\nMacOSX and it takes days to get the regressions fixed.\n\nI vote for this patch:\n\n> -- >8 --\n> Subject: [PATCH] difftool: hack around -Wzero-length-format warning\n> \n> Building with \"gcc -Wall\" will complain that the format in:\n> \n> warning(\"\")\n> \n> is empty. Which is true, but the warning is over-eager. We\n> are calling the function for its side effect of printing\n> \"warning:\", even with an empty string.\n> \n> Our DEVELOPER Makefile knob disables the warning, but not\n> everybody uses it. Let's silence the warning in the code so\n> that nobody reports it or tries to \"fix\" it.\n> \n> Signed-off-by: Jeff King \n> ---\n> builtin/difftool.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/difftool.c b/builtin/difftool.c\n> index 42ad9e804..b5e85ab07 100644\n> --- a/builtin/difftool.c\n> +++ b/builtin/difftool.c\n> @@ -567,7 +567,7 @@ static int run_dir_diff(const char *extcmd, int symlinks, const char *prefix,\n> \t\t\t\twarning(_(\"both files modified: '%s' and '%s'.\"),\n> \t\t\t\t\twtdir.buf, rdir.buf);\n> \t\t\t\twarning(_(\"working tree file has been left.\"));\n> -\t\t\t\twarning(\"\");\n> +\t\t\t\twarning(\"%s\", \"\");\n> \t\t\t\terr = 1;\n> \t\t\t} else if (unlink(wtdir.buf) ||\n> \t\t\t\t copy_file(wtdir.buf, rdir.buf, st.st_mode))\n> -- \n> 2.11.0.840.gd37c5973a\n\nCiao,\nDscho\n"},{"id":"310304","messageId":"alpine.DEB.2.20.1701261226220.3469@virtualbox","threadId":"44568","inReplyTo":"546179e0-1d6e-86f7-00cf-e13218b76de1@kdbg.org","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-26T11:37:46Z","receivedAt":"2017-01-26T11:38:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Hannes,\n\nOn Thu, 26 Jan 2017, Johannes Sixt wrote:\n\n> Am 25.01.2017 um 23:01 schrieb Jeff King:\n> > +#pragma GCC diagnostic ignored \"-Wformat-zero-length\"\n> \n> Last time I used #pragma GCC in a cross-platform project, it triggered\n> an \"unknown pragma\" warning for MSVC.\n\nIt is starting to become a little funny how many ways we can discuss the\nresolution of the GCC compiler warning.\n\nAnd it starts to show: we try to solve the thing in so many ways, just to\navoid the obviously most-trivial patch to change warning(\"\"); to\nwarning(\"%s\", \"\") (the change to warning(\" \"); would change behavior, but\nI would be fine with that, too).\n\nI am not really interested in any of these complicated workarounds. If you\ngentle people decide they are better in Git's source code, go ahead. I do\nnot have to like what you are doing, I just have to work with it.\n\n> (It was the C++ compiler, I don't know if the C compiler would also\n> warn.) It would have to be spelled like this:\n> \n> #pragma warning(disable: 4068) /* MSVC: unknown pragma */\n> #pragma GCC diagnostic ignored \"-Wformat-zero-length\"\n> \n> Dscho mentioned that he's compiling with MSVC. It would do him a favor.\n\nI am compiling with MSVC, and the idea is to tap into that large number of\nWindows developers who Git traditionally has had a really bad time\nattracting. From that perspective, I would say it would not only do me a\nfavor, but anybody who builds Git for Windows using Visual Studio.\n\nBut we also have to consider whether it would do anybody a \"dis-favor\".\n#pragma statements are by definition highly dependent on the compiler. I\nhave no idea whether there are developers out there building Git with\nC compilers other than GCC, clang or MSVC (as I did back in the days on\nIRIX and HP/UX), but there is quite the potential for problems here [*1*].\n\nTo keep Git's source code truly portable, the #pragma would have to be\nguarded by a GCC-specific #ifdef ... #endif.\n\nCiao,\nDscho\n\nFootnote *1*: This is just another instance where a discussion on the Git\nmailing list reminds me of\nhttp://thedailywtf.com/articles/The_Complicator_0x27_s_Gloves, as it tries\nto avoid an obvious solution by trying to come up with a different\nsolution that in turn requires additional solutions to additional problems\ncaused by the alternative solution.\n"},{"id":"310319","messageId":"20170126143252.ne533mcv3n2ksbai@sigill.intra.peff.net","threadId":"44568","inReplyTo":"546179e0-1d6e-86f7-00cf-e13218b76de1@kdbg.org","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-26T14:32:53Z","receivedAt":"2017-01-26T14:33:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 26, 2017 at 07:39:55AM +0100, Johannes Sixt wrote:\n\n> Am 25.01.2017 um 23:01 schrieb Jeff King:\n> > +#pragma GCC diagnostic ignored \"-Wformat-zero-length\"\n> \n> Last time I used #pragma GCC in a cross-platform project, it triggered an\n> \"unknown pragma\" warning for MSVC. (It was the C++ compiler, I don't know if\n> the C compiler would also warn.) It would have to be spelled like this:\n> \n> #pragma warning(disable: 4068) /* MSVC: unknown pragma */\n> #pragma GCC diagnostic ignored \"-Wformat-zero-length\"\n> \n> Dscho mentioned that he's compiling with MSVC. It would do him a favor.\n\nBleh. The point of #pragma is to ignore ones you don't know about.\n\nIt would be easy to wrap it in an #ifdef for __GNUC__ (there is already\na similar pragma with similar wrapping in the code base).\n\nAnyway. I do not want to make life harder for anyone. I think there are\nseveral options floating around now, so I will let Junio decide which\none he wants to pick up.\n\n-Peff\n"},{"id":"310320","messageId":"20170126143534.mekszrwxnytadwyo@sigill.intra.peff.net","threadId":"44568","inReplyTo":"alpine.DEB.2.20.1701261226220.3469@virtualbox","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-26T14:35:34Z","receivedAt":"2017-01-26T14:35:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 26, 2017 at 12:37:46PM +0100, Johannes Schindelin wrote:\n\n> > Am 25.01.2017 um 23:01 schrieb Jeff King:\n> > > +#pragma GCC diagnostic ignored \"-Wformat-zero-length\"\n> > \n> > Last time I used #pragma GCC in a cross-platform project, it triggered\n> > an \"unknown pragma\" warning for MSVC.\n> \n> It is starting to become a little funny how many ways we can discuss the\n> resolution of the GCC compiler warning.\n> \n> And it starts to show: we try to solve the thing in so many ways, just to\n> avoid the obviously most-trivial patch to change warning(\"\"); to\n> warning(\"%s\", \"\") (the change to warning(\" \"); would change behavior, but\n> I would be fine with that, too).\n\nThe point is that the trivial patch fixes _this_ case, but does not\nprevent the discussion from happening again later. They are two separate\nproblems. I am OK not solving the latter one and relying on review (as\nI've already said), but the solutions do not do the same thing.\n\n-Peff\n"},{"id":"310321","messageId":"20170126143906.j6j64v4cyatwvlik@sigill.intra.peff.net","threadId":"44568","inReplyTo":"alpine.DEB.2.20.1701261213060.3469@virtualbox","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-01-26T14:39:06Z","receivedAt":"2017-01-26T14:39:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 26, 2017 at 12:16:10PM +0100, Johannes Schindelin wrote:\n\n> We could switch the DEVELOPER option on by default, when gcc or clang is\n> used at least. Otherwise the DEVELOPER option (which I like very much)\n> would not be able to live up to its full potential.\n\nI'm not sure that is a good idea. The options include -Werror, which is\na good thing for developers to respect. But people using older versions\nof compilers, or on systems with slightly different header files, may\nsee extraneous warnings. It's good to fix those warnings, but it is a\nbig inconvenience to regular users who just want to build and use git.\n\nYou could split the DEVELOPER options into two groups, though, and only\nenable when (after verifying that it is indeed gcc/clang in use). But\nnow who is coming up with complicated fixes for the warning(\"\") issue?\n:)\n\n-Peff\n"},{"id":"310324","messageId":"alpine.DEB.2.20.1701261547470.3469@virtualbox","threadId":"44568","inReplyTo":"20170126143906.j6j64v4cyatwvlik@sigill.intra.peff.net","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-01-26T14:49:51Z","receivedAt":"2017-01-26T14:50:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 26 Jan 2017, Jeff King wrote:\n\n> On Thu, Jan 26, 2017 at 12:16:10PM +0100, Johannes Schindelin wrote:\n> \n> > We could switch the DEVELOPER option on by default, when gcc or clang\n> > is used at least. Otherwise the DEVELOPER option (which I like very\n> > much) would not be able to live up to its full potential.\n> \n> I'm not sure that is a good idea. The options include -Werror, which is\n> a good thing for developers to respect. But people using older versions\n> of compilers, or on systems with slightly different header files, may\n> see extraneous warnings. It's good to fix those warnings, but it is a\n> big inconvenience to regular users who just want to build and use git.\n\nYeah, you cannot have the cake and eat it, too: on the one side, we want\nGit contributors to see problems early (preferably before even submitting\nthe patch) and at the same time, we want users who compile their Git\nthemselves to have no trouble doing so.\n\n> You could split the DEVELOPER options into two groups, though, and only\n> enable when (after verifying that it is indeed gcc/clang in use). But\n> now who is coming up with complicated fixes for the warning(\"\") issue?\n> :)\n\nThat is yet another instance of the complicator's glove; I would rather\navoid that.\n\nTo me, the obvious solution is to pay more attention to Continuous\nIntegration, in particular on fixing problems right after they are\nreported.\n\nCiao,\nJohannes\n"},{"id":"310339","messageId":"xmqqlgtx65l0.fsf@gitster.mtv.corp.google.com","threadId":"44568","inReplyTo":"20170126143252.ne533mcv3n2ksbai@sigill.intra.peff.net","subject":"Re: Fixing the warning about warning(\"\"); was: Re: [PATCH] difftool.c: mark a file-local symbol with static","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-01-26T18:26:03Z","receivedAt":"2017-01-26T18:26:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Jan 26, 2017 at 07:39:55AM +0100, Johannes Sixt wrote:\n>\n>> Am 25.01.2017 um 23:01 schrieb Jeff King:\n>> > +#pragma GCC diagnostic ignored \"-Wformat-zero-length\"\n>> \n>> Last time I used #pragma GCC in a cross-platform project, it triggered an\n>> \"unknown pragma\" warning for MSVC. (It was the C++ compiler, I don't know if\n>> the C compiler would also warn.) It would have to be spelled like this:\n>> \n>> #pragma warning(disable: 4068) /* MSVC: unknown pragma */\n>> #pragma GCC diagnostic ignored \"-Wformat-zero-length\"\n>> \n>> Dscho mentioned that he's compiling with MSVC. It would do him a favor.\n>\n> Bleh. The point of #pragma is to ignore ones you don't know about.\n\nYes. Let's not go there; somebody else's compiler will complain\nabout \"#pragma warning(disable: 4068)\" that it does not understand.\n\n> Anyway. I do not want to make life harder for anyone. I think there are\n> several options floating around now, so I will let Junio decide which\n> one he wants to pick up.\n\nWell, I'll keep the \"do nothing other than squelching this instance\"\nto solve one of the two problems for now. \n\nThe other \"can we make it harder to make the same issue and reduce\nthe need to discuss this again on the list?\" can be an independent\nfollow-up patch, and I do have a preference (the \"less horrible\nversion, that is static inline warning_blank_line(void)\" you gave us\nin <20170124230500.h3fasbvutjkkke5h@sigill.intra.peff.net>), but I\ndo not think we are in a hurry.\n\nThanks.\n\n\n"}]}