Hi Dscho,

On Fri, Jan 31, 2020 at 10:07 AM Johannes Schindelin
[off-list ref] wrote:

On Thu, 30 Jan 2020, Christian Couder wrote:

quoted

On Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin
[off-list ref] wrote:

quoted

On Tue, 28 Jan 2020, Miriam Rubio wrote:

quoted

quoted

+             /* Create file BISECT_ANCESTORS_OK. */
+             fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+             if (fd < 0)
+                     warning_errno(_("could not create file '%s'"),
+                                   filename);
+             else
+                     close(fd);
+     }

I wonder whether this would be easier to read:

        if (res == -11)
                res = 0;
        else if (res)
                ; /* error out */
        else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)
                res = warning_errno(_("could not create file '%s'"), filename);
        else
                close(fd);

Note: my code explicitly assigns `res = -1` if the file could not be
created, which is technically a change in behavior, but I think it is
actually a bug fix.

I don't think so. I think creating the BISECT_ANCESTORS_OK file is not
absolutely necessary. If it doesn't exist we will just check if
ancestors are ok again at the next bisection step, which will take a
bit of time, but which will not make us give any false result, or
prevent us from continuing the bisection process.

I think that it's also the reason why warning_errno(...) is used in
case we could not create the file instead of error_errno(...). We just
want to signal with a warning that something might be wrong because we
could not create the file, but not stop everything because of that.

Thank you for this explanation, it makes sense to me.

Maybe a code comment would be in order?

Yeah, I agree it would help.

Thanks for your review,
Christian.