diff mbox series

[06/10] bisect--helper: reimplement `bisect_autostart` shell function in C

Message ID 20200226101429.81327-7-mirucam@gmail.com (mailing list archive)
State New, archived
Headers show
Series Finish converting git bisect to C part 2 | expand

Commit Message

Miriam R. Feb. 26, 2020, 10:14 a.m. UTC 
From: Pranit Bauva <pranit.bauva@gmail.com>

Reimplement the `bisect_autostart()` shell function in C and add the
C implementation from `bisect_next()` which was previously left
uncovered. Also add a subcommand `--bisect-autostart` to
`git bisect--helper` be called from `bisect_state()` from
git-bisect.sh .

Using `--bisect-autostart` subcommand is a temporary measure to port
shell function to C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired and
bisect_autostart() will be called directly by `bisect_state()`.

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 | 39 +++++++++++++++++++++++++++++++++++++++
 git-bisect.sh            | 23 +----------------------
 2 files changed, 40 insertions(+), 22 deletions(-)

Comments

Johannes Schindelin Feb. 27, 2020, 9:40 p.m. UTC | #1 
Hi,

On Wed, 26 Feb 2020, Miriam Rubio wrote:

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c3bb936a40..f9b04bee23 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -56,6 +57,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,
>  static const char vocab_bad[] = "bad|new";
>  static const char vocab_good[] = "good|old";
>
> +static int bisect_autostart(struct bisect_terms *terms);
> +

Can we move the definition of said function here? If so, we can save
ourselves the forward-declaration of it.

>  /*
>   * Check whether the string `term` belongs to the set of strings
>   * included in the variable arguments.
> @@ -555,6 +558,7 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
>  	int no_checkout;
>  	enum bisect_error res;
>
> +	bisect_autostart(terms);
>  	if (bisect_next_check(terms, terms->term_good))
>  		return BISECT_FAILED;
>
> @@ -803,6 +807,32 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkou
>  	return res;
>  }
>
> +static int bisect_autostart(struct bisect_terms *terms)
> +{
> +	if (is_empty_or_missing_file(git_path_bisect_start())) {

As a rule, we do prefer to handle early exits first. In other words, I
would suggest starting this function by:

	if (!is_empty_or_missing_file(git_path_bisect_start()))
		return BISECT_OK;

That way, not only can you save on indentation, but it also frees one slot
of the readers' working memory.

(Of course you would still need to ove the declarations before this
statement.)

> +		const char *yesno;
> +		const char *argv[] = {NULL};

Please separate declarations and statements with an empty line. I would
also prefer spaces around the curly brackets, i.e. `{ NULL }` instead of
`{NULL}`.

> +		fprintf(stderr, _("You need to start by \"git bisect "
> +				  "start\"\n"));
> +
> +		if (!isatty(STDIN_FILENO))
> +			return 1;
> +
> +		/*
> +		 * TRANSLATORS: Make sure to include [Y] and [n] in your
> +		 * translation. The program will only accept English input
> +		 * at this point.
> +		 */
> +		yesno = git_prompt(_("Do you want me to do it for you "
> +				     "[Y/n]? "), PROMPT_ECHO);
> +		if (starts_with(yesno, _("n")) || starts_with(yesno, _("N")))
> +			return 1;
> +
> +		return bisect_start(terms, 0, argv, 0);

Do we really need this `argv`, or can we pass `NULL` directly? We say that
there are zero arguments, after all, therefore `bisect_start()` should not
even look at `argv`.

The rest of the patch looks good to me.

Thank you,
Dscho

> +	}
> +	return BISECT_OK;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -816,6 +846,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		BISECT_START,
>  		BISECT_NEXT,
>  		BISECT_AUTO_NEXT,
> +		BISECT_AUTOSTART,
>  	} cmdmode = 0;
>  	int no_checkout = 0, res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -839,6 +870,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  			 N_("find the next bisection commit"), BISECT_NEXT),
>  		OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
>  			 N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
> +		OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> +			 N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
>  		OPT_BOOL(0, "no-checkout", &no_checkout,
>  			 N_("update BISECT_HEAD instead of checking out the current commit")),
>  		OPT_BOOL(0, "no-log", &nolog,
> @@ -905,6 +938,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		get_terms(&terms);
>  		res = bisect_auto_next(&terms, prefix);
>  		break;
> +	case BISECT_AUTOSTART:
> +		if (argc)
> +			return error(_("--bisect-autostart requires 0 arguments"));
> +		set_terms(&terms, "bad", "good");
> +		res = bisect_autostart(&terms);
> +		break;
>  	default:
>  		return error("BUG: unknown subcommand '%d'", cmdmode);
>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 166f6a64dd..049ffacdff 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -49,27 +49,6 @@ bisect_head()
>  	fi
>  }
>
> -bisect_autostart() {
> -	test -s "$GIT_DIR/BISECT_START" || {
> -		gettextln "You need to start by \"git bisect start\"" >&2
> -		if test -t 0
> -		then
> -			# TRANSLATORS: Make sure to include [Y] and [n] in your
> -			# translation. The program will only accept English input
> -			# at this point.
> -			gettext "Do you want me to do it for you [Y/n]? " >&2
> -			read yesno
> -			case "$yesno" in
> -			[Nn]*)
> -				exit ;;
> -			esac
> -			git bisect--helper --bisect-start
> -		else
> -			exit 1
> -		fi
> -	}
> -}
> -
>  bisect_skip() {
>  	all=''
>  	for arg in "$@"
> @@ -86,7 +65,7 @@ bisect_skip() {
>  }
>
>  bisect_state() {
> -	bisect_autostart
> +	git bisect--helper --bisect-autostart
>  	state=$1
>  	git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
>  	get_terms
> --
> 2.25.0
>
>
diff mbox series

Patch

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c3bb936a40..f9b04bee23 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -30,6 +30,7 @@  static const char * const git_bisect_helper_usage[] = {
 					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
 	N_("git bisect--helper --bisect-next"),
 	N_("git bisect--helper --bisect-auto-next"),
+	N_("git bisect--helper --bisect-autostart"),
 	NULL
 };
 
@@ -56,6 +57,8 @@  static void set_terms(struct bisect_terms *terms, const char *bad,
 static const char vocab_bad[] = "bad|new";
 static const char vocab_good[] = "good|old";
 
+static int bisect_autostart(struct bisect_terms *terms);
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -555,6 +558,7 @@  static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
 	int no_checkout;
 	enum bisect_error res;
 
+	bisect_autostart(terms);
 	if (bisect_next_check(terms, terms->term_good))
 		return BISECT_FAILED;
 
@@ -803,6 +807,32 @@  static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkou
 	return res;
 }
 
+static int bisect_autostart(struct bisect_terms *terms)
+{
+	if (is_empty_or_missing_file(git_path_bisect_start())) {
+		const char *yesno;
+		const char *argv[] = {NULL};
+		fprintf(stderr, _("You need to start by \"git bisect "
+				  "start\"\n"));
+
+		if (!isatty(STDIN_FILENO))
+			return 1;
+
+		/*
+		 * TRANSLATORS: Make sure to include [Y] and [n] in your
+		 * translation. The program will only accept English input
+		 * at this point.
+		 */
+		yesno = git_prompt(_("Do you want me to do it for you "
+				     "[Y/n]? "), PROMPT_ECHO);
+		if (starts_with(yesno, _("n")) || starts_with(yesno, _("N")))
+			return 1;
+
+		return bisect_start(terms, 0, argv, 0);
+	}
+	return BISECT_OK;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -816,6 +846,7 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_START,
 		BISECT_NEXT,
 		BISECT_AUTO_NEXT,
+		BISECT_AUTOSTART,
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
@@ -839,6 +870,8 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("find the next bisection commit"), BISECT_NEXT),
 		OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
 			 N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
+		OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
+			 N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_BOOL(0, "no-log", &nolog,
@@ -905,6 +938,12 @@  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		get_terms(&terms);
 		res = bisect_auto_next(&terms, prefix);
 		break;
+	case BISECT_AUTOSTART:
+		if (argc)
+			return error(_("--bisect-autostart requires 0 arguments"));
+		set_terms(&terms, "bad", "good");
+		res = bisect_autostart(&terms);
+		break;
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 166f6a64dd..049ffacdff 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -49,27 +49,6 @@  bisect_head()
 	fi
 }
 
-bisect_autostart() {
-	test -s "$GIT_DIR/BISECT_START" || {
-		gettextln "You need to start by \"git bisect start\"" >&2
-		if test -t 0
-		then
-			# TRANSLATORS: Make sure to include [Y] and [n] in your
-			# translation. The program will only accept English input
-			# at this point.
-			gettext "Do you want me to do it for you [Y/n]? " >&2
-			read yesno
-			case "$yesno" in
-			[Nn]*)
-				exit ;;
-			esac
-			git bisect--helper --bisect-start
-		else
-			exit 1
-		fi
-	}
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -86,7 +65,7 @@  bisect_skip() {
 }
 
 bisect_state() {
-	bisect_autostart
+	git bisect--helper --bisect-autostart
 	state=$1
 	git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
 	get_terms