Hi Rafael,
El dom, 24 ene 2021 a las 14:56, Rafael Silva
(<rafaeloliveira.cs@xxxxxxxxx>) escribió:
>
>
> 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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> > Mentored-by: Johannes Schindelin <Johannes.Schindelin@xxxxxx>
> > Signed-off-by: Pranit Bauva <pranit.bauva@xxxxxxxxx>
> > Signed-off-by: Tanushree Tumane <tanushreetumane@xxxxxxxxx>
> > Signed-off-by: Miriam Rubio <mirucam@xxxxxxxxx>
> > ---
> > 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.
Ok, thank you.
I am not the original author of this subcommand reimplementation and I
don't know if there is a reason
for the difference with the error message. Maybe we can wait for some
other reviewers opinion.
>
> >
> > @@ -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 `;;`.
Noted.
>
> --
> Thanks
> Rafael
Thank you for your suggestions.
Best,
Miriam
