On Thu, Sep 30, 2021 at 3:06 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> 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:
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
