{"thread":{"id":"34385","subject":"[PATCH] remote-http: use argv-array","startedAt":"2013-07-09T05:18:35Z","lastAt":"2013-07-12T20:44:37Z","messageCount":17,"participants":["Junio C Hamano","Bert Wesarg","Jeff King","Matt Kraai"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"222880","messageId":"7vfvvoxqdw.fsf@alter.siamese.dyndns.org","threadId":"34385","inReplyTo":null,"subject":"[PATCH] remote-http: use argv-array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-09T05:18:35Z","receivedAt":"2013-07-09T05:18:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Instead of using a hand-managed argument array, use argv-array API\nto manage dynamically formulated command line.\n\nSigned-off-by: Junio C Hamano \n---\n remote-curl.c | 31 +++++++++++++++----------------\n 1 file changed, 15 insertions(+), 16 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 60eda63..884b3a3 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -7,6 +7,7 @@\n #include \"run-command.h\"\n #include \"pkt-line.h\"\n #include \"sideband.h\"\n+#include \"argv-array.h\"\n \n static struct remote *remote;\n static const char *url; /* always ends with a trailing slash */\n@@ -787,36 +788,34 @@ static int push_dav(int nr_spec, char **specs)\n static int push_git(struct discovery *heads, int nr_spec, char **specs)\n {\n \tstruct rpc_state rpc;\n-\tconst char **argv;\n-\tint argc = 0, i, err;\n+\tint i, err;\n+\tstruct argv_array args;\n+\n+\targv_array_init(&args);\n+\targv_array_pushl(&args, \"send-pack\", \"--stateless-rpc\", \"--helper-status\");\n \n-\targv = xmalloc((10 + nr_spec) * sizeof(char*));\n-\targv[argc++] = \"send-pack\";\n-\targv[argc++] = \"--stateless-rpc\";\n-\targv[argc++] = \"--helper-status\";\n \tif (options.thin)\n-\t\targv[argc++] = \"--thin\";\n+\t\targv_array_push(&args, \"--thin\");\n \tif (options.dry_run)\n-\t\targv[argc++] = \"--dry-run\";\n+\t\targv_array_push(&args, \"--dry-run\");\n \tif (options.verbosity == 0)\n-\t\targv[argc++] = \"--quiet\";\n+\t\targv_array_push(&args, \"--quiet\");\n \telse if (options.verbosity > 1)\n-\t\targv[argc++] = \"--verbose\";\n-\targv[argc++] = options.progress ? \"--progress\" : \"--no-progress\";\n-\targv[argc++] = url;\n+\t\targv_array_push(&args, \"--verbose\");\n+\targv_array_push(&args, options.progress ? \"--progress\" : \"--no-progress\");\n+\targv_array_push(&args, url);\n \tfor (i = 0; i < nr_spec; i++)\n-\t\targv[argc++] = specs[i];\n-\targv[argc++] = NULL;\n+\t\targv_array_push(&args, specs[i]);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n \trpc.service_name = \"git-receive-pack\",\n-\trpc.argv = argv;\n+\trpc.argv = args.argv;\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n \t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n \tstrbuf_release(&rpc.result);\n-\tfree(argv);\n+\targv_array_clear(&args);\n \treturn err;\n }\n \n-- \n1.8.3.2-876-ge3d3f5e\n"},{"id":"222887","messageId":"CAKPyHN0DG0c2vxWtybYtDmFKMo369PZcbqCfDJaXeiRV+PP8pQ@mail.gmail.com","threadId":"34385","inReplyTo":"7vfvvoxqdw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] remote-http: use argv-array","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2013-07-09T06:05:19Z","receivedAt":"2013-07-09T06:05:19Z","isPatch":true,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"On Tue, Jul 9, 2013 at 7:18 AM, Junio C Hamano wrote:\n> Instead of using a hand-managed argument array, use argv-array API\n> to manage dynamically formulated command line.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> remote-curl.c | 31 +++++++++++++++----------------\n> 1 file changed, 15 insertions(+), 16 deletions(-)\n>\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 60eda63..884b3a3 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -7,6 +7,7 @@\n> #include \"run-command.h\"\n> #include \"pkt-line.h\"\n> #include \"sideband.h\"\n> +#include \"argv-array.h\"\n>\n> static struct remote *remote;\n> static const char *url; /* always ends with a trailing slash */\n> @@ -787,36 +788,34 @@ static int push_dav(int nr_spec, char **specs)\n> static int push_git(struct discovery *heads, int nr_spec, char **specs)\n> {\n> struct rpc_state rpc;\n> - const char **argv;\n> - int argc = 0, i, err;\n> + int i, err;\n> + struct argv_array args;\n> +\n> + argv_array_init(&args);\n> + argv_array_pushl(&args, \"send-pack\", \"--stateless-rpc\", \"--helper-status\");\n\nmissing NULL sentinel. GCC has the 'sentinel' [1] attribute to catch\nsuch errors. Or use macro magic:\n\nvoid argv_array_pushl_(struct argv_array *array, ...);\n#define argv_array_pushl(array, ...) argv_array_pushl_(array, __VA_ARGS__, NULL)\n\nBert\n\n[1] http://gcc.gnu.org/onlinedocs/gcc/Function-Attributes.html#index-g_t_0040code_007bsentinel_007d-function-attribute-2708\n"},{"id":"222888","messageId":"20130709063840.GA8015@sigill.intra.peff.net","threadId":"34385","inReplyTo":"CAKPyHN0DG0c2vxWtybYtDmFKMo369PZcbqCfDJaXeiRV+PP8pQ@mail.gmail.com","subject":"Re: [PATCH] remote-http: use argv-array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-09T06:38:40Z","receivedAt":"2013-07-09T06:38:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 09, 2013 at 08:05:19AM +0200, Bert Wesarg wrote:\n\n> > + argv_array_pushl(&args, \"send-pack\", \"--stateless-rpc\", \"--helper-status\");\n> \n> missing NULL sentinel. GCC has the 'sentinel' [1] attribute to catch\n> such errors. Or use macro magic:\n> \n> void argv_array_pushl_(struct argv_array *array, ...);\n> #define argv_array_pushl(array, ...) argv_array_pushl_(array, __VA_ARGS__, NULL)\n\nNice catch. We cannot use variadic macros, because we support pre-C99\ncompilers that do not have them. But the sentinel attribute is a good\nidea. Here's a patch.\n\n-- >8 --\nSubject: [PATCH] argv-array: add sentinel attribute to argv_array_pushl\n\nThis attribute can help gcc notice when callers forget to add a\nNULL sentinel to the end of the function. We shouldn't need\nto #ifdef for other compilers, as __attribute__ is already a\nno-op on non-gcc-compatible compilers.\n\nSuggested-by: Bert Wesarg \nSigned-off-by: Jeff King \n---\nThis is our first use of an __attribute__ that is not \"noreturn\" or\n\"format\". I assume this one should be supported on other gcc-compatible\ncompilers like clang.\n\n argv-array.h | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/argv-array.h b/argv-array.h\nindex 40248d4..e805748 100644\n--- a/argv-array.h\n+++ b/argv-array.h\n@@ -15,6 +15,7 @@ void argv_array_pushf(struct argv_array *, const char *fmt, ...);\n void argv_array_push(struct argv_array *, const char *);\n __attribute__((format (printf,2,3)))\n void argv_array_pushf(struct argv_array *, const char *fmt, ...);\n+__attribute__((sentinel))\n void argv_array_pushl(struct argv_array *, ...);\n void argv_array_pop(struct argv_array *);\n void argv_array_clear(struct argv_array *);\n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"222980","messageId":"loom.20130710T002441-147@post.gmane.org","threadId":"34385","inReplyTo":"20130709063840.GA8015@sigill.intra.peff.net","subject":"Re: [PATCH] remote-http: use argv-array","fromName":"Matt Kraai","fromEmail":"kraai@ftbfs.org","sentAt":"2013-07-09T22:27:29Z","receivedAt":"2013-07-09T22:27:29Z","isPatch":true,"sender":{"key":"kraai@ftbfs.org","avatar":null},"body":"Jeff King writes:\n> On Tue, Jul 09, 2013 at 08:05:19AM +0200, Bert Wesarg wrote:\n> > > + argv_array_pushl(&args, \"send-pack\", \"--stateless-rpc\",\n\"--helper-status\");\n> > \n> > missing NULL sentinel. GCC has the 'sentinel' [1] attribute to catch\n> > such errors. Or use macro magic:\n> > \n> > void argv_array_pushl_(struct argv_array *array, ...);\n> > #define argv_array_pushl(array, ...) argv_array_pushl_(array,\n__VA_ARGS__, NULL)\n> \n> Nice catch. We cannot use variadic macros, because we support pre-C99\n> compilers that do not have them. But the sentinel attribute is a good\n> idea. Here's a patch.\n\nThis attribute could also be used for\nbuiltin/revert.c:verify_opt_compatible,\nbuiltin/revert.c:verify_opt_mutually_compatible, exec_cmd.h:execl_git_cmd,\nand run-command.h:run_hook.\n\n-- \nMatt\n"},{"id":"222984","messageId":"20130710001659.GA11643@sigill.intra.peff.net","threadId":"34385","inReplyTo":"loom.20130710T002441-147@post.gmane.org","subject":"Re: [PATCH] remote-http: use argv-array","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-10T00:16:59Z","receivedAt":"2013-07-10T00:16:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 09, 2013 at 10:27:29PM +0000, Matt Kraai wrote:\n\n> > Nice catch. We cannot use variadic macros, because we support pre-C99\n> > compilers that do not have them. But the sentinel attribute is a good\n> > idea. Here's a patch.\n> \n> This attribute could also be used for\n> builtin/revert.c:verify_opt_compatible,\n> builtin/revert.c:verify_opt_mutually_compatible, exec_cmd.h:execl_git_cmd,\n> and run-command.h:run_hook.\n\nThanks. I did a full grep of '\\.\\.\\.' on the source, and found that we\nhave missed some cases for the \"format\" attribute, too.\n\nThis series fixes all of them in the main code base (not compat/ or\ncontrib/). But see the comments in patch 3, as I'm not sure that case is\nworth doing.\n\n [1/3]: add missing \"format\" function attributes\n [2/3]: use \"sentinel\" function attribute for variadic lists\n [3/3]: wt-status: use \"format\" function attribute for status_printf\n\n-Peff\n"},{"id":"222985","messageId":"20130710001840.GA19423@sigill.intra.peff.net","threadId":"34385","inReplyTo":"20130710001659.GA11643@sigill.intra.peff.net","subject":"[PATCH 1/3] add missing \"format\" function attributes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-10T00:18:40Z","receivedAt":"2013-07-10T00:18:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For most of our functions that take printf-like formats, we\nuse gcc's __attribute__((format)) to get compiler warnings\nwhen the functions are misused. Let's give a few more\nfunctions the same protection.\n\nIn most cases, the annotations do not uncover any actual\nbugs; the only code change needed is that we passed a size_t\nto transfer_debug, which expected an int. Since we expect\nthe passed-in value to be a relatively small buffer size\n(and cast a similar value to int directly below), we can\njust cast away the problem.\n\nSigned-off-by: Jeff King \n---\n advice.h | 1 +\n trace.c | 1 +\n transport-helper.c | 3 ++-\n utf8.h | 1 +\n 4 files changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/advice.h b/advice.h\nindex 93a7d11..08fbc8e 100644\n--- a/advice.h\n+++ b/advice.h\n@@ -21,6 +21,7 @@ int git_default_advice_config(const char *var, const char *value);\n extern int advice_rm_hints;\n \n int git_default_advice_config(const char *var, const char *value);\n+__attribute__((format (printf, 1, 2)))\n void advise(const char *advice, ...);\n int error_resolve_conflict(const char *me);\n extern void NORETURN die_resolve_conflict(const char *me);\ndiff --git a/trace.c b/trace.c\nindex 5ec0e3b..3d744d1 100644\n--- a/trace.c\n+++ b/trace.c\n@@ -75,6 +75,7 @@ static void trace_vprintf(const char *key, const char *fmt, va_list ap)\n \tstrbuf_release(&buf);\n }\n \n+__attribute__((format (printf, 2, 3)))\n static void trace_printf_key(const char *key, const char *fmt, ...)\n {\n \tva_list ap;\ndiff --git a/transport-helper.c b/transport-helper.c\nindex db9bd18..45a35df 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -982,6 +982,7 @@ int transport_helper_init(struct transport *transport, const char *name)\n #define PBUFFERSIZE 8192\n \n /* Print bidirectional transfer loop debug message. */\n+__attribute__((format (printf, 1, 2)))\n static void transfer_debug(const char *fmt, ...)\n {\n \tva_list args;\n@@ -1067,7 +1068,7 @@ static int udt_do_read(struct unidirectional_transfer *t)\n \t\treturn -1;\n \t} else if (bytes == 0) {\n \t\ttransfer_debug(\"%s EOF (with %i bytes in buffer)\",\n-\t\t\tt->src_name, t->bufuse);\n+\t\t\tt->src_name, (int)t->bufuse);\n \t\tt->state = SSTATE_FLUSHING;\n \t} else if (bytes > 0) {\n \t\tt->bufuse += bytes;\ndiff --git a/utf8.h b/utf8.h\nindex 32a7bfb..65d0e42 100644\n--- a/utf8.h\n+++ b/utf8.h\n@@ -10,6 +10,7 @@ int same_encoding(const char *, const char *);\n int is_utf8(const char *text);\n int is_encoding_utf8(const char *name);\n int same_encoding(const char *, const char *);\n+__attribute__((format (printf, 2, 3)))\n int utf8_fprintf(FILE *, const char *, ...);\n \n void strbuf_add_wrapped_text(struct strbuf *buf,\n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"222986","messageId":"20130710001911.GB19423@sigill.intra.peff.net","threadId":"34385","inReplyTo":"20130710001659.GA11643@sigill.intra.peff.net","subject":"[PATCH 2/3] use \"sentinel\" function attribute for variadic lists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-10T00:19:12Z","receivedAt":"2013-07-10T00:19:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This attribute can help gcc notice when callers forget to\nadd a NULL sentinel to the end of the function. This is our\nfirst use of the sentinel attribute, but we shouldn't need\nto #ifdef for other compilers, as __attribute__ is already a\nno-op on non-gcc-compatible compilers.\n\nSuggested-by: Bert Wesarg \nMore-Spots-Found-By: Matt Kraai \nSigned-off-by: Jeff King \n---\n argv-array.h | 1 +\n builtin/revert.c | 2 ++\n exec_cmd.h | 1 +\n run-command.h | 1 +\n 4 files changed, 5 insertions(+)\n\ndiff --git a/argv-array.h b/argv-array.h\nindex 40248d4..e805748 100644\n--- a/argv-array.h\n+++ b/argv-array.h\n@@ -15,6 +15,7 @@ void argv_array_pushf(struct argv_array *, const char *fmt, ...);\n void argv_array_push(struct argv_array *, const char *);\n __attribute__((format (printf,2,3)))\n void argv_array_pushf(struct argv_array *, const char *fmt, ...);\n+__attribute__((sentinel))\n void argv_array_pushl(struct argv_array *, ...);\n void argv_array_pop(struct argv_array *);\n void argv_array_clear(struct argv_array *);\ndiff --git a/builtin/revert.c b/builtin/revert.c\nindex 0401fdb..b8b5174 100644\n--- a/builtin/revert.c\n+++ b/builtin/revert.c\n@@ -54,6 +54,7 @@ static int option_parse_x(const struct option *opt,\n \treturn 0;\n }\n \n+__attribute__((sentinel))\n static void verify_opt_compatible(const char *me, const char *base_opt, ...)\n {\n \tconst char *this_opt;\n@@ -70,6 +71,7 @@ static void verify_opt_compatible(const char *me, const char *base_opt, ...)\n \t\tdie(_(\"%s: %s cannot be used with %s\"), me, this_opt, base_opt);\n }\n \n+__attribute__((sentinel))\n static void verify_opt_mutually_compatible(const char *me, ...)\n {\n \tconst char *opt1, *opt2 = NULL;\ndiff --git a/exec_cmd.h b/exec_cmd.h\nindex e2b546b..307b55c 100644\n--- a/exec_cmd.h\n+++ b/exec_cmd.h\n@@ -7,6 +7,7 @@ extern int execv_git_cmd(const char **argv); /* NULL terminated */\n extern void setup_path(void);\n extern const char **prepare_git_cmd(const char **argv);\n extern int execv_git_cmd(const char **argv); /* NULL terminated */\n+__attribute__((sentinel))\n extern int execl_git_cmd(const char *cmd, ...);\n extern const char *system_path(const char *path);\n \ndiff --git a/run-command.h b/run-command.h\nindex 221ce33..0a47679 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -46,6 +46,7 @@ extern char *find_hook(const char *name);\n int run_command(struct child_process *);\n \n extern char *find_hook(const char *name);\n+__attribute__((sentinel))\n extern int run_hook(const char *index_file, const char *name, ...);\n \n #define RUN_COMMAND_NO_STDIN 1\n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"222987","messageId":"20130710002328.GC19423@sigill.intra.peff.net","threadId":"34385","inReplyTo":"20130710001659.GA11643@sigill.intra.peff.net","subject":"[PATCH 3/3] wt-status: use \"format\" function attribute for status_printf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-10T00:23:28Z","receivedAt":"2013-07-10T00:23:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These functions could benefit from the added compile-time\nsafety of having the compiler check printf arguments.\n\nUnfortunately, we also sometimes pass an empty format string,\nwhich will cause false positives with -Wformat-zero-length.\nIn this case, that warning is wrong because our function is\nnot a no-op with an empty format: it may be printing\ncolorized output along with a trailing newline.\n\nSigned-off-by: Jeff King \n---\nI'm torn on this one. It really does provide us with more compile-time\nsafety checks, but it's annoying that \"-Wall -Werror\" will no longer\nwork out of the box.\n\nWe could also add a status_printf_empty_line() function, but that feels\nlike a bit of a hack. Searching online, I also found the amusing\nsuggestion to do:\n\n status_printf_ln(s, color, \"%.*s\", 0,\n \"-Wformat-zero-length please choke on a bucket of socks\");\n\nbut I think that is probably worse than adding a specialized function.\n\n wt-status.h | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/wt-status.h b/wt-status.h\nindex 4121bc2..fb7152e 100644\n--- a/wt-status.h\n+++ b/wt-status.h\n@@ -96,9 +96,9 @@ void wt_porcelain_print(struct wt_status *s);\n void wt_shortstatus_print(struct wt_status *s);\n void wt_porcelain_print(struct wt_status *s);\n \n-void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...)\n-\t;\n-void status_printf(struct wt_status *s, const char *color, const char *fmt, ...)\n-\t;\n+__attribute__((format (printf, 3, 4)))\n+void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...);\n+__attribute__((format (printf, 3, 4)))\n+void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);\n \n #endif /* STATUS_H */\n-- \n1.8.3.rc3.24.gec82cb9\n"},{"id":"222990","messageId":"7vmwpvt28j.fsf@alter.siamese.dyndns.org","threadId":"34385","inReplyTo":"20130710002328.GC19423@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] wt-status: use \"format\" function attribute for status_printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-10T05:26:04Z","receivedAt":"2013-07-10T05:26:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> These functions could benefit from the added compile-time\n> safety of having the compiler check printf arguments.\n>\n> Unfortunately, we also sometimes pass an empty format string,\n> which will cause false positives with -Wformat-zero-length.\n> In this case, that warning is wrong because our function is\n> not a no-op with an empty format: it may be printing\n> colorized output along with a trailing newline.\n>\n> Signed-off-by: Jeff King \n> ---\n> I'm torn on this one. It really does provide us with more compile-time\n> safety checks, but it's annoying that \"-Wall -Werror\" will no longer\n> work out of the box.\n\nYeah, that is a show-stopper for me X-<.\n\n> We could also add a status_printf_empty_line() function, but that feels\n> like a bit of a hack. Searching online, I also found the amusing\n> suggestion to do:\n>\n> status_printf_ln(s, color, \"%.*s\", 0,\n> \"-Wformat-zero-length please choke on a bucket of socks\");\n>\n> but I think that is probably worse than adding a specialized function.\n>\n> wt-status.h | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/wt-status.h b/wt-status.h\n> index 4121bc2..fb7152e 100644\n> --- a/wt-status.h\n> +++ b/wt-status.h\n> @@ -96,9 +96,9 @@ void wt_porcelain_print(struct wt_status *s);\n> void wt_shortstatus_print(struct wt_status *s);\n> void wt_porcelain_print(struct wt_status *s);\n> \n> -void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...)\n> -\t;\n> -void status_printf(struct wt_status *s, const char *color, const char *fmt, ...)\n> -\t;\n> +__attribute__((format (printf, 3, 4)))\n> +void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...);\n> +__attribute__((format (printf, 3, 4)))\n> +void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);\n> \n> #endif /* STATUS_H */\n"},{"id":"222992","messageId":"20130710052859.GA5339@sigill.intra.peff.net","threadId":"34385","inReplyTo":"7vmwpvt28j.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] wt-status: use \"format\" function attribute for status_printf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-10T05:28:59Z","receivedAt":"2013-07-10T05:28:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 09, 2013 at 10:26:04PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > These functions could benefit from the added compile-time\n> > safety of having the compiler check printf arguments.\n> >\n> > Unfortunately, we also sometimes pass an empty format string,\n> > which will cause false positives with -Wformat-zero-length.\n> > In this case, that warning is wrong because our function is\n> > not a no-op with an empty format: it may be printing\n> > colorized output along with a trailing newline.\n> >\n> > Signed-off-by: Jeff King \n> > ---\n> > I'm torn on this one. It really does provide us with more compile-time\n> > safety checks, but it's annoying that \"-Wall -Werror\" will no longer\n> > work out of the box.\n> \n> Yeah, that is a show-stopper for me X-<.\n\nYou can \"fix\" it with -Wno-zero-format-length, so the hassle is not\nhuge. But I am also inclined to just drop this one. We have lived\nwithout the extra safety for a long time, and list review does tend to\ncatch such problems in practice.\n\n-Peff\n"},{"id":"222993","messageId":"7vip0jt1sy.fsf@alter.siamese.dyndns.org","threadId":"34385","inReplyTo":"20130710052859.GA5339@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] wt-status: use \"format\" function attribute for status_printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-10T05:35:25Z","receivedAt":"2013-07-10T05:35:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jul 09, 2013 at 10:26:04PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> > These functions could benefit from the added compile-time\n>> > safety of having the compiler check printf arguments.\n>> >\n>> > Unfortunately, we also sometimes pass an empty format string,\n>> > which will cause false positives with -Wformat-zero-length.\n>> > In this case, that warning is wrong because our function is\n>> > not a no-op with an empty format: it may be printing\n>> > colorized output along with a trailing newline.\n>> >\n>> > Signed-off-by: Jeff King \n>> > ---\n>> > I'm torn on this one. It really does provide us with more compile-time\n>> > safety checks, but it's annoying that \"-Wall -Werror\" will no longer\n>> > work out of the box.\n>> \n>> Yeah, that is a show-stopper for me X-<.\n>\n> You can \"fix\" it with -Wno-zero-format-length, so the hassle is not\n> huge. \n\nYes, or just do func(..., \"%s\", \"\") perhaps? That also sound iffy.\n\n> But I am also inclined to just drop this one. We have lived\n> without the extra safety for a long time, and list review does tend to\n> catch such problems in practice.\n>\n> -Peff\n"},{"id":"222994","messageId":"20130710054050.GA7206@sigill.intra.peff.net","threadId":"34385","inReplyTo":"7vip0jt1sy.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] wt-status: use \"format\" function attribute for status_printf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-10T05:40:50Z","receivedAt":"2013-07-10T05:40:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 09, 2013 at 10:35:25PM -0700, Junio C Hamano wrote:\n\n> > You can \"fix\" it with -Wno-zero-format-length, so the hassle is not\n> > huge. \n> \n> Yes, or just do func(..., \"%s\", \"\") perhaps? That also sound iffy.\n\nI imagine that is the method intended by upstream (though who\nknows...the whole warning seems kind of stupid to me; it is clear that\nprintf(\"\") is a no-op, but it is obviously not clear that arbitrary\nfunctions using __attribute__(format) are).\n\nThe patch to do it is below, but I actually think an explicit blank-line\nfunction like:\n\n status_print_empty_line(s, color);\n\nwould be more obvious to a reader.\n\n---\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 6b693c1..1fe81bc 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -768,7 +768,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\t\tcommitter_ident.buf);\n \n \t\tif (ident_shown)\n-\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"\");\n+\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\n \n \t\tsaved_color_setting = s->use_color;\n \t\ts->use_color = 0;\ndiff --git a/wt-status.c b/wt-status.c\nindex b191c65..5876032 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -178,7 +178,7 @@ static void wt_status_print_unmerged_header(struct wt_status *s)\n \t} else {\n \t\tstatus_printf_ln(s, c, _(\" (use \\\"git add/rm ...\\\" as appropriate to mark resolution)\"));\n \t}\n-\tstatus_printf_ln(s, c, \"\");\n+\tstatus_printf_ln(s, c, \"%s\", \"\");\n }\n \n static void wt_status_print_cached_header(struct wt_status *s)\n@@ -194,7 +194,7 @@ static void wt_status_print_cached_header(struct wt_status *s)\n \t\tstatus_printf_ln(s, c, _(\" (use \\\"git reset %s ...\\\" to unstage)\"), s->reference);\n \telse\n \t\tstatus_printf_ln(s, c, _(\" (use \\\"git rm --cached ...\\\" to unstage)\"));\n-\tstatus_printf_ln(s, c, \"\");\n+\tstatus_printf_ln(s, c, \"%s\", \"\");\n }\n \n static void wt_status_print_dirty_header(struct wt_status *s,\n@@ -213,7 +213,7 @@ static void wt_status_print_dirty_header(struct wt_status *s,\n \tstatus_printf_ln(s, c, _(\" (use \\\"git checkout -- ...\\\" to discard changes in working directory)\"));\n \tif (has_dirty_submodules)\n \t\tstatus_printf_ln(s, c, _(\" (commit or discard the untracked or modified content in submodules)\"));\n-\tstatus_printf_ln(s, c, \"\");\n+\tstatus_printf_ln(s, c, \"%s\", \"\");\n }\n \n static void wt_status_print_other_header(struct wt_status *s,\n@@ -225,12 +225,12 @@ static void wt_status_print_trailer(struct wt_status *s)\n \tif (!advice_status_hints)\n \t\treturn;\n \tstatus_printf_ln(s, c, _(\" (use \\\"git %s ...\\\" to include in what will be committed)\"), how);\n-\tstatus_printf_ln(s, c, \"\");\n+\tstatus_printf_ln(s, c, \"%s\", \"\");\n }\n \n static void wt_status_print_trailer(struct wt_status *s)\n {\n-\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"\");\n+\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n }\n \n #define quote_path quote_path_relative\n@@ -1191,7 +1191,7 @@ void wt_status_print(struct wt_status *s)\n \t\t\t\ton_what = _(\"Not currently on any branch.\");\n \t\t\t}\n \t\t}\n-\t\tstatus_printf(s, color(WT_STATUS_HEADER, s), \"\");\n+\t\tstatus_printf(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n \t\tstatus_printf_more(s, branch_status_color, \"%s\", on_what);\n \t\tstatus_printf_more(s, branch_color, \"%s\\n\", branch_name);\n \t\tif (!s->is_initial)\n@@ -1204,9 +1204,9 @@ void wt_status_print(struct wt_status *s)\n \tfree(state.detached_from);\n \n \tif (s->is_initial) {\n-\t\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"\");\n+\t\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n \t\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), _(\"Initial commit\"));\n-\t\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"\");\n+\t\tstatus_printf_ln(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n \t}\n \n \twt_status_print_updated(s);\n@@ -1223,7 +1223,7 @@ void wt_status_print(struct wt_status *s)\n \t\tif (s->show_ignored_files)\n \t\t\twt_status_print_other(s, &s->ignored, _(\"Ignored files\"), \"add -f\");\n \t\tif (advice_status_u_option && 2000 < s->untracked_in_ms) {\n-\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"\");\n+\t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"%s\", \"\");\n \t\t\tstatus_printf_ln(s, GIT_COLOR_NORMAL,\n \t\t\t\t\t _(\"It took %.2f seconds to enumerate untracked files. 'status -uno'\\n\"\n \t\t\t\t\t \"may speed it up, but you have to be careful not to forget to add\\n\"\n"},{"id":"222995","messageId":"7vehb7t0zs.fsf@alter.siamese.dyndns.org","threadId":"34385","inReplyTo":"20130710054050.GA7206@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] wt-status: use \"format\" function attribute for status_printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-10T05:52:55Z","receivedAt":"2013-07-10T05:52:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jul 09, 2013 at 10:35:25PM -0700, Junio C Hamano wrote:\n>\n>> > You can \"fix\" it with -Wno-zero-format-length, so the hassle is not\n>> > huge. \n>> \n>> Yes, or just do func(..., \"%s\", \"\") perhaps? That also sound iffy.\n>\n> I imagine that is the method intended by upstream (though who\n> knows...the whole warning seems kind of stupid to me; it is clear that\n> printf(\"\") is a no-op, but it is obviously not clear that arbitrary\n> functions using __attribute__(format) are).\n>\n> The patch to do it is below, but I actually think an explicit blank-line\n> function like:\n>\n> status_print_empty_line(s, color);\n>\n> would be more obvious to a reader.\n\nI noticed that all but one can be dealt with with\n\n perl -p -i -e 's/status_printf_ln\\((.*), \"\"\\);/status_printf($1, \"\\\\n\");/'\n\nThat is,\n\n-\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"\");\n+\tstatus_printf(s, GIT_COLOR_NORMAL, \"\\n\");\n\nwhich does not look _too_ bad.\n\nThere is one instance that needs this, though.\n\n-\t\tstatus_printf(s, color(WT_STATUS_HEADER, s), \"\");\n+\t\tstatus_printf(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n"},{"id":"222996","messageId":"20130710061115.GA11741@sigill.intra.peff.net","threadId":"34385","inReplyTo":"7vehb7t0zs.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] wt-status: use \"format\" function attribute for status_printf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-10T06:11:15Z","receivedAt":"2013-07-10T06:11:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 09, 2013 at 10:52:55PM -0700, Junio C Hamano wrote:\n\n> > The patch to do it is below, but I actually think an explicit blank-line\n> > function like:\n> >\n> > status_print_empty_line(s, color);\n> >\n> > would be more obvious to a reader.\n> \n> I noticed that all but one can be dealt with with\n> \n> perl -p -i -e 's/status_printf_ln\\((.*), \"\"\\);/status_printf($1, \"\\\\n\");/'\n> \n> That is,\n> \n> -\tstatus_printf_ln(s, GIT_COLOR_NORMAL, \"\");\n> +\tstatus_printf(s, GIT_COLOR_NORMAL, \"\\n\");\n> \n> which does not look _too_ bad.\n\nIs that correct, though? The reason we have *_printf_ln in the\nfirst place is that we want to do:\n\n ${color}content${reset}\\n\n\nto make sure that the newline does not happen inside the colorization.\nAt least that is why I added color_printf_ln long ago.\n\nIt would probably improve the code quite a bit if we could simply feed\nmulti-line strings to status_printf, and have it stick the colorization\nin the correct spot of each line. And hmm...it kind of looks like\nstatus_vprintf already does that. Now I'm puzzled why many of these do\nnot simply include the newline along with the string being printed.\n\n> There is one instance that needs this, though.\n> \n> -\t\tstatus_printf(s, color(WT_STATUS_HEADER, s), \"\");\n> +\t\tstatus_printf(s, color(WT_STATUS_HEADER, s), \"%s\", \"\");\n\nHmm, yeah. It cannot be combined with the lines following it, either,\nbecause they are using different colorization.\n\nIf you want to keep refactoring this, I don't mind, but I kind of feel\nlike we are going down a rabbit hole for very little gain.\n\n-Peff\n"},{"id":"222997","messageId":"7va9lvszvi.fsf@alter.siamese.dyndns.org","threadId":"34385","inReplyTo":"20130710061115.GA11741@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] wt-status: use \"format\" function attribute for status_printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-10T06:17:05Z","receivedAt":"2013-07-10T06:17:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If you want to keep refactoring this, I don't mind, but I kind of feel\n> like we are going down a rabbit hole for very little gain.\n\nRight, and right. The rewrite to move _ln to \"\\n\" was wrong, and\nthis is too much hassle for too little gain. If we were to do\nsomething, I agree that it would be the best to have dedicated\n\"empty-output\" function.\n"},{"id":"223167","messageId":"7vfvvjoj2h.fsf@alter.siamese.dyndns.org","threadId":"34385","inReplyTo":"20130710052859.GA5339@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] wt-status: use \"format\" function attribute for status_printf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-07-12T16:10:30Z","receivedAt":"2013-07-12T16:10:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jul 09, 2013 at 10:26:04PM -0700, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> ...\n>> > I'm torn on this one. It really does provide us with more compile-time\n>> > safety checks, but it's annoying that \"-Wall -Werror\" will no longer\n>> > work out of the box.\n>> \n>> Yeah, that is a show-stopper for me X-<.\n>\n> You can \"fix\" it with -Wno-zero-format-length, so the hassle is not\n> huge. But I am also inclined to just drop this one. We have lived\n> without the extra safety for a long time, and list review does tend to\n> catch such problems in practice.\n\nI am tempted to actually merge the original one as-is without any of\nthe workaround, and just tell people to use -Wno-format-zero-length.\n"},{"id":"223217","messageId":"20130712204437.GC5276@sigill.intra.peff.net","threadId":"34385","inReplyTo":"7vfvvjoj2h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] wt-status: use \"format\" function attribute for status_printf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-07-12T20:44:37Z","receivedAt":"2013-07-12T20:44:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 12, 2013 at 09:10:30AM -0700, Junio C Hamano wrote:\n\n> > You can \"fix\" it with -Wno-zero-format-length, so the hassle is not\n> > huge. But I am also inclined to just drop this one. We have lived\n> > without the extra safety for a long time, and list review does tend to\n> > catch such problems in practice.\n> \n> I am tempted to actually merge the original one as-is without any of\n> the workaround, and just tell people to use -Wno-format-zero-length.\n\nYeah, I think the only downside is the cognitive burden on individual\ndevelopers who try -Wall and have to figure out that we need\n-Wno-zero-format-length (and that the warnings are not interesting).\n\nIt would be nice to add it automatically to CFLAGS, but I do not know if\nwe can reliably detect from the Makefile that we are compiling under\ngcc.\n\n-Peff\n"}]}