On Thu, Feb 3, 2022 at 2:26 AM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

On Thu, Feb 03 2022, Elijah Newren wrote:

quoted

On Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

[...]

quoted

quoted

I would get it if the point was to actually use the full usage.c
machinery, but we're just either calling warning(), or printing a
formatted string to a file FILE *. There's no need to go through usage.c
for that, and adding an API to it that behaves like this new
warning_fp() is really confusing.

Because the formatted string being printed to the file won't have the
same "warning: " prefix that is normally added to stuff in usage?

But the pre-image doesn't add that either. We're just calling
vfprintf(), not our own vreportf().

Right, I'm saying that I thought you were reporting the original patch
as buggy because it doesn't produce the same message when given a
different stream; it'll omit the "warning: " prefix.  And I was
agreeing that it was buggy for those reasons.

Or was there a different reason you didn't like that function being in usage.c?

quoted

That's a fair point; that does have a bit of a consistency problem.
And I'd rather the messages were consistent regardless of where they
are printed.

I think that makes sense, that's why I added die_message() recently. If
you meant to print a "warning: " prefix I think it would also be fine in
this case to just do it inline. See prior art at:

    git grep '"(fatal|error|warning):' -- '*.c'

So, making diff_warn_rename_limit() stop using warning(), and just
always directly writing out and including "warning:" in its message?

I'm wondering if that might cause problems if there are any existing
callers of diff_warn_rename_limit() that might also be using
set_warn_routine() (e.g. perhaps apply.c?).  Of course, those callers
probably couldn't handle anything other than the default stream.
Hmm...

quoted

quoted

diff --git a/diff.c b/diff.c
index 28368110147..4cf67e93dea 100644
--- a/diff.c
+++ b/diff.c

@@ -6377,14 +6377,21 @@ static const char rename_limit_advice[] =
 N_("you may want to set your %s variable to at least "
    "%d and retry the command.");

+#define warning_fp(out, fmt, ...) do { \
+       if (out == stderr) \
+               warning(fmt, __VA_ARGS__); \
+       else \
+               fprintf(out, fmt, __VA_ARGS__); \
+} while (0)
+
 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
                            FILE *out)
 {
        fflush(stdout);
        if (degraded_cc)
-               warning_fp(out, _(degrade_cc_to_c_warning));
+               warning_fp(out, _(degrade_cc_to_c_warning), NULL);
        else if (needed)
-               warning_fp(out, _(rename_limit_warning));
+               warning_fp(out, _(rename_limit_warning), NULL);

Why do the only callers have a NULL parameter here?  Is this one of
those va_list/va_args things I never bothered to properly learn?

That's wrong (I blame tiredness last night),an actual working version is
produced below. Clang accepted my broken code, but gcc rightly yells
about it:

Well, seeing the new code makes me feel better as it makes more sense
to me now.  ;-)

Note that both your pre-image, my macro version and Johannes's
linked-to-above are technically buggy in that they treat a
non-formatting format as a formatting format. I.e. we should use
warning("%s", msg) in that case, not warning(msg).

See 927dc330705 (advice.h: add missing __attribute__((format)) & fix
usage, 2021-07-13) for a similar bug/fix.

Good point.


Man, what a can of worms this all is.  Maybe I really should just drop
patches 5, 6, and 8 for now...