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

Commit Message

Elijah Newren Feb. 2, 2022, 7:34 a.m. UTC

From: Elijah Newren <[e-mail]>

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 <[e-mail]>
---
 merge-ort.c | 9 +++++----
 merge-ort.h | 3 ++-
 2 files changed, 7 insertions(+), 5 deletions(-)

Comments

Ævar Arnfjörð Bjarmason Feb. 3, 2022, 1:48 a.m. UTC | #1

On Wed, Feb 02 2022, Elijah Newren via GitGitGadget wrote:

> From: Elijah Newren <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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?

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.

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.

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.

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);
 	else
 		return;
 
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;

Elijah Newren Feb. 3, 2022, 9:12 a.m. UTC | #2

On Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason <[e-mail]> wrote:
>
> On Wed, Feb 02 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <[e-mail]>
> >
> > 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 <[e-mail]>
> > ---
> >  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 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?

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

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

>         else
>                 return;
>
> 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;

Ævar Arnfjörð Bjarmason Feb. 3, 2022, 10:01 a.m. UTC | #3

On Thu, Feb 03 2022, Elijah Newren wrote:

> On Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason <[e-mail]> wrote:
>>
>> On Wed, Feb 02 2022, Elijah Newren via GitGitGadget wrote:
>>
>> > From: Elijah Newren <[e-mail]>
>> >
>> > 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 <[e-mail]>
>> > ---
>> >  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/[e-mail]/

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/6fb4[e-mail]/

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.

Elijah Newren Feb. 3, 2022, 4:09 p.m. UTC | #4

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

Ævar Arnfjörð Bjarmason Feb. 3, 2022, 4:19 p.m. UTC | #5

On Thu, Feb 03 2022, Elijah Newren wrote:

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

Elijah Newren Feb. 3, 2022, 5 p.m. UTC | #6

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

Johannes Schindelin Feb. 21, 2022, 9:13 a.m. UTC | #7

Hi Elijah,

On Thu, 3 Feb 2022, Elijah Newren wrote:

> On Thu, Feb 3, 2022 at 8:24 AM Ævar Arnfjörð Bjarmason <[e-mail]> wrote:
> >
> > On Thu, Feb 03 2022, Elijah Newren wrote:
> >
> > > 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.

So we dropped some useful patches future-proofing `merge-tree` for the
sake of appeasing a refactoring with no immediately obvious benefit? I
really don't like that direction.

Ciao,
Dscho

Elijah Newren Feb. 22, 2022, 1:54 a.m. UTC | #8

On Mon, Feb 21, 2022 at 1:13 AM Johannes Schindelin
<[e-mail]> wrote:
>
> Hi Elijah,
>
> On Thu, 3 Feb 2022, Elijah Newren wrote:
>
> > On Thu, Feb 3, 2022 at 8:24 AM Ævar Arnfjörð Bjarmason <[e-mail]> wrote:
> > >
> > > On Thu, Feb 03 2022, Elijah Newren wrote:
> > >
> > > > 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.
>
> So we dropped some useful patches future-proofing `merge-tree` for the
> sake of appeasing a refactoring with no immediately obvious benefit? I
> really don't like that direction.

Even before any of Ævar's comments, I had already noted on my cover
letter[1] that "to be honest, patches 5, 6, & 8 may be less relevant
since we're now including these messages on stdout anyway" -- so I was
already wondering if I should defer them to some future series.  Then
when Ævar reviewed the series, he noted (1) that I lacked tests of
these changes (which is true, because nothing uses them, and I can't
easily add a test as I have no current usecase in mind), and (2) these
patches would print a "warning: " prefix when printing to stdout but
not print such a prefix otherwise, which felt inconsistent.  Those
seemed to reinforce the comment I had already made that these changes
were unused in my series and maybe should be separated.  I still like
the general idea behind the future proofing you did here, so maybe I
was just being lazy, but between those factors I decided that punting
until later made sense.

[1] https://lore.kernel.org/git/[e-mail]/

Johannes Schindelin Feb. 22, 2022, 4:48 p.m. UTC | #9

Hi Elijah,

On Mon, 21 Feb 2022, Elijah Newren wrote:

> On Mon, Feb 21, 2022 at 1:13 AM Johannes Schindelin
> <[e-mail]> wrote:
> >
> > Hi Elijah,
> >
> > On Thu, 3 Feb 2022, Elijah Newren wrote:
> >
> > > On Thu, Feb 3, 2022 at 8:24 AM Ævar Arnfjörð Bjarmason <[e-mail]> wrote:
> > > >
> > > > On Thu, Feb 03 2022, Elijah Newren wrote:
> > > >
> > > > > 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.
> >
> > So we dropped some useful patches future-proofing `merge-tree` for the
> > sake of appeasing a refactoring with no immediately obvious benefit? I
> > really don't like that direction.
>
> Even before any of Ævar's comments, I had already noted on my cover
> letter[1] that "to be honest, patches 5, 6, & 8 may be less relevant
> since we're now including these messages on stdout anyway" -- so I was
> already wondering if I should defer them to some future series.

Ah, that was not clear to me. In that case, I retract my objections.

Thanks for clarifying,
Dscho

Patch

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);
 
 	trace2_region_leave("merge", "display messages", opt->repo);
 }
@@ -4313,7 +4314,7 @@  void merge_switch_to_result(struct merge_options *opt,
 	}
 
 	if (display_update_msgs)
-		merge_display_update_messages(opt, result);
+		merge_display_update_messages(opt, result, stdout);
 
 	merge_finalize(opt, result);
 }
diff --git a/merge-ort.h b/merge-ort.h
index e5aec45b18f..d643b47cb7c 100644
--- a/merge-ort.h
+++ b/merge-ort.h
@@ -86,7 +86,8 @@  void merge_switch_to_result(struct merge_options *opt,
  * so only call this when bypassing merge_switch_to_result().
  */
 void merge_display_update_messages(struct merge_options *opt,
-				   struct merge_result *result);
+				   struct merge_result *result,
+				   FILE *stream);
 
 /* Do needed cleanup when not calling merge_switch_to_result() */
 void merge_finalize(struct merge_options *opt,