diff mbox series

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

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

Commit Message

Miriam R. Jan. 25, 2021, 7:17 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

Đoàn Trần Công Danh Jan. 30, 2021, 1:46 a.m. UTC | #1 
> +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;

The old code will write "We are not bisection." to stderr when, well,
we're not bisecting. This message suggested an alternative
 https://lore.kernel.org/git/gohp6kv9bml9qc.fsf@gmail.com

> +}
> +
>  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..05863cc142 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 ;;

The original code was "die" when no bisect_log available.

I think we need to "exit 1" here to indicate a failure, i.e.

	git bisect--helper --bisect-log || exit 1 ;;

>  	run)
>  		bisect_run "$@" ;;
>  	terms)
> -- 
> 2.29.2
>
Christian Couder Jan. 30, 2021, 9:49 a.m. UTC | #2 
On Sat, Jan 30, 2021 at 2:46 AM Đoàn Trần Công Danh
<congdanhqx@gmail.com> wrote:

> > @@ -210,7 +205,7 @@ case "$#" in
> >       replay)
> >               bisect_replay "$@" ;;
> >       log)
> > -             bisect_log ;;
> > +             git bisect--helper --bisect-log || exit ;;
>
> The original code was "die" when no bisect_log available.
>
> I think we need to "exit 1" here to indicate a failure, i.e.
>
>         git bisect--helper --bisect-log || exit 1 ;;

Actually:

             git bisect--helper --bisect-log || exit ;;

will indicate failure. See:

$ ( false || exit )
$ echo $?
1

The difference with what you suggest is that the exit code will be the
same instead of always 1:

$ ( ( exit 4 ) || exit )
echo $?
4

which is a good thing, as die() exits with code 128, so if `git
bisect--helper --bisect-log` die()s then we will get a 128 exit code
with `|| exit` instead of 1 with `|| exit 1`.

Thanks for your review,
Christian.
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..05863cc142 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)