git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v7 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Sep 3, 2020, 05:50 UTC
Message-ID
<nycvar.QRO.7.76.6.2009030737080.56@tvgsbejvaqbjf.bet>
In-Reply-To
<20200831105043.97665-5-mirucam@gmail.com>
Hi Miriam,
On Mon, 31 Aug 2020, Miriam Rubio wrote:
Show 56 quoted lines
> 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.
>
> Add `--bisect-autostart` subcommand to be called from git-bisect.sh.
> Using `--bisect-autostart` subcommand is a temporary measure to port
> the 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 | 44 +++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            | 25 ++---------------------
>  2 files changed, 45 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index bae09ce65d..f71e8e54d2 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -29,6 +29,7 @@ static const char * const git_bisect_helper_usage[] = {
>  	N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
>  	N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
>  					    " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
> +	N_("git bisect--helper --bisect-autostart"),
>  	NULL
>  };
>
> @@ -653,6 +654,38 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
>  	return res;
>  }
>
> +static inline int file_is_not_empty(const char *path)
> +{
> +	return !is_empty_or_missing_file(path);
> +}
> +
> +static int bisect_autostart(struct bisect_terms *terms)
> +{
> +	int res;
> +	const char *yesno;
> +
> +	if (file_is_not_empty(git_path_bisect_start()))
> +		return 0;
> +
> +	fprintf_ln(stderr, _("You need to start by \"git bisect "
> +			  "start\"\n"));
> +
> +	if (!isatty(STDIN_FILENO))

Not a big deal, but we have only two callers to `isatty()` that use the `_FILENO` constants, and neither of them is about stdin. But that is not a big deal.

> +		return 0;

However, when we cannot auto-start, the shell script version calls `exit 1` to cause a failure. I think we need to do the same here, i.e. `return -1;`.

Show 10 quoted lines
> +
> +	/*
> +	 * 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);
> +	res = tolower(*yesno) == 'n' ?
> +		-1 : bisect_start(terms, empty_strvec, 0);
The corresponding shell script code reads like this:
                        read yesno
                        case "$yesno" in
                        [Nn]*)
                                exit ;;
                        esac

That is, if the user does not want to start, the command exits. With exit code 0, i.e. success.

However, we do not do that here.

Now, you could argue (in the commit message) that this actually fixes a bug (because if the bisection was aborted, that does not count for success), and I'd be fine with that.

Show 77 quoted lines
> +
> +	return res;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -665,7 +698,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		CHECK_AND_SET_TERMS,
>  		BISECT_NEXT_CHECK,
>  		BISECT_TERMS,
> -		BISECT_START
> +		BISECT_START,
> +		BISECT_AUTOSTART,
>  	} cmdmode = 0;
>  	int res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -689,6 +723,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  			 N_("print out the bisect terms"), BISECT_TERMS),
>  		OPT_CMDMODE(0, "bisect-start", &cmdmode,
>  			 N_("start the bisect session"), BISECT_START),
> +		OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> +			 N_("start the bisection if it has not yet been started"), BISECT_AUTOSTART),
>  		OPT_BOOL(0, "no-log", &nolog,
>  			 N_("no log for BISECT_WRITE")),
>  		OPT_END()
> @@ -748,6 +784,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		set_terms(&terms, "bad", "good");
>  		res = bisect_start(&terms, argv, argc);
>  		break;
> +	case BISECT_AUTOSTART:
> +		if (argc)
> +			return error(_("--bisect-autostart does not accept arguments"));
> +		set_terms(&terms, "bad", "good");
> +		res = bisect_autostart(&terms);
> +		break;
>  	default:
>  		BUG("unknown subcommand %d", cmdmode);
>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index c7580e51a0..9ca583d964 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
> -			bisect_start
> -		else
> -			exit 1
> -		fi
> -	}
> -}
> -
>  bisect_start() {
>  	git bisect--helper --bisect-start $@ || exit
>
> @@ -108,7 +87,7 @@ bisect_skip() {
>  }
>
>  bisect_state() {
> -	bisect_autostart
> +	git bisect--helper --bisect-autostart

This (and the change below as well) is insufficient, as it won't `exit` in case of an error (or in case the user aborted). I think you need to append `|| exit` as is done two lines below this line.

Ciao, Dscho

Show 16 quoted lines
>  	state=$1
>  	git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
>  	get_terms
> @@ -149,7 +128,7 @@ bisect_auto_next() {
>
>  bisect_next() {
>  	case "$#" in 0) ;; *) usage ;; esac
> -	bisect_autostart
> +	git bisect--helper --bisect-autostart
>  	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
>
>  	# Perform all bisection computation, display and checkout
> --
> 2.25.0
>
>
Previous: Miriam RubioNext: Miriam Rubio
Message 6 of 27 in “Finish converting git bisect to C part 2”
  1. 00/13 Finish converting git bisect to C part 2Miriam Rubio, Aug 31, 2020
  2. 01/13 bisect--helper: BUG() in cmd_*() on invalid subcommandMiriam Rubio, Aug 31, 2020
  3. 02/13 bisect--helper: use '-res' in 'cmd_bisect__helper' returnMiriam Rubio, Aug 31, 2020
  4. 03/13 bisect--helper: introduce new `write_in_file()` functionMiriam Rubio, Aug 31, 2020
  5. 04/13 bisect--helper: reimplement `bisect_autostart` shell function in CMiriam Rubio, Aug 31, 2020
  6. Johannes SchindelinSep 3, 2020
  7. 05/13 bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'Miriam Rubio, Aug 31, 2020
  8. 08/13 bisect--helper: retire `--bisect-clean-state` subcommandMiriam Rubio, Aug 31, 2020
  9. 13/13 bisect--helper: retire `--bisect-autostart` subcommandMiriam Rubio, Aug 31, 2020
  10. 11/13 bisect--helper: retire `--check-expected-revs` subcommandMiriam Rubio, Aug 31, 2020
  11. 12/13 bisect--helper: retire `--write-terms` subcommandMiriam Rubio, Aug 31, 2020
  12. 10/13 bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in CMiriam Rubio, Aug 31, 2020
  13. Johannes SchindelinSep 3, 2020
  14. 09/13 bisect--helper: retire `--next-all` subcommandMiriam Rubio, Aug 31, 2020
  15. 07/13 bisect--helper: finish porting `bisect_start()` to CMiriam Rubio, Aug 31, 2020
  16. Johannes SchindelinSep 3, 2020
  17. 06/13 bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in CMiriam Rubio, Aug 31, 2020
  18. Johannes SchindelinSep 3, 2020
  19. Johannes SchindelinSep 3, 2020
  20. Miriam R.Sep 23, 2020
  21. Johannes SchindelinSep 23, 2020
  22. Johannes SchindelinSep 23, 2020
  23. Christian CouderSep 24, 2020
  24. Johannes SchindelinSep 24, 2020
  25. Christian CouderSep 24, 2020
  26. Miriam R.Sep 24, 2020
  27. Miriam R.Sep 24, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.