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

["Miriam R." <mirucam@gmail.com>]

Hi,

El vie., 31 ene. 2020 a las 10:15, Christian Couder
(<christian.couder@gmail.com>) escribió:
>
> Hi Dscho,
>
> On Fri, Jan 31, 2020 at 10:07 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Thu, 30 Jan 2020, Christian Couder wrote:
> >
> > > On Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
> > > >
> > > > On Tue, 28 Jan 2020, Miriam Rubio wrote:
> > >
> > > > > +             /* 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.
>
Noted.
Thank you both for the review!

Best,
Miriam
> Thanks for your review,
> Christian.