Re: [PATCH 06/12] merge-ort: allow update messages to be written to different file stream

[Elijah Newren <newren@gmail.com>]

On Fri, Jan 28, 2022 at 8:31 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> Hi Elijah,
>
> On Sat, 22 Jan 2022, Elijah Newren via GitGitGadget wrote:
>
> > diff --git a/merge-ort.c b/merge-ort.c
> > index f9e35b0f96b..b78dde55ad9 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,7 +4264,7 @@ 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);
> > +             fprintf(stream, "%s", sb->buf);
>
> Maybe `strbuf_write(sb, stream);` instead? Whenever I see a `"%s"`, I feel
> like it's unnecessary churn...

Makes sense.

> >       }
> >       string_list_clear(&olist, 0);
> >
>
> Missing from this hunk:
>
>         /* Also include needed rename limit adjustment now */
>         diff_warn_rename_limit("merge.renamelimit",
>                                opti->renames.needed_limit, 0);
>
> This function explicitly writes to `stdout`, and will need to be adjusted,
> I think, before we can include an adjustment to this call in this patch.

Oh, good point.

> Unless we override `warn_routine()` (which is used inside that function),
> that is. Which is hacky, and we would not have addressed the
> `fflush(stdout)` in `diff_warn_rename_limit()`. So I would much prefer
> something like this:
>
> -- snip --
> diff --git a/diff.c b/diff.c
> index 861282db1c3..87966602cbd 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6312,17 +6312,25 @@ static const char rename_limit_advice[] =
>  N_("you may want to set your %s variable to at least "
>     "%d and retry the command.");
>
> -void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc)
> +void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
> +                           FILE *out)
>  {
> -       fflush(stdout);
> +       const char *fmt = NULL;
> +
>         if (degraded_cc)
> -               warning(_(degrade_cc_to_c_warning));
> +               fmt = _(degrade_cc_to_c_warning);
>         else if (needed)
> -               warning(_(rename_limit_warning));
> +               fmt = _(rename_limit_warning);
>         else
>                 return;
>         if (0 < needed)
> -               warning(_(rename_limit_advice), varname, needed);
> +               fmt = _(rename_limit_advice);
> +
> +       fflush(out);
> +       if (out == stdout)
> +               warning(fmt, varname, needed);
> +       else
> +               fprintf(out, fmt, varname, needed);
>  }
>
>  static void diff_flush_patch_all_file_pairs(struct diff_options *o)
> @@ -6754,7 +6762,7 @@ int diff_result_code(struct diff_options *opt, int status)
>
>         diff_warn_rename_limit("diff.renameLimit",
>                                opt->needed_rename_limit,
> -                              opt->degraded_cc_to_c);
> +                              opt->degraded_cc_to_c, stdout);
>         if (!opt->flags.exit_with_status &&
>             !(opt->output_format & DIFF_FORMAT_CHECKDIFF))
>                 return status;
> diff --git a/diff.h b/diff.h
> index 8ba85c5e605..be4ee68c0a2 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -596,7 +596,8 @@ void diffcore_fix_diff_index(void);
>  int diff_queue_is_empty(void);
>  void diff_flush(struct diff_options*);
>  void diff_free(struct diff_options*);
> -void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc);
> +void diff_warn_rename_limit(const char *varname, int needed, int degraded_cc,
> +                           FILE *out);
>
>  /* diff-raw status letters */
>  #define DIFF_STATUS_ADDED              'A'
> diff --git a/merge-ort.c b/merge-ort.c
> index 0342f104836..e6b5a0e7c64 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4264,7 +4264,7 @@ void merge_switch_to_result(struct merge_options *opt,
>
>                 /* Also include needed rename limit adjustment now */
>                 diff_warn_rename_limit("merge.renamelimit",
> -                                      opti->renames.needed_limit, 0);
> +                                      opti->renames.needed_limit, 0, stdout);
>
>                 trace2_region_leave("merge", "display messages", opt->repo);
>         }
> diff --git a/merge-recursive.c b/merge-recursive.c
> index d9457797dbb..10b2948678c 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3731,7 +3731,8 @@ static void merge_finalize(struct merge_options *opt)
>                 strbuf_release(&opt->obuf);
>         if (show(opt, 2))
>                 diff_warn_rename_limit("merge.renamelimit",
> -                                      opt->priv->needed_rename_limit, 0);
> +                                      opt->priv->needed_rename_limit, 0,
> +                                      stdout);
>         FREE_AND_NULL(opt->priv);
>  }
>
> -- snap --

I like this, but believe it makes more sense as a preparatory patch.
So I created one and made you the author, and included your
signed-off-by.  That okay with you?

>
> The rest of the patch looks good to me.

Thanks!