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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

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