On Thu, Feb 3, 2022 at 2:26 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>
> On Thu, Feb 03 2022, Elijah Newren wrote:
>
> > On Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
[...]
> >> 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?
> > 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...
> >> 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...
