Re: [PATCH v3 1/7] bisect--helper: reimplement `bisect_log` shell function in C

>
>
> Nice work on this series.
>
> I have one comment on this series regarding a behavior diff between the
> C and shell version, and small comment about style, see below.
>
> Miriam Rubio writes:
>
> > From: Pranit Bauva <pranit.bauva@gmail.com>
> >
> > Reimplement the `bisect_log()` shell function in C and also add
> > `--bisect-log` subcommand to `git bisect--helper` to call it from
> > git-bisect.sh .
> >
> > Using `--bisect-log` subcommand is a temporary measure to port shell
> > function to C so as to use the existing test suite.
> >
> > Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> > Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> > ---
> >  builtin/bisect--helper.c | 22 +++++++++++++++++++++-
> >  git-bisect.sh            |  7 +------
> >  2 files changed, 22 insertions(+), 7 deletions(-)
> >
> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > index 709eb713a3..a65244a0f5 100644
> > --- a/builtin/bisect--helper.c
> > +++ b/builtin/bisect--helper.c
> > @@ -904,6 +904,18 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a
> >       return bisect_auto_next(terms, NULL);
> >  }
> >
> > +static enum bisect_error bisect_log(void)
> > +{
> > +     int fd, status;
> > +     fd = open(git_path_bisect_log(), O_RDONLY);
> > +     if (fd < 0)
> > +             return BISECT_FAILED;
> > +
> > +     status = copy_fd(fd, STDOUT_FILENO);
> > +     close(fd);
> > +     return status ? BISECT_FAILED : BISECT_OK;
> > +}
> > +
>
> In the shell version, when we are not bisecting it the `git bisect log`
> command will `die` with the text "We are not bisecting." which state to
> the user that a bisect is not yet started. The shell version does that
> by checking if the `$GIT_DIR/BISECT_LOG` file doesn't exists or it's
> an empty file as the following code snippet copied from the shell
> version that is remove by this patch:
>
>    test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")"
>
> This seems to be "missing" from the new C version implemented by this
> patch and perhaps we should add it. I'm not sure whether this change was
> intentional and I'm missing some context here of why we are dropping
> the message, if that's the case I apologize in advance. But, IMHO
> outputting the error message provides a better user experience as it
> would be obvious that the user forgot to `git bisect start` instead of
> silently failing.
>
> With that said, perhaps an obvious way of implementing is to use
> `is_empty_or_missing_file()`, much like `bisect_replay()` does it in the
> [2/7] patch on this series, and return the same error message from
> the shell version:
>
> -- >8 --
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index a65244a0f5..ce11383125 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -907,7 +907,12 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a
>  static enum bisect_error bisect_log(void)
>  {
>         int fd, status;
> -       fd = open(git_path_bisect_log(), O_RDONLY);
> +       const char* filename = git_path_bisect_log();
> +
> +       if (is_empty_or_missing_file(filename))
> +               return error(_("We are not bisecting."));
> +
> +       fd = open(filename, O_RDONLY);
>         if (fd < 0)
>                 return BISECT_FAILED;
> -- >8 --
>
> Although I compiled and did small test on the above code snippet, don't
> trust it blindly and perform your own test and judge whether this is the
> best way to implement this shortcoming.

>
> >
> > @@ -210,7 +205,7 @@ case "$#" in
> >       replay)
> >               bisect_replay "$@" ;;
> >       log)
> > -             bisect_log ;;
> > +             git bisect--helper --bisect-log || exit;;
>
> Style: just a minor change to add a space between `exit` and `;;`.