[PATCH 0/6] add missing __attribute__((format))

[PATCH 0/6] add missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

Adds missing add missing __attribute__((format)) in various places,
which improves compile-time checking.

Ævar Arnfjörð Bjarmason (6):
  *.c static functions: don't forward-declare __attribute__
  sequencer.c: move static function to avoid forward decl
  *.c static functions: add missing __attribute__((format))
  *.h: add a few missing  __attribute__((format))
  bugreport.c: tweak cmd_bugreport() to use __attribute__((printf))
  git-compat-util.h: add __attribute__((printf)) to git_*printf*

 add-patch.c                                   |  1 +
 advice.h                                      |  1 +
 builtin/am.c                                  |  1 +
 builtin/bisect--helper.c                      |  2 +
 builtin/bugreport.c                           | 11 ++++-
 builtin/index-pack.c                          |  4 +-
 builtin/receive-pack.c                        |  5 +--
 cache.h                                       |  1 +
 commit-graph.c                                |  1 +
 compat/mingw.c                                |  1 +
 compat/win32/syslog.h                         |  1 +
 compat/winansi.c                              |  1 +
 .../osxkeychain/git-credential-osxkeychain.c  |  1 +
 .../wincred/git-credential-wincred.c          |  1 +
 gettext.c                                     |  1 +
 git-compat-util.h                             |  2 +
 imap-send.c                                   |  3 ++
 mailmap.c                                     |  1 +
 merge-ort.c                                   |  1 +
 merge-recursive.c                             |  1 +
 midx.c                                        |  1 +
 quote.h                                       |  1 +
 ref-filter.c                                  |  1 +
 sequencer.c                                   | 43 +++++++++----------
 server-info.c                                 |  1 +
 strbuf.h                                      |  2 +
 t/helper/test-advise.c                        |  2 +-
 worktree.c                                    |  1 +
 28 files changed, 62 insertions(+), 31 deletions(-)

-- 
2.32.0.636.g43e71d69cff

[PATCH 1/6] *.c static functions: don't forward-declare __attribute__

[Ævar Arnfjörð Bjarmason]

9cf6d3357aa (Add git-index-pack utility, 2005-10-12) and
466dbc42f58 (receive-pack: Send internal errors over side-band #2,
2010-02-10) we added these static functions and forward-declared their
__attribute__((printf)).

I think this may have been to work around some compiler limitation at
the time, but in any case we have a lot of code that uses the briefer
way of declaring these that I'm using here, so if we had any such
issues with compilers we'd have seen them already.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/index-pack.c   | 4 +---
 builtin/receive-pack.c | 5 ++---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3fbc5d70777..8336466865c 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -369,9 +369,7 @@ static void parse_pack_header(void)
 	use(sizeof(struct pack_header));
 }
 
-static NORETURN void bad_object(off_t offset, const char *format,
-		       ...) __attribute__((format (printf, 2, 3)));
-
+__attribute__((format (printf, 2, 3)))
 static NORETURN void bad_object(off_t offset, const char *format, ...)
 {
 	va_list params;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a34742513ac..2d1f97e1ca7 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -425,9 +425,6 @@ static int proc_receive_ref_matches(struct command *cmd)
 	return 0;
 }
 
-static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));
-static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
-
 static void report_message(const char *prefix, const char *err, va_list params)
 {
 	int sz;
@@ -445,6 +442,7 @@ static void report_message(const char *prefix, const char *err, va_list params)
 		xwrite(2, msg, sz);
 }
 
+__attribute__((format (printf, 1, 2)))
 static void rp_warning(const char *err, ...)
 {
 	va_list params;
@@ -453,6 +451,7 @@ static void rp_warning(const char *err, ...)
 	va_end(params);
 }
 
+__attribute__((format (printf, 1, 2)))
 static void rp_error(const char *err, ...)
 {
 	va_list params;
-- 
2.32.0.636.g43e71d69cff

[PATCH 2/6] sequencer.c: move static function to avoid forward decl

[Ævar Arnfjörð Bjarmason]

Move the reflog_message() function added in
96e832a5fd6 (sequencer (rebase -i): refactor setting the reflog
message, 2017-01-02), it gained another user in
9055e401dd6 (sequencer: introduce new commands to reset the revision,
2018-04-25). Let's move it around and remove the forward declaration
added in the latter commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38e..c316d8374a7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3599,7 +3599,25 @@ static int do_label(struct repository *r, const char *name, int len)
 }
 
 static const char *reflog_message(struct replay_opts *opts,
-	const char *sub_action, const char *fmt, ...);
+	const char *sub_action, const char *fmt, ...)
+{
+	va_list ap;
+	static struct strbuf buf = STRBUF_INIT;
+	char *reflog_action = getenv(GIT_REFLOG_ACTION);
+
+	va_start(ap, fmt);
+	strbuf_reset(&buf);
+	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
+	if (sub_action)
+		strbuf_addf(&buf, " (%s)", sub_action);
+	if (fmt) {
+		strbuf_addstr(&buf, ": ");
+		strbuf_vaddf(&buf, fmt, ap);
+	}
+	va_end(ap);
+
+	return buf.buf;
+}
 
 static int do_reset(struct repository *r,
 		    const char *name, int len,
@@ -4178,27 +4196,6 @@ int apply_autostash_oid(const char *stash_oid)
 	return apply_save_autostash_oid(stash_oid, 1);
 }
 
-static const char *reflog_message(struct replay_opts *opts,
-	const char *sub_action, const char *fmt, ...)
-{
-	va_list ap;
-	static struct strbuf buf = STRBUF_INIT;
-	char *reflog_action = getenv(GIT_REFLOG_ACTION);
-
-	va_start(ap, fmt);
-	strbuf_reset(&buf);
-	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
-	if (sub_action)
-		strbuf_addf(&buf, " (%s)", sub_action);
-	if (fmt) {
-		strbuf_addstr(&buf, ": ");
-		strbuf_vaddf(&buf, fmt, ap);
-	}
-	va_end(ap);
-
-	return buf.buf;
-}
-
 static int run_git_checkout(struct repository *r, struct replay_opts *opts,
 			    const char *commit, const char *action)
 {
-- 
2.32.0.636.g43e71d69cff

[PATCH 3/6] *.c static functions: add missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

Add missing __attribute__((format)) function attributes to various
"static" functions that take printf arguments.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 add-patch.c                                                 | 1 +
 builtin/am.c                                                | 1 +
 builtin/bisect--helper.c                                    | 2 ++
 commit-graph.c                                              | 1 +
 compat/mingw.c                                              | 1 +
 compat/winansi.c                                            | 1 +
 contrib/credential/osxkeychain/git-credential-osxkeychain.c | 1 +
 contrib/credential/wincred/git-credential-wincred.c         | 1 +
 gettext.c                                                   | 1 +
 imap-send.c                                                 | 3 +++
 mailmap.c                                                   | 1 +
 merge-ort.c                                                 | 1 +
 merge-recursive.c                                           | 1 +
 midx.c                                                      | 1 +
 ref-filter.c                                                | 1 +
 sequencer.c                                                 | 2 ++
 server-info.c                                               | 1 +
 worktree.c                                                  | 1 +
 18 files changed, 22 insertions(+)

diff --git a/add-patch.c b/add-patch.c
index 2fad92ca373..8c41cdfe39b 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -280,6 +280,7 @@ static void add_p_state_clear(struct add_p_state *s)
 	clear_add_i_state(&s->s);
 }
 
+__attribute__((format (printf, 2, 3)))
 static void err(struct add_p_state *s, const char *fmt, ...)
 {
 	va_list args;
diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81b..0c2ad96b70e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -210,6 +210,7 @@ static void write_state_bool(const struct am_state *state,
  * If state->quiet is false, calls fprintf(fp, fmt, ...), and appends a newline
  * at the end.
  */
+__attribute__((format (printf, 3, 4)))
 static void say(const struct am_state *state, FILE *fp, const char *fmt, ...)
 {
 	va_list ap;
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 9d9540a0abf..f184eaeac6d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -117,6 +117,7 @@ static int write_in_file(const char *path, const char *mode, const char *format,
 	return fclose(fp);
 }
 
+__attribute__((format (printf, 2, 3)))
 static int write_to_file(const char *path, const char *format, ...)
 {
 	int res;
@@ -129,6 +130,7 @@ static int write_to_file(const char *path, const char *format, ...)
 	return res;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int append_to_file(const char *path, const char *format, ...)
 {
 	int res;
diff --git a/commit-graph.c b/commit-graph.c
index 2bcb4e0f89e..9179a3a6476 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2408,6 +2408,7 @@ int write_commit_graph(struct object_directory *odb,
 #define VERIFY_COMMIT_GRAPH_ERROR_HASH 2
 static int verify_commit_graph_error;
 
+__attribute__((format (printf, 1, 2)))
 static void graph_report(const char *fmt, ...)
 {
 	va_list ap;
diff --git a/compat/mingw.c b/compat/mingw.c
index aa647b367b0..e6afb4ad9e8 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -189,6 +189,7 @@ static int read_yes_no_answer(void)
 	return -1;
 }
 
+__attribute__((format (printf, 1, 2)))
 static int ask_yes_no_if_possible(const char *format, ...)
 {
 	char question[4096];
diff --git a/compat/winansi.c b/compat/winansi.c
index c27b20a79d9..9cf95747d41 100644
--- a/compat/winansi.c
+++ b/compat/winansi.c
@@ -461,6 +461,7 @@ static void winansi_exit(void)
 	CloseHandle(hthread);
 }
 
+__attribute__((format (printf, 1, 2)))
 static void die_lasterr(const char *fmt, ...)
 {
 	va_list params;
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index bcd3f575a3e..0b44a9b7cc6 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -10,6 +10,7 @@ static char *username;
 static char *password;
 static UInt16 port;
 
+__attribute__((format (printf, 1, 2)))
 static void die(const char *err, ...)
 {
 	char msg[4096];
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 5bdad41de1f..5091048f9c6 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -11,6 +11,7 @@
 
 #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
 
+__attribute__((format (printf, 1, 2)))
 static void die(const char *err, ...)
 {
 	char msg[4096];
diff --git a/gettext.c b/gettext.c
index af2413b47e8..bb5ba1fe7cc 100644
--- a/gettext.c
+++ b/gettext.c
@@ -66,6 +66,7 @@ const char *get_preferred_languages(void)
 }
 
 #ifndef NO_GETTEXT
+__attribute__((format (printf, 1, 2)))
 static int test_vsnprintf(const char *fmt, ...)
 {
 	char buf[26];
diff --git a/imap-send.c b/imap-send.c
index bb085d66d10..ce5a0461a43 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -451,6 +451,7 @@ static int buffer_gets(struct imap_buffer *b, char **s)
 	/* not reached */
 }
 
+__attribute__((format (printf, 1, 2)))
 static void imap_info(const char *msg, ...)
 {
 	va_list va;
@@ -463,6 +464,7 @@ static void imap_info(const char *msg, ...)
 	}
 }
 
+__attribute__((format (printf, 1, 2)))
 static void imap_warn(const char *msg, ...)
 {
 	va_list va;
@@ -504,6 +506,7 @@ static char *next_arg(char **s)
 	return ret;
 }
 
+__attribute__((format (printf, 3, 4)))
 static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
 {
 	int ret;
diff --git a/mailmap.c b/mailmap.c
index d1f7c0d272d..462b3956340 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -8,6 +8,7 @@
 #define debug_mm(...) fprintf(stderr, __VA_ARGS__)
 #define debug_str(X) ((X) ? (X) : "(none)")
 #else
+__attribute__((format (printf, 1, 2)))
 static inline void debug_mm(const char *format, ...) {}
 static inline const char *debug_str(const char *s) { return s; }
 #endif
diff --git a/merge-ort.c b/merge-ort.c
index b954f7184a5..955d1d05027 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -529,6 +529,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
 	renames->callback_data_nr = renames->callback_data_alloc = 0;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int err(struct merge_options *opt, const char *err, ...)
 {
 	va_list params;
diff --git a/merge-recursive.c b/merge-recursive.c
index 4327e0cfa33..5d54990af9a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -167,6 +167,7 @@ static void flush_output(struct merge_options *opt)
 	}
 }
 
+__attribute__((format (printf, 2, 3)))
 static int err(struct merge_options *opt, const char *err, ...)
 {
 	va_list params;
diff --git a/midx.c b/midx.c
index 21d6a05e887..0956849e2a1 100644
--- a/midx.c
+++ b/midx.c
@@ -1162,6 +1162,7 @@ void clear_midx_file(struct repository *r)
 
 static int verify_midx_error;
 
+__attribute__((format (printf, 1, 2)))
 static void midx_report(const char *fmt, ...)
 {
 	va_list ap;
diff --git a/ref-filter.c b/ref-filter.c
index 4db0e40ff4c..f45d3a1b26d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -213,6 +213,7 @@ static int used_atom_cnt, need_tagged, need_symref;
  * Expand string, append it to strbuf *sb, then return error code ret.
  * Allow to save few lines of code.
  */
+__attribute__((format (printf, 3, 4)))
 static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...)
 {
 	va_list ap;
diff --git a/sequencer.c b/sequencer.c
index c316d8374a7..7f07cd00f3f 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3521,6 +3521,7 @@ static int do_exec(struct repository *r, const char *command_line)
 	return status;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int safe_append(const char *filename, const char *fmt, ...)
 {
 	va_list ap;
@@ -3598,6 +3599,7 @@ static int do_label(struct repository *r, const char *name, int len)
 	return ret;
 }
 
+__attribute__((format (printf, 3, 4)))
 static const char *reflog_message(struct replay_opts *opts,
 	const char *sub_action, const char *fmt, ...)
 {
diff --git a/server-info.c b/server-info.c
index de0aa4498c6..7701d7c20a1 100644
--- a/server-info.c
+++ b/server-info.c
@@ -27,6 +27,7 @@ static int uic_is_stale(const struct update_info_ctx *uic)
 	return uic->old_fp == NULL;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int uic_printf(struct update_info_ctx *uic, const char *fmt, ...)
 {
 	va_list ap;
diff --git a/worktree.c b/worktree.c
index 237517baee6..092a4f92ad2 100644
--- a/worktree.c
+++ b/worktree.c
@@ -265,6 +265,7 @@ const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
 }
 
 /* convenient wrapper to deal with NULL strbuf */
+__attribute__((format (printf, 2, 3)))
 static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
 {
 	va_list params;
-- 
2.32.0.636.g43e71d69cff

[PATCH 4/6] *.h: add a few missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

Add missing format attributes to those function that were missing
them.

In the case of advice_enabled() this revealed a trivial issue
introduced in b3b18d16213 (advice: revamp advise API, 2020-03-02). We
treated the argv[1] as a format string, but did not intend to do
so. Let's use "%s" and pass argv[1] as an argument instead.

For strbuf_addftime() let's add a strftime() format checker. Our
function understands the non-portable %z and %Z, see
c3fbf81a853 (strbuf: let strbuf_addftime handle %z and %Z itself,
2017-06-15).

That might be an issue in theory, but in practice we have existing
codepath that supplies a fixed string to strbuf_addftime(). We're
unlikely to run into the "%z" and "%Z" case at all, since it's used by
date.c and passed via e.g. "git log --date=<format>".

In fact, we had no in-tree user of strbuf_addftime() with an inline
fixed format string at all. A subsequent commit will tweak an existing
one to use the format checking.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.h               | 1 +
 cache.h                | 1 +
 compat/win32/syslog.h  | 1 +
 quote.h                | 1 +
 strbuf.h               | 2 ++
 t/helper/test-advise.c | 2 +-
 6 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/advice.h b/advice.h
index bd26c385d00..9f8ffc73546 100644
--- a/advice.h
+++ b/advice.h
@@ -90,6 +90,7 @@ int advice_enabled(enum advice_type type);
 /**
  * Checks the visibility of the advice before printing.
  */
+__attribute__((format (printf, 2, 3)))
 void advise_if_enabled(enum advice_type type, const char *advice, ...);
 
 int error_resolve_conflict(const char *me);
diff --git a/cache.h b/cache.h
index ba04ff8bd36..f9aed2d45c7 100644
--- a/cache.h
+++ b/cache.h
@@ -1385,6 +1385,7 @@ enum get_oid_result {
 };
 
 int repo_get_oid(struct repository *r, const char *str, struct object_id *oid);
+__attribute__((format (printf, 2, 3)))
 int get_oidf(struct object_id *oid, const char *fmt, ...);
 int repo_get_oid_commit(struct repository *r, const char *str, struct object_id *oid);
 int repo_get_oid_committish(struct repository *r, const char *str, struct object_id *oid);
diff --git a/compat/win32/syslog.h b/compat/win32/syslog.h
index 70daa7c08b8..28e2c96c52d 100644
--- a/compat/win32/syslog.h
+++ b/compat/win32/syslog.h
@@ -15,6 +15,7 @@
 #define LOG_DAEMON  (3<<3)
 
 void openlog(const char *ident, int logopt, int facility);
+__attribute__((format (printf, 2, 3)))
 void syslog(int priority, const char *fmt, ...);
 
 #endif /* SYSLOG_H */
diff --git a/quote.h b/quote.h
index 768cc6338e2..049d8dd0b3d 100644
--- a/quote.h
+++ b/quote.h
@@ -31,6 +31,7 @@ struct strbuf;
 
 void sq_quote_buf(struct strbuf *, const char *src);
 void sq_quote_argv(struct strbuf *, const char **argv);
+__attribute__((format (printf, 2, 3)))
 void sq_quotef(struct strbuf *, const char *fmt, ...);
 
 /*
diff --git a/strbuf.h b/strbuf.h
index 223ee2094af..215fbdd64bc 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -263,6 +263,7 @@ static inline void strbuf_insertstr(struct strbuf *sb, size_t pos,
 void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt,
 		     va_list ap);
 
+__attribute__((format (printf, 3, 4)))
 void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...);
 
 /**
@@ -425,6 +426,7 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
  * `suppress_tz_name`, when set, expands %Z internally to the empty
  * string rather than passing it to `strftime`.
  */
+__attribute__((format (strftime, 2, 0)))
 void strbuf_addftime(struct strbuf *sb, const char *fmt,
 		    const struct tm *tm, int tz_offset,
 		    int suppress_tz_name);
diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
index a7043df1d38..cb881139f73 100644
--- a/t/helper/test-advise.c
+++ b/t/helper/test-advise.c
@@ -16,7 +16,7 @@ int cmd__advise_if_enabled(int argc, const char **argv)
 	 * selected here and in t0018 where this command is being
 	 * executed.
 	 */
-	advise_if_enabled(ADVICE_NESTED_TAG, argv[1]);
+	advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[1]);
 
 	return 0;
 }
-- 
2.32.0.636.g43e71d69cff

Re: [PATCH 4/6] *.h: add a few missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

On Sat, Jul 10 2021, Ævar Arnfjörð Bjarmason wrote:

So this:
> [...]
> For strbuf_addftime() let's add a strftime() format checker. Our
> function understands the non-portable %z and %Z, see
> c3fbf81a853 (strbuf: let strbuf_addftime handle %z and %Z itself,
> 2017-06-15).
>
> That might be an issue in theory, but in practice we have existing
> codepath that supplies a fixed string to strbuf_addftime(). We're
> unlikely to run into the "%z" and "%Z" case at all, since it's used by
> date.c and passed via e.g. "git log --date=<format>".
> [...]
>  /**
> @@ -425,6 +426,7 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
>   * `suppress_tz_name`, when set, expands %Z internally to the empty
>   * string rather than passing it to `strftime`.
>   */
> +__attribute__((format (strftime, 2, 0)))
>  void strbuf_addftime(struct strbuf *sb, const char *fmt,
>  		    const struct tm *tm, int tz_offset,
>  		    int suppress_tz_name);

Fails with compat/mingw.[ch] doing:

    [...]
    compat/mingw.c:#undef strftime
    compat/mingw.c-size_t mingw_strftime(char *s, size_t max,
    compat/mingw.c-               const char *format, const struct tm *tm)
    [...]
    compat/mingw.h:#define strftime mingw_strftime
    [...]

What's a good macro idiom to dig oneself out of that hole? The only
similar thing I could find was NORETURN in git-compat-util.h being
defined differently on various platforms, and then things like:

	+#if !defined(__MINGW32__) && !defined(_MSC_VER)
	 __attribute__((format (strftime, 2, 0)))
	+#endif

Which seems to work, and may be the simplest workaround...

Re: [PATCH 4/6] *.h: add a few missing __attribute__((format))

[Jeff King]

On Mon, Jul 12, 2021 at 01:05:53AM +0200, Ævar Arnfjörð Bjarmason wrote:

> So this:
> > [...]
> > For strbuf_addftime() let's add a strftime() format checker. Our
> > function understands the non-portable %z and %Z, see
> > c3fbf81a853 (strbuf: let strbuf_addftime handle %z and %Z itself,
> > 2017-06-15).
> >
> > That might be an issue in theory, but in practice we have existing
> > codepath that supplies a fixed string to strbuf_addftime(). We're
> > unlikely to run into the "%z" and "%Z" case at all, since it's used by
> > date.c and passed via e.g. "git log --date=<format>".
> > [...]
> >  /**
> > @@ -425,6 +426,7 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
> >   * `suppress_tz_name`, when set, expands %Z internally to the empty
> >   * string rather than passing it to `strftime`.
> >   */
> > +__attribute__((format (strftime, 2, 0)))
> >  void strbuf_addftime(struct strbuf *sb, const char *fmt,
> >  		    const struct tm *tm, int tz_offset,
> >  		    int suppress_tz_name);
> 
> Fails with compat/mingw.[ch] doing:
> 
>     [...]
>     compat/mingw.c:#undef strftime
>     compat/mingw.c-size_t mingw_strftime(char *s, size_t max,
>     compat/mingw.c-               const char *format, const struct tm *tm)
>     [...]
>     compat/mingw.h:#define strftime mingw_strftime
>     [...]
> 
> What's a good macro idiom to dig oneself out of that hole? The only
> similar thing I could find was NORETURN in git-compat-util.h being
> defined differently on various platforms, and then things like:
> 
> 	+#if !defined(__MINGW32__) && !defined(_MSC_VER)
> 	 __attribute__((format (strftime, 2, 0)))
> 	+#endif
> 
> Which seems to work, and may be the simplest workaround...

I don't think there's a general standard-C way to override the macro
temporarily. You could perhaps fix it with re-ordering, but it is
super-awkward in this case (mingw stuff clearly should go in
git-compat-util.h or its includes, and the strbuf declaration should
come after that).

Gcc at least supports __strftime__ in the attribute, so that could be an
option (I've no clue how portable that is, though).

I'm skeptical on the utility of this strftime format checking in the
first place, though. The printf one is matching up the format string
with the otherwise-unchecked variable-length parameter list. That's very
easy to get wrong, very bad if we do get it wrong, and very hard for
anybody but the compiler to check accurately.

But a typo in the strftime placeholders is really no more interesting
than a typo in any other error message. If it helps even some obscure
cases it might be worth it, but:

  - we almost never feed a string literal anyway (and unlike printf,
    feeding non-literals is not dangerous). And in the one literal case
    you touched in the next patch, we had to make the code uglier to
    enable the checking.

  - as you note, this function isn't even a true strftime() replacement
    anyway. We allow %z, and I would not be at all opposed to adding
    more custom bits (either something regular strftime doesn't support
    at all, or extensions that are not universally available and we have
    to provide a fallback for).

  - it creates the macro headache you saw

  - I'm not sure if it creates other portability concerns. Does
    everybody who supports the format attribute support the strftime
    archetype? I have no clue.

So it doesn't really seem to be to be worth it on balance.

-Peff

[PATCH 5/6] bugreport.c: tweak cmd_bugreport() to use __attribute__((printf))

[Ævar Arnfjörð Bjarmason]

As noted in the preceding commit we had no __attribute__((format))
check for strbuf_addftime(), and furthermore had no in-tree user that
passed it a fixed string.

Let's tweak this codepath added in 238b439d698 (bugreport: add tool to
generate debugging info, 2020-04-16) to make use of the compile-time
check.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/bugreport.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 9915a5841de..02edfb9a047 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -127,7 +127,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	time_t now = time(NULL);
 	struct tm tm;
 	char *option_output = NULL;
-	char *option_suffix = "%Y-%m-%d-%H%M";
+	char *option_suffix = NULL;
 	const char *user_relative_path = NULL;
 	char *prefixed_filename;
 
@@ -149,7 +149,14 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	strbuf_complete(&report_path, '/');
 
 	strbuf_addstr(&report_path, "git-bugreport-");
-	strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+	/*
+	 * This is an if/else for the benefit of the compile-time
+	 * strftime() format checking we have for strbuf_addftime().
+	 */
+	if (option_suffix)
+		strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+	else
+		strbuf_addftime(&report_path, "%Y-%m-%d-%H%M", localtime_r(&now, &tm), 0, 0);
 	strbuf_addstr(&report_path, ".txt");
 
 	switch (safe_create_leading_directories(report_path.buf)) {
-- 
2.32.0.636.g43e71d69cff

Re: [PATCH 5/6] bugreport.c: tweak cmd_bugreport() to use __attribute__((printf))

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> As noted in the preceding commit we had no __attribute__((format))
> check for strbuf_addftime(), and furthermore had no in-tree user that
> passed it a fixed string.
>
> Let's tweak this codepath added in 238b439d698 (bugreport: add tool to
> generate debugging info, 2020-04-16) to make use of the compile-time
> check.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/bugreport.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 9915a5841de..02edfb9a047 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -127,7 +127,7 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
>  	time_t now = time(NULL);
>  	struct tm tm;
>  	char *option_output = NULL;
> -	char *option_suffix = "%Y-%m-%d-%H%M";
> +	char *option_suffix = NULL;
> ...
> +	if (option_suffix)
> +		strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
> +	else
> +		strbuf_addftime(&report_path, "%Y-%m-%d-%H%M", localtime_r(&now, &tm), 0, 0);
>  	strbuf_addstr(&report_path, ".txt");
>  
>  	switch (safe_create_leading_directories(report_path.buf)) {

The use of "the variable option_suffix whose default value is
'%Y-%m-%d-%H%M'" may happen to be only to generate report_path in
the current code, but there is little reason to expect that will
stay to be true.  With this change, however, future developers has
to know that option_suffix may not have its default value in it and
they need to do the "if it is NULL, then use the default", and
without any benefit of proprocessor macro.

And in exchange of it, we gain what?  Letting some compilers ensure
that "%Y-%m-%d-%H%M" is a valid strftime format string?  That does
not sound all that much interesting.

This step I am not impressed all that much, even though the earlier
clean-up steps all looked sensible.

[PATCH 6/6] git-compat-util.h: add __attribute__((printf)) to git_*printf*

[Ævar Arnfjörð Bjarmason]

Add __attribute__((printf)) to the compatibility functions we use
under SNPRINTF_RETURNS_BOGUS=Y.

In practice this is redundant to the compiler's default printf format
checking, since we mostly (entirely?)  develop and test on platforms
where SNPRINTF_RETURNS_BOGUS is not set. I'm doing this mainly for
consistency with other code, now we don't need to think about why this
particular case is different.

See c4582f93a26 (Add compat/snprintf.c for systems that return bogus,
2008-03-05) for the commit that added these functions.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 git-compat-util.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index dca72cba294..af098d5c932 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -788,12 +788,14 @@ FILE *git_fopen(const char*, const char*);
 #undef snprintf
 #endif
 #define snprintf git_snprintf
+__attribute__((format (printf, 3, 4)))
 int git_snprintf(char *str, size_t maxsize,
 		 const char *format, ...);
 #ifdef vsnprintf
 #undef vsnprintf
 #endif
 #define vsnprintf git_vsnprintf
+__attribute__((format (printf, 3, 0)))
 int git_vsnprintf(char *str, size_t maxsize,
 		  const char *format, va_list ap);
 #endif
-- 
2.32.0.636.g43e71d69cff

Re: [PATCH 6/6] git-compat-util.h: add __attribute__((printf)) to git_*printf*

[Jeff King]

On Sat, Jul 10, 2021 at 10:47:32AM +0200, Ævar Arnfjörð Bjarmason wrote:

> Add __attribute__((printf)) to the compatibility functions we use
> under SNPRINTF_RETURNS_BOGUS=Y.
> 
> In practice this is redundant to the compiler's default printf format
> checking, since we mostly (entirely?)  develop and test on platforms
> where SNPRINTF_RETURNS_BOGUS is not set. I'm doing this mainly for
> consistency with other code, now we don't need to think about why this
> particular case is different.
> 
> See c4582f93a26 (Add compat/snprintf.c for systems that return bogus,
> 2008-03-05) for the commit that added these functions.

I'm slightly lukewarm on general on adding this to a compat function.
Those are meant to be a lowest-common-denominator fallback, and we
usually avoid fancy features or our usual styles there in favor of
simplicity.

I guess this probably isn't _hurting_ anything, but it makes me wonder
how many systems have a broken snprintf _and_ support the attribute.

-Peff

RE: [PATCH 6/6] git-compat-util.h: add __attribute__((printf)) to git_*printf*

[Randall S. Becker]

On July 12, 2021 4:13 PM, Jeff King wrote:
>On Sat, Jul 10, 2021 at 10:47:32AM +0200, Ævar Arnfjörð Bjarmason wrote:
>
>> Add __attribute__((printf)) to the compatibility functions we use
>> under SNPRINTF_RETURNS_BOGUS=Y.
>>
>> In practice this is redundant to the compiler's default printf format
>> checking, since we mostly (entirely?)  develop and test on platforms
>> where SNPRINTF_RETURNS_BOGUS is not set. I'm doing this mainly for
>> consistency with other code, now we don't need to think about why this
>> particular case is different.
>>
>> See c4582f93a26 (Add compat/snprintf.c for systems that return bogus,
>> 2008-03-05) for the commit that added these functions.
>
>I'm slightly lukewarm on general on adding this to a compat function.
>Those are meant to be a lowest-common-denominator fallback, and we usually avoid fancy features or our usual styles there in favor of
>simplicity.
>
>I guess this probably isn't _hurting_ anything, but it makes me wonder how many systems have a broken snprintf _and_ support the
>attribute.

NonStop does not support __attribute__ on any compiler I know of. This appears to be a gcc extension, so compat.c would create a gcc dependency, which is also not on the platform. snprintf is in place.

-Randall

Re: [PATCH 6/6] git-compat-util.h: add __attribute__((printf)) to git_*printf*

[Jeff King]

On Mon, Jul 12, 2021 at 04:26:16PM -0400, Randall S. Becker wrote:

> >> In practice this is redundant to the compiler's default printf format
> >> checking, since we mostly (entirely?)  develop and test on platforms
> >> where SNPRINTF_RETURNS_BOGUS is not set. I'm doing this mainly for
> >> consistency with other code, now we don't need to think about why this
> >> particular case is different.
> >>
> >> See c4582f93a26 (Add compat/snprintf.c for systems that return bogus,
> >> 2008-03-05) for the commit that added these functions.
> >
> >I'm slightly lukewarm on general on adding this to a compat function.
> >Those are meant to be a lowest-common-denominator fallback, and we usually avoid fancy features or our usual styles there in favor of
> >simplicity.
> >
> >I guess this probably isn't _hurting_ anything, but it makes me wonder how many systems have a broken snprintf _and_ support the
> >attribute.
> 
> NonStop does not support __attribute__ on any compiler I know of. This
> appears to be a gcc extension, so compat.c would create a gcc
> dependency, which is also not on the platform. snprintf is in place.

We already turn __attribute__ into a noop on unsupported platforms early
in git-compat-util.h (around line 443). So this would be OK, since the
snprintf macro hackery is later in the file (around line 786).

-Peff

RE: [PATCH 6/6] git-compat-util.h: add __attribute__((printf)) to git_*printf*

[Randall S. Becker]

On July 12, 2021 5:45 PM, Jeff King wrote:
>On Mon, Jul 12, 2021 at 04:26:16PM -0400, Randall S. Becker wrote:
>> >> In practice this is redundant to the compiler's default printf
>> >> format checking, since we mostly (entirely?)  develop and test on
>> >> platforms where SNPRINTF_RETURNS_BOGUS is not set. I'm doing this
>> >> mainly for consistency with other code, now we don't need to think
>> >> about why this particular case is different.
>> >>
>> >> See c4582f93a26 (Add compat/snprintf.c for systems that return
>> >> bogus,
>> >> 2008-03-05) for the commit that added these functions.
>> >
>> >I'm slightly lukewarm on general on adding this to a compat function.
>> >Those are meant to be a lowest-common-denominator fallback, and we
>> >usually avoid fancy features or our usual styles there in favor of simplicity.
>> >
>> >I guess this probably isn't _hurting_ anything, but it makes me
>> >wonder how many systems have a broken snprintf _and_ support the attribute.
>>
>> NonStop does not support __attribute__ on any compiler I know of. This
>> appears to be a gcc extension, so compat.c would create a gcc
>> dependency, which is also not on the platform. snprintf is in place.
>
>We already turn __attribute__ into a noop on unsupported platforms early in git-compat-util.h (around line 443). So this would be OK,
>since the snprintf macro hackery is later in the file (around line 786).

Works for me, in that case.

Re: [PATCH 0/6] add missing __attribute__((format))

[Jeff King]

On Sat, Jul 10, 2021 at 10:47:26AM +0200, Ævar Arnfjörð Bjarmason wrote:

> Adds missing add missing __attribute__((format)) in various places,
> which improves compile-time checking.

Thanks, this is definitely worth doing. There were more of these than I
expected.

Patches 1-3 look obviously good. I didn't double-check all of your
parameter-numbers, but the compiler should make it painfully obvious if
you got any wrong. :)

I'm iffy on the strftime and compat things in the latter patches; I left
more detailed responses there.

-Peff

[PATCH v2 0/6] add missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

Adds missing add missing __attribute__((format)) in various places,
which improves compile-time checking.

v2: Let's drop the whole bending over backwards to do mostly/entirely
useless strftime() checking. That's gone, I added a patch at the end
with a comment for strbuf_addftime() to say why it's not there, and
also split up the advise_if_enabled() change into its own commit.

I also removed the other cases of adding attribute checking to
compat/*. I can't easily test those, and I don't know if there's
potential bad interactions with git-compat-util.h.

Ævar Arnfjörð Bjarmason (6):
  *.c static functions: don't forward-declare __attribute__
  sequencer.c: move static function to avoid forward decl
  *.c static functions: add missing __attribute__((format))
  *.h: add a few missing  __attribute__((format))
  advice.h: add missing __attribute__((format)) & fix usage
  strbuf.h: add a comment about "missing" strftime() checking

 add-patch.c                                   |  1 +
 advice.h                                      |  1 +
 builtin/am.c                                  |  1 +
 builtin/bisect--helper.c                      |  2 +
 builtin/index-pack.c                          |  4 +-
 builtin/receive-pack.c                        |  5 +--
 cache.h                                       |  1 +
 commit-graph.c                                |  1 +
 .../osxkeychain/git-credential-osxkeychain.c  |  1 +
 .../wincred/git-credential-wincred.c          |  1 +
 gettext.c                                     |  1 +
 imap-send.c                                   |  3 ++
 mailmap.c                                     |  1 +
 merge-ort.c                                   |  1 +
 merge-recursive.c                             |  1 +
 midx.c                                        |  1 +
 quote.h                                       |  1 +
 ref-filter.c                                  |  1 +
 sequencer.c                                   | 43 +++++++++----------
 server-info.c                                 |  1 +
 strbuf.h                                      |  7 +++
 t/helper/test-advise.c                        |  2 +-
 worktree.c                                    |  1 +
 23 files changed, 53 insertions(+), 29 deletions(-)

Range-diff against v1:
1:  a855bfceb2 = 1:  a855bfceb2 *.c static functions: don't forward-declare __attribute__
2:  9c1492b006 = 2:  9c1492b006 sequencer.c: move static function to avoid forward decl
3:  bc3fee3b7a ! 3:  e2e039f481 *.c static functions: add missing __attribute__((format))
    @@ commit-graph.c: int write_commit_graph(struct object_directory *odb,
      {
      	va_list ap;
     
    - ## compat/mingw.c ##
    -@@ compat/mingw.c: static int read_yes_no_answer(void)
    - 	return -1;
    - }
    - 
    -+__attribute__((format (printf, 1, 2)))
    - static int ask_yes_no_if_possible(const char *format, ...)
    - {
    - 	char question[4096];
    -
    - ## compat/winansi.c ##
    -@@ compat/winansi.c: static void winansi_exit(void)
    - 	CloseHandle(hthread);
    - }
    - 
    -+__attribute__((format (printf, 1, 2)))
    - static void die_lasterr(const char *fmt, ...)
    - {
    - 	va_list params;
    -
      ## contrib/credential/osxkeychain/git-credential-osxkeychain.c ##
     @@ contrib/credential/osxkeychain/git-credential-osxkeychain.c: static char *username;
      static char *password;
4:  3bf8637c16 ! 4:  fd70d512b4 *.h: add a few missing  __attribute__((format))
    @@ Metadata
      ## Commit message ##
         *.h: add a few missing  __attribute__((format))
     
    -    Add missing format attributes to those function that were missing
    -    them.
    -
    -    In the case of advice_enabled() this revealed a trivial issue
    -    introduced in b3b18d16213 (advice: revamp advise API, 2020-03-02). We
    -    treated the argv[1] as a format string, but did not intend to do
    -    so. Let's use "%s" and pass argv[1] as an argument instead.
    -
    -    For strbuf_addftime() let's add a strftime() format checker. Our
    -    function understands the non-portable %z and %Z, see
    -    c3fbf81a853 (strbuf: let strbuf_addftime handle %z and %Z itself,
    -    2017-06-15).
    -
    -    That might be an issue in theory, but in practice we have existing
    -    codepath that supplies a fixed string to strbuf_addftime(). We're
    -    unlikely to run into the "%z" and "%Z" case at all, since it's used by
    -    date.c and passed via e.g. "git log --date=<format>".
    -
    -    In fact, we had no in-tree user of strbuf_addftime() with an inline
    -    fixed format string at all. A subsequent commit will tweak an existing
    -    one to use the format checking.
    +    Add missing format attributes to API functions that take printf
    +    arguments.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## advice.h ##
    -@@ advice.h: int advice_enabled(enum advice_type type);
    - /**
    -  * Checks the visibility of the advice before printing.
    -  */
    -+__attribute__((format (printf, 2, 3)))
    - void advise_if_enabled(enum advice_type type, const char *advice, ...);
    - 
    - int error_resolve_conflict(const char *me);
    -
      ## cache.h ##
     @@ cache.h: enum get_oid_result {
      };
    @@ cache.h: enum get_oid_result {
      int repo_get_oid_commit(struct repository *r, const char *str, struct object_id *oid);
      int repo_get_oid_committish(struct repository *r, const char *str, struct object_id *oid);
     
    - ## compat/win32/syslog.h ##
    -@@
    - #define LOG_DAEMON  (3<<3)
    - 
    - void openlog(const char *ident, int logopt, int facility);
    -+__attribute__((format (printf, 2, 3)))
    - void syslog(int priority, const char *fmt, ...);
    - 
    - #endif /* SYSLOG_H */
    -
      ## quote.h ##
     @@ quote.h: struct strbuf;
      
    @@ strbuf.h: static inline void strbuf_insertstr(struct strbuf *sb, size_t pos,
      void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...);
      
      /**
    -@@ strbuf.h: void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
    -  * `suppress_tz_name`, when set, expands %Z internally to the empty
    -  * string rather than passing it to `strftime`.
    -  */
    -+__attribute__((format (strftime, 2, 0)))
    - void strbuf_addftime(struct strbuf *sb, const char *fmt,
    - 		    const struct tm *tm, int tz_offset,
    - 		    int suppress_tz_name);
    -
    - ## t/helper/test-advise.c ##
    -@@ t/helper/test-advise.c: int cmd__advise_if_enabled(int argc, const char **argv)
    - 	 * selected here and in t0018 where this command is being
    - 	 * executed.
    - 	 */
    --	advise_if_enabled(ADVICE_NESTED_TAG, argv[1]);
    -+	advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[1]);
    - 
    - 	return 0;
    - }
5:  daca344a16 < -:  ---------- bugreport.c: tweak cmd_bugreport() to use __attribute__((printf))
6:  365c5cf50b < -:  ---------- git-compat-util.h: add __attribute__((printf)) to git_*printf*
-:  ---------- > 5:  a001e851d2 advice.h: add missing __attribute__((format)) & fix usage
-:  ---------- > 6:  fe66e06754 strbuf.h: add a comment about "missing" strftime() checking
-- 
2.32.0-dev

[PATCH v2 1/6] *.c static functions: don't forward-declare __attribute__

[Ævar Arnfjörð Bjarmason]

9cf6d3357aa (Add git-index-pack utility, 2005-10-12) and
466dbc42f58 (receive-pack: Send internal errors over side-band #2,
2010-02-10) we added these static functions and forward-declared their
__attribute__((printf)).

I think this may have been to work around some compiler limitation at
the time, but in any case we have a lot of code that uses the briefer
way of declaring these that I'm using here, so if we had any such
issues with compilers we'd have seen them already.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/index-pack.c   | 4 +---
 builtin/receive-pack.c | 5 ++---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3fbc5d7077..8336466865 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -369,9 +369,7 @@ static void parse_pack_header(void)
 	use(sizeof(struct pack_header));
 }
 
-static NORETURN void bad_object(off_t offset, const char *format,
-		       ...) __attribute__((format (printf, 2, 3)));
-
+__attribute__((format (printf, 2, 3)))
 static NORETURN void bad_object(off_t offset, const char *format, ...)
 {
 	va_list params;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a34742513a..2d1f97e1ca 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -425,9 +425,6 @@ static int proc_receive_ref_matches(struct command *cmd)
 	return 0;
 }
 
-static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));
-static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
-
 static void report_message(const char *prefix, const char *err, va_list params)
 {
 	int sz;
@@ -445,6 +442,7 @@ static void report_message(const char *prefix, const char *err, va_list params)
 		xwrite(2, msg, sz);
 }
 
+__attribute__((format (printf, 1, 2)))
 static void rp_warning(const char *err, ...)
 {
 	va_list params;
@@ -453,6 +451,7 @@ static void rp_warning(const char *err, ...)
 	va_end(params);
 }
 
+__attribute__((format (printf, 1, 2)))
 static void rp_error(const char *err, ...)
 {
 	va_list params;
-- 
2.32.0-dev

[PATCH v2 2/6] sequencer.c: move static function to avoid forward decl

[Ævar Arnfjörð Bjarmason]

Move the reflog_message() function added in
96e832a5fd6 (sequencer (rebase -i): refactor setting the reflog
message, 2017-01-02), it gained another user in
9055e401dd6 (sequencer: introduce new commands to reset the revision,
2018-04-25). Let's move it around and remove the forward declaration
added in the latter commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38..c316d8374a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3599,7 +3599,25 @@ static int do_label(struct repository *r, const char *name, int len)
 }
 
 static const char *reflog_message(struct replay_opts *opts,
-	const char *sub_action, const char *fmt, ...);
+	const char *sub_action, const char *fmt, ...)
+{
+	va_list ap;
+	static struct strbuf buf = STRBUF_INIT;
+	char *reflog_action = getenv(GIT_REFLOG_ACTION);
+
+	va_start(ap, fmt);
+	strbuf_reset(&buf);
+	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
+	if (sub_action)
+		strbuf_addf(&buf, " (%s)", sub_action);
+	if (fmt) {
+		strbuf_addstr(&buf, ": ");
+		strbuf_vaddf(&buf, fmt, ap);
+	}
+	va_end(ap);
+
+	return buf.buf;
+}
 
 static int do_reset(struct repository *r,
 		    const char *name, int len,
@@ -4178,27 +4196,6 @@ int apply_autostash_oid(const char *stash_oid)
 	return apply_save_autostash_oid(stash_oid, 1);
 }
 
-static const char *reflog_message(struct replay_opts *opts,
-	const char *sub_action, const char *fmt, ...)
-{
-	va_list ap;
-	static struct strbuf buf = STRBUF_INIT;
-	char *reflog_action = getenv(GIT_REFLOG_ACTION);
-
-	va_start(ap, fmt);
-	strbuf_reset(&buf);
-	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
-	if (sub_action)
-		strbuf_addf(&buf, " (%s)", sub_action);
-	if (fmt) {
-		strbuf_addstr(&buf, ": ");
-		strbuf_vaddf(&buf, fmt, ap);
-	}
-	va_end(ap);
-
-	return buf.buf;
-}
-
 static int run_git_checkout(struct repository *r, struct replay_opts *opts,
 			    const char *commit, const char *action)
 {
-- 
2.32.0-dev

[PATCH v2 3/6] *.c static functions: add missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

Add missing __attribute__((format)) function attributes to various
"static" functions that take printf arguments.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 add-patch.c                                                 | 1 +
 builtin/am.c                                                | 1 +
 builtin/bisect--helper.c                                    | 2 ++
 commit-graph.c                                              | 1 +
 contrib/credential/osxkeychain/git-credential-osxkeychain.c | 1 +
 contrib/credential/wincred/git-credential-wincred.c         | 1 +
 gettext.c                                                   | 1 +
 imap-send.c                                                 | 3 +++
 mailmap.c                                                   | 1 +
 merge-ort.c                                                 | 1 +
 merge-recursive.c                                           | 1 +
 midx.c                                                      | 1 +
 ref-filter.c                                                | 1 +
 sequencer.c                                                 | 2 ++
 server-info.c                                               | 1 +
 worktree.c                                                  | 1 +
 16 files changed, 20 insertions(+)

diff --git a/add-patch.c b/add-patch.c
index 2fad92ca37..8c41cdfe39 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -280,6 +280,7 @@ static void add_p_state_clear(struct add_p_state *s)
 	clear_add_i_state(&s->s);
 }
 
+__attribute__((format (printf, 2, 3)))
 static void err(struct add_p_state *s, const char *fmt, ...)
 {
 	va_list args;
diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81..0c2ad96b70 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -210,6 +210,7 @@ static void write_state_bool(const struct am_state *state,
  * If state->quiet is false, calls fprintf(fp, fmt, ...), and appends a newline
  * at the end.
  */
+__attribute__((format (printf, 3, 4)))
 static void say(const struct am_state *state, FILE *fp, const char *fmt, ...)
 {
 	va_list ap;
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 9d9540a0ab..f184eaeac6 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -117,6 +117,7 @@ static int write_in_file(const char *path, const char *mode, const char *format,
 	return fclose(fp);
 }
 
+__attribute__((format (printf, 2, 3)))
 static int write_to_file(const char *path, const char *format, ...)
 {
 	int res;
@@ -129,6 +130,7 @@ static int write_to_file(const char *path, const char *format, ...)
 	return res;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int append_to_file(const char *path, const char *format, ...)
 {
 	int res;
diff --git a/commit-graph.c b/commit-graph.c
index 2bcb4e0f89..9179a3a647 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2408,6 +2408,7 @@ int write_commit_graph(struct object_directory *odb,
 #define VERIFY_COMMIT_GRAPH_ERROR_HASH 2
 static int verify_commit_graph_error;
 
+__attribute__((format (printf, 1, 2)))
 static void graph_report(const char *fmt, ...)
 {
 	va_list ap;
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index bcd3f575a3..0b44a9b7cc 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -10,6 +10,7 @@ static char *username;
 static char *password;
 static UInt16 port;
 
+__attribute__((format (printf, 1, 2)))
 static void die(const char *err, ...)
 {
 	char msg[4096];
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 5bdad41de1..5091048f9c 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -11,6 +11,7 @@
 
 #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
 
+__attribute__((format (printf, 1, 2)))
 static void die(const char *err, ...)
 {
 	char msg[4096];
diff --git a/gettext.c b/gettext.c
index af2413b47e..bb5ba1fe7c 100644
--- a/gettext.c
+++ b/gettext.c
@@ -66,6 +66,7 @@ const char *get_preferred_languages(void)
 }
 
 #ifndef NO_GETTEXT
+__attribute__((format (printf, 1, 2)))
 static int test_vsnprintf(const char *fmt, ...)
 {
 	char buf[26];
diff --git a/imap-send.c b/imap-send.c
index bb085d66d1..ce5a0461a4 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -451,6 +451,7 @@ static int buffer_gets(struct imap_buffer *b, char **s)
 	/* not reached */
 }
 
+__attribute__((format (printf, 1, 2)))
 static void imap_info(const char *msg, ...)
 {
 	va_list va;
@@ -463,6 +464,7 @@ static void imap_info(const char *msg, ...)
 	}
 }
 
+__attribute__((format (printf, 1, 2)))
 static void imap_warn(const char *msg, ...)
 {
 	va_list va;
@@ -504,6 +506,7 @@ static char *next_arg(char **s)
 	return ret;
 }
 
+__attribute__((format (printf, 3, 4)))
 static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
 {
 	int ret;
diff --git a/mailmap.c b/mailmap.c
index d1f7c0d272..462b395634 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -8,6 +8,7 @@
 #define debug_mm(...) fprintf(stderr, __VA_ARGS__)
 #define debug_str(X) ((X) ? (X) : "(none)")
 #else
+__attribute__((format (printf, 1, 2)))
 static inline void debug_mm(const char *format, ...) {}
 static inline const char *debug_str(const char *s) { return s; }
 #endif
diff --git a/merge-ort.c b/merge-ort.c
index b954f7184a..955d1d0502 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -529,6 +529,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
 	renames->callback_data_nr = renames->callback_data_alloc = 0;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int err(struct merge_options *opt, const char *err, ...)
 {
 	va_list params;
diff --git a/merge-recursive.c b/merge-recursive.c
index 4327e0cfa3..5d54990af9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -167,6 +167,7 @@ static void flush_output(struct merge_options *opt)
 	}
 }
 
+__attribute__((format (printf, 2, 3)))
 static int err(struct merge_options *opt, const char *err, ...)
 {
 	va_list params;
diff --git a/midx.c b/midx.c
index 21d6a05e88..0956849e2a 100644
--- a/midx.c
+++ b/midx.c
@@ -1162,6 +1162,7 @@ void clear_midx_file(struct repository *r)
 
 static int verify_midx_error;
 
+__attribute__((format (printf, 1, 2)))
 static void midx_report(const char *fmt, ...)
 {
 	va_list ap;
diff --git a/ref-filter.c b/ref-filter.c
index 4db0e40ff4..f45d3a1b26 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -213,6 +213,7 @@ static int used_atom_cnt, need_tagged, need_symref;
  * Expand string, append it to strbuf *sb, then return error code ret.
  * Allow to save few lines of code.
  */
+__attribute__((format (printf, 3, 4)))
 static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...)
 {
 	va_list ap;
diff --git a/sequencer.c b/sequencer.c
index c316d8374a..7f07cd00f3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3521,6 +3521,7 @@ static int do_exec(struct repository *r, const char *command_line)
 	return status;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int safe_append(const char *filename, const char *fmt, ...)
 {
 	va_list ap;
@@ -3598,6 +3599,7 @@ static int do_label(struct repository *r, const char *name, int len)
 	return ret;
 }
 
+__attribute__((format (printf, 3, 4)))
 static const char *reflog_message(struct replay_opts *opts,
 	const char *sub_action, const char *fmt, ...)
 {
diff --git a/server-info.c b/server-info.c
index de0aa4498c..7701d7c20a 100644
--- a/server-info.c
+++ b/server-info.c
@@ -27,6 +27,7 @@ static int uic_is_stale(const struct update_info_ctx *uic)
 	return uic->old_fp == NULL;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int uic_printf(struct update_info_ctx *uic, const char *fmt, ...)
 {
 	va_list ap;
diff --git a/worktree.c b/worktree.c
index 237517baee..092a4f92ad 100644
--- a/worktree.c
+++ b/worktree.c
@@ -265,6 +265,7 @@ const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
 }
 
 /* convenient wrapper to deal with NULL strbuf */
+__attribute__((format (printf, 2, 3)))
 static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
 {
 	va_list params;
-- 
2.32.0-dev

[PATCH v2 4/6] *.h: add a few missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

Add missing format attributes to API functions that take printf
arguments.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 cache.h  | 1 +
 quote.h  | 1 +
 strbuf.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/cache.h b/cache.h
index ba04ff8bd3..f9aed2d45c 100644
--- a/cache.h
+++ b/cache.h
@@ -1385,6 +1385,7 @@ enum get_oid_result {
 };
 
 int repo_get_oid(struct repository *r, const char *str, struct object_id *oid);
+__attribute__((format (printf, 2, 3)))
 int get_oidf(struct object_id *oid, const char *fmt, ...);
 int repo_get_oid_commit(struct repository *r, const char *str, struct object_id *oid);
 int repo_get_oid_committish(struct repository *r, const char *str, struct object_id *oid);
diff --git a/quote.h b/quote.h
index 768cc6338e..049d8dd0b3 100644
--- a/quote.h
+++ b/quote.h
@@ -31,6 +31,7 @@ struct strbuf;
 
 void sq_quote_buf(struct strbuf *, const char *src);
 void sq_quote_argv(struct strbuf *, const char **argv);
+__attribute__((format (printf, 2, 3)))
 void sq_quotef(struct strbuf *, const char *fmt, ...);
 
 /*
diff --git a/strbuf.h b/strbuf.h
index 223ee2094a..f1e9821a54 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -263,6 +263,7 @@ static inline void strbuf_insertstr(struct strbuf *sb, size_t pos,
 void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt,
 		     va_list ap);
 
+__attribute__((format (printf, 3, 4)))
 void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...);
 
 /**
-- 
2.32.0-dev

[PATCH v2 5/6] advice.h: add missing __attribute__((format)) & fix usage

[Ævar Arnfjörð Bjarmason]

Add the missing __attribute__((format)) checking to
advise_if_enabled(). This revealed a trivial issue introduced in
b3b18d16213 (advice: revamp advise API, 2020-03-02). We treated the
argv[1] as a format string, but did not intend to do so. Let's use
"%s" and pass argv[1] as an argument instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.h               | 1 +
 t/helper/test-advise.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/advice.h b/advice.h
index bd26c385d0..9f8ffc7354 100644
--- a/advice.h
+++ b/advice.h
@@ -90,6 +90,7 @@ int advice_enabled(enum advice_type type);
 /**
  * Checks the visibility of the advice before printing.
  */
+__attribute__((format (printf, 2, 3)))
 void advise_if_enabled(enum advice_type type, const char *advice, ...);
 
 int error_resolve_conflict(const char *me);
diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
index a7043df1d3..cb881139f7 100644
--- a/t/helper/test-advise.c
+++ b/t/helper/test-advise.c
@@ -16,7 +16,7 @@ int cmd__advise_if_enabled(int argc, const char **argv)
 	 * selected here and in t0018 where this command is being
 	 * executed.
 	 */
-	advise_if_enabled(ADVICE_NESTED_TAG, argv[1]);
+	advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[1]);
 
 	return 0;
 }
-- 
2.32.0-dev

[PATCH v2 6/6] strbuf.h: add a comment about "missing" strftime() checking

[Ævar Arnfjörð Bjarmason]

We've recently added missing __attribute__((format)) checking of
printf formats in various places, but in the case of strbuf_addftime()
we've intentionally omitted adding an:

    __attribute__((format (strftime, 2, 0)))

There was a proposed change[1] to do that, but I agree that it's not
worth it, see e.g. [2] for the rationale. Let's add a comment to note
this.

1. http://lore.kernel.org/git/patch-4.6-3bf8637c16a-20210710T084445Z-avarab@gmail.com
2. https://lore.kernel.org/git/YOyhd%2FodtQxwQk2W@coredump.intra.peff.net/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 strbuf.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/strbuf.h b/strbuf.h
index f1e9821a54..04952e94c4 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -425,6 +425,12 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
  * with modifiers (e.g. %Ez) are passed to `strftime`.
  * `suppress_tz_name`, when set, expands %Z internally to the empty
  * string rather than passing it to `strftime`.
+ *
+ * Note: The omission of "__attribute__((format (strftime, [...])))"
+ * is intentional. As noted above we take %z and %Z which aren't
+ * portable. It would "work" anyway on modern compilers, but since
+ * this is mainly used in date.c (via"git log --date=<format>") we
+ * don't have any constant formats to check.
  */
 void strbuf_addftime(struct strbuf *sb, const char *fmt,
 		    const struct tm *tm, int tz_offset,
-- 
2.32.0-dev

Re: [PATCH v2 6/6] strbuf.h: add a comment about "missing" strftime() checking

[Jeff King]

On Tue, Jul 13, 2021 at 10:05:21AM +0200, Ævar Arnfjörð Bjarmason wrote:

> + *
> + * Note: The omission of "__attribute__((format (strftime, [...])))"
> + * is intentional. As noted above we take %z and %Z which aren't
> + * portable. It would "work" anyway on modern compilers, but since
> + * this is mainly used in date.c (via"git log --date=<format>") we
> + * don't have any constant formats to check.

Missing whitespace after "via" here.

-Peff

Re: [PATCH v2 0/6] add missing __attribute__((format))

[Jeff King]

On Tue, Jul 13, 2021 at 10:05:15AM +0200, Ævar Arnfjörð Bjarmason wrote:

> Adds missing add missing __attribute__((format)) in various places,
> which improves compile-time checking.
> 
> v2: Let's drop the whole bending over backwards to do mostly/entirely
> useless strftime() checking. That's gone, I added a patch at the end
> with a comment for strbuf_addftime() to say why it's not there, and
> also split up the advise_if_enabled() change into its own commit.
> 
> I also removed the other cases of adding attribute checking to
> compat/*. I can't easily test those, and I don't know if there's
> potential bad interactions with git-compat-util.h.

Thanks. Modulo a whitespace typo in the final patch, these all look good
to me.

I could take or leave the final patch, as I never wondered about
strftime format-checking in the first place. :) But I don't mind it
either way (the alternative is that the v1 discussion in the list could
serve the purpose).

-Peff

Re: [PATCH v2 0/6] add missing __attribute__((format))

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> v2: Let's drop the whole bending over backwards to do mostly/entirely
> useless strftime() checking. That's gone, I added a patch at the end
> with a comment for strbuf_addftime() to say why it's not there, and
> also split up the advise_if_enabled() change into its own commit.

OK.

> I also removed the other cases of adding attribute checking to
> compat/*. I can't easily test those, and I don't know if there's
> potential bad interactions with git-compat-util.h.

Sensible.

> 3:  bc3fee3b7a ! 3:  e2e039f481 *.c static functions: add missing __attribute__((format))
>     @@ commit-graph.c: int write_commit_graph(struct object_directory *odb,
>       {
>       	va_list ap;
>      
>     - ## compat/mingw.c ##
>     -@@ compat/mingw.c: static int read_yes_no_answer(void)
>     - 	return -1;
>     - }
>     - 
>     -+__attribute__((format (printf, 1, 2)))
>     - static int ask_yes_no_if_possible(const char *format, ...)
>     - {
>     - 	char question[4096];
>     -
>     - ## compat/winansi.c ##
>     -@@ compat/winansi.c: static void winansi_exit(void)
>     - 	CloseHandle(hthread);
>     - }
>     - 
>     -+__attribute__((format (printf, 1, 2)))
>     - static void die_lasterr(const char *fmt, ...)
>     - {
>     - 	va_list params;
>     -
>       ## contrib/credential/osxkeychain/git-credential-osxkeychain.c ##

These refrain from touching some compat stuff, OK.

> 4:  3bf8637c16 ! 4:  fd70d512b4 *.h: add a few missing  __attribute__((format))
>     @@ Metadata
>       ## Commit message ##
>          *.h: add a few missing  __attribute__((format))
>      
>     -    Add missing format attributes to those function that were missing
>     -    them.
>     -
>     -    In the case of advice_enabled() this revealed a trivial issue
>     -    introduced in b3b18d16213 (advice: revamp advise API, 2020-03-02). We
>     -    treated the argv[1] as a format string, but did not intend to do
>     -    so. Let's use "%s" and pass argv[1] as an argument instead.
>     -
>     -    For strbuf_addftime() let's add a strftime() format checker. Our
>     -    function understands the non-portable %z and %Z, see
>     -    c3fbf81a853 (strbuf: let strbuf_addftime handle %z and %Z itself,
>     -    2017-06-15).
>     -
>     -    That might be an issue in theory, but in practice we have existing
>     -    codepath that supplies a fixed string to strbuf_addftime(). We're
>     -    unlikely to run into the "%z" and "%Z" case at all, since it's used by
>     -    date.c and passed via e.g. "git log --date=<format>".
>     -
>     -    In fact, we had no in-tree user of strbuf_addftime() with an inline
>     -    fixed format string at all. A subsequent commit will tweak an existing
>     -    one to use the format checking.
>     +    Add missing format attributes to API functions that take printf
>     +    arguments.
>      
>          Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

OK.  strftime() is gone.

>     - ## advice.h ##
>     -@@ advice.h: int advice_enabled(enum advice_type type);
>     - /**
>     -  * Checks the visibility of the advice before printing.
>     -  */
>     -+__attribute__((format (printf, 2, 3)))
>     - void advise_if_enabled(enum advice_type type, const char *advice, ...);

This has become a separate one, because...?

OK, the addition to advise_if_enabled() reveals an existing iffy
caller, so you chose to fix it and to annotate the function at the
same time in a single commit at step [5/6].  Makes sense.

Re: [PATCH v2 0/6] add missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 13 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
> [...]
>>     - ## advice.h ##
>>     -@@ advice.h: int advice_enabled(enum advice_type type);
>>     - /**
>>     -  * Checks the visibility of the advice before printing.
>>     -  */
>>     -+__attribute__((format (printf, 2, 3)))
>>     - void advise_if_enabled(enum advice_type type, const char *advice, ...);
>
> This has become a separate one, because...?
>
> OK, the addition to advise_if_enabled() reveals an existing iffy
> caller, so you chose to fix it and to annotate the function at the
> same time in a single commit at step [5/6].  Makes sense.

Right, it's the only case that revealed an in-codebase warning, so I
thought it made sense to split that up from the mechanical addition of
__attribute__ elsewhere.

[PATCH v3 0/5] add missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

Adds missing add missing __attribute__((format)) in various places,
which improves compile-time checking.

v3: Dropped the 6th patch per feedback from Jeff King. Yes, we can do
without that strftime() comment in strbuf.c.

v2 at: https://lore.kernel.org/git/cover-0.6-0000000000-20210713T080411Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (5):
  *.c static functions: don't forward-declare __attribute__
  sequencer.c: move static function to avoid forward decl
  *.c static functions: add missing __attribute__((format))
  *.h: add a few missing  __attribute__((format))
  advice.h: add missing __attribute__((format)) & fix usage

 add-patch.c                                   |  1 +
 advice.h                                      |  1 +
 builtin/am.c                                  |  1 +
 builtin/bisect--helper.c                      |  2 +
 builtin/index-pack.c                          |  4 +-
 builtin/receive-pack.c                        |  5 +--
 cache.h                                       |  1 +
 commit-graph.c                                |  1 +
 .../osxkeychain/git-credential-osxkeychain.c  |  1 +
 .../wincred/git-credential-wincred.c          |  1 +
 gettext.c                                     |  1 +
 imap-send.c                                   |  3 ++
 mailmap.c                                     |  1 +
 merge-ort.c                                   |  1 +
 merge-recursive.c                             |  1 +
 midx.c                                        |  1 +
 quote.h                                       |  1 +
 ref-filter.c                                  |  1 +
 sequencer.c                                   | 43 +++++++++----------
 server-info.c                                 |  1 +
 strbuf.h                                      |  1 +
 t/helper/test-advise.c                        |  2 +-
 worktree.c                                    |  1 +
 23 files changed, 47 insertions(+), 29 deletions(-)

Range-diff against v2:
1:  a855bfceb2 = 1:  a855bfceb2 *.c static functions: don't forward-declare __attribute__
2:  9c1492b006 = 2:  9c1492b006 sequencer.c: move static function to avoid forward decl
3:  e2e039f481 = 3:  e2e039f481 *.c static functions: add missing __attribute__((format))
4:  fd70d512b4 = 4:  fd70d512b4 *.h: add a few missing  __attribute__((format))
5:  a001e851d2 = 5:  a001e851d2 advice.h: add missing __attribute__((format)) & fix usage
6:  fe66e06754 < -:  ---------- strbuf.h: add a comment about "missing" strftime() checking
-- 
2.32.0-dev

[PATCH v3 1/5] *.c static functions: don't forward-declare __attribute__

[Ævar Arnfjörð Bjarmason]

9cf6d3357aa (Add git-index-pack utility, 2005-10-12) and
466dbc42f58 (receive-pack: Send internal errors over side-band #2,
2010-02-10) we added these static functions and forward-declared their
__attribute__((printf)).

I think this may have been to work around some compiler limitation at
the time, but in any case we have a lot of code that uses the briefer
way of declaring these that I'm using here, so if we had any such
issues with compilers we'd have seen them already.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/index-pack.c   | 4 +---
 builtin/receive-pack.c | 5 ++---
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3fbc5d7077..8336466865 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -369,9 +369,7 @@ static void parse_pack_header(void)
 	use(sizeof(struct pack_header));
 }
 
-static NORETURN void bad_object(off_t offset, const char *format,
-		       ...) __attribute__((format (printf, 2, 3)));
-
+__attribute__((format (printf, 2, 3)))
 static NORETURN void bad_object(off_t offset, const char *format, ...)
 {
 	va_list params;
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index a34742513a..2d1f97e1ca 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -425,9 +425,6 @@ static int proc_receive_ref_matches(struct command *cmd)
 	return 0;
 }
 
-static void rp_error(const char *err, ...) __attribute__((format (printf, 1, 2)));
-static void rp_warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
-
 static void report_message(const char *prefix, const char *err, va_list params)
 {
 	int sz;
@@ -445,6 +442,7 @@ static void report_message(const char *prefix, const char *err, va_list params)
 		xwrite(2, msg, sz);
 }
 
+__attribute__((format (printf, 1, 2)))
 static void rp_warning(const char *err, ...)
 {
 	va_list params;
@@ -453,6 +451,7 @@ static void rp_warning(const char *err, ...)
 	va_end(params);
 }
 
+__attribute__((format (printf, 1, 2)))
 static void rp_error(const char *err, ...)
 {
 	va_list params;
-- 
2.32.0-dev

[PATCH v3 2/5] sequencer.c: move static function to avoid forward decl

[Ævar Arnfjörð Bjarmason]

Move the reflog_message() function added in
96e832a5fd6 (sequencer (rebase -i): refactor setting the reflog
message, 2017-01-02), it gained another user in
9055e401dd6 (sequencer: introduce new commands to reset the revision,
2018-04-25). Let's move it around and remove the forward declaration
added in the latter commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 sequencer.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 0bec01cf38..c316d8374a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3599,7 +3599,25 @@ static int do_label(struct repository *r, const char *name, int len)
 }
 
 static const char *reflog_message(struct replay_opts *opts,
-	const char *sub_action, const char *fmt, ...);
+	const char *sub_action, const char *fmt, ...)
+{
+	va_list ap;
+	static struct strbuf buf = STRBUF_INIT;
+	char *reflog_action = getenv(GIT_REFLOG_ACTION);
+
+	va_start(ap, fmt);
+	strbuf_reset(&buf);
+	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
+	if (sub_action)
+		strbuf_addf(&buf, " (%s)", sub_action);
+	if (fmt) {
+		strbuf_addstr(&buf, ": ");
+		strbuf_vaddf(&buf, fmt, ap);
+	}
+	va_end(ap);
+
+	return buf.buf;
+}
 
 static int do_reset(struct repository *r,
 		    const char *name, int len,
@@ -4178,27 +4196,6 @@ int apply_autostash_oid(const char *stash_oid)
 	return apply_save_autostash_oid(stash_oid, 1);
 }
 
-static const char *reflog_message(struct replay_opts *opts,
-	const char *sub_action, const char *fmt, ...)
-{
-	va_list ap;
-	static struct strbuf buf = STRBUF_INIT;
-	char *reflog_action = getenv(GIT_REFLOG_ACTION);
-
-	va_start(ap, fmt);
-	strbuf_reset(&buf);
-	strbuf_addstr(&buf, reflog_action ? reflog_action : action_name(opts));
-	if (sub_action)
-		strbuf_addf(&buf, " (%s)", sub_action);
-	if (fmt) {
-		strbuf_addstr(&buf, ": ");
-		strbuf_vaddf(&buf, fmt, ap);
-	}
-	va_end(ap);
-
-	return buf.buf;
-}
-
 static int run_git_checkout(struct repository *r, struct replay_opts *opts,
 			    const char *commit, const char *action)
 {
-- 
2.32.0-dev

[PATCH v3 3/5] *.c static functions: add missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

Add missing __attribute__((format)) function attributes to various
"static" functions that take printf arguments.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 add-patch.c                                                 | 1 +
 builtin/am.c                                                | 1 +
 builtin/bisect--helper.c                                    | 2 ++
 commit-graph.c                                              | 1 +
 contrib/credential/osxkeychain/git-credential-osxkeychain.c | 1 +
 contrib/credential/wincred/git-credential-wincred.c         | 1 +
 gettext.c                                                   | 1 +
 imap-send.c                                                 | 3 +++
 mailmap.c                                                   | 1 +
 merge-ort.c                                                 | 1 +
 merge-recursive.c                                           | 1 +
 midx.c                                                      | 1 +
 ref-filter.c                                                | 1 +
 sequencer.c                                                 | 2 ++
 server-info.c                                               | 1 +
 worktree.c                                                  | 1 +
 16 files changed, 20 insertions(+)

diff --git a/add-patch.c b/add-patch.c
index 2fad92ca37..8c41cdfe39 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -280,6 +280,7 @@ static void add_p_state_clear(struct add_p_state *s)
 	clear_add_i_state(&s->s);
 }
 
+__attribute__((format (printf, 2, 3)))
 static void err(struct add_p_state *s, const char *fmt, ...)
 {
 	va_list args;
diff --git a/builtin/am.c b/builtin/am.c
index 0b2d886c81..0c2ad96b70 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -210,6 +210,7 @@ static void write_state_bool(const struct am_state *state,
  * If state->quiet is false, calls fprintf(fp, fmt, ...), and appends a newline
  * at the end.
  */
+__attribute__((format (printf, 3, 4)))
 static void say(const struct am_state *state, FILE *fp, const char *fmt, ...)
 {
 	va_list ap;
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 9d9540a0ab..f184eaeac6 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -117,6 +117,7 @@ static int write_in_file(const char *path, const char *mode, const char *format,
 	return fclose(fp);
 }
 
+__attribute__((format (printf, 2, 3)))
 static int write_to_file(const char *path, const char *format, ...)
 {
 	int res;
@@ -129,6 +130,7 @@ static int write_to_file(const char *path, const char *format, ...)
 	return res;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int append_to_file(const char *path, const char *format, ...)
 {
 	int res;
diff --git a/commit-graph.c b/commit-graph.c
index 2bcb4e0f89..9179a3a647 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -2408,6 +2408,7 @@ int write_commit_graph(struct object_directory *odb,
 #define VERIFY_COMMIT_GRAPH_ERROR_HASH 2
 static int verify_commit_graph_error;
 
+__attribute__((format (printf, 1, 2)))
 static void graph_report(const char *fmt, ...)
 {
 	va_list ap;
diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index bcd3f575a3..0b44a9b7cc 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -10,6 +10,7 @@ static char *username;
 static char *password;
 static UInt16 port;
 
+__attribute__((format (printf, 1, 2)))
 static void die(const char *err, ...)
 {
 	char msg[4096];
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 5bdad41de1..5091048f9c 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -11,6 +11,7 @@
 
 #define ARRAY_SIZE(x) (sizeof(x)/sizeof(x[0]))
 
+__attribute__((format (printf, 1, 2)))
 static void die(const char *err, ...)
 {
 	char msg[4096];
diff --git a/gettext.c b/gettext.c
index af2413b47e..bb5ba1fe7c 100644
--- a/gettext.c
+++ b/gettext.c
@@ -66,6 +66,7 @@ const char *get_preferred_languages(void)
 }
 
 #ifndef NO_GETTEXT
+__attribute__((format (printf, 1, 2)))
 static int test_vsnprintf(const char *fmt, ...)
 {
 	char buf[26];
diff --git a/imap-send.c b/imap-send.c
index bb085d66d1..ce5a0461a4 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -451,6 +451,7 @@ static int buffer_gets(struct imap_buffer *b, char **s)
 	/* not reached */
 }
 
+__attribute__((format (printf, 1, 2)))
 static void imap_info(const char *msg, ...)
 {
 	va_list va;
@@ -463,6 +464,7 @@ static void imap_info(const char *msg, ...)
 	}
 }
 
+__attribute__((format (printf, 1, 2)))
 static void imap_warn(const char *msg, ...)
 {
 	va_list va;
@@ -504,6 +506,7 @@ static char *next_arg(char **s)
 	return ret;
 }
 
+__attribute__((format (printf, 3, 4)))
 static int nfsnprintf(char *buf, int blen, const char *fmt, ...)
 {
 	int ret;
diff --git a/mailmap.c b/mailmap.c
index d1f7c0d272..462b395634 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -8,6 +8,7 @@
 #define debug_mm(...) fprintf(stderr, __VA_ARGS__)
 #define debug_str(X) ((X) ? (X) : "(none)")
 #else
+__attribute__((format (printf, 1, 2)))
 static inline void debug_mm(const char *format, ...) {}
 static inline const char *debug_str(const char *s) { return s; }
 #endif
diff --git a/merge-ort.c b/merge-ort.c
index b954f7184a..955d1d0502 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -529,6 +529,7 @@ static void clear_or_reinit_internal_opts(struct merge_options_internal *opti,
 	renames->callback_data_nr = renames->callback_data_alloc = 0;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int err(struct merge_options *opt, const char *err, ...)
 {
 	va_list params;
diff --git a/merge-recursive.c b/merge-recursive.c
index 4327e0cfa3..5d54990af9 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -167,6 +167,7 @@ static void flush_output(struct merge_options *opt)
 	}
 }
 
+__attribute__((format (printf, 2, 3)))
 static int err(struct merge_options *opt, const char *err, ...)
 {
 	va_list params;
diff --git a/midx.c b/midx.c
index 21d6a05e88..0956849e2a 100644
--- a/midx.c
+++ b/midx.c
@@ -1162,6 +1162,7 @@ void clear_midx_file(struct repository *r)
 
 static int verify_midx_error;
 
+__attribute__((format (printf, 1, 2)))
 static void midx_report(const char *fmt, ...)
 {
 	va_list ap;
diff --git a/ref-filter.c b/ref-filter.c
index 4db0e40ff4..f45d3a1b26 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -213,6 +213,7 @@ static int used_atom_cnt, need_tagged, need_symref;
  * Expand string, append it to strbuf *sb, then return error code ret.
  * Allow to save few lines of code.
  */
+__attribute__((format (printf, 3, 4)))
 static int strbuf_addf_ret(struct strbuf *sb, int ret, const char *fmt, ...)
 {
 	va_list ap;
diff --git a/sequencer.c b/sequencer.c
index c316d8374a..7f07cd00f3 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3521,6 +3521,7 @@ static int do_exec(struct repository *r, const char *command_line)
 	return status;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int safe_append(const char *filename, const char *fmt, ...)
 {
 	va_list ap;
@@ -3598,6 +3599,7 @@ static int do_label(struct repository *r, const char *name, int len)
 	return ret;
 }
 
+__attribute__((format (printf, 3, 4)))
 static const char *reflog_message(struct replay_opts *opts,
 	const char *sub_action, const char *fmt, ...)
 {
diff --git a/server-info.c b/server-info.c
index de0aa4498c..7701d7c20a 100644
--- a/server-info.c
+++ b/server-info.c
@@ -27,6 +27,7 @@ static int uic_is_stale(const struct update_info_ctx *uic)
 	return uic->old_fp == NULL;
 }
 
+__attribute__((format (printf, 2, 3)))
 static int uic_printf(struct update_info_ctx *uic, const char *fmt, ...)
 {
 	va_list ap;
diff --git a/worktree.c b/worktree.c
index 237517baee..092a4f92ad 100644
--- a/worktree.c
+++ b/worktree.c
@@ -265,6 +265,7 @@ const char *worktree_prune_reason(struct worktree *wt, timestamp_t expire)
 }
 
 /* convenient wrapper to deal with NULL strbuf */
+__attribute__((format (printf, 2, 3)))
 static void strbuf_addf_gently(struct strbuf *buf, const char *fmt, ...)
 {
 	va_list params;
-- 
2.32.0-dev

[PATCH v3 4/5] *.h: add a few missing __attribute__((format))

[Ævar Arnfjörð Bjarmason]

Add missing format attributes to API functions that take printf
arguments.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 cache.h  | 1 +
 quote.h  | 1 +
 strbuf.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/cache.h b/cache.h
index ba04ff8bd3..f9aed2d45c 100644
--- a/cache.h
+++ b/cache.h
@@ -1385,6 +1385,7 @@ enum get_oid_result {
 };
 
 int repo_get_oid(struct repository *r, const char *str, struct object_id *oid);
+__attribute__((format (printf, 2, 3)))
 int get_oidf(struct object_id *oid, const char *fmt, ...);
 int repo_get_oid_commit(struct repository *r, const char *str, struct object_id *oid);
 int repo_get_oid_committish(struct repository *r, const char *str, struct object_id *oid);
diff --git a/quote.h b/quote.h
index 768cc6338e..049d8dd0b3 100644
--- a/quote.h
+++ b/quote.h
@@ -31,6 +31,7 @@ struct strbuf;
 
 void sq_quote_buf(struct strbuf *, const char *src);
 void sq_quote_argv(struct strbuf *, const char **argv);
+__attribute__((format (printf, 2, 3)))
 void sq_quotef(struct strbuf *, const char *fmt, ...);
 
 /*
diff --git a/strbuf.h b/strbuf.h
index 223ee2094a..f1e9821a54 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -263,6 +263,7 @@ static inline void strbuf_insertstr(struct strbuf *sb, size_t pos,
 void strbuf_vinsertf(struct strbuf *sb, size_t pos, const char *fmt,
 		     va_list ap);
 
+__attribute__((format (printf, 3, 4)))
 void strbuf_insertf(struct strbuf *sb, size_t pos, const char *fmt, ...);
 
 /**
-- 
2.32.0-dev

[PATCH v3 5/5] advice.h: add missing __attribute__((format)) & fix usage

[Ævar Arnfjörð Bjarmason]

Add the missing __attribute__((format)) checking to
advise_if_enabled(). This revealed a trivial issue introduced in
b3b18d16213 (advice: revamp advise API, 2020-03-02). We treated the
argv[1] as a format string, but did not intend to do so. Let's use
"%s" and pass argv[1] as an argument instead.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 advice.h               | 1 +
 t/helper/test-advise.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/advice.h b/advice.h
index bd26c385d0..9f8ffc7354 100644
--- a/advice.h
+++ b/advice.h
@@ -90,6 +90,7 @@ int advice_enabled(enum advice_type type);
 /**
  * Checks the visibility of the advice before printing.
  */
+__attribute__((format (printf, 2, 3)))
 void advise_if_enabled(enum advice_type type, const char *advice, ...);
 
 int error_resolve_conflict(const char *me);
diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
index a7043df1d3..cb881139f7 100644
--- a/t/helper/test-advise.c
+++ b/t/helper/test-advise.c
@@ -16,7 +16,7 @@ int cmd__advise_if_enabled(int argc, const char **argv)
 	 * selected here and in t0018 where this command is being
 	 * executed.
 	 */
-	advise_if_enabled(ADVICE_NESTED_TAG, argv[1]);
+	advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[1]);
 
 	return 0;
 }
-- 
2.32.0-dev

Re: [PATCH v3 0/5] add missing __attribute__((format))

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Adds missing add missing __attribute__((format)) in various places,
> which improves compile-time checking.
>
> v3: Dropped the 6th patch per feedback from Jeff King. Yes, we can do
> without that strftime() comment in strbuf.c.

OK.  I am OK with or without it, but let's take this version and
merge it down.

Thanks.