Re: [PATCH v2 03/11] Start reporting missing commits in `repo_in_merge_bases_many()`

[Jeff King <peff@peff.net>]

On Thu, Feb 22, 2024 at 01:21:42PM +0000, Johannes Schindelin via GitGitGadget wrote:

> @@ -1402,6 +1436,8 @@ static int merge_mode_and_contents(struct merge_options *opt,
>  							&o->oid,
>  							&a->oid,
>  							&b->oid);
> +			if (result->clean < 0)
> +				return -1;

Coverity flagged this code as NO_EFFECT. The issue is that result->clean
is an unsigned single-bit boolean, so it can never be negative. If we
expand the context a bit, it's

                        result->clean = merge_submodule(opt, &result->blob.oid,
                                                        o->path,
                                                        &o->oid,
                                                        &a->oid,
                                                        &b->oid);
                        if (result->clean < 0)
                                return -1;

So it's possible there's also a problem in the existing assignment, as a
negative return from merge_submodule() would be truncated. But from my
read of it, it will always return either 0 or 1 (if it sees errors from
repo_in_merge_bases(), for example, it will output an error message and
return 0).

So I think we'd want either:

  1. Drop this hunk, and let result->clean stay as a pure boolean.

  2. Propagate errors from merge_submodule() using -1, in which case the
     code needs to be more like:

       int ret = merge_submodule(...);
       if (ret < 0)
               return -1;
       result->clean = !!ret;

I didn't follow the series closely enough to know which would be better.
I guess alternatively it could be a tri-state that carries the error
around (it looks like it is in ort's "struct merge_result"), but that
probably means auditing all of the spots that look at "clean" to see
what they would do with a negative value.

-Peff