Re: [PATCH v3 08/15] merge-ort: allow update messages to be written to different file stream
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Feb 3, 2022, 01:48 UTC
- Message-ID
- <220203.86ee4k7lo8.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <04c3bdc44d2c76ffc82a95db3ca4fd07270f94cf.1643787281.git.gitgitgadget@gmail.com>
On Wed, Feb 02 2022, Elijah Newren via GitGitGadget wrote:
Show 40 quoted lines
> 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;