The merge-base logic vs missing commit objects (follow-up)

Jeff King reported that Coverity pointed out a problem in the patch series "The merge-base logic vs missing commit objects" (which made it into the next branch already): The return value of merge_submodules() is assigned to an unsigned, single-bit variable, which as a consequence is not able to hold a negative value indicating a non-recoverable error.

I looked into this issue and am happy to report that there are no other instances of the same issue in that patch series. The first patch in this here patch series addresses that issue.

While looking into this issue I also noticed that the merge_submodule() function did not even return negative values! This was an oversight on my part (which I attribute with a large amount of self-compassion to my utter lack of enthusiasm for submodules as a Git feature), and the second patch in this here patch series addresses that.

This is a follow-up for
https://lore.kernel.org/git/[email protected]/,
based on the js/merge-base-with-missing-commit branch.

Jeff King reported that Coverity pointed out a problem in the patch series "The merge-base logic vs missing commit objects" (which made it into the next branch already): The return value of merge_submodules() is assigned to an unsigned, single-bit variable, which as a consequence is not able to hold a negative value indicating a non-recoverable error.

I looked into this issue and am happy to report that there are no other instances of the same issue in that patch series. The first patch in this here patch series addresses that issue.

While looking into this issue I also noticed that the merge_submodule() function did not even return negative values! This was an oversight on my part (which I attribute with a large amount of self-compassion to my utter lack of enthusiasm for submodules as a Git feature), and the second patch in this here patch series addresses that.

This is a follow-up for
https://lore.kernel.org/git/[email protected]/,
based on the js/merge-base-with-missing-commit branch.

Johannes Schindelin (2):
merge-recursive: prepare for merge_submodule() to report errors merge-ort/merge-recursive: do report errors in merge_submodule()

 merge-ort.c       |  5 +++++
 merge-recursive.c | 21 +++++++++++++++------
 2 files changed, 20 insertions(+), 6 deletions(-)

base-commit: caaf1a2942c25c1f1a15818b718c9f641e52beef
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1686%2Fdscho%2Fmerge-base-and-missing-objects-followup-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1686/dscho/merge-base-and-missing-objects-followup-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1686

From: Johannes Schindelin <[email protected]>

In 24876ebf68b (commit-reach(repo_in_merge_bases_many): report missing commits, 2024-02-28), I taught merge_submodule() to handle errors reported by repo_in_merge_bases_many().

However, those errors were not passed through to the callers. That was unintentional, and this commit remedies that.

Note that find_first_merges() can now also return -1 (because it passes through that return value from repo_in_merge_bases()), and this commit also adds the forgotten handling for that scenario.

Signed-off-by Johannes Schindelin <[email protected]>

---
 merge-ort.c       | 5 +++++
 merge-recursive.c | 8 ++++++++
 2 files changed, 13 insertions(+)

merge-ort.c +5 -0

index 033c4348e2d..5d36c04f509 100644
@@ -1819,6 +1819,7 @@ static int merge_submodule(struct merge_options *opt,
 			 _("Failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
+		ret = -1;
 		goto cleanup;
 	}
 	if (ret2 > 0)

@@ -1829,6 +1830,7 @@ static int merge_submodule(struct merge_options *opt,
 			 _("Failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
+		ret = -1;
 		goto cleanup;
 	}
 	if (!ret2) {

@@ -1848,6 +1850,7 @@ static int merge_submodule(struct merge_options *opt,
 			 _("Failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
+		ret = -1;
 		goto cleanup;
 	}
 	if (ret2 > 0) {

@@ -1866,6 +1869,7 @@ static int merge_submodule(struct merge_options *opt,
 			 _("Failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
+		ret = -1;
 		goto cleanup;
 	}
 	if (ret2 > 0) {

@@ -1899,6 +1903,7 @@ static int merge_submodule(struct merge_options *opt,
 			 _("Failed to merge submodule %s "
 			   "(repository corrupt)"),
 			 path);
+		ret = -1;
 		break;
 	case 0:
 		path_msg(opt, CONFLICT_SUBMODULE_FAILED_TO_MERGE, 0,

merge-recursive.c +8 -0

index f3132a9ecae..fc772c2b113 100644
@@ -1246,12 +1246,14 @@ static int merge_submodule(struct merge_options *opt,
 	ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_a);
 	if (ret2 < 0) {
 		output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+		ret = -1;
 		goto cleanup;
 	}
 	if (ret2 > 0)
 		ret2 = repo_in_merge_bases(&subrepo, commit_base, commit_b);
 	if (ret2 < 0) {
 		output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+		ret = -1;
 		goto cleanup;
 	}
 	if (!ret2) {

@@ -1263,6 +1265,7 @@ static int merge_submodule(struct merge_options *opt,
 	ret2 = repo_in_merge_bases(&subrepo, commit_a, commit_b);
 	if (ret2 < 0) {
 		output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+		ret = -1;
 		goto cleanup;
 	}
 	if (ret2) {

@@ -1281,6 +1284,7 @@ static int merge_submodule(struct merge_options *opt,
 	ret2 = repo_in_merge_bases(&subrepo, commit_b, commit_a);
 	if (ret2 < 0) {
 		output(opt, 1, _("Failed to merge submodule %s (repository corrupt)"), path);
+		ret = -1;
 		goto cleanup;
 	}
 	if (ret2) {

@@ -1312,6 +1316,10 @@ static int merge_submodule(struct merge_options *opt,
 	parent_count = find_first_merges(&subrepo, &merges, path,
 					 commit_a, commit_b);
 	switch (parent_count) {
+	case -1:
+		output(opt, 1,_("Failed to merge submodule %s (repository corrupt)"), path);
+		ret = -1;
+		break;
 	case 0:
 		output(opt, 1, _("Failed to merge submodule %s (merge following commits not found)"), path);
 		break;

Quoted 3 lines

This is a follow-up for
https://lore.kernel.org/git/[email protected]/,
based on the js/merge-base-with-missing-commit branch.

This series looks good to me; thanks.

"Johannes Schindelin via GitGitGadget" <[email protected]>

Quoted 6 lines · writes:

From: Johannes Schindelin <[email protected]>

In 24876ebf68b (commit-reach(repo_in_merge_bases_many): report missing commits, 2024-02-28), I taught merge_submodule() to handle errors reported by repo_in_merge_bases_many().

Quoted 6 lines

However, those errors were not passed through to the callers. That was unintentional, and this commit remedies that.

Note that find_first_merges() can now also return -1 (because it passes through that return value from repo_in_merge_bases()), and this commit also adds the forgotten handling for that scenario.

Good clean-up. But this "oops, we did not check for errors" makes me wonder if we are better off adopting "by default we assume an error, until we are sure we are good" pattern, i.e.

        func()
        {
                int ret = -1; /* assume worst */

                do stuff;
                if (...) {
                        error(_("this is bad"));
                        goto cleanup;
                }
                do stuff;
                if (...) {
                        error(_("this is bad, too"));
                        goto cleanup;
                }

                /* ok we are happy */
                ret = 0;

        cleanup:
                release resources;
                return ret;
        }

The patch to both functions do make it appear that they are good candidates for application of the pattern to me.

By my counting, 5 of the handled cases result in ret = -1, i.e. non-recoverable errors, but 8 of the cases result in ret = 0, i.e. unclean merges, whereas only 2 cases return 1, i.e. clean merges.