[PATCH] remote-http: use argv-array

[PATCH] remote-http: use argv-array

[Junio C Hamano]

Instead of using a hand-managed argument array, use argv-array API
to manage dynamically formulated command line.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 remote-curl.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/remote-curl.c b/remote-curl.c
index 60eda63..884b3a3 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -7,6 +7,7 @@
 #include "run-command.h"
 #include "pkt-line.h"
 #include "sideband.h"
+#include "argv-array.h"
 
 static struct remote *remote;
 static const char *url; /* always ends with a trailing slash */
@@ -787,36 +788,34 @@ static int push_dav(int nr_spec, char **specs)
 static int push_git(struct discovery *heads, int nr_spec, char **specs)
 {
 	struct rpc_state rpc;
-	const char **argv;
-	int argc = 0, i, err;
+	int i, err;
+	struct argv_array args;
+
+	argv_array_init(&args);
+	argv_array_pushl(&args, "send-pack", "--stateless-rpc", "--helper-status");
 
-	argv = xmalloc((10 + nr_spec) * sizeof(char*));
-	argv[argc++] = "send-pack";
-	argv[argc++] = "--stateless-rpc";
-	argv[argc++] = "--helper-status";
 	if (options.thin)
-		argv[argc++] = "--thin";
+		argv_array_push(&args, "--thin");
 	if (options.dry_run)
-		argv[argc++] = "--dry-run";
+		argv_array_push(&args, "--dry-run");
 	if (options.verbosity == 0)
-		argv[argc++] = "--quiet";
+		argv_array_push(&args, "--quiet");
 	else if (options.verbosity > 1)
-		argv[argc++] = "--verbose";
-	argv[argc++] = options.progress ? "--progress" : "--no-progress";
-	argv[argc++] = url;
+		argv_array_push(&args, "--verbose");
+	argv_array_push(&args, options.progress ? "--progress" : "--no-progress");
+	argv_array_push(&args, url);
 	for (i = 0; i < nr_spec; i++)
-		argv[argc++] = specs[i];
-	argv[argc++] = NULL;
+		argv_array_push(&args, specs[i]);
 
 	memset(&rpc, 0, sizeof(rpc));
 	rpc.service_name = "git-receive-pack",
-	rpc.argv = argv;
+	rpc.argv = args.argv;
 
 	err = rpc_service(&rpc, heads);
 	if (rpc.result.len)
 		write_or_die(1, rpc.result.buf, rpc.result.len);
 	strbuf_release(&rpc.result);
-	free(argv);
+	argv_array_clear(&args);
 	return err;
 }
 
-- 
1.8.3.2-876-ge3d3f5e

Re: [PATCH] remote-http: use argv-array

[Bert Wesarg]

On Tue, Jul 9, 2013 at 7:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Instead of using a hand-managed argument array, use argv-array API
> to manage dynamically formulated command line.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  remote-curl.c | 31 +++++++++++++++----------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/remote-curl.c b/remote-curl.c
> index 60eda63..884b3a3 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -7,6 +7,7 @@
>  #include "run-command.h"
>  #include "pkt-line.h"
>  #include "sideband.h"
> +#include "argv-array.h"
>
>  static struct remote *remote;
>  static const char *url; /* always ends with a trailing slash */
> @@ -787,36 +788,34 @@ static int push_dav(int nr_spec, char **specs)
>  static int push_git(struct discovery *heads, int nr_spec, char **specs)
>  {
>         struct rpc_state rpc;
> -       const char **argv;
> -       int argc = 0, i, err;
> +       int i, err;
> +       struct argv_array args;
> +
> +       argv_array_init(&args);
> +       argv_array_pushl(&args, "send-pack", "--stateless-rpc", "--helper-status");

missing NULL sentinel. GCC has the 'sentinel' [1] attribute to catch
such errors. Or use macro magic:

void argv_array_pushl_(struct argv_array *array, ...);
#define argv_array_pushl(array, ...) argv_array_pushl_(array, __VA_ARGS__, NULL)

Bert

[1] http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bsentinel_007d-function-attribute-2708

Re: [PATCH] remote-http: use argv-array

[Jeff King]

On Tue, Jul 09, 2013 at 08:05:19AM +0200, Bert Wesarg wrote:

> > +       argv_array_pushl(&args, "send-pack", "--stateless-rpc", "--helper-status");
> 
> missing NULL sentinel. GCC has the 'sentinel' [1] attribute to catch
> such errors. Or use macro magic:
> 
> void argv_array_pushl_(struct argv_array *array, ...);
> #define argv_array_pushl(array, ...) argv_array_pushl_(array, __VA_ARGS__, NULL)

Nice catch. We cannot use variadic macros, because we support pre-C99
compilers that do not have them. But the sentinel attribute is a good
idea. Here's a patch.

-- >8 --
Subject: [PATCH] argv-array: add sentinel attribute to argv_array_pushl

This attribute can help gcc notice when callers forget to add a
NULL sentinel to the end of the function. We shouldn't need
to #ifdef for other compilers, as __attribute__ is already a
no-op on non-gcc-compatible compilers.

Suggested-by: Bert Wesarg <bert.wesarg@googlemail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
This is our first use of an __attribute__ that is not "noreturn" or
"format". I assume this one should be supported on other gcc-compatible
compilers like clang.

 argv-array.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/argv-array.h b/argv-array.h
index 40248d4..e805748 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -15,6 +15,7 @@ void argv_array_pushf(struct argv_array *, const char *fmt, ...);
 void argv_array_push(struct argv_array *, const char *);
 __attribute__((format (printf,2,3)))
 void argv_array_pushf(struct argv_array *, const char *fmt, ...);
+__attribute__((sentinel))
 void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pop(struct argv_array *);
 void argv_array_clear(struct argv_array *);
-- 
1.8.3.rc3.24.gec82cb9

Re: [PATCH] remote-http: use argv-array

[Matt Kraai]

Jeff King <peff@peff.net> writes:
> On Tue, Jul 09, 2013 at 08:05:19AM +0200, Bert Wesarg wrote:
> > > +       argv_array_pushl(&args, "send-pack", "--stateless-rpc",
"--helper-status");
> > 
> > missing NULL sentinel. GCC has the 'sentinel' [1] attribute to catch
> > such errors. Or use macro magic:
> > 
> > void argv_array_pushl_(struct argv_array *array, ...);
> > #define argv_array_pushl(array, ...) argv_array_pushl_(array,
__VA_ARGS__, NULL)
> 
> Nice catch. We cannot use variadic macros, because we support pre-C99
> compilers that do not have them. But the sentinel attribute is a good
> idea. Here's a patch.

This attribute could also be used for
builtin/revert.c:verify_opt_compatible,
builtin/revert.c:verify_opt_mutually_compatible, exec_cmd.h:execl_git_cmd,
and run-command.h:run_hook.

-- 
Matt

Re: [PATCH] remote-http: use argv-array

[Jeff King]

On Tue, Jul 09, 2013 at 10:27:29PM +0000, Matt Kraai wrote:

> > Nice catch. We cannot use variadic macros, because we support pre-C99
> > compilers that do not have them. But the sentinel attribute is a good
> > idea. Here's a patch.
> 
> This attribute could also be used for
> builtin/revert.c:verify_opt_compatible,
> builtin/revert.c:verify_opt_mutually_compatible, exec_cmd.h:execl_git_cmd,
> and run-command.h:run_hook.

Thanks. I did a full grep of '\.\.\.' on the source, and found that we
have missed some cases for the "format" attribute, too.

This series fixes all of them in the main code base (not compat/ or
contrib/). But see the comments in patch 3, as I'm not sure that case is
worth doing.

  [1/3]: add missing "format" function attributes
  [2/3]: use "sentinel" function attribute for variadic lists
  [3/3]: wt-status: use "format" function attribute for status_printf

-Peff

[PATCH 1/3] add missing "format" function attributes

[Jeff King]

For most of our functions that take printf-like formats, we
use gcc's __attribute__((format)) to get compiler warnings
when the functions are misused. Let's give a few more
functions the same protection.

In most cases, the annotations do not uncover any actual
bugs; the only code change needed is that we passed a size_t
to transfer_debug, which expected an int. Since we expect
the passed-in value to be a relatively small buffer size
(and cast a similar value to int directly below), we can
just cast away the problem.

Signed-off-by: Jeff King <peff@peff.net>
---
 advice.h           | 1 +
 trace.c            | 1 +
 transport-helper.c | 3 ++-
 utf8.h             | 1 +
 4 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/advice.h b/advice.h
index 93a7d11..08fbc8e 100644
--- a/advice.h
+++ b/advice.h
@@ -21,6 +21,7 @@ int git_default_advice_config(const char *var, const char *value);
 extern int advice_rm_hints;
 
 int git_default_advice_config(const char *var, const char *value);
+__attribute__((format (printf, 1, 2)))
 void advise(const char *advice, ...);
 int error_resolve_conflict(const char *me);
 extern void NORETURN die_resolve_conflict(const char *me);
diff --git a/trace.c b/trace.c
index 5ec0e3b..3d744d1 100644
--- a/trace.c
+++ b/trace.c
@@ -75,6 +75,7 @@ static void trace_vprintf(const char *key, const char *fmt, va_list ap)
 	strbuf_release(&buf);
 }
 
+__attribute__((format (printf, 2, 3)))
 static void trace_printf_key(const char *key, const char *fmt, ...)
 {
 	va_list ap;
diff --git a/transport-helper.c b/transport-helper.c
index db9bd18..45a35df 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -982,6 +982,7 @@ int transport_helper_init(struct transport *transport, const char *name)
 #define PBUFFERSIZE 8192
 
 /* Print bidirectional transfer loop debug message. */
+__attribute__((format (printf, 1, 2)))
 static void transfer_debug(const char *fmt, ...)
 {
 	va_list args;
@@ -1067,7 +1068,7 @@ static int udt_do_read(struct unidirectional_transfer *t)
 		return -1;
 	} else if (bytes == 0) {
 		transfer_debug("%s EOF (with %i bytes in buffer)",
-			t->src_name, t->bufuse);
+			t->src_name, (int)t->bufuse);
 		t->state = SSTATE_FLUSHING;
 	} else if (bytes > 0) {
 		t->bufuse += bytes;
diff --git a/utf8.h b/utf8.h
index 32a7bfb..65d0e42 100644
--- a/utf8.h
+++ b/utf8.h
@@ -10,6 +10,7 @@ int same_encoding(const char *, const char *);
 int is_utf8(const char *text);
 int is_encoding_utf8(const char *name);
 int same_encoding(const char *, const char *);
+__attribute__((format (printf, 2, 3)))
 int utf8_fprintf(FILE *, const char *, ...);
 
 void strbuf_add_wrapped_text(struct strbuf *buf,
-- 
1.8.3.rc3.24.gec82cb9

[PATCH 2/3] use "sentinel" function attribute for variadic lists

[Jeff King]

This attribute can help gcc notice when callers forget to
add a NULL sentinel to the end of the function. This is our
first use of the sentinel attribute, but we shouldn't need
to #ifdef for other compilers, as __attribute__ is already a
no-op on non-gcc-compatible compilers.

Suggested-by: Bert Wesarg <bert.wesarg@googlemail.com>
More-Spots-Found-By: Matt Kraai <kraai@ftbfs.org>
Signed-off-by: Jeff King <peff@peff.net>
---
 argv-array.h     | 1 +
 builtin/revert.c | 2 ++
 exec_cmd.h       | 1 +
 run-command.h    | 1 +
 4 files changed, 5 insertions(+)

diff --git a/argv-array.h b/argv-array.h
index 40248d4..e805748 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -15,6 +15,7 @@ void argv_array_pushf(struct argv_array *, const char *fmt, ...);
 void argv_array_push(struct argv_array *, const char *);
 __attribute__((format (printf,2,3)))
 void argv_array_pushf(struct argv_array *, const char *fmt, ...);
+__attribute__((sentinel))
 void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pop(struct argv_array *);
 void argv_array_clear(struct argv_array *);
diff --git a/builtin/revert.c b/builtin/revert.c
index 0401fdb..b8b5174 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -54,6 +54,7 @@ static int option_parse_x(const struct option *opt,
 	return 0;
 }
 
+__attribute__((sentinel))
 static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 {
 	const char *this_opt;
@@ -70,6 +71,7 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)
 		die(_("%s: %s cannot be used with %s"), me, this_opt, base_opt);
 }
 
+__attribute__((sentinel))
 static void verify_opt_mutually_compatible(const char *me, ...)
 {
 	const char *opt1, *opt2 = NULL;
diff --git a/exec_cmd.h b/exec_cmd.h
index e2b546b..307b55c 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -7,6 +7,7 @@ extern int execv_git_cmd(const char **argv); /* NULL terminated */
 extern void setup_path(void);
 extern const char **prepare_git_cmd(const char **argv);
 extern int execv_git_cmd(const char **argv); /* NULL terminated */
+__attribute__((sentinel))
 extern int execl_git_cmd(const char *cmd, ...);
 extern const char *system_path(const char *path);
 
diff --git a/run-command.h b/run-command.h
index 221ce33..0a47679 100644
--- a/run-command.h
+++ b/run-command.h
@@ -46,6 +46,7 @@ extern char *find_hook(const char *name);
 int run_command(struct child_process *);
 
 extern char *find_hook(const char *name);
+__attribute__((sentinel))
 extern int run_hook(const char *index_file, const char *name, ...);
 
 #define RUN_COMMAND_NO_STDIN 1
-- 
1.8.3.rc3.24.gec82cb9

[PATCH 3/3] wt-status: use "format" function attribute for status_printf

[Jeff King]

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 <peff@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.

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 */
-- 
1.8.3.rc3.24.gec82cb9

Re: [PATCH 3/3] wt-status: use "format" function attribute for status_printf

[Junio C Hamano]

Jeff King <peff@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 <peff@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 */

Re: [PATCH 3/3] wt-status: use "format" function attribute for status_printf

[Jeff King]

On Tue, Jul 09, 2013 at 10:26:04PM -0700, Junio C Hamano wrote:

> Jeff King <peff@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 <peff@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

Re: [PATCH 3/3] wt-status: use "format" function attribute for status_printf

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jul 09, 2013 at 10:26:04PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@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 <peff@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

Re: [PATCH 3/3] wt-status: use "format" function attribute for status_printf

[Jeff King]

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"

Re: [PATCH 3/3] wt-status: use "format" function attribute for status_printf

[Junio C Hamano]

Jeff King <peff@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", "");

Re: [PATCH 3/3] wt-status: use "format" function attribute for status_printf

[Jeff King]

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

Re: [PATCH 3/3] wt-status: use "format" function attribute for status_printf

[Junio C Hamano]

Jeff King <peff@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.

Re: [PATCH 3/3] wt-status: use "format" function attribute for status_printf

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jul 09, 2013 at 10:26:04PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@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.

Re: [PATCH 3/3] wt-status: use "format" function attribute for status_printf

[Jeff King]

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