Fix and improve some error codepaths in merge-ort #1748
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There are issues in commit ef9b1a6: |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
|
User |
|
On the Git mailing list, Taylor Blau wrote (reply to this): On Thu, Jun 13, 2024 at 08:25:00PM +0000, Elijah Newren via GitGitGadget wrote:
> Elijah Newren (7):
> merge-ort: extract handling of priv member into reusable function
> merge-ort: maintain expected invariant for priv member
> merge-ort: fix type of local 'clean' var in handle_content_merge()
> merge-ort: clearer propagation of failure-to-function from
> merge_submodule
> merge-ort: loosen commented requirements
> merge-ort: upon merge abort, only show messages causing the abort
> merge-ort: convert more error() cases to path_msg()
>
> merge-ort.c | 167 +++++++++++++++++++++++++++++++-----------
> t/t6406-merge-attr.sh | 42 ++++++++++-
> 2 files changed, 164 insertions(+), 45 deletions(-)
Very nice. I had a couple of minor thoughts on the earlier patches, but
I agree with the substantive ones that printing only messages related to
paths that we were processing when something went horribly wrong is a
good idea.
I don't think that either of my comments alone merit a reroll, and I'd
be happy to see this version move along as-is. But I'm equally happy if
you want to fix a typo here or there or do some bikeshedding ;-).
Thanks for fixing this issue.
Thanks,
Taylor |
|
This branch is now known as |
|
This patch series was integrated into seen via git@ff97f26. |
|
User |
|
This patch series was integrated into seen via git@95a77a6. |
|
This patch series was integrated into seen via git@499db5b. |
|
There was a status update in the "New Topics" section about the branch The "ort" merge backend saw one bugfix for a crash that happens when inner merge gets killed, and assorted code clean-ups. Waiting for comments or a reroll. source: <[email protected]> |
In preparation for a subsequent commit which will ensure we do not forget to maintain our invariants for the priv member in error codepaths, extract the necessary functionality out into a separate function. This change is cosmetic at this point, and introduces no changes beyond an extra assertion sanity check. Signed-off-by: Elijah Newren <[email protected]>
The calling convention for the merge machinery is
One call to init_merge_options()
One or more calls to merge_incore_[non]recursive()
One call to merge_finalize()
(possibly indirectly via merge_switch_to_result())
Both merge_switch_to_result() and merge_finalize() expect
opt->priv == NULL && result->priv != NULL
which is supposed to be set up by our move_opt_priv_to_result_priv()
function. However, two codepaths dealing with error cases did not
execute this necessary logic, which could result in assertion failures
(or, if assertions were compiled out, could result in segfaults). Fix
the oversight and add a test that would have caught one of these
problems.
While at it, also tighten an existing test for a non-recursive merge
to verify that it fails with appropriate status. Most merge tests in
the testsuite check either for success or conflicts; those testing for
neither are rare and it is good to ensure they support the invariant
assumed by builtin/merge.c in this comment:
/*
* The backend exits with 1 when conflicts are
* left to be resolved, with 2 when it does not
* handle the given merge at all.
*/
So, explicitly check for the exit status of 2 in these cases.
Reported-by: Matt Cree <[email protected]>
Signed-off-by: Elijah Newren <[email protected]>
handle_content_merge() returns an int. Every caller of handle_content_merge() expects an int. However, we declare a local variable 'clean' that we use for the return value to be unsigned. To make matters worse, we also assign 'clean' the return value of merge_submodule() in one codepath, which is defined to return an int. It seems that the only reason to have 'clean' be unsigned was to allow a cutesy bit manipulation operation to be well-defined. Fix the type of the 'clean' local in handle_content_merge(). Signed-off-by: Elijah Newren <[email protected]>
…odule The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 = conflicted, -1 = failure-to-determine), but we often like to think of it as binary (ignoring the possibility of a negative value) and use constructs like '!clean' to reflect this. However, these constructs can make codepaths more difficult to understand, unless we handle the negative case early and return pre-emptively; do that in handle_content_merge() to make the code a bit easier to read. Signed-off-by: Elijah Newren <[email protected]>
The comment above type_short_descriptions claimed that the order had to match what was found in the conflict_info_and_types enum. Since type_short_descriptions uses designated initializers, the order should not actually matter; I am guessing that positional initializers may have been under consideration when that comment was added, but the comment was not updated when designated initializers were chosen. Signed-off-by: Elijah Newren <[email protected]>
When something goes wrong enough that we need to abort early and not even attempt merging the remaining files, it probably does not make sense to report conflicts messages for the subset of files we processed before hitting the fatal error. Instead, only show the messages associated with paths where we hit the fatal error. Also, print these messages to stderr rather than stdout. Signed-off-by: Elijah Newren <[email protected]>
merge_submodule() stores errors using path_msg(), whereas other call sites make use of the error() function. This is inconsistent, and moving towards path_msg() seems more friendly for libification efforts since it will allow the caller to determine whether the error messages need to be printed. Note that this deferred handling of error messages changes the error message in a recursive merge from error: failed to execute internal merge to From inner merge: error: failed to execute internal merge which provides a little more information about the error which may be useful. Since the recursive merge strategy still only shows the older error, we had to adjust the new testcase introduced a few commits ago to just search for the older message somewhere in the output. Signed-off-by: Elijah Newren <[email protected]>
|
This patch series was integrated into seen via git@c3720a3. |
|
There was a status update in the "Cooking" section about the branch The "ort" merge backend saw one bugfix for a crash that happens when inner merge gets killed, and assorted code clean-ups. Waiting for comments or a reroll. source: <[email protected]> |
|
This patch series was integrated into seen via git@73e0dff. |
|
/submit |
|
Submitted as [email protected] To fetch this version into To fetch this version to local tag |
|
User |
|
This patch series was integrated into seen via git@2065d0d. |
|
This patch series was integrated into seen via git@998b73e. |
|
This patch series was integrated into seen via git@23f9407. |
|
This patch series was integrated into seen via git@46afa73. |
| @@ -5030,6 +5050,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt, | |||
| oid_to_hex(&side1->object.oid), | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[email protected]>
> > The calling convention for the merge machinery is
> One call to init_merge_options()
> One or more calls to merge_incore_[non]recursive()
> One call to merge_finalize()
> (possibly indirectly via merge_switch_to_result())
> Both merge_switch_to_result() and merge_finalize() expect
> opt->priv == NULL && result->priv != NULL
> which is supposed to be set up by our move_opt_priv_to_result_priv()
> function. However, two codepaths dealing with error cases did not
> execute this necessary logic, which could result in assertion failures
> (or, if assertions were compiled out, could result in segfaults). Fix
> the oversight and add a test that would have caught one of these
> problems.
"one of these problems" is key here.
> @@ -5050,6 +5050,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
> oid_to_hex(&side1->object.oid),
> oid_to_hex(&side2->object.oid));
> result->clean = -1;
> + move_opt_priv_to_result_priv(opt, result);
> return;
> } > trace2_region_leave("merge", "collect_merge_info", opt->repo);
Removing this line does not cause your test script to fail. This is
understandable as this case only happens during a parse failure, so
it would be unreasonable to generate a test case that only fails
for such a reason.
> @@ -5080,7 +5081,7 @@ static void merge_ort_nonrecursive_internal(struct merge_options *opt,
> /* existence of conflicted entries implies unclean */
> result->clean &= strmap_empty(&opt->priv->conflicted);
> }
> - if (!opt->priv->call_depth)
> + if (!opt->priv->call_depth || result->clean < 0)
> move_opt_priv_to_result_priv(opt, result);
> }
Removing this change does get caught by the new test.
Thanks,
-Stolee
|
User |
| @@ -2184,14 +2184,17 @@ static int handle_content_merge(struct merge_options *opt, | |||
| free(result_buf.ptr); | |||
| if (ret) | |||
| return -1; | |||
| clean &= (merge_status == 0); | |||
| if (merge_status > 0) | |||
| clean = 0; | |||
| path_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL, | |||
| _("Auto-merging %s"), path); | |||
| } else if (S_ISGITLINK(a->mode)) { | |||
| int two_way = ((S_IFMT & o->mode) != (S_IFMT & a->mode)); | |||
| clean = merge_submodule(opt, pathnames[0], | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[email protected]>
> > The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =
> conflicted, -1 = failure-to-determine), but we often like to think of
> it as binary (ignoring the possibility of a negative value) and use
> constructs like '!clean' to reflect this. However, these constructs
> can make codepaths more difficult to understand, unless we handle the
> negative case early and return pre-emptively; do that in
> handle_content_merge() to make the code a bit easier to read.
This patch is correct and valuable.
Would it be valuable to go a bit further and turn 'clean' into
an enum that reflects these states? Perhaps that would prevent
future changes from slipping into this mistake.
Thanks,
-StoleeThere was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Thu, Jun 27, 2024 at 7:12 PM Derrick Stolee <[email protected]> wrote:
>
> On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <[email protected]>
> >
> > The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =
> > conflicted, -1 = failure-to-determine), but we often like to think of
> > it as binary (ignoring the possibility of a negative value) and use
> > constructs like '!clean' to reflect this. However, these constructs
> > can make codepaths more difficult to understand, unless we handle the
> > negative case early and return pre-emptively; do that in
> > handle_content_merge() to make the code a bit easier to read.
>
> This patch is correct and valuable.
>
> Would it be valuable to go a bit further and turn 'clean' into
> an enum that reflects these states? Perhaps that would prevent
> future changes from slipping into this mistake.
That may make sense to investigate, but I suspect it may be a bigger
change and would recommend making such a clean up a separate series.
Also, I'm curious if it makes sense to finish off replacing recursive
with ort first; as long as recursive exists, it has the same problem
and in fact was the source of using a tri-state 'clean' variable and
thus would need the same cleanup. But if we replace recursive with
ort (making explicit requests for 'recursive' be handled by 'ort', as
originally designed and intended), that cuts the number of sites
needing this cleanup in half.There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 6/27/24 10:38 PM, Elijah Newren wrote:
> On Thu, Jun 27, 2024 at 7:12 PM Derrick Stolee <[email protected]> wrote:
>>
>> On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
>>> From: Elijah Newren <[email protected]>
>>>
>>> The 'clean' member variable is somewhat of a tri-state (1 = clean, 0 =
>>> conflicted, -1 = failure-to-determine), but we often like to think of
>>> it as binary (ignoring the possibility of a negative value) and use
>>> constructs like '!clean' to reflect this. However, these constructs
>>> can make codepaths more difficult to understand, unless we handle the
>>> negative case early and return pre-emptively; do that in
>>> handle_content_merge() to make the code a bit easier to read.
>>
>> This patch is correct and valuable.
>>
>> Would it be valuable to go a bit further and turn 'clean' into
>> an enum that reflects these states? Perhaps that would prevent
>> future changes from slipping into this mistake.
> > That may make sense to investigate, but I suspect it may be a bigger
> change and would recommend making such a clean up a separate series.
> > Also, I'm curious if it makes sense to finish off replacing recursive
> with ort first; as long as recursive exists, it has the same problem
> and in fact was the source of using a tri-state 'clean' variable and
> thus would need the same cleanup. But if we replace recursive with
> ort (making explicit requests for 'recursive' be handled by 'ort', as
> originally designed and intended), that cuts the number of sites
> needing this cleanup in half.
Your fast response to this message means that I didn't see this when
I mentioned it in my closing of the review (in response to your
cover letter).
Reducing the size of the conversion would definitely be good to do,
and then you could also consider using the existing ll_merge_result
enum, though it technically has four states.
Thanks,
-Stolee
| @@ -2109,7 +2109,7 @@ static int handle_content_merge(struct merge_options *opt, | |||
| * merges, which happens for example with rename/rename(2to1) and | |||
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 6/18/24 11:00 PM, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <[email protected]>
> > handle_content_merge() returns an int. Every caller of
> handle_content_merge() expects an int. However, we declare a local
> variable 'clean' that we use for the return value to be unsigned. To
> make matters worse, we also assign 'clean' the return value of
> merge_submodule() in one codepath, which is defined to return an int.
> It seems that the only reason to have 'clean' be unsigned was to allow a
> cutesy bit manipulation operation to be well-defined. Fix the type of
> the 'clean' local in handle_content_merge().
> @@ -2184,7 +2184,8 @@ static int handle_content_merge(struct merge_options *opt,
> free(result_buf.ptr);
> if (ret)
> return -1;
> - clean &= (merge_status == 0);
> + if (merge_status > 0)
> + clean = 0;
> path_msg(opt, INFO_AUTO_MERGING, 1, path, NULL, NULL, NULL,
> _("Auto-merging %s"), path);
> } else if (S_ISGITLINK(a->mode)) {
Even after this removal of this cute bitflip, there is one more
subtle use still in the code:
diff --git a/merge-ort.c b/merge-ort.c
index 8dfe80f1009..569014eef31 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -2629,7 +2629,8 @@ static char *check_for_directory_rename(struct merge_options *opt,
new_path = handle_path_level_conflicts(opt, path, side_index,
rename_info,
&collisions[side_index]);
- *clean_merge &= (new_path != NULL);
+ if (*clean_merge && !new_path)
+ *clean_merge = 0;
return new_path;
}
I had to think very carefully about this cleverness to be sure
that this conversion is right (and I'm only mostly sure). When
(new_path != NULL) is false, then we definitely set *clean_merge
to zero. Otherwise, we set it to 1 (but only when it was already
1 or -1). Technically, this does change the behavior by not
squashing -1 into 1, but that is less likely to be an existing
value of *clean_merge.
There are other uses of "clean &= collect_renames(...)" but that
appears to be fine because collect_renames() never results in an
abort state (returning -1).
Thanks,
-Stolee
|
On the Git mailing list, Derrick Stolee wrote (reply to this): 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/[email protected]/).
> 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 |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): Derrick Stolee <[email protected]> writes:
> 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. ...
> ... 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 for having done a thorough review, including exploration of
an alternative. Really appreciated.
|
|
This patch series was integrated into seen via git@5bf2b9b. |
|
This patch series was integrated into next via git@a85fe27. |
|
There was a status update in the "Cooking" section about the branch The "ort" merge backend saw one bugfix for a crash that happens when inner merge gets killed, and assorted code clean-ups. Will merge to 'master'. cf. <[email protected]> source: <[email protected]> |
|
This patch series was integrated into seen via git@f4b80da. |
|
There was a status update in the "Cooking" section about the branch The "ort" merge backend saw one bugfix for a crash that happens when inner merge gets killed, and assorted code clean-ups. Will merge to 'master'. cf. <[email protected]> source: <[email protected]> |
|
|
||
| /* | ||
| * Something is seriously wrong; cannot even perform merge; | ||
| * Keep this group _last_ other than NB_TOTAL_TYPES |
There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Wed, Jun 19, 2024 at 03:00:19AM +0000, Elijah Newren via GitGitGadget wrote:
> +static int read_oid_strbuf(struct merge_options *opt,
> + const struct object_id *oid,
> + struct strbuf *dst,
> + const char *path)
> {
> void *buf;
> enum object_type type;
> unsigned long size;
> buf = repo_read_object_file(the_repository, oid, &type, &size);
> - if (!buf)
> - return error(_("cannot read object %s"), oid_to_hex(oid));
> + if (!buf) {
> + path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
> + path, NULL, NULL, NULL,
> + _("error: cannot read object %s"), oid_to_hex(oid));
> + return -1;
> + }
> if (type != OBJ_BLOB) {
> free(buf);
> - return error(_("object %s is not a blob"), oid_to_hex(oid));
> + path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
> + path, NULL, NULL, NULL,
> + _("error: object %s is not a blob"), oid_to_hex(oid));
> }
> strbuf_attach(dst, buf, size, size + 1);
> return 0;
This loses the early return in the "type != OBJ_BLOB" code path. So we
free(buf), but then continue on to the strbuf_attach() call on the
dangling pointer. Should it "return -1" like the earlier conditional?
-PeffThere was a problem hiding this comment.
On the Git mailing list, Elijah Newren wrote (reply to this):
On Tue, Jul 2, 2024 at 2:33 PM Jeff King <[email protected]> wrote:
>
> On Wed, Jun 19, 2024 at 03:00:19AM +0000, Elijah Newren via GitGitGadget wrote:
>
> > +static int read_oid_strbuf(struct merge_options *opt,
> > + const struct object_id *oid,
> > + struct strbuf *dst,
> > + const char *path)
> > {
> > void *buf;
> > enum object_type type;
> > unsigned long size;
> > buf = repo_read_object_file(the_repository, oid, &type, &size);
> > - if (!buf)
> > - return error(_("cannot read object %s"), oid_to_hex(oid));
> > + if (!buf) {
> > + path_msg(opt, ERROR_OBJECT_READ_FAILED, 0,
> > + path, NULL, NULL, NULL,
> > + _("error: cannot read object %s"), oid_to_hex(oid));
> > + return -1;
> > + }
> > if (type != OBJ_BLOB) {
> > free(buf);
> > - return error(_("object %s is not a blob"), oid_to_hex(oid));
> > + path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
> > + path, NULL, NULL, NULL,
> > + _("error: object %s is not a blob"), oid_to_hex(oid));
> > }
> > strbuf_attach(dst, buf, size, size + 1);
> > return 0;
>
> This loses the early return in the "type != OBJ_BLOB" code path. So we
> free(buf), but then continue on to the strbuf_attach() call on the
> dangling pointer. Should it "return -1" like the earlier conditional?
>
> -Peff
Oops! That's embarrassing. Thanks for catching; I'll send in a patch
on top since this is already in next.There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Elijah Newren <[email protected]> writes:
> Oops! That's embarrassing. Thanks for catching; I'll send in a patch
> on top since this is already in next.
Thanks.There was a problem hiding this comment.
On the Git mailing list, Jeff King wrote (reply to this):
On Wed, Jul 03, 2024 at 08:48:59AM -0700, Elijah Newren wrote:
> > > if (type != OBJ_BLOB) {
> > > free(buf);
> > > - return error(_("object %s is not a blob"), oid_to_hex(oid));
> > > + path_msg(opt, ERROR_OBJECT_NOT_A_BLOB, 0,
> > > + path, NULL, NULL, NULL,
> > > + _("error: object %s is not a blob"), oid_to_hex(oid));
> > > }
> > > strbuf_attach(dst, buf, size, size + 1);
> > > return 0;
> >
> > This loses the early return in the "type != OBJ_BLOB" code path. So we
> > free(buf), but then continue on to the strbuf_attach() call on the
> > dangling pointer. Should it "return -1" like the earlier conditional?
>
> Oops! That's embarrassing. Thanks for catching; I'll send in a patch
> on top since this is already in next.
Yeah, I only caught it because Coverity flagged it when it hit 'next'.
It is quite subtle. :)
-Peff
|
User |
|
This patch series was integrated into seen via git@fb7bfc6. |
|
There was a status update in the "Cooking" section about the branch The "ort" merge backend saw one bugfix for a crash that happens when inner merge gets killed, and assorted code clean-ups. Will merge to 'next' and then to 'master'. source: <[email protected]> |
|
This patch series was integrated into seen via git@093f5ca. |
|
This patch series was integrated into seen via git@775c78a. |
|
This patch series was integrated into seen via git@a356df4. |
|
This patch series was integrated into seen via git@a7a4b67. |
|
This patch series was integrated into seen via git@ffc8f11. |
|
This patch series was integrated into master via git@ffc8f11. |
|
Closed via ffc8f11. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes since v1:
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/[email protected]/). However, while working on that, I found a few other issues around error codepaths in merge-ort. So this series:
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.
cc: Taylor Blau [email protected]
cc: Eric Sunshine [email protected]
cc: Elijah Newren [email protected]
cc: Derrick Stolee [email protected]
cc: Jeff King [email protected]