Re: [PATCH 11/29] bisect: libify `bisect_next_all`

Hi Chris,

On Tue, 21 Jan 2020, Christian Couder wrote:

On Mon, Jan 20, 2020 at 11:29 PM Johannes Schindelin
[off-list ref] wrote:

quoted

On Mon, 20 Jan 2020, Miriam Rubio wrote:

quoted

quoted

Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.

Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.

Turn `exit()` to `return` calls in `bisect_next_all()`.

All the functions calling `bisect_next_all()` are already able to
handle return values from it.

So this patch brings more magic values that really would like to become
`enum` values. At this point, we're talking about `bisect_next_all()`
which is _not_ a file-local (`static`) function. Therefore, we now really
wade into API territory where an `enum` is no longer just a nice thing,
but a necessary thing: `bisect_next_all()` is a function that can be
called from other source files.

I agree that return values could be better documented. About enum
though, I am perhaps wrong but I don't think that we use them often
for error codes.

It does not matter how often we actually use them, it matters more whether
we _want_ to use them. And in the recent years, we have definitely made
more use of enums than before, to allow the compiler to catch mistakes
earlier. We even started to convert existing constants to enums, for
example.

So I don't think that it is sound advice here to point to the code base.

If you _must_ value the existing practice over a clearly communicated
review, then please look no further than at the declaration of
`safe_create_leading_directories()`.

Do you have examples in the code base where they are used for such a
purpose along with regular `error(...)` calls?

It's all in the code. You don't need me to read it aloud for you.

:-)

Ciao,
Dscho