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
