diff mbox series

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

Message ID 20210123154056.48234-2-mirucam@gmail.com (mailing list archive)
State Superseded
Headers show
Series Finish converting git bisect to C part 3 | expand

Commit Message

Miriam Rubio Jan. 23, 2021, 3:40 p.m. UTC
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(-)

Comments

Rafael Silva Jan. 24, 2021, 1:56 p.m. UTC | #1
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 `;;`.
Miriam Rubio Jan. 25, 2021, 7:23 p.m. UTC | #2
Hi Rafael,

El dom, 24 ene 2021 a las 14:56, Rafael Silva
(<rafaeloliveira.cs@gmail.com>) 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@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.
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
Junio C Hamano Jan. 26, 2021, 6:32 p.m. UTC | #3
"Miriam R." <mirucam@gmail.com> writes:

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

Sorry I missed this thread.

My understanding is that this topic is an attempt to "reimplement"
what is there in the scripted version, so any deviation of behaviour
obserbable from outside, which is *not* justified, should by
definition be treated as a bug.

If the original author did not explain why the behaviour difference
exists and defend why the new behaviour in the reimplementation is
better, and if you do not think of a good reason why the behaviour
should be different and the new behaviour is better, then let's
treat it in a bug and fix it.

Thanks.
Miriam Rubio Jan. 27, 2021, 2:10 p.m. UTC | #4
Hi,

El mar, 26 ene 2021 a las 19:32, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> "Miriam R." <mirucam@gmail.com> writes:
>
> >> 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.
>
> Sorry I missed this thread.
>
> My understanding is that this topic is an attempt to "reimplement"
> what is there in the scripted version, so any deviation of behaviour
> obserbable from outside, which is *not* justified, should by
> definition be treated as a bug.
>
> If the original author did not explain why the behaviour difference
> exists and defend why the new behaviour in the reimplementation is
> better, and if you do not think of a good reason why the behaviour
> should be different and the new behaviour is better, then let's
> treat it in a bug and fix it.
>
Ok, I will send another patch series adding this.
Thank you.
Miriam.
> Thanks.
diff mbox series

Patch

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;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -916,7 +928,8 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_AUTOSTART,
 		BISECT_NEXT,
 		BISECT_AUTO_NEXT,
-		BISECT_STATE
+		BISECT_STATE,
+		BISECT_LOG
 	} cmdmode = 0;
 	int res = 0, nolog = 0;
 	struct option options[] = {
@@ -938,6 +951,8 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
 		OPT_CMDMODE(0, "bisect-state", &cmdmode,
 			 N_("mark the state of ref (or refs)"), BISECT_STATE),
+		OPT_CMDMODE(0, "bisect-log", &cmdmode,
+			 N_("list the bisection steps so far"), BISECT_LOG),
 		OPT_BOOL(0, "no-log", &nolog,
 			 N_("no log for BISECT_WRITE")),
 		OPT_END()
@@ -1000,6 +1015,11 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		get_terms(&terms);
 		res = bisect_state(&terms, argv, argc);
 		break;
+	case BISECT_LOG:
+		if (argc)
+			return error(_("--bisect-log requires 0 arguments"));
+		res = bisect_log();
+		break;
 	default:
 		BUG("unknown subcommand %d", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 1f3f6e9fc5..c6149846ff 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -169,11 +169,6 @@  exit code \$res from '\$command' is < 0 or >= 128" >&2
 	done
 }
 
-bisect_log () {
-	test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")"
-	cat "$GIT_DIR/BISECT_LOG"
-}
-
 get_terms () {
 	if test -s "$GIT_DIR/BISECT_TERMS"
 	then
@@ -210,7 +205,7 @@  case "$#" in
 	replay)
 		bisect_replay "$@" ;;
 	log)
-		bisect_log ;;
+		git bisect--helper --bisect-log || exit;;
 	run)
 		bisect_run "$@" ;;
 	terms)