On Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

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

quoted

From: Elijah Newren <redacted>

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 <redacted>
---
 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/ (local)

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.

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?