David Aguilar <davvid@xxxxxxxxx> writes:
> difftool was forwarding the -1 result from error() to cmd_main(), which
> is implementation-defined since it is outside of the 0-255 range
> specified by POSIX for program exit codes.
>
> Stop assigning the result of error() to `ret`. Assign a value of 1
> whenever internal errors are detected instead.
Many existing codepaths take advantage of error() returning -1 and I
do not see anything is wrong to keep that "negative is error"
convention for the value of "ret" variable. Most lines in this
patch however split that "ret = error(...)" pattern into two
statements and I do not see a very good reason for it.
Worse yet, after applying this patch, there still are at least three
assignments to "ret" that can give it a negative value:
if (!mkdtemp(tmpdir.buf)) {
ret = error("could not create '%s'", tmpdir.buf);
goto finish;
}
ret = run_command_v_opt(helper_argv, flags);
strbuf_addf(&buf, "%s/wtindex", tmpdir.buf);
if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {
ret = error("could not write %s", buf.buf);
goto finish;
}
Among them, the return value from run_command_v_opt() eventually
come from wait_or_whine(), I think, so it may be generic -1 or
it may be WEXITSTATUS() of the child process.
But I am not sure if this partcular caller cares. It is not
prepared to handle -1 and positive return from run_command_v_opt()
any differently. So I think a single
- return ret;
+ return !!ret;
at the end would be much easier to reason about and maintain.
