Re: [PATCH 1/4] sequencer: Warn when internal merge may be suboptimal due to renameLimit

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 13, 2017, 05:16 UTC
Message-ID: <xmqqy3na92rv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171110173956.25105-2-newren@gmail.com>

Elijah Newren <newren@gmail.com> writes:

> Subject: Re: [PATCH 1/4] sequencer: Warn when internal merge may be suboptimal due to renameLimit

Style: please downcase s/Warn/warn/ to fit this better in "shortlog --no-merges"
output.

Show 5 quoted lines

> Having renamed files silently treated as deleted/modified conflicts
> instead of correctly resolving the renamed/modified case has caused lots
> of pain for some users.  Eventually, some of them figured out to set
> merge.renameLimit to something higher, but without feedback about what
> value it should have, they were just repeatedly guessing and retrying.

It would have been _much_ easier to read if you refrained from stating the gripes more prominently than the source of the gripe that you are fixing. E.g.

	If one side of a merge have renamed more paths than
	merge.renamelimit since the sides diverged, the recursive
	merge strategy stops detecting renames and instead leaves
	these many paths as delete/modify conflicts, without warning
	what is going on and giving hints on how to tell Git that it
	is allowed to spend more cycles to detect renames.

perhaps.

The addition of a call to diff_warn_rename_limit looks quite sensible.

Thanks.

Show 16 quoted lines

> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  sequencer.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/sequencer.c b/sequencer.c
> index f2a10cc4f2..2b4cecb617 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -462,6 +462,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
>  	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)
>  		return clean;