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 Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>
>> On Wed, Feb 02 2022, Elijah Newren via GitGitGadget wrote:
>>
>> > From: Elijah Newren <newren@gmail.com>
>> >
>> > This modifies the new display_update_messages() function to allow
>> > printing to somewhere other than stdout.  It also consolidates the
>> > location of the diff_warn_rename_limit() message with the rest of the
>> > CONFLICT and other update messages to all go to the same stream.
>> >
>> > Signed-off-by: Elijah Newren <newren@gmail.com>
>> > ---
>> >  merge-ort.c | 9 +++++----
>> >  merge-ort.h | 3 ++-
>> >  2 files changed, 7 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/merge-ort.c b/merge-ort.c
>> > index 82d2faf5bf9..d28d1721d14 100644
>> > --- a/merge-ort.c
>> > +++ b/merge-ort.c
>> > @@ -4236,7 +4236,8 @@ static int record_conflicted_index_entries(struct merge_options *opt)
>> >  }
>> >
>> >  void merge_display_update_messages(struct merge_options *opt,
>> > -                                struct merge_result *result)
>> > +                                struct merge_result *result,
>> > +                                FILE *stream)
>> >  {
>> >       struct merge_options_internal *opti = result->priv;
>> >       struct hashmap_iter iter;
>> > @@ -4263,13 +4264,13 @@ void merge_display_update_messages(struct merge_options *opt,
>> >       for (i = 0; i < olist.nr; ++i) {
>> >               struct strbuf *sb = olist.items[i].util;
>> >
>> > -             printf("%s", sb->buf);
>> > +             strbuf_write(sb, stream);
>> >       }
>> >       string_list_clear(&olist, 0);
>> >
>> >       /* Also include needed rename limit adjustment now */
>> >       diff_warn_rename_limit("merge.renamelimit",
>> > -                            opti->renames.needed_limit, 0, stderr);
>> > +                            opti->renames.needed_limit, 0, stream);
>>
>> At the tip of this series I tried to s/stream/stderr/g this, and
>> t4301-merge-tree-write-tree.sh passes, doesn't this warning_fp() special
>> behavior need a test somewhere?
>
> That's a fair point, but...this gets back to my cover letter comments
> about patches 5, 6, and 8.  They implement a code feature that seems
> useful in general...but which Dscho and Christian didn't like using in
> this particular command; they just wanted all output on stdout.
>
> So, it's hard to add a test, because there's no code anywhere that
> exercises it in this series anymore.  I originally wanted this feature
> in my remerge-diff series, but the idea of conflict headers made me
> punt it to this series.  I wanted it for this series, but Dscho and
> Christian didn't.  I could have punted again, but decided the
> underlying want kept coming up and decided to not excise it --
> especially since Dscho was helping improve it.  And Junio commented
> that he liked the idea[1].
>
> [1] https://lore.kernel.org/git/xmqqh79hx8g1.fsf@gitster.g/
>
> But yeah, it does leave it feeling slightly odd that we implemented a
> feature that nothing is currently using.  Maybe these 3 should be
> split off into their own series?  Still wouldn't have a test yet,
> though.
>
>> I assumed that warning_fp() would be using vreportf() in usage.c, but
>> it's not, it's just falling back to the equivalent of fprintf(out, ...),
>> no? I don't really see why 05/15 and parts of 06/15 & this are needed
>> over a much simpler simple helper macro like the below. applied on top
>> of this series.
>
> That macro is simple?  I thought I basically understood Dscho's code,
> but looking at what you did with diff_warn_rename_limit(), I think I'm
> lost.

I guess that's a matter of taste, yeah it's a bit of macro soup if
you're not familiar with it. FWIW (sans bug I noted below) it's the
macro soup we already use for other functions in usage.c.

>> 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().

> 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'

>> I.e. an API in usage.c that allowed warning to a given FD would be
>> trying to replace the "2" in the write_in_full() call in vreportf(), I
>> would think.
>
> Hmm, makes sense.

The reason I'm barking up this particular tree is that I've got some
upcoming patches for usage.c (the C99-only macro series):
https://lore.kernel.org/git/RFC-cover-00.21-00000000000-20211115T220831Z-avarab@gmail.com/

It would need to deal with anything in the API. In this case there's not
much to deal with, since it's really not at all using the rest of
usage.c, it's just a "or to stderr".

>> 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:

diff --git a/diff.c b/diff.c
index 28368110147..a2bc2595533 100644
--- a/diff.c
+++ b/diff.c
@@ -6377,6 +6377,13 @@ 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, ...) do { \
+	if (out == stderr) \
+		warning(__VA_ARGS__); \
+	else \
+		fprintf(out, __VA_ARGS__); \
+} while (0)
+
 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
 			    FILE *out)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
index 64ba60e5c71..d70ce142861 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -475,7 +475,6 @@ int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-void warning_fp(FILE *out, const char *warn, ...) __attribute__((format (printf, 2, 3)));
 
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
diff --git a/usage.c b/usage.c
index 0bfd2c603c0..c7d233b0de9 100644
--- a/usage.c
+++ b/usage.c
@@ -253,20 +253,6 @@ void warning(const char *warn, ...)
 	va_end(params);
 }
 
-void warning_fp(FILE *out, const char *warn, ...)
-{
-	va_list params;
-
-	va_start(params, warn);
-	if (out == stderr)
-		warn_routine(warn, params);
-	else {
-		vfprintf(out, warn, params);
-		fputc('\n', out);
-	}
-	va_end(params);
-}
-
 /* Only set this, ever, from t/helper/, when verifying that bugs are caught. */
 int BUG_exit_code;
 
I do think you'd probably prefer the non-macro version, which is pretty
much just going back to this:
https://lore.kernel.org/git/6fb4f4580a581b2e43bc4b8deaa3d2d2bf4a8756.1643479633.git.gitgitgadget@gmail.com/

diff --git a/diff.c b/diff.c
index 28368110147..21c9561f546 100644
--- a/diff.c
+++ b/diff.c
@@ -6380,17 +6380,28 @@ N_("you may want to set your %s variable to at least "
 void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
 			    FILE *out)
 {
+	const char *msg;
+
 	fflush(stdout);
 	if (degraded_cc)
-		warning_fp(out, _(degrade_cc_to_c_warning));
+		msg = _(degrade_cc_to_c_warning);
 	else if (needed)
-		warning_fp(out, _(rename_limit_warning));
+		msg = _(rename_limit_warning);
 	else
 		return;
 
+	if (out == stderr)
+		warning("%s", msg);
+	else
+		fprintf(stderr, "%s", msg);
 
-	if (0 < needed)
-		warning_fp(out, _(rename_limit_advice), varname, needed);
+	if (0 >= needed)
+		return;
+
+	if (out == stderr)
+		warning(_(rename_limit_advice), varname, needed);
+	else
+		fprintf(stderr, _(rename_limit_advice), varname, needed);
 }
 
 static void create_filepairs_for_header_only_notifications(struct diff_options *o)
diff --git a/git-compat-util.h b/git-compat-util.h
index 64ba60e5c71..d70ce142861 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -475,7 +475,6 @@ int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
-void warning_fp(FILE *out, const char *warn, ...) __attribute__((format (printf, 2, 3)));
 
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
diff --git a/usage.c b/usage.c
index 0bfd2c603c0..c7d233b0de9 100644
--- a/usage.c
+++ b/usage.c
@@ -253,20 +253,6 @@ void warning(const char *warn, ...)
 	va_end(params);
 }
 
-void warning_fp(FILE *out, const char *warn, ...)
-{
-	va_list params;
-
-	va_start(params, warn);
-	if (out == stderr)
-		warn_routine(warn, params);
-	else {
-		vfprintf(out, warn, params);
-		fputc('\n', out);
-	}
-	va_end(params);
-}
-
 /* Only set this, ever, from t/helper/, when verifying that bugs are caught. */
 int BUG_exit_code;
 
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.