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
