Fix and improve some error codepaths in merge-ort

[newren]

Changes since v1:

• Remove the conditional outside of move_opt_priv_to_result_priv() for patches 1 & 2
• Fix the consistency issues in patch 6 (refer to updated enum value in comment, use consistent prefix on error messages)
• Fix typo in patch 2
• Fix incorrect claim in commit message of patch 2 and provide more detail about which merges we are interested in checking for a very specific exit code.

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

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 me@ttaylorr.com cc: Eric Sunshine sunshine@sunshineco.com cc: Elijah Newren newren@gmail.com cc: Derrick Stolee stolee@gmail.com cc: Jeff King peff@peff.net

[gitgitgadget[bot]]

There are issues in commit ef9b1a69c0d534fc9e28b375cf0da72741d7ef5f: merge-ort: Upon merge abort, only show messages causing the abort Prefixed commit message must be in lower case

[newren]

/submit

[gitgitgadget[bot]]

Submitted as pull.1748.git.1718310307.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1748/newren/fix-error-cases-v1

To fetch this version to local tag pr-1748/newren/fix-error-cases-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1748/newren/fix-error-cases-v1

[gitgitgadget[bot]]

User Taylor Blau <me@ttaylorr.com> has been added to the cc: list.

[gitgitgadget[bot]]

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

[gitgitgadget[bot]]

This branch is now known as en/ort-inner-merge-error-fix.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/ff97f26fd56602a9b8d3ce02af58aa3d44ca8ba0.

[gitgitgadget[bot]]

User Eric Sunshine <sunshine@sunshineco.com> has been added to the cc: list.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/95a77a602c2bf929f2c31c7b4fffd26371195d92.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/499db5bace103d05d532b069113845f894fd4fde.

[gitgitgadget[bot]]

There was a status update in the "New Topics" section about the branch en/ort-inner-merge-error-fix on the Git mailing list:

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: <pull.1748.git.1718310307.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/c3720a3480fe01b13c49f68c1d4d55c0bd45dc40.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch en/ort-inner-merge-error-fix on the Git mailing list:

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: <pull.1748.git.1718310307.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/73e0dff5847ef3e0cab42ebc7fc12916ee77a773.

[newren]

/submit

[gitgitgadget[bot]]

Submitted as pull.1748.v2.git.1718766019.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1748/newren/fix-error-cases-v2

To fetch this version to local tag pr-1748/newren/fix-error-cases-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1748/newren/fix-error-cases-v2

[gitgitgadget[bot]]

User Elijah Newren <newren@gmail.com> has been added to the cc: list.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/7bea088bbed1b75daef71cc364a9565ffd921468.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch en/ort-inner-merge-error-fix on the Git mailing list:

The "ort" merge backend saw one bugfix for a crash that happens
when inner merge gets killed, and assorted code clean-ups.

Needs review.
source: <pull.1748.v2.git.1718766019.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/7e0b1adb28ae52934df767c9cd7b720eeea34e08.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/a1cfee0d725503b26ec5ebdd3fdeac91e24714ad.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch en/ort-inner-merge-error-fix on the Git mailing list:

The "ort" merge backend saw one bugfix for a crash that happens
when inner merge gets killed, and assorted code clean-ups.

Needs review.
source: <pull.1748.v2.git.1718766019.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/2065d0d739800bcaa2297098e82242fe81f568be.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/998b73e950d406c96fd80c3da7aa8ef9465c9a2c.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/23f94076a238f10d26abdfc06ea68e41aa69a3d0.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/46afa73517973d93890b57dce30efd9ba04a4412.

[gitgitgadget[bot]]

User Derrick Stolee <stolee@gmail.com> has been added to the cc: list.

[gitgitgadget[bot]]

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

[gitgitgadget[bot]]

On the Git mailing list, Junio C Hamano wrote (reply to this):

Derrick Stolee <stolee@gmail.com> 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.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/5bf2b9b83271943f1e33e659ccfd4858348f1cab.

[gitgitgadget[bot]]

This patch series was integrated into next via https://github.com/git/git/commit/a85fe270e6d64f2eb281a0440b605506d80ad119.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch en/ort-inner-merge-error-fix on the Git mailing list:

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. <3f1e155f-f559-42ac-9454-8ddcf7873f48@gmail.com>
source: <pull.1748.v2.git.1718766019.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/f4b80da2adac03d2fff95b87b63193d5efbef2ed.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch en/ort-inner-merge-error-fix on the Git mailing list:

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. <3f1e155f-f559-42ac-9454-8ddcf7873f48@gmail.com>
source: <pull.1748.v2.git.1718766019.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

User Jeff King <peff@peff.net> has been added to the cc: list.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/fb7bfc645a73e7415c6657c615b002c43882c2ae.

[gitgitgadget[bot]]

There was a status update in the "Cooking" section about the branch en/ort-inner-merge-error-fix on the Git mailing list:

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: <pull.1748.v2.git.1718766019.gitgitgadget@gmail.com>

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/093f5caed748b1a42e4e61c3ee44fcb8986ef59e.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/775c78a6449eae42d0fbdc448d25c8455fe2f747.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/a356df4150eec8f98415c321225162ce6518098f.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/a7a4b673f7c56e7517b5b262674e9a532365772f.

[gitgitgadget[bot]]

This patch series was integrated into seen via https://github.com/git/git/commit/ffc8f1142c9ecb6f83cffe4c33c6baacf6d38026.

[gitgitgadget[bot]]

This patch series was integrated into master via https://github.com/git/git/commit/ffc8f1142c9ecb6f83cffe4c33c6baacf6d38026.

[gitgitgadget[bot]]

Closed via ffc8f1142c9ecb6f83cffe4c33c6baacf6d38026.