Hi Junio,
On Sat, 9 Mar 2024, Junio C Hamano wrote:
> "Johannes Schindelin via GitGitGadget" <gitgitgadget@xxxxxxxxx>
> writes:
>
> > From: Johannes Schindelin <johannes.schindelin@xxxxxx>
> >
> > 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
