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

From: Elijah Newren <newren@gmail.com>
Date: Feb 3, 2022, 09:12 UTC
Message-ID: <CABPp-BHye_Zyw=x8B+QoSxWA1b0xyVL9==7kA4CD0q3eTrk8cQ@mail.gmail.com>
In-Reply-To: <220203.86ee4k7lo8.gmgdl@evledraar.gmail.com>

On Wed, Feb 2, 2022 at 6:01 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 47 quoted lines

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

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.

Show 5 quoted lines

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

Show 5 quoted lines

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

Show 25 quoted lines

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

Show 39 quoted lines

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