| Message ID | 50fe1a26515c06afec5ac7fb723727e1365a14fc.1709993397.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 25fd20eb44cfcbb0652595de2144f0e077a957ec |
| Headers | show |
| Series | The merge-base logic vs missing commit objects (follow-up) | expand |
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > 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. 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. Thanks. > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > merge-ort.c | 5 +++++ > merge-recursive.c | 8 ++++++++ > 2 files changed, 13 insertions(+) > > diff --git a/merge-ort.c b/merge-ort.c > index 033c4348e2d..5d36c04f509 100644 > --- a/merge-ort.c > +++ b/merge-ort.c > @@ -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, > diff --git a/merge-recursive.c b/merge-recursive.c > index f3132a9ecae..fc772c2b113 100644 > --- a/merge-recursive.c > +++ b/merge-recursive.c > @@ -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;
Hi Junio, On Sat, 9 Mar 2024, Junio C Hamano wrote: > "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> > writes: > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > 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. > > 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. This is a very fitting pattern here, and it is in fact used already! It is used with `ret = 0`, though, to indicate unclean merges. This makes sense, as most of the specifically-handled cases have that outcome. 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. Those numbers are for the `merge_submodule()` variant in `merge-ort.c`. In `merge-recursive.c`, by my counting there are 10 instead of 8 `ret = 0` cases, the other two numbers are the same. Ciao, Johannes
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > 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. Sounds good. Thanks.
diff --git a/merge-ort.c b/merge-ort.c index 033c4348e2d..5d36c04f509 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -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, diff --git a/merge-recursive.c b/merge-recursive.c index f3132a9ecae..fc772c2b113 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -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;