Re: [PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff()

From: David Aguilar <davvid@gmail.com>
Date: Sep 30, 2021, 23:25 UTC
Message-ID: <CAJDDKr41qzbUudAHgokRARK-kEG4araFwJ0UhLmJ4Pca=xFZRA@mail.gmail.com>
In-Reply-To: <xmqq7dexafch.fsf@gitster.g>

On Thu, Sep 30, 2021 at 3:06 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 18 quoted lines

>
> David Aguilar <davvid@gmail.com> 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.

Show 20 quoted lines

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

Show 7 quoted lines

> 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