rebase-interactive.c: silence format-zero-length warnings

PATCH In review Ralf Thielow via GitGitGadget 4 participants 6 messages Feb 27, 2020 – Mar 3, 2020

lore

Share & tools

lore

Ralf Thielow via GitGitGadget

author

2020-02-27 20:25 UTC #0 lore raw

PATCH 1/1 rebase-interactive.c: silence format-zero-length warnings

From: Ralf Thielow <[email protected]>

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(), "");

Signed-off-by Ralf Thielow <[email protected]>

---
rebase-interactive.c: silence format-zero-length warnings

I noticed these warnings a while ago and they're still there, so here's my fix.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-567%2Fralfth%2Fformat-zero-length-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-567/ralfth/format-zero-length-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/567

 rebase-interactive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

rebase-interactive.c +2 -2

index ac001dea588..0a4572e67ea 100644
@@ -134,14 +134,14 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
 
 	if (incorrect) {
 		if (todo_list_check_against_backup(r, new_todo)) {
-			write_file(rebase_path_dropped(), "");
+			write_file(rebase_path_dropped(), "%s", "");
 			return -4;
 		}
 
 		if (incorrect > 0)
 			unlink(rebase_path_dropped());
 	} else if (todo_list_check(todo_list, new_todo)) {
-		write_file(rebase_path_dropped(), "");
+		write_file(rebase_path_dropped(), "%s", "");
 		return -4;
 	}

base-commit: 2d2118b814c11f509e1aa76cb07110f7231668dc

Signature · 1 line

gitgitgadget

Jeff King

2020-02-27 23:54 UTC #1 lore raw

PATCH 1/1 config.mak.dev: re-enable -Wformat-zero-length

Quoted 7 lines · Ralf Thielow via GitGitGadget

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/[email protected]/

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 <[email protected]>

---
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(-)

config.mak.dev +0 -1

index bf1f3fcdee..89b218d11a 100644
@@ -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

Signature · 1 line

2.25.1.911.g022f5304bc

Junio C Hamano

2020-02-28 16:42 UTC #2 lore raw

Re: [PATCH] config.mak.dev: re-enable -Wformat-zero-length

Quoted 3 lines · Jeff King

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.

Quoted 25 lines

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 <[email protected]>
---
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(-)
config.mak.dev +0 -1
index bf1f3fcdee..89b218d11a 100644
@@ -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

2020-02-28 17:06 UTC #3 lore raw

Re: [PATCH] config.mak.dev: re-enable -Wformat-zero-length

Quoted 11 lines · Junio C Hamano

Quoted 3 lines · Jeff King
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

Alban Gruin

2020-03-03 10:17 UTC #4 lore raw

In reply to Ralf Thielow via GitGitGadget

Hi Ralf,

Le 27/02/2020 à 21:25, Ralf Thielow via GitGitGadget a écrit :

Quoted 46 lines

From: Ralf Thielow <[email protected]>
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(), "");
Signed-off-by Ralf Thielow <[email protected]>
---
rebase-interactive.c: silence format-zero-length warnings
I noticed these warnings a while ago and they're still there, so here's my fix.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-567%2Fralfth%2Fformat-zero-length-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-567/ralfth/format-zero-length-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/567
 rebase-interactive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
rebase-interactive.c +2 -2
index ac001dea588..0a4572e67ea 100644
@@ -134,14 +134,14 @@ int edit_todo_list(struct repository *r, struct todo_list *todo_list,
 
 	if (incorrect) {
 		if (todo_list_check_against_backup(r, new_todo)) {
-			write_file(rebase_path_dropped(), "");
+			write_file(rebase_path_dropped(), "%s", "");
 			return -4;
 		}
 
 		if (incorrect > 0)
 			unlink(rebase_path_dropped());
 	} else if (todo_list_check(todo_list, new_todo)) {
-		write_file(rebase_path_dropped(), "");
+		write_file(rebase_path_dropped(), "%s", "");
 		return -4;
 	}
 

Quoted 2 lines

base-commit: 2d2118b814c11f509e1aa76cb07110f7231668dc

Ack.

On a tangent: what's wrong with empty format strings?

Cheers,
Alban

Junio C Hamano

2020-03-03 14:20 UTC #5 lore raw

Quoted 8 lines · Alban Gruin

Quoted 6 lines
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]
...
On a tangent: what's wrong with empty format strings?

Those functions that are truly printf-like, such a call would be no-op and an indication of possible typo ("did you forget a %s or something?"), I presume.

But many of our functions that take printf-like format strings will do useful things even when an empty string is given, so the warning is unwanted.