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

[Rafael Silva <rafaeloliveira.cs@gmail.com>]

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 `;;`.

-- 
Thanks
Rafael