On Fri, Jan 28, 2022 at 8:31 AM Johannes Schindelin
<Johannes.Schindelin@xxxxxx> 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!
