On Thu, Feb 3, 2022 at 8:24 AM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>
> On Thu, Feb 03 2022, Elijah Newren wrote:
>
> > 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?
>
> Maybe it was accidentally a bug report :) But no, I was just observing
> that it was odd that it was in usage.c when it seemingly had almost
> nothing to do with what that API accomplishes.
>
> But maybe the underlying issue is that the "warning: " part is missing
> here. But I didn't mean to report that/missed it.
>
> >> > 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...
>
> Using set_warn_routine() is the "right" way to do it currently, and with
> or without a "warning: " prefix the current API use is "wrong" if the
> purpose is to have it behave nicely with the pluggable usage.c API.
>
> But of course that may not be the goal at all, i.e. I think here we've
> probably stopped caring about usage.c's formatting, logging
> etc. entirely, and are just emitting a string.
>
> Just like serve.c emits "E <msg>" or whatever (and not with error()).
>
> >> >> 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...
>
> Yeah, I really think it's worth it to just sprinkle a tiny bit of
> if/else (or a macro) here and print to stderr inline or not. We can make
> some use of some usage.c when there's good reason to do so, but this bit
> just seems like a needless digression.
>
> I hope all of this has helped somewhat ...
Absolutely; thanks for reviewing! These parts may just end up in me
dropping some patches for now (since they're not actually being used
anyway), but I think it's all good feedback.
