{"thread":{"id":"61078","subject":"[PATCH 0/2] The merge-base logic vs missing commit objects (follow-up)","startedAt":"2024-03-09T14:10:01Z","lastAt":"2024-03-09T23:18:53Z","messageCount":9,"participants":["Johannes Schindelin via GitGitGadget","Elijah Newren","Junio C Hamano","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"490293","messageId":"pull.1686.git.1709993397.gitgitgadget@gmail.com","threadId":"61078","inReplyTo":null,"subject":"[PATCH 0/2] The merge-base logic vs missing commit objects (follow-up)","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-09T14:09:55Z","receivedAt":"2024-03-09T14:10:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Jeff King reported that Coverity pointed out a problem in the patch series\n\"The merge-base logic vs missing commit objects\" (which made it into the\nnext branch already): The return value of merge_submodules() is assigned to\nan unsigned, single-bit variable, which as a consequence is not able to hold\na negative value indicating a non-recoverable error.\n\nI looked into this issue and am happy to report that there are no other\ninstances of the same issue in that patch series. The first patch in this\nhere patch series addresses that issue.\n\nWhile looking into this issue I also noticed that the merge_submodule()\nfunction did not even return negative values! This was an oversight on my\npart (which I attribute with a large amount of self-compassion to my utter\nlack of enthusiasm for submodules as a Git feature), and the second patch in\nthis here patch series addresses that.\n\nThis is a follow-up for\nhttps://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,\nbased on the js/merge-base-with-missing-commit branch.\n\nJohannes Schindelin (2):\n merge-recursive: prepare for `merge_submodule()` to report errors\n merge-ort/merge-recursive: do report errors in `merge_submodule()`\n\n merge-ort.c | 5 +++++\n merge-recursive.c | 21 +++++++++++++++------\n 2 files changed, 20 insertions(+), 6 deletions(-)\n\n\nbase-commit: caaf1a2942c25c1f1a15818b718c9f641e52beef\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1686%2Fdscho%2Fmerge-base-and-missing-objects-followup-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1686/dscho/merge-base-and-missing-objects-followup-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1686\n-- \ngitgitgadget\n"},{"id":"490294","messageId":"533df4c6659439d0a241a8b2e329c88d475d4136.1709993397.git.gitgitgadget@gmail.com","threadId":"61078","inReplyTo":"pull.1686.git.1709993397.gitgitgadget@gmail.com","subject":"[PATCH 1/2] merge-recursive: prepare for `merge_submodule()` to report errors","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-09T14:09:56Z","receivedAt":"2024-03-09T14:10:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe `merge_submodule()` function returns an integer that indicates\nwhether the merge was clean (returning 1) or unclean (returning 0).\n\nLike the version in `merge-ort.c`, the version in `merge-recursive.c`\ndoes not report any errors (such as repository corruption) by returning\n-1 as of time of writing, even if the callers in `merge-ort.c` are\nprepared for exactly such errors.\n\nHowever, we want to teach (both variants of) the `merge_submodule()`\nfunction that trick: to report errors by returning -1. Therefore,\nprepare the caller in `merge-recursive.c` to handle that scenario.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-recursive.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 32e9d6665de..f3132a9ecae 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1426,13 +1426,14 @@ static int merge_mode_and_contents(struct merge_options *opt,\n \t\t\t/* FIXME: bug, what if modes didn't match? */\n \t\t\tresult->clean = (merge_status == 0);\n \t\t} else if (S_ISGITLINK(a->mode)) {\n-\t\t\tresult->clean = merge_submodule(opt, &result->blob.oid,\n-\t\t\t\t\t\t\to->path,\n-\t\t\t\t\t\t\t&o->oid,\n-\t\t\t\t\t\t\t&a->oid,\n-\t\t\t\t\t\t\t&b->oid);\n-\t\t\tif (result->clean < 0)\n+\t\t\tint clean = merge_submodule(opt, &result->blob.oid,\n+\t\t\t\t\t\t o->path,\n+\t\t\t\t\t\t &o->oid,\n+\t\t\t\t\t\t &a->oid,\n+\t\t\t\t\t\t &b->oid);\n+\t\t\tif (clean < 0)\n \t\t\t\treturn -1;\n+\t\t\tresult->clean = clean;\n \t\t} else if (S_ISLNK(a->mode)) {\n \t\t\tswitch (opt->recursive_variant) {\n \t\t\tcase MERGE_VARIANT_NORMAL:\n-- \ngitgitgadget\n\n"},{"id":"490295","messageId":"50fe1a26515c06afec5ac7fb723727e1365a14fc.1709993397.git.gitgitgadget@gmail.com","threadId":"61078","inReplyTo":"pull.1686.git.1709993397.gitgitgadget@gmail.com","subject":"[PATCH 2/2] merge-ort/merge-recursive: do report errors in `merge_submodule()`","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-09T14:09:57Z","receivedAt":"2024-03-09T14:10:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn 24876ebf68b (commit-reach(repo_in_merge_bases_many): report missing\ncommits, 2024-02-28), I taught `merge_submodule()` to handle errors\nreported by `repo_in_merge_bases_many()`.\n\nHowever, those errors were not passed through to the callers. That was\nunintentional, and this commit remedies that.\n\nNote that `find_first_merges()` can now also return -1 (because it\npasses through that return value from `repo_in_merge_bases()`), and this\ncommit also adds the forgotten handling for that scenario.\n\nSigned-off-by: Johannes Schindelin \n---\n merge-ort.c | 5 +++++\n merge-recursive.c | 8 ++++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/merge-ort.c b/merge-ort.c\nindex 033c4348e2d..5d36c04f509 100644\n--- a/merge-ort.c\n+++ b/merge-ort.c\n@@ -1819,6 +1819,7 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t _(\"Failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \tif (ret2 > 0)\n@@ -1829,6 +1830,7 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t _(\"Failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \tif (!ret2) {\n@@ -1848,6 +1850,7 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t _(\"Failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \tif (ret2 > 0) {\n@@ -1866,6 +1869,7 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t _(\"Failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \tif (ret2 > 0) {\n@@ -1899,6 +1903,7 @@ static int merge_submodule(struct merge_options *opt,\n \t\t\t _(\"Failed to merge submodule %s \"\n \t\t\t \"(repository corrupt)\"),\n \t\t\t path);\n+\t\tret = -1;\n \t\tbreak;\n \tcase 0:\n \t\tpath_msg(opt, CONFLICT_SUBMODULE_FAILED_TO_MERGE, 0,\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex f3132a9ecae..fc772c2b113 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1246,12 +1246,14 @@ static int merge_submodule(struct merge_options *opt,\n \tret2 = repo_in_merge_bases(&subrepo, commit_base, commit_a);\n \tif (ret2 < 0) {\n \t\toutput(opt, 1, _(\"Failed to merge submodule %s (repository corrupt)\"), path);\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \tif (ret2 > 0)\n \t\tret2 = repo_in_merge_bases(&subrepo, commit_base, commit_b);\n \tif (ret2 < 0) {\n \t\toutput(opt, 1, _(\"Failed to merge submodule %s (repository corrupt)\"), path);\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \tif (!ret2) {\n@@ -1263,6 +1265,7 @@ static int merge_submodule(struct merge_options *opt,\n \tret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);\n \tif (ret2 < 0) {\n \t\toutput(opt, 1, _(\"Failed to merge submodule %s (repository corrupt)\"), path);\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \tif (ret2) {\n@@ -1281,6 +1284,7 @@ static int merge_submodule(struct merge_options *opt,\n \tret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);\n \tif (ret2 < 0) {\n \t\toutput(opt, 1, _(\"Failed to merge submodule %s (repository corrupt)\"), path);\n+\t\tret = -1;\n \t\tgoto cleanup;\n \t}\n \tif (ret2) {\n@@ -1312,6 +1316,10 @@ static int merge_submodule(struct merge_options *opt,\n \tparent_count = find_first_merges(&subrepo, &merges, path,\n \t\t\t\t\t commit_a, commit_b);\n \tswitch (parent_count) {\n+\tcase -1:\n+\t\toutput(opt, 1,_(\"Failed to merge submodule %s (repository corrupt)\"), path);\n+\t\tret = -1;\n+\t\tbreak;\n \tcase 0:\n \t\toutput(opt, 1, _(\"Failed to merge submodule %s (merge following commits not found)\"), path);\n \t\tbreak;\n-- \ngitgitgadget\n"},{"id":"490296","messageId":"CABPp-BFKciBPN0WAaGaK4tb8hXit22Up4LMoJNxo-+DqEspD+A@mail.gmail.com","threadId":"61078","inReplyTo":"pull.1686.git.1709993397.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] The merge-base logic vs missing commit objects (follow-up)","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-03-09T16:51:48Z","receivedAt":"2024-03-09T16:52:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Mar 9, 2024 at 6:10 AM Johannes Schindelin via GitGitGadget\n wrote:\n>\n> Jeff King reported that Coverity pointed out a problem in the patch series\n> \"The merge-base logic vs missing commit objects\" (which made it into the\n> next branch already): The return value of merge_submodules() is assigned to\n> an unsigned, single-bit variable, which as a consequence is not able to hold\n> a negative value indicating a non-recoverable error.\n>\n> I looked into this issue and am happy to report that there are no other\n> instances of the same issue in that patch series. The first patch in this\n> here patch series addresses that issue.\n>\n> While looking into this issue I also noticed that the merge_submodule()\n> function did not even return negative values! This was an oversight on my\n> part (which I attribute with a large amount of self-compassion to my utter\n> lack of enthusiasm for submodules as a Git feature), and the second patch in\n> this here patch series addresses that.\n>\n> This is a follow-up for\n> https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,\n> based on the js/merge-base-with-missing-commit branch.\n\nThis series looks good to me; thanks.\n"},{"id":"490299","messageId":"xmqq4jdfe1ur.fsf@gitster.g","threadId":"61078","inReplyTo":"pull.1686.git.1709993397.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] The merge-base logic vs missing commit objects (follow-up)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-09T17:45:00Z","receivedAt":"2024-03-09T17:45:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> Jeff King reported that Coverity pointed out a problem in the patch series\n> \"The merge-base logic vs missing commit objects\" (which made it into the\n> next branch already): The return value of merge_submodules() is assigned to\n> an unsigned, single-bit variable, which as a consequence is not able to hold\n> a negative value indicating a non-recoverable error.\n>\n> I looked into this issue and am happy to report that there are no other\n> instances of the same issue in that patch series. The first patch in this\n> here patch series addresses that issue.\n>\n> While looking into this issue I also noticed that the merge_submodule()\n> function did not even return negative values! This was an oversight on my\n> part (which I attribute with a large amount of self-compassion to my utter\n> lack of enthusiasm for submodules as a Git feature), and the second patch in\n> this here patch series addresses that.\n>\n> This is a follow-up for\n> https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,\n> based on the js/merge-base-with-missing-commit branch.\n\nThanks.\n\n>\n> Johannes Schindelin (2):\n> merge-recursive: prepare for `merge_submodule()` to report errors\n> merge-ort/merge-recursive: do report errors in `merge_submodule()`\n>\n> merge-ort.c | 5 +++++\n> merge-recursive.c | 21 +++++++++++++++------\n> 2 files changed, 20 insertions(+), 6 deletions(-)\n>\n>\n> base-commit: caaf1a2942c25c1f1a15818b718c9f641e52beef\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1686%2Fdscho%2Fmerge-base-and-missing-objects-followup-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1686/dscho/merge-base-and-missing-objects-followup-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1686\n"},{"id":"490301","messageId":"xmqqwmqbcmrz.fsf@gitster.g","threadId":"61078","inReplyTo":"50fe1a26515c06afec5ac7fb723727e1365a14fc.1709993397.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] merge-ort/merge-recursive: do report errors in `merge_submodule()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-09T17:56:00Z","receivedAt":"2024-03-09T17:56:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> In 24876ebf68b (commit-reach(repo_in_merge_bases_many): report missing\n> commits, 2024-02-28), I taught `merge_submodule()` to handle errors\n> reported by `repo_in_merge_bases_many()`.\n>\n> However, those errors were not passed through to the callers. That was\n> unintentional, and this commit remedies that.\n>\n> Note that `find_first_merges()` can now also return -1 (because it\n> passes through that return value from `repo_in_merge_bases()`), and this\n> commit also adds the forgotten handling for that scenario.\n\nGood clean-up. But this \"oops, we did not check for errors\" makes\nme wonder if we are better off adopting \"by default we assume an\nerror, until we are sure we are good\" pattern, i.e.\n\n func()\n {\n int ret = -1; /* assume worst */\n\n do stuff;\n if (...) {\n error(_(\"this is bad\"));\n goto cleanup;\n }\n do stuff;\n if (...) {\n error(_(\"this is bad, too\"));\n goto cleanup;\n }\n\n /* ok we are happy */\n ret = 0;\n\n cleanup:\n release resources;\n return ret;\n }\n\nThe patch to both functions do make it appear that they are good\ncandidates for application of the pattern to me.\n\nThanks.\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> merge-ort.c | 5 +++++\n> merge-recursive.c | 8 ++++++++\n> 2 files changed, 13 insertions(+)\n>\n> diff --git a/merge-ort.c b/merge-ort.c\n> index 033c4348e2d..5d36c04f509 100644\n> --- a/merge-ort.c\n> +++ b/merge-ort.c\n> @@ -1819,6 +1819,7 @@ static int merge_submodule(struct merge_options *opt,\n> \t\t\t _(\"Failed to merge submodule %s \"\n> \t\t\t \"(repository corrupt)\"),\n> \t\t\t path);\n> +\t\tret = -1;\n> \t\tgoto cleanup;\n> \t}\n> \tif (ret2 > 0)\n> @@ -1829,6 +1830,7 @@ static int merge_submodule(struct merge_options *opt,\n> \t\t\t _(\"Failed to merge submodule %s \"\n> \t\t\t \"(repository corrupt)\"),\n> \t\t\t path);\n> +\t\tret = -1;\n> \t\tgoto cleanup;\n> \t}\n> \tif (!ret2) {\n> @@ -1848,6 +1850,7 @@ static int merge_submodule(struct merge_options *opt,\n> \t\t\t _(\"Failed to merge submodule %s \"\n> \t\t\t \"(repository corrupt)\"),\n> \t\t\t path);\n> +\t\tret = -1;\n> \t\tgoto cleanup;\n> \t}\n> \tif (ret2 > 0) {\n> @@ -1866,6 +1869,7 @@ static int merge_submodule(struct merge_options *opt,\n> \t\t\t _(\"Failed to merge submodule %s \"\n> \t\t\t \"(repository corrupt)\"),\n> \t\t\t path);\n> +\t\tret = -1;\n> \t\tgoto cleanup;\n> \t}\n> \tif (ret2 > 0) {\n> @@ -1899,6 +1903,7 @@ static int merge_submodule(struct merge_options *opt,\n> \t\t\t _(\"Failed to merge submodule %s \"\n> \t\t\t \"(repository corrupt)\"),\n> \t\t\t path);\n> +\t\tret = -1;\n> \t\tbreak;\n> \tcase 0:\n> \t\tpath_msg(opt, CONFLICT_SUBMODULE_FAILED_TO_MERGE, 0,\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index f3132a9ecae..fc772c2b113 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1246,12 +1246,14 @@ static int merge_submodule(struct merge_options *opt,\n> \tret2 = repo_in_merge_bases(&subrepo, commit_base, commit_a);\n> \tif (ret2 < 0) {\n> \t\toutput(opt, 1, _(\"Failed to merge submodule %s (repository corrupt)\"), path);\n> +\t\tret = -1;\n> \t\tgoto cleanup;\n> \t}\n> \tif (ret2 > 0)\n> \t\tret2 = repo_in_merge_bases(&subrepo, commit_base, commit_b);\n> \tif (ret2 < 0) {\n> \t\toutput(opt, 1, _(\"Failed to merge submodule %s (repository corrupt)\"), path);\n> +\t\tret = -1;\n> \t\tgoto cleanup;\n> \t}\n> \tif (!ret2) {\n> @@ -1263,6 +1265,7 @@ static int merge_submodule(struct merge_options *opt,\n> \tret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);\n> \tif (ret2 < 0) {\n> \t\toutput(opt, 1, _(\"Failed to merge submodule %s (repository corrupt)\"), path);\n> +\t\tret = -1;\n> \t\tgoto cleanup;\n> \t}\n> \tif (ret2) {\n> @@ -1281,6 +1284,7 @@ static int merge_submodule(struct merge_options *opt,\n> \tret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);\n> \tif (ret2 < 0) {\n> \t\toutput(opt, 1, _(\"Failed to merge submodule %s (repository corrupt)\"), path);\n> +\t\tret = -1;\n> \t\tgoto cleanup;\n> \t}\n> \tif (ret2) {\n> @@ -1312,6 +1316,10 @@ static int merge_submodule(struct merge_options *opt,\n> \tparent_count = find_first_merges(&subrepo, &merges, path,\n> \t\t\t\t\t commit_a, commit_b);\n> \tswitch (parent_count) {\n> +\tcase -1:\n> +\t\toutput(opt, 1,_(\"Failed to merge submodule %s (repository corrupt)\"), path);\n> +\t\tret = -1;\n> +\t\tbreak;\n> \tcase 0:\n> \t\toutput(opt, 1, _(\"Failed to merge submodule %s (merge following commits not found)\"), path);\n> \t\tbreak;\n"},{"id":"490302","messageId":"xmqqsf0zcmqg.fsf@gitster.g","threadId":"61078","inReplyTo":"CABPp-BFKciBPN0WAaGaK4tb8hXit22Up4LMoJNxo-+DqEspD+A@mail.gmail.com","subject":"Re: [PATCH 0/2] The merge-base logic vs missing commit objects (follow-up)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-09T17:56:55Z","receivedAt":"2024-03-09T17:57:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n>> This is a follow-up for\n>> https://lore.kernel.org/git/pull.1657.v4.git.1709113457.gitgitgadget@gmail.com/,\n>> based on the js/merge-base-with-missing-commit branch.\n>\n> This series looks good to me; thanks.\n\nThanks, both.\n"},{"id":"490310","messageId":"a0af1155-59d9-0c35-f3e9-c0fe1ba0e11b@gmx.de","threadId":"61078","inReplyTo":"xmqqwmqbcmrz.fsf@gitster.g","subject":"Re: [PATCH 2/2] merge-ort/merge-recursive: do report errors in `merge_submodule()`","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2024-03-09T20:46:04Z","receivedAt":"2024-03-09T20:46:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sat, 9 Mar 2024, Junio C Hamano wrote:\n\n> \"Johannes Schindelin via GitGitGadget\" \n> writes:\n>\n> > From: Johannes Schindelin \n> >\n> > In 24876ebf68b (commit-reach(repo_in_merge_bases_many): report missing\n> > commits, 2024-02-28), I taught `merge_submodule()` to handle errors\n> > reported by `repo_in_merge_bases_many()`.\n> >\n> > However, those errors were not passed through to the callers. That was\n> > unintentional, and this commit remedies that.\n> >\n> > Note that `find_first_merges()` can now also return -1 (because it\n> > passes through that return value from `repo_in_merge_bases()`), and this\n> > commit also adds the forgotten handling for that scenario.\n>\n> Good clean-up. But this \"oops, we did not check for errors\" makes\n> me wonder if we are better off adopting \"by default we assume an\n> error, until we are sure we are good\" pattern, i.e.\n>\n> func()\n> {\n> int ret = -1; /* assume worst */\n>\n> do stuff;\n> if (...) {\n> error(_(\"this is bad\"));\n> goto cleanup;\n> }\n> do stuff;\n> if (...) {\n> error(_(\"this is bad, too\"));\n> goto cleanup;\n> }\n>\n> /* ok we are happy */\n> ret = 0;\n>\n> cleanup:\n> release resources;\n> return ret;\n> }\n>\n> The patch to both functions do make it appear that they are good\n> candidates for application of the pattern to me.\n\nThis is a very fitting pattern here, and it is in fact used already! It is\nused with `ret = 0`, though, to indicate unclean merges.\n\nThis makes sense, as most of the specifically-handled cases have that\noutcome. By my counting, 5 of the handled cases result in ret = -1, i.e.\nnon-recoverable errors, but 8 of the cases result in ret = 0, i.e. unclean\nmerges, whereas only 2 cases return 1, i.e. clean merges.\n\nThose numbers are for the `merge_submodule()` variant in `merge-ort.c`. In\n`merge-recursive.c`, by my counting there are 10 instead of 8 `ret = 0`\ncases, the other two numbers are the same.\n\nCiao,\nJohannes\n"},{"id":"490312","messageId":"xmqqle6r0zaj.fsf@gitster.g","threadId":"61078","inReplyTo":"a0af1155-59d9-0c35-f3e9-c0fe1ba0e11b@gmx.de","subject":"Re: [PATCH 2/2] merge-ort/merge-recursive: do report errors in `merge_submodule()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-09T23:18:44Z","receivedAt":"2024-03-09T23:18:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> By my counting, 5 of the handled cases result in ret = -1, i.e.\n> non-recoverable errors, but 8 of the cases result in ret = 0,\n> i.e. unclean merges, whereas only 2 cases return 1, i.e. clean\n> merges.\n\nSounds good. Thanks.\n"}]}