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

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;