From: Derrick Stolee Date: Fri, 28 Jun 2024 02:45:34 GMT Subject: Re: [PATCH v2 0/7] Fix and improve some error codepaths in merge-ort Message-ID: <3f1e155f-f559-42ac-9454-8ddcf7873f48@gmail.com> In-Reply-To: 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