Re: [PATCH 2/2] merge: avoid searching for strategies with fewer than 0 conflicts

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 21, 2022, 18:05 UTC
Message-ID: <xmqqwnb1fp5n.fsf@gitster.g>
In-Reply-To: <5657a05e7635ecadbb8d2e41ad97fe19f3633fdd.1661056709.git.gitgitgadget@gmail.com>

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 13 quoted lines

> From: Elijah Newren <newren@gmail.com>
>
> builtin/merge.c has a loop over the specified strategies, where if
> they all fail with conflicts, it picks the one with the least number
> of conflicts.
>
> In the codepath that finds a successful merge, if an automatic commit
> was wanted, the code breaks out of the above loop, which makes sense.
> However, if the user requested there be no automatic commit, the loop
> would continue looking for a "better" strategy.  Since it had just
> found a strategy with 0 conflicts though, and it is not possible to
> have fewer than 0 conflicts, the continuing search is guaranteed to be
> futile.

Let's imagine "git merge -s ours -s ort X", both of which resolve the merge cleanly but differently.

The "best_cnt" thing tries to find the last strategy with the lowest score from evaluate_result(), so strictly speaking I think this changes the behaviour. Am I mistaken?

When two strategies 'ours' and 'ort' resolved a given merge cleanly (but differently), we would have taken the result from 'ort' in the original code, but now we stop at seeing that 'ours' resolved the merge cleanly and use its result.

>  			cnt = (use_strategies_nr > 1) ? evaluate_result() : 0;
>  			if (best_cnt <= 0 || cnt <= best_cnt) {

While I think "we see one clean merge, so it is OK to quit" is more intuitive than the current behaviour, we need to update this comparison to require the new candidate to have strictly better result to take over the tentative best from the current candidate. That will make things consistent with this change and makes it easier to sell.

As the userbase of Git is so wide, it is very plausible that somebody already invented and publisized that prepending "-s ours" before the real strategy they want to use as an idiom to say "fall back to pretend merge cleanly succeeded but did not affect the tree" in a script that automate rebuilding tentative integration branch to test, or something. They can "fix" their "idiom" to append instead of prepend "-s ours", and that would arguably make the resulting command line more intuitive and easier to understand, but the fact that whatever that was working for them to their liking suddenly changes the behaviour is a regression we have to ask them to accept.

I do not mind writing something like this

    "git merge" with multiple strategies chooses to leave the least
    number of conflicted paths as the result.  Among multiple strategies
    that give the same number of conflicted paths, it used to use the
    last such strategy with the smallest number of conflicted paths.
    The command now prefers the first such strategy instead.

    If you have been using "git merge -s ours -s another X" as a way to
    say "we prefer 'another' strategy to merge, but use 'ours' strategy
    to make progress if it does not", you now have to swap the order of
    strategies you list on the command line.

in the "Backward Incompatibility Notes" section of the release notes.

Thanks.