On Sun, Aug 21, 2022 at 11:05 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > 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.
