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

[Junio C Hamano <gitster@pobox.com>]

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

> 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.