Subject: Re: [PATCH v2 06/20] merge-recursive: don't force external callers to do our logging

From: Junio C Hamano <[email-masked]>
Date: Fri, 26 Jul 2019 12:57:24 -0700

Elijah Newren <[email-masked]> writes:

> Alternatively, you can view this as "make the merge functions behave
> more similarly."  merge-recursive has three different entry points:
> merge_trees(), merge_recursive(), and merge_recursive_generic().  Two of
> these would call diff_warn_rename_limit(), but merge_trees() didn't.
> This lead to callers of merge_trees() needing to manually call
> diff_warn_rename_limit() themselves.  Move this to the new
> merge_finalize() function to make sure that all three entry points run
> this function.

Interesting.  It seems that b520abf1c8f did a suboptimal jobs but
this step cleans it up quite nicely.

Are there callers of merge_trees() in other codepaths, and do they
want this change?

There is only one, builtin/checkout.c::merge_working_tree(), which
is used when switching to a different commit.  I think it would not
hurt to give the same warning (but I also think it would not hurt to
simply disable rename detection in that context); we should record
the whatever decision we make in the log message.

> Signed-off-by: Elijah Newren <[email-masked]>
> ---
>  merge-recursive.c | 7 +++----
>  sequencer.c       | 1 -
>  2 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 4a481c3929..308e350ff1 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -3583,9 +3583,6 @@ static int merge_recursive_internal(struct merge_options *opt,
>  	flush_output(opt);
>  	if (!opt->call_depth && opt->buffer_output < 2)
>  		strbuf_release(&opt->obuf);
> -	if (show(opt, 2))
> -		diff_warn_rename_limit("merge.renamelimit",
> -				       opt->needed_rename_limit, 0);
>  	return clean;
>  }
>  
> @@ -3607,7 +3604,9 @@ static int merge_start(struct merge_options *opt, struct tree *head)
>  
>  static void merge_finalize(struct merge_options *opt)
>  {
> -	/* Common code for wrapping up merges will be added here later */
> +	if (show(opt, 2))
> +		diff_warn_rename_limit("merge.renamelimit",
> +				       opt->needed_rename_limit, 0);
>  }
>  
>  int merge_trees(struct merge_options *opt,
> diff --git a/sequencer.c b/sequencer.c
> index c4ed30f1b4..094a4dd03d 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -617,7 +617,6 @@ static int do_recursive_merge(struct repository *r,
>  	if (is_rebase_i(opts) && clean <= 0)
>  		fputs(o.obuf.buf, stdout);
>  	strbuf_release(&o.obuf);
> -	diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0);
>  	if (clean < 0) {
>  		rollback_lock_file(&index_lock);
>  		return clean;