On Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> wrote:
>
> On Wed, Feb 02 2022, Elijah Newren via GitGitGadget wrote:
>
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> > 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;
