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

[Elijah Newren <newren@gmail.com>]

On Sun, Aug 21, 2022 at 11:05 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> "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?

Yes, you are.

Though, to be fair, my commit message is wrong too.

I'll explain below...

> 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) {

No, the original code would not have taken 'ort'.  You have overlooked
the part of the code immediately above these two lines:

    if (ret < 2) {
        if (!ret) {
            if (option_commit) {
                /* Automerge succeeded. */
                automerge_was_ok = 1;
                break;
            }
        merge_was_ok = 1;
        }

In particular, the "break" is key.  If the first strategy succeeds
(and the user did not specify --no-commit), then the loop will hit
that break statement and bail out of the loop, preventing any other
merge strategy from being tried.  In contrast, if the user did specify
--no-commit and the previous strategy succeeded, then we will continue
the loop.  That seems rather inconsistent, since --no-commit should
not affect the choice of strategy.

However, I missed two things in my reading.  You are correct that I
missed the "<=" as opposed to "<" when I wrote my commit message,
though that turns out to not matter much due to the second thing.  The
second thing I missed was part of the code at the beginning of the
loop:

    for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {

In particular, that "!merge_was_ok" check means that the code would
bail early whenever ret was 0, regardless of whether --no-commit was
passed.  So, my code turns out to not only leave behavior alone, but
also doesn't count as an optimization either -- it's simply a
half-baked cleanup.  If I also get rid of the now-redundant
"!merge_was_ok" check in the for loop and fix my commit message, then
I think it'd be a fully-baked code cleanup.

I'll submit an update.