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

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 30, 2020, 21:59 UTC
Message-ID: <CAP8UFD3mhirdjHnT+XRq1mPzii3O+mtAMrYCy7mf4HKQZo8Acw@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.2001301341100.46@tvgsbejvaqbjf.bet>

Hi Dscho,

On Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

>
> On Tue, 28 Jan 2020, Miriam Rubio wrote:

Show 23 quoted lines

> > +             /* 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.

Best, Christian.