config.mak.dev: re-enable -Wformat-zero-length

Commit Message

Jeff King Feb. 27, 2020, 11:54 p.m. UTC

On Thu, Feb 27, 2020 at 08:25:30PM +0000, Ralf Thielow via GitGitGadget wrote:

> Fixes the following warnings:
> 
> rebase-interactive.c: In function ‘edit_todo_list’:
> rebase-interactive.c:137:38: warning: zero-length gnu_printf format string [-Wformat-zero-length]
>     write_file(rebase_path_dropped(), "");
> rebase-interactive.c:144:37: warning: zero-length gnu_printf format string [-Wformat-zero-length]
>    write_file(rebase_path_dropped(), "");

Thanks, I think this is worth doing.

I had noticed them, too, but then they "went away" so I assumed they had
already been fixed. It turns out that it's the difference between a
build with and without the DEVELOPER Makefile knob set.

I think we should do this on top:

-- >8 --
Subject: [PATCH] config.mak.dev: re-enable -Wformat-zero-length

We recently triggered some -Wformat-zero-length warnings in the code,
but no developers noticed because we suppress that warning in builds
with the DEVELOPER=1 Makefile knob set. But we _don't_ suppress them in
a non-developer build (and they're part of -Wall). So even though
non-developers probably aren't using -Werror, they see the annoying
warnings when they build.

We've had back and forth discussion over the years on whether this
warning is useful or not. In most cases we've seen, it's not true that
the call is a mistake, since we're using its side effects (like adding a
newline status_printf_ln()) or writing an empty string to a destination
which is handled by the function (as in write_file()). And so we end up
working around it in the source by passing ("%s", "").

There's more discussion in the subthread starting at:

  https://lore.kernel.org/git/[e-mail]/

The short of it is that we probably can't just disable the warning for
everybody because of portability issues. And ignoring it for developers
puts us in the situation we're in now, where non-dev builds are annoyed.

Since the workaround is both rarely needed and fairly straight-forward,
let's just commit to doing it as necessary, and re-enable the warning.

Signed-off-by: Jeff King <[e-mail]>
---
I had totally forgotten about that thread until researching the history
just now. There's another option there involving #pragma, but it was too
gross for me to even suggest now as an alternative in the commit
message. ;) I think this is the most practical improvement.

 config.mak.dev | 1 -
 1 file changed, 1 deletion(-)

Comments

Junio C Hamano Feb. 28, 2020, 4:42 p.m. UTC | #1

Jeff King <[e-mail]> writes:

> The short of it is that we probably can't just disable the warning for
> everybody because of portability issues. And ignoring it for developers
> puts us in the situation we're in now, where non-dev builds are annoyed.

"git blame" unfortunately is very bad at poing at a commit that
removed something, so I do not offhand know how much it would help
readers who later wonder "oh, I am sure we had thing to disable
format-zero-length warning, and I want to learn the reason why we
dropped it", but thanks for writing this down.

> Since the workaround is both rarely needed and fairly straight-forward,
> let's just commit to doing it as necessary, and re-enable the warning.
>
> Signed-off-by: Jeff King <[e-mail]>
> ---
> I had totally forgotten about that thread until researching the history
> just now. There's another option there involving #pragma, but it was too
> gross for me to even suggest now as an alternative in the commit
> message. ;) I think this is the most practical improvement.
>
>  config.mak.dev | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/config.mak.dev b/config.mak.dev
> index bf1f3fcdee..89b218d11a 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -9,7 +9,6 @@ endif
>  DEVELOPER_CFLAGS += -Wall
>  DEVELOPER_CFLAGS += -Wdeclaration-after-statement
>  DEVELOPER_CFLAGS += -Wformat-security
> -DEVELOPER_CFLAGS += -Wno-format-zero-length
>  DEVELOPER_CFLAGS += -Wold-style-definition
>  DEVELOPER_CFLAGS += -Woverflow
>  DEVELOPER_CFLAGS += -Wpointer-arith

Jeff King Feb. 28, 2020, 5:06 p.m. UTC | #2

On Fri, Feb 28, 2020 at 08:42:47AM -0800, Junio C Hamano wrote:

> Jeff King <[e-mail]> writes:
> 
> > The short of it is that we probably can't just disable the warning for
> > everybody because of portability issues. And ignoring it for developers
> > puts us in the situation we're in now, where non-dev builds are annoyed.
> 
> "git blame" unfortunately is very bad at poing at a commit that
> removed something, so I do not offhand know how much it would help
> readers who later wonder "oh, I am sure we had thing to disable
> format-zero-length warning, and I want to learn the reason why we
> dropped it", but thanks for writing this down.

I often turn to "git log -Sformat-zero" for this (and in fact that was
very useful for the research I did yesterday). But of course you have to
first _know_ about the warning and wonder "hey, didn't used ignore it?"
for that to be useful.

-Peff

Patch

diff --git a/config.mak.dev b/config.mak.dev
index bf1f3fcdee..89b218d11a 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -9,7 +9,6 @@  endif
 DEVELOPER_CFLAGS += -Wall
 DEVELOPER_CFLAGS += -Wdeclaration-after-statement
 DEVELOPER_CFLAGS += -Wformat-security
-DEVELOPER_CFLAGS += -Wno-format-zero-length
 DEVELOPER_CFLAGS += -Wold-style-definition
 DEVELOPER_CFLAGS += -Woverflow
 DEVELOPER_CFLAGS += -Wpointer-arith