Re: [PATCH v2 0/7] Fix and improve some error codepaths in merge-ort

Derrick Stolee
2024-06-28T02:45:36Z

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> This series started as a just a fix for the abort hit in merge-ort when
> custom merge drivers error out (see
> https://lore.kernel.org/git/75F8BD12-7743-4863-B4C5-049FDEC4645E@gearset.com/).
> However, while working on that, I found a few other issues around error
> codepaths in merge-ort. So this series:
>
> * Patches 1-2: fix the reported abort problem
> * Patches 3-4: make code in handle_content_merges() easier to handle when
> we hit errors
> * Patch 5: fix a misleading comment
> * Patches 6-7: make error handling (immediate print vs. letting callers get
> the error information) more consistent

I saw this in Junio's email about series requiring review, so I took a
look despite missing v1. Stepping through each commit in my local copy
helped make sure that these changes were correct in their proper context.

> The last two patches change the behavior slightly for error codepaths, and
> there's a question about whether we should show only the error messages that
> caused an early termination of the merge, or if we should also show any
> conflict messages for other paths that were handled before we hit the early
> termination. These patches made a decision but feel free to take those last
> two patches as more of an RFC.

I also support this change in the final two patches.

One thing I mentioned in an earlier reply was "why not use an
enum in this tri-state 'clean' variable?" and then I tried to
make just such a patch. There were two problems:

1. There were hundreds of changes that would be difficult to
do in small bits (but maybe doable).

2. There is already an 'enum ll_merge_result' in merge-ll.h
that is silently passed back from merge_3way() in merge-ort.c.
While it's technically four states, maybe it would suffice to
use that enum instead of creating a new one.

But I'll leave that as something to think about another time. It
does not change the merit of this series. I left a note about
another "&=" case that wasn't touched, but it's not wrong as-is.

Thanks,
-Stolee