David Aguilar [off-list ref] 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.
On Thu, Sep 30, 2021 at 3:06 PM Junio C Hamano [off-list ref] wrote:
David Aguilar [off-list ref] writes:
quoted
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:
Indeed.
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 particular caller cares. It is not
The property I was trying to maintain is that we would forward the result
from the child process in most situations, so we should try and forward
the result from run_command_v_opt() whenever possible.
But for the others we would have to add an "ret = 1" there,
and that doesn't seem worth it since it's too hard to maintain.
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.
Hmm I don't think we can use "return !!ret".
In C this does a bool cast so we lose the positive value from the
underlying diff tool when the value is quantized to 0/1 via !!ret.
That suggests that Ævar's sug is better...
return (ret < 0) ? 1 : ret;
If that sounds good I can send a replacement series that squashes this into the
repeated-symlinks patch. It doesn't seem like we'll need a separate
patch for this.
--
David