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@xxxxxxxxxxx/).
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
