| Message ID | pull.1748.git.1718310307.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix and improve some error codepaths in merge-ort | expand |
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 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. 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(-) base-commit: 8d94cfb54504f2ec9edc7ca3eb5c29a3dd3675ae Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1748%2Fnewren%2Ffix-error-cases-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1748/newren/fix-error-cases-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1748