{"thread":{"id":"52902","subject":"[PATCH] rebase-interactive.c: silence format-zero-length warnings","startedAt":"2020-02-27T20:25:34Z","lastAt":"2020-03-03T14:20:39Z","messageCount":6,"participants":["Ralf Thielow via GitGitGadget","Jeff King","Junio C Hamano","Alban Gruin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"392638","messageId":"pull.567.git.1582835130592.gitgitgadget@gmail.com","threadId":"52902","inReplyTo":null,"subject":"[PATCH] rebase-interactive.c: silence format-zero-length warnings","fromName":"Ralf Thielow via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-02-27T20:25:30Z","receivedAt":"2020-02-27T20:25:34Z","isPatch":true,"sender":{"key":"ralf.thielow@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1275832?v=4"},"body":"From: Ralf Thielow \n\nFixes the following warnings:\n\nrebase-interactive.c: In function ‘edit_todo_list’:\nrebase-interactive.c:137:38: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n write_file(rebase_path_dropped(), \"\");\nrebase-interactive.c:144:37: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n write_file(rebase_path_dropped(), \"\");\n\nSigned-off-by: Ralf Thielow \n---\n rebase-interactive.c: silence format-zero-length warnings\n \n I noticed these warnings a while ago and they're still there, so here's\n my fix.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-567%2Fralfth%2Fformat-zero-length-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-567/ralfth/format-zero-length-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/567\n\n rebase-interactive.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/rebase-interactive.c b/rebase-interactive.c\nindex ac001dea588..0a4572e67ea 100644\n--- a/rebase-interactive.c\n+++ b/rebase-interactive.c\n@@ -134,14 +134,14 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n \n \tif (incorrect) {\n \t\tif (todo_list_check_against_backup(r, new_todo)) {\n-\t\t\twrite_file(rebase_path_dropped(), \"\");\n+\t\t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n \t\t\treturn -4;\n \t\t}\n \n \t\tif (incorrect > 0)\n \t\t\tunlink(rebase_path_dropped());\n \t} else if (todo_list_check(todo_list, new_todo)) {\n-\t\twrite_file(rebase_path_dropped(), \"\");\n+\t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n \t\treturn -4;\n \t}\n \n\nbase-commit: 2d2118b814c11f509e1aa76cb07110f7231668dc\n-- \ngitgitgadget\n"},{"id":"392648","messageId":"20200227235445.GA1371170@coredump.intra.peff.net","threadId":"52902","inReplyTo":"pull.567.git.1582835130592.gitgitgadget@gmail.com","subject":"[PATCH] config.mak.dev: re-enable -Wformat-zero-length","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-27T23:54:45Z","receivedAt":"2020-02-27T23:54:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 27, 2020 at 08:25:30PM +0000, Ralf Thielow via GitGitGadget wrote:\n\n> Fixes the following warnings:\n> \n> rebase-interactive.c: In function ‘edit_todo_list’:\n> rebase-interactive.c:137:38: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> write_file(rebase_path_dropped(), \"\");\n> rebase-interactive.c:144:37: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> write_file(rebase_path_dropped(), \"\");\n\nThanks, I think this is worth doing.\n\nI had noticed them, too, but then they \"went away\" so I assumed they had\nalready been fixed. It turns out that it's the difference between a\nbuild with and without the DEVELOPER Makefile knob set.\n\nI think we should do this on top:\n\n-- >8 --\nSubject: [PATCH] config.mak.dev: re-enable -Wformat-zero-length\n\nWe recently triggered some -Wformat-zero-length warnings in the code,\nbut no developers noticed because we suppress that warning in builds\nwith the DEVELOPER=1 Makefile knob set. But we _don't_ suppress them in\na non-developer build (and they're part of -Wall). So even though\nnon-developers probably aren't using -Werror, they see the annoying\nwarnings when they build.\n\nWe've had back and forth discussion over the years on whether this\nwarning is useful or not. In most cases we've seen, it's not true that\nthe call is a mistake, since we're using its side effects (like adding a\nnewline status_printf_ln()) or writing an empty string to a destination\nwhich is handled by the function (as in write_file()). And so we end up\nworking around it in the source by passing (\"%s\", \"\").\n\nThere's more discussion in the subthread starting at:\n\n https://lore.kernel.org/git/xmqqtwaod7ly.fsf@gitster.mtv.corp.google.com/\n\nThe short of it is that we probably can't just disable the warning for\neverybody because of portability issues. And ignoring it for developers\nputs us in the situation we're in now, where non-dev builds are annoyed.\n\nSince the workaround is both rarely needed and fairly straight-forward,\nlet's just commit to doing it as necessary, and re-enable the warning.\n\nSigned-off-by: Jeff King \n---\nI had totally forgotten about that thread until researching the history\njust now. There's another option there involving #pragma, but it was too\ngross for me to even suggest now as an alternative in the commit\nmessage. ;) I think this is the most practical improvement.\n\n config.mak.dev | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex bf1f3fcdee..89b218d11a 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -9,7 +9,6 @@ endif\n DEVELOPER_CFLAGS += -Wall\n DEVELOPER_CFLAGS += -Wdeclaration-after-statement\n DEVELOPER_CFLAGS += -Wformat-security\n-DEVELOPER_CFLAGS += -Wno-format-zero-length\n DEVELOPER_CFLAGS += -Wold-style-definition\n DEVELOPER_CFLAGS += -Woverflow\n DEVELOPER_CFLAGS += -Wpointer-arith\n-- \n2.25.1.911.g022f5304bc\n\n"},{"id":"392670","messageId":"xmqqtv3aek8o.fsf@gitster-ct.c.googlers.com","threadId":"52902","inReplyTo":"20200227235445.GA1371170@coredump.intra.peff.net","subject":"Re: [PATCH] config.mak.dev: re-enable -Wformat-zero-length","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-28T16:42:47Z","receivedAt":"2020-02-28T16:42:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The short of it is that we probably can't just disable the warning for\n> everybody because of portability issues. And ignoring it for developers\n> puts us in the situation we're in now, where non-dev builds are annoyed.\n\n\"git blame\" unfortunately is very bad at poing at a commit that\nremoved something, so I do not offhand know how much it would help\nreaders who later wonder \"oh, I am sure we had thing to disable\nformat-zero-length warning, and I want to learn the reason why we\ndropped it\", but thanks for writing this down.\n\n> Since the workaround is both rarely needed and fairly straight-forward,\n> let's just commit to doing it as necessary, and re-enable the warning.\n>\n> Signed-off-by: Jeff King \n> ---\n> I had totally forgotten about that thread until researching the history\n> just now. There's another option there involving #pragma, but it was too\n> gross for me to even suggest now as an alternative in the commit\n> message. ;) I think this is the most practical improvement.\n>\n> config.mak.dev | 1 -\n> 1 file changed, 1 deletion(-)\n>\n> diff --git a/config.mak.dev b/config.mak.dev\n> index bf1f3fcdee..89b218d11a 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -9,7 +9,6 @@ endif\n> DEVELOPER_CFLAGS += -Wall\n> DEVELOPER_CFLAGS += -Wdeclaration-after-statement\n> DEVELOPER_CFLAGS += -Wformat-security\n> -DEVELOPER_CFLAGS += -Wno-format-zero-length\n> DEVELOPER_CFLAGS += -Wold-style-definition\n> DEVELOPER_CFLAGS += -Woverflow\n> DEVELOPER_CFLAGS += -Wpointer-arith\n"},{"id":"392673","messageId":"20200228170641.GA1405401@coredump.intra.peff.net","threadId":"52902","inReplyTo":"xmqqtv3aek8o.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] config.mak.dev: re-enable -Wformat-zero-length","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-28T17:06:41Z","receivedAt":"2020-02-28T17:06:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2020 at 08:42:47AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The short of it is that we probably can't just disable the warning for\n> > everybody because of portability issues. And ignoring it for developers\n> > puts us in the situation we're in now, where non-dev builds are annoyed.\n> \n> \"git blame\" unfortunately is very bad at poing at a commit that\n> removed something, so I do not offhand know how much it would help\n> readers who later wonder \"oh, I am sure we had thing to disable\n> format-zero-length warning, and I want to learn the reason why we\n> dropped it\", but thanks for writing this down.\n\nI often turn to \"git log -Sformat-zero\" for this (and in fact that was\nvery useful for the research I did yesterday). But of course you have to\nfirst _know_ about the warning and wonder \"hey, didn't used ignore it?\"\nfor that to be useful.\n\n-Peff\n"},{"id":"392775","messageId":"87196142-8473-7d19-4edd-7452eaefda1c@gmail.com","threadId":"52902","inReplyTo":"pull.567.git.1582835130592.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase-interactive.c: silence format-zero-length warnings","fromName":"Alban Gruin","fromEmail":"alban.gruin@gmail.com","sentAt":"2020-03-03T10:17:46Z","receivedAt":"2020-03-03T10:17:59Z","isPatch":true,"sender":{"key":"alban.gruin@gmail.com","avatar":"https://avatars.githubusercontent.com/u/6310153?v=4"},"body":"Hi Ralf,\n\nLe 27/02/2020 à 21:25, Ralf Thielow via GitGitGadget a écrit :\n> From: Ralf Thielow \n> \n> Fixes the following warnings:\n> \n> rebase-interactive.c: In function ‘edit_todo_list’:\n> rebase-interactive.c:137:38: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> write_file(rebase_path_dropped(), \"\");\n> rebase-interactive.c:144:37: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> write_file(rebase_path_dropped(), \"\");\n> \n> Signed-off-by: Ralf Thielow \n> ---\n> rebase-interactive.c: silence format-zero-length warnings\n> \n> I noticed these warnings a while ago and they're still there, so here's\n> my fix.\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-567%2Fralfth%2Fformat-zero-length-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-567/ralfth/format-zero-length-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/567\n> \n> rebase-interactive.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/rebase-interactive.c b/rebase-interactive.c\n> index ac001dea588..0a4572e67ea 100644\n> --- a/rebase-interactive.c\n> +++ b/rebase-interactive.c\n> @@ -134,14 +134,14 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,\n> \n> \tif (incorrect) {\n> \t\tif (todo_list_check_against_backup(r, new_todo)) {\n> -\t\t\twrite_file(rebase_path_dropped(), \"\");\n> +\t\t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n> \t\t\treturn -4;\n> \t\t}\n> \n> \t\tif (incorrect > 0)\n> \t\t\tunlink(rebase_path_dropped());\n> \t} else if (todo_list_check(todo_list, new_todo)) {\n> -\t\twrite_file(rebase_path_dropped(), \"\");\n> +\t\twrite_file(rebase_path_dropped(), \"%s\", \"\");\n> \t\treturn -4;\n> \t}\n> \n> \n> base-commit: 2d2118b814c11f509e1aa76cb07110f7231668dc\n> \n\nAck.\n\nOn a tangent: what's wrong with empty format strings?\n\nCheers,\nAlban\n\n"},{"id":"392783","messageId":"xmqqh7z5a5an.fsf@gitster-ct.c.googlers.com","threadId":"52902","inReplyTo":"87196142-8473-7d19-4edd-7452eaefda1c@gmail.com","subject":"Re: [PATCH] rebase-interactive.c: silence format-zero-length warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-03T14:20:32Z","receivedAt":"2020-03-03T14:20:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alban Gruin writes:\n\n>> Fixes the following warnings:\n>> \n>> rebase-interactive.c: In function ‘edit_todo_list’:\n>> rebase-interactive.c:137:38: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n>> write_file(rebase_path_dropped(), \"\");\n>> rebase-interactive.c:144:37: warning: zero-length gnu_printf format string [-Wformat-zero-length]\n> ...\n> On a tangent: what's wrong with empty format strings?\n\nThose functions that are truly printf-like, such a call would be\nno-op and an indication of possible typo (\"did you forget a %s or\nsomething?\"), I presume.\n\nBut many of our functions that take printf-like format strings will\ndo useful things even when an empty string is given, so the warning\nis unwanted.\n"}]}