Re: [PATCH 3/3] wt-status: use format function attribute for status_printf
Jeff King p...@peff.net writes: On Tue, Jul 09, 2013 at 10:26:04PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: ... I'm torn on this one. It really does provide us with more compile-time safety checks, but it's annoying that -Wall -Werror will no longer work out of the box. Yeah, that is a show-stopper for me X-. You can fix it with -Wno-zero-format-length, so the hassle is not huge. But I am also inclined to just drop this one. We have lived without the extra safety for a long time, and list review does tend to catch such problems in practice. I am tempted to actually merge the original one as-is without any of the workaround, and just tell people to use -Wno-format-zero-length. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] wt-status: use format function attribute for status_printf
On Fri, Jul 12, 2013 at 09:10:30AM -0700, Junio C Hamano wrote: You can fix it with -Wno-zero-format-length, so the hassle is not huge. But I am also inclined to just drop this one. We have lived without the extra safety for a long time, and list review does tend to catch such problems in practice. I am tempted to actually merge the original one as-is without any of the workaround, and just tell people to use -Wno-format-zero-length. Yeah, I think the only downside is the cognitive burden on individual developers who try -Wall and have to figure out that we need -Wno-zero-format-length (and that the warnings are not interesting). It would be nice to add it automatically to CFLAGS, but I do not know if we can reliably detect from the Makefile that we are compiling under gcc. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] wt-status: use format function attribute for status_printf
On Tue, Jul 09, 2013 at 10:52:55PM -0700, Junio C Hamano wrote:
The patch to do it is below, but I actually think an explicit blank-line
function like:
status_print_empty_line(s, color);
would be more obvious to a reader.
I noticed that all but one can be dealt with with
perl -p -i -e 's/status_printf_ln\((.*), \);/status_printf($1, \\n);/'
That is,
- status_printf_ln(s, GIT_COLOR_NORMAL, );
+ status_printf(s, GIT_COLOR_NORMAL, \n);
which does not look _too_ bad.
Is that correct, though? The reason we have *_printf_ln in the
first place is that we want to do:
${color}content${reset}\n
to make sure that the newline does not happen inside the colorization.
At least that is why I added color_printf_ln long ago.
It would probably improve the code quite a bit if we could simply feed
multi-line strings to status_printf, and have it stick the colorization
in the correct spot of each line. And hmm...it kind of looks like
status_vprintf already does that. Now I'm puzzled why many of these do
not simply include the newline along with the string being printed.
There is one instance that needs this, though.
- status_printf(s, color(WT_STATUS_HEADER, s), );
+ status_printf(s, color(WT_STATUS_HEADER, s), %s, );
Hmm, yeah. It cannot be combined with the lines following it, either,
because they are using different colorization.
If you want to keep refactoring this, I don't mind, but I kind of feel
like we are going down a rabbit hole for very little gain.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] wt-status: use format function attribute for status_printf
Jeff King p...@peff.net writes: If you want to keep refactoring this, I don't mind, but I kind of feel like we are going down a rabbit hole for very little gain. Right, and right. The rewrite to move _ln to \n was wrong, and this is too much hassle for too little gain. If we were to do something, I agree that it would be the best to have dedicated empty-output function. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] wt-status: use format function attribute for status_printf
Jeff King p...@peff.net writes: These functions could benefit from the added compile-time safety of having the compiler check printf arguments. Unfortunately, we also sometimes pass an empty format string, which will cause false positives with -Wformat-zero-length. In this case, that warning is wrong because our function is not a no-op with an empty format: it may be printing colorized output along with a trailing newline. Signed-off-by: Jeff King p...@peff.net --- I'm torn on this one. It really does provide us with more compile-time safety checks, but it's annoying that -Wall -Werror will no longer work out of the box. Yeah, that is a show-stopper for me X-. We could also add a status_printf_empty_line() function, but that feels like a bit of a hack. Searching online, I also found the amusing suggestion to do: status_printf_ln(s, color, %.*s, 0, -Wformat-zero-length please choke on a bucket of socks); but I think that is probably worse than adding a specialized function. wt-status.h | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/wt-status.h b/wt-status.h index 4121bc2..fb7152e 100644 --- a/wt-status.h +++ b/wt-status.h @@ -96,9 +96,9 @@ void wt_porcelain_print(struct wt_status *s); void wt_shortstatus_print(struct wt_status *s); void wt_porcelain_print(struct wt_status *s); -void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...) - ; -void status_printf(struct wt_status *s, const char *color, const char *fmt, ...) - ; +__attribute__((format (printf, 3, 4))) +void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...); +__attribute__((format (printf, 3, 4))) +void status_printf(struct wt_status *s, const char *color, const char *fmt, ...); #endif /* STATUS_H */ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] wt-status: use format function attribute for status_printf
On Tue, Jul 09, 2013 at 10:26:04PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: These functions could benefit from the added compile-time safety of having the compiler check printf arguments. Unfortunately, we also sometimes pass an empty format string, which will cause false positives with -Wformat-zero-length. In this case, that warning is wrong because our function is not a no-op with an empty format: it may be printing colorized output along with a trailing newline. Signed-off-by: Jeff King p...@peff.net --- I'm torn on this one. It really does provide us with more compile-time safety checks, but it's annoying that -Wall -Werror will no longer work out of the box. Yeah, that is a show-stopper for me X-. You can fix it with -Wno-zero-format-length, so the hassle is not huge. But I am also inclined to just drop this one. We have lived without the extra safety for a long time, and list review does tend to catch such problems in practice. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] wt-status: use format function attribute for status_printf
Jeff King p...@peff.net writes: On Tue, Jul 09, 2013 at 10:26:04PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: These functions could benefit from the added compile-time safety of having the compiler check printf arguments. Unfortunately, we also sometimes pass an empty format string, which will cause false positives with -Wformat-zero-length. In this case, that warning is wrong because our function is not a no-op with an empty format: it may be printing colorized output along with a trailing newline. Signed-off-by: Jeff King p...@peff.net --- I'm torn on this one. It really does provide us with more compile-time safety checks, but it's annoying that -Wall -Werror will no longer work out of the box. Yeah, that is a show-stopper for me X-. You can fix it with -Wno-zero-format-length, so the hassle is not huge. Yes, or just do func(..., %s, ) perhaps? That also sound iffy. But I am also inclined to just drop this one. We have lived without the extra safety for a long time, and list review does tend to catch such problems in practice. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] wt-status: use format function attribute for status_printf
On Tue, Jul 09, 2013 at 10:35:25PM -0700, Junio C Hamano wrote:
You can fix it with -Wno-zero-format-length, so the hassle is not
huge.
Yes, or just do func(..., %s, ) perhaps? That also sound iffy.
I imagine that is the method intended by upstream (though who
knows...the whole warning seems kind of stupid to me; it is clear that
printf() is a no-op, but it is obviously not clear that arbitrary
functions using __attribute__(format) are).
The patch to do it is below, but I actually think an explicit blank-line
function like:
status_print_empty_line(s, color);
would be more obvious to a reader.
---
diff --git a/builtin/commit.c b/builtin/commit.c
index 6b693c1..1fe81bc 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -768,7 +768,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 b191c65..5876032 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -178,7 +178,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)
@@ -194,7 +194,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,
@@ -213,7 +213,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,
@@ -225,12 +225,12 @@ static void wt_status_print_trailer(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
@@ -1191,7 +1191,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)
@@ -1204,9 +1204,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);
@@ -1223,7 +1223,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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] wt-status: use format function attribute for status_printf
Jeff King p...@peff.net writes: On Tue, Jul 09, 2013 at 10:35:25PM -0700, Junio C Hamano wrote: You can fix it with -Wno-zero-format-length, so the hassle is not huge. Yes, or just do func(..., %s, ) perhaps? That also sound iffy. I imagine that is the method intended by upstream (though who knows...the whole warning seems kind of stupid to me; it is clear that printf() is a no-op, but it is obviously not clear that arbitrary functions using __attribute__(format) are). The patch to do it is below, but I actually think an explicit blank-line function like: status_print_empty_line(s, color); would be more obvious to a reader. I noticed that all but one can be dealt with with perl -p -i -e 's/status_printf_ln\((.*), \);/status_printf($1, \\n);/' That is, - status_printf_ln(s, GIT_COLOR_NORMAL, ); + status_printf(s, GIT_COLOR_NORMAL, \n); which does not look _too_ bad. There is one instance that needs this, though. - status_printf(s, color(WT_STATUS_HEADER, s), ); + status_printf(s, color(WT_STATUS_HEADER, s), %s, ); -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
