Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 30, 2020, 15:01 UTC
Message-ID: <nycvar.QRO.7.76.6.2001301557300.46@tvgsbejvaqbjf.bet>
In-Reply-To: <CAN7CjDCiP_KVC5Ey6cMuPN17vRNp_s4vy94qsaAKRPG=k3Rj3g@mail.gmail.com>

Hi Miriam,

On Thu, 30 Jan 2020, Miriam R. wrote:

Show 15 quoted lines

> El jue., 30 ene. 2020 a las 14:46, Johannes Schindelin
> (<Johannes.Schindelin@gmx.de>) escribió:
> >
> > On Tue, 28 Jan 2020, Miriam Rubio wrote:
> >
> > > +
> > > +     return res < 0 ? -res : res;
> >
> > This is a change in behavior, though: previously we guaranteed that the
> > exit code is either 0 on success or 1 upon failure. I am not quite sure
> > that we want to change that behavior.
>
> I think this is because different error codes might enable a bisecting
> script calling the bisect command that uses this function to do
> different things depending on the exit status of the bisect command.

Oops. I am _totally_ wrong on this.

As you are changing a lot of `exit(<n>)` to `return -<n>` with the intention to turn the value into an exit code only at the `cmd_bisect__helper()` level, this is actually required a change.

I am a bit uneasy about this, but I could not see any return values in `bisect.c` other than 0 and -1, prior to this patch series.

However, I would love to see this refactored into its own commit, more toward the beginning of the patch series, with a very clean commit message that describes that intention of being _the_ exit point from `bisect.c`.

Without this change, you simply cannot change the `exit(<n>);` calls in `bisect.c` for any `<n>` other than 0 or 1.

Sorry that it took me so long to wrap my head around this rather trivial idea.

Ciao, Dscho