Re: [PATCH 5/6] bugreport.c: tweak cmd_bugreport() to use __attribute__((printf))
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 12, 2021, 19:39 UTC
- Message-ID
- <xmqqczrn48z8.fsf@gitster.g>
- In-Reply-To
- <patch-5.6-daca344a165-20210710T084445Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 31 quoted lines
> 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.