[PATCH] status_printf_ln: Suppress false positive warnings of empty format string.

[PATCH] status_printf_ln: Suppress false positive warnings of empty format string.

[Stefan Beller]

This is a response to 8dd0ee823f1829a3aa228c3c73e31de5c89b5317.

Instead of having an empty string as format for the printf like function
status_printf_ln, we could insert an empty string into the format
parameter.

A similar fixup commit is found in linux (2e4c332913b5), but there
the empty string is replaced by a string containing one whitespace.

To determine, which approach is better I setup 2 test programs, which
either have a whitespace format (" ") or the empty string ("%s", ""),
looking like:
-
	#include <stdlib.h>
	#include <stdio.h>
	int main (int argc, char** argv) {
		long i;
		for (i = 0; i < 1024*1024*1024; ++i)
			printf(" ");
	}
-
Checking the required time of the programs, while redirecting the actual
output (the billion white spaces compared to nothing) to /dev/null
indicates that the approach used in this patch is faster regardless
of the optimization level of gcc.

Also this patch doesn't change output, which favors this approach over
the whitespace approach.

The only thing left to discuss, whether this patch is worth it, as it
only suppresses false positive warnings from gcc, but makes the
code slightly harder to read.

Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
---
 builtin/commit.c |  2 +-
 wt-status.c      | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 65cf2a7..34bc274 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -773,7 +773,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				committer_ident.buf);
 
 		if (ident_shown)
-			status_printf_ln(s, GIT_COLOR_NORMAL, "");
+			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
diff --git a/wt-status.c b/wt-status.c
index cb24f1f..912ed88 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -179,7 +179,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s)
 	} else {
 		status_printf_ln(s, c, _("  (use \"git add/rm <file>...\" as appropriate to mark resolution)"));
 	}
-	status_printf_ln(s, c, "");
+	status_printf_ln(s, c, "%s", "");
 }
 
 static void wt_status_print_cached_header(struct wt_status *s)
@@ -195,7 +195,7 @@ static void wt_status_print_cached_header(struct wt_status *s)
 		status_printf_ln(s, c, _("  (use \"git reset %s <file>...\" to unstage)"), s->reference);
 	else
 		status_printf_ln(s, c, _("  (use \"git rm --cached <file>...\" to unstage)"));
-	status_printf_ln(s, c, "");
+	status_printf_ln(s, c, "%s", "");
 }
 
 static void wt_status_print_dirty_header(struct wt_status *s,
@@ -214,7 +214,7 @@ static void wt_status_print_dirty_header(struct wt_status *s,
 	status_printf_ln(s, c, _("  (use \"git checkout -- <file>...\" to discard changes in working directory)"));
 	if (has_dirty_submodules)
 		status_printf_ln(s, c, _("  (commit or discard the untracked or modified content in submodules)"));
-	status_printf_ln(s, c, "");
+	status_printf_ln(s, c, "%s", "");
 }
 
 static void wt_status_print_other_header(struct wt_status *s,
@@ -226,12 +226,12 @@ static void wt_status_print_other_header(struct wt_status *s,
 	if (!advice_status_hints)
 		return;
 	status_printf_ln(s, c, _("  (use \"git %s <file>...\" to include in what will be committed)"), how);
-	status_printf_ln(s, c, "");
+	status_printf_ln(s, c, "%s", "");
 }
 
 static void wt_status_print_trailer(struct wt_status *s)
 {
-	status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
+	status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
 
 #define quote_path quote_path_relative
@@ -1192,7 +1192,7 @@ void wt_status_print(struct wt_status *s)
 				on_what = _("Not currently on any branch.");
 			}
 		}
-		status_printf(s, color(WT_STATUS_HEADER, s), "");
+		status_printf(s, color(WT_STATUS_HEADER, s), "%s", "");
 		status_printf_more(s, branch_status_color, "%s", on_what);
 		status_printf_more(s, branch_color, "%s\n", branch_name);
 		if (!s->is_initial)
@@ -1205,9 +1205,9 @@ void wt_status_print(struct wt_status *s)
 	free(state.detached_from);
 
 	if (s->is_initial) {
-		status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
+		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 		status_printf_ln(s, color(WT_STATUS_HEADER, s), _("Initial commit"));
-		status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
+		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 	}
 
 	wt_status_print_updated(s);
@@ -1224,7 +1224,7 @@ void wt_status_print(struct wt_status *s)
 		if (s->show_ignored_files)
 			wt_status_print_other(s, &s->ignored, _("Ignored files"), "add -f");
 		if (advice_status_u_option && 2000 < s->untracked_in_ms) {
-			status_printf_ln(s, GIT_COLOR_NORMAL, "");
+			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
 			status_printf_ln(s, GIT_COLOR_NORMAL,
 					 _("It took %.2f seconds to enumerate untracked files. 'status -uno'\n"
 					   "may speed it up, but you have to be careful not to forget to add\n"
-- 
1.8.3.3.754.g9c3c367.dirty

Re: [PATCH] status_printf_ln: Suppress false positive warnings of empty format string.

[Junio C Hamano]

Stefan Beller <stefanbeller@googlemail.com> writes:

> This is a response to 8dd0ee823f1829a3aa228c3c73e31de5c89b5317.
>
> Instead of having an empty string as format for the printf like function
> status_printf_ln, we could insert an empty string into the format
> parameter.
>
> A similar fixup commit is found in linux (2e4c332913b5), but there
> the empty string is replaced by a string containing one whitespace.
>
> To determine, which approach is better I setup 2 test programs, which
> either have a whitespace format (" ") or the empty string ("%s", ""),
> looking like:
> -
> 	#include <stdlib.h>
> 	#include <stdio.h>
> 	int main (int argc, char** argv) {
> 		long i;
> 		for (i = 0; i < 1024*1024*1024; ++i)
> 			printf(" ");
> 	}
> -
> Checking the required time of the programs, while redirecting the actual
> output (the billion white spaces compared to nothing) to /dev/null
> indicates that the approach used in this patch is faster regardless
> of the optimization level of gcc.
>
> Also this patch doesn't change output, which favors this approach over
> the whitespace approach.

Even if the " " variant is faster, it does not matter if its output
is incorrect ;-)

> The only thing left to discuss, whether this patch is worth it, as it
> only suppresses false positive warnings from gcc, but makes the
> code slightly harder to read.

I think we discussed this already?  The conclusion was it is silly
for GCC to warn on -Wformat-zero-length for user-defined function in
the first place, IIRC.  func(other, args, fmt,...), when invoked as
func(other, args, ""), may very well do something useful regardless
of the formatting part based on other args.

For that matter, I personally think -Wformat-zero-length warning for
standard ones, like

	printf("");

is silly, too.  It is not like

	printf("", extra, arg);

is not caught as an error.

So we can just add -Wno-format-zero-length after -Wall and be done
with it?

>
> Signed-off-by: Stefan Beller <stefanbeller@googlemail.com>
> ---
>  builtin/commit.c |  2 +-
>  wt-status.c      | 18 +++++++++---------
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 65cf2a7..34bc274 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -773,7 +773,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  				committer_ident.buf);
>  
>  		if (ident_shown)
> -			status_printf_ln(s, GIT_COLOR_NORMAL, "");
> +			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
>  
>  		saved_color_setting = s->use_color;
>  		s->use_color = 0;
> diff --git a/wt-status.c b/wt-status.c
> index cb24f1f..912ed88 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -179,7 +179,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s)
>  	} else {
>  		status_printf_ln(s, c, _("  (use \"git add/rm <file>...\" as appropriate to mark resolution)"));
>  	}
> -	status_printf_ln(s, c, "");
> +	status_printf_ln(s, c, "%s", "");
>  }
>  
>  static void wt_status_print_cached_header(struct wt_status *s)
> @@ -195,7 +195,7 @@ static void wt_status_print_cached_header(struct wt_status *s)
>  		status_printf_ln(s, c, _("  (use \"git reset %s <file>...\" to unstage)"), s->reference);
>  	else
>  		status_printf_ln(s, c, _("  (use \"git rm --cached <file>...\" to unstage)"));
> -	status_printf_ln(s, c, "");
> +	status_printf_ln(s, c, "%s", "");
>  }
>  
>  static void wt_status_print_dirty_header(struct wt_status *s,
> @@ -214,7 +214,7 @@ static void wt_status_print_dirty_header(struct wt_status *s,
>  	status_printf_ln(s, c, _("  (use \"git checkout -- <file>...\" to discard changes in working directory)"));
>  	if (has_dirty_submodules)
>  		status_printf_ln(s, c, _("  (commit or discard the untracked or modified content in submodules)"));
> -	status_printf_ln(s, c, "");
> +	status_printf_ln(s, c, "%s", "");
>  }
>  
>  static void wt_status_print_other_header(struct wt_status *s,
> @@ -226,12 +226,12 @@ static void wt_status_print_other_header(struct wt_status *s,
>  	if (!advice_status_hints)
>  		return;
>  	status_printf_ln(s, c, _("  (use \"git %s <file>...\" to include in what will be committed)"), how);
> -	status_printf_ln(s, c, "");
> +	status_printf_ln(s, c, "%s", "");
>  }
>  
>  static void wt_status_print_trailer(struct wt_status *s)
>  {
> -	status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
> +	status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
>  }
>  
>  #define quote_path quote_path_relative
> @@ -1192,7 +1192,7 @@ void wt_status_print(struct wt_status *s)
>  				on_what = _("Not currently on any branch.");
>  			}
>  		}
> -		status_printf(s, color(WT_STATUS_HEADER, s), "");
> +		status_printf(s, color(WT_STATUS_HEADER, s), "%s", "");
>  		status_printf_more(s, branch_status_color, "%s", on_what);
>  		status_printf_more(s, branch_color, "%s\n", branch_name);
>  		if (!s->is_initial)
> @@ -1205,9 +1205,9 @@ void wt_status_print(struct wt_status *s)
>  	free(state.detached_from);
>  
>  	if (s->is_initial) {
> -		status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
> +		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
>  		status_printf_ln(s, color(WT_STATUS_HEADER, s), _("Initial commit"));
> -		status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
> +		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
>  	}
>  
>  	wt_status_print_updated(s);
> @@ -1224,7 +1224,7 @@ void wt_status_print(struct wt_status *s)
>  		if (s->show_ignored_files)
>  			wt_status_print_other(s, &s->ignored, _("Ignored files"), "add -f");
>  		if (advice_status_u_option && 2000 < s->untracked_in_ms) {
> -			status_printf_ln(s, GIT_COLOR_NORMAL, "");
> +			status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
>  			status_printf_ln(s, GIT_COLOR_NORMAL,
>  					 _("It took %.2f seconds to enumerate untracked files. 'status -uno'\n"
>  					   "may speed it up, but you have to be careful not to forget to add\n"

Re: [PATCH] status_printf_ln: Suppress false positive warnings of empty format string.

[Stefan Beller]

[-- Attachment #1: Type: text/plain, Size: 595 bytes --]


> 
> Even if the " " variant is faster, it does not matter if its output
> is incorrect ;-)

Agreed.

> I think we discussed this already?  The conclusion was it is silly
> for GCC to warn on -Wformat-zero-length for user-defined function in
> the first place, IIRC.  func(other, args, fmt,...), when invoked as
> func(other, args, ""), may very well do something useful regardless
> of the formatting part based on other args.
> 

I am sorry, I did not pay attention enough to that specific topic. I
was just finding warnings on next and pu, so I wondered how to fix it.




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]