Re: [PATCH v3 08/15] merge-ort: allow update messages to be written to different file stream

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Feb 03 2022, Elijah Newren wrote:

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