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

From: Derrick Stolee <stolee@gmail.com>
Date: Jun 28, 2024, 02:45 UTC
Message-ID: <3f1e155f-f559-42ac-9454-8ddcf7873f48@gmail.com>
In-Reply-To: <pull.1748.v2.git.1718766019.gitgitgadget@gmail.com>

On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:

Show 12 quoted lines

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

Show 6 quoted lines

> 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