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

From: Elijah Newren <newren@gmail.com>
Date: Aug 22, 2022, 15:00 UTC
Message-ID: <CABPp-BHy-Xs0S1-9kL7X4mi6JNfd420-vpVsswXNuXsc5C7AtA@mail.gmail.com>
In-Reply-To: <xmqqwnb1fp5n.fsf@gitster.g>

On Sun, Aug 21, 2022 at 11:05 AM Junio C Hamano <gitster@pobox.com> wrote:

Show 23 quoted lines

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

Show 7 quoted lines

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