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

From: Jeff King <peff@peff.net>
Date: Mar 1, 2024, 06:56 UTC
Message-ID: <20240301065647.GA2680308@coredump.intra.peff.net>
In-Reply-To: <4dd214f91d4783f29b03908cc0034156253889a7.1708608110.git.gitgitgadget@gmail.com>

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

Show 6 quoted lines

> @@ -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