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

Re: [PATCH v7 10/13] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Sep 3, 2020, 12:03 UTC
Message-ID
<nycvar.QRO.7.76.6.2009031158080.56@tvgsbejvaqbjf.bet>
In-Reply-To
<20200831105043.97665-11-mirucam@gmail.com>
Hi Miriam,
On Mon, 31 Aug 2020, Miriam Rubio wrote:
Show 53 quoted lines
> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Reimplement the `bisect_state()` shell functions in C and also add a
> subcommand `--bisect-state` to `git-bisect--helper` to call them from
> git-bisect.sh .
>
> Using `--bisect-state` 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 will be called by some
> other methods.
>
> `bisect_head()` is only called from `bisect_state()`, thus it is not
> required to introduce another subcommand.
>
> 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 | 78 +++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            | 55 +++-------------------------
>  2 files changed, 81 insertions(+), 52 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 0796e51672..a976f4739c 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
>  	N_("git bisect--helper --bisect-next"),
>  	N_("git bisect--helper --bisect-auto-next"),
>  	N_("git bisect--helper --bisect-autostart"),
> +	N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
> +	N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
>  	NULL
>  };
>
> @@ -862,6 +864,72 @@ static int bisect_autostart(struct bisect_terms *terms)
>  	return res;
>  }
>
> +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
> +				      int argc)
> +{
> +	const char *state;
> +	int i, verify_expected = 1;
> +	struct object_id oid, expected;
> +	struct strbuf buf = STRBUF_INIT;
> +	struct oid_array revs = OID_ARRAY_INIT;
> +
> +	if (!argc)
> +		return error(_("Please call `--bisect-state` with at least one argument"));

Good. Technically, this is a deviation from the scripted version, but it is good not to check and set the terms first, only to then error out anyway.

However, I think we will need a call to `bisect_autostart()` somewhere around here, as that is what the scripted version did first thing.

Show 17 quoted lines
> +
> +	state = argv[0];
> +	if (check_and_set_terms(terms, state) ||
> +	    !one_of(state, terms->term_good, terms->term_bad, "skip", NULL))
> +		return BISECT_FAILED;
> +
> +	argv++;
> +	argc--;
> +	if (argc > 1 && !strcmp(state, terms->term_bad))
> +		return error(_("'git bisect %s' can take only one argument."), terms->term_bad);
> +
> +	if (argc == 0) {
> +		enum get_oid_result res_head = get_oid("BISECT_HEAD", &oid);
> +		if (res_head == MISSING_OBJECT)
> +			res_head = get_oid("HEAD", &oid);
> +		if (res_head) {
> +			error(_("Bad bisect_head rev input"));

If I am not mistaken, we should convert this shell command more faithfully here:

			die "$(eval_gettext "Bad rev input: \$bisected_head")"
i.e. something like this:
		const char *head = "BISECT_HEAD";
		enum get_oid_result res_head = get_oid(head, &oid);
		if (res_head == MISSING_OBJECT) {
			head = "HEAD";
			res_head = get_oid(head, &oid);
		}
		if (res_head)
			error(_("Bad rev input: %s"), head);
Show 33 quoted lines
> +			return BISECT_FAILED;
> +		}
> +		oid_array_append(&revs, &oid);
> +	}
> +
> +	/*
> +	 * All input revs must be checked before executing bisect_write()
> +	 * to discard junk revs.
> +	 */
> +
> +	for (; argc; argc--, argv++) {
> +		if (get_oid(*argv, &oid)){
> +			error(_("Bad rev input: %s"), *argv);
> +			return BISECT_FAILED;
> +		}
> +		oid_array_append(&revs, &oid);
> +	}
> +
> +	if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
> +	    get_oid_hex(buf.buf, &expected) < 0)
> +		verify_expected = 0; /* Ignore invalid file contents */
> +	strbuf_release(&buf);
> +
> +	for (i = 0; i < revs.nr; i++) {
> +		if (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0))
> +			return BISECT_FAILED;
> +
> +		if (verify_expected && !oideq(&revs.oid[i], &expected)) {
> +			unlink_or_warn(git_path_bisect_ancestors_ok());
> +			unlink_or_warn(git_path_bisect_expected_rev());
> +			verify_expected = 0;
> +		}
> +	}
Nice! This reads so much easier than the shell script version.
Show 102 quoted lines
> +
> +	oid_array_clear(&revs);
> +	return bisect_auto_next(terms, NULL);
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -875,7 +943,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		BISECT_START,
>  		BISECT_NEXT,
>  		BISECT_AUTO_NEXT,
> -		BISECT_AUTOSTART
> +		BISECT_AUTOSTART,
> +		BISECT_STATE
>  	} cmdmode = 0;
>  	int res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -901,6 +970,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-autostart", &cmdmode,
>  			 N_("start the bisection if it has not yet been started"), BISECT_AUTOSTART),
> +		OPT_CMDMODE(0, "bisect-state", &cmdmode,
> +			 N_("mark the state of ref (or refs)"), BISECT_STATE),
>  		OPT_BOOL(0, "no-log", &nolog,
>  			 N_("no log for BISECT_WRITE")),
>  		OPT_END()
> @@ -971,6 +1042,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		set_terms(&terms, "bad", "good");
>  		res = bisect_autostart(&terms);
>  		break;
> +	case BISECT_STATE:
> +		set_terms(&terms, "bad", "good");
> +		get_terms(&terms);
> +		res = bisect_state(&terms, argv, argc);
> +		break;
>  	default:
>  		BUG("unknown subcommand %d", cmdmode);
>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 356264caf0..7a8f796251 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
>  TERM_BAD=bad
>  TERM_GOOD=good
>
> -bisect_head()
> -{
> -	if git rev-parse --verify -q BISECT_HEAD > /dev/null
> -	then
> -		echo BISECT_HEAD
> -	else
> -		echo HEAD
> -	fi
> -}
> -
>  bisect_skip() {
>  	all=''
>  	for arg in "$@"
> @@ -61,43 +51,7 @@ bisect_skip() {
>  		esac
>  		all="$all $revs"
>  	done
> -	eval bisect_state 'skip' $all
> -}
> -
> -bisect_state() {
> -	git bisect--helper --bisect-autostart
> -	state=$1
> -	git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> -	get_terms
> -	case "$#,$state" in
> -	0,*)
> -		die "Please call 'bisect_state' with at least one argument." ;;
> -	1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
> -		bisected_head=$(bisect_head)
> -		rev=$(git rev-parse --verify "$bisected_head") ||
> -			die "$(eval_gettext "Bad rev input: \$bisected_head")"
> -		git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> -		git bisect--helper --check-expected-revs "$rev" ;;
> -	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
> -		shift
> -		hash_list=''
> -		for rev in "$@"
> -		do
> -			sha=$(git rev-parse --verify "$rev^{commit}") ||
> -				die "$(eval_gettext "Bad rev input: \$rev")"
> -			hash_list="$hash_list $sha"
> -		done
> -		for rev in $hash_list
> -		do
> -			git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> -		done
> -		git bisect--helper --check-expected-revs $hash_list ;;
> -	*,"$TERM_BAD")
> -		die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
> -	*)
> -		usage ;;
> -	esac
> -	git bisect--helper --bisect-auto-next
> +	eval git bisect--helper --bisect-state 'skip' $all

Since `bisect--helper` cannot cause the shell script to `exit`, the `eval` should no longer be necessary.

Show 19 quoted lines
>  }
>
>  bisect_visualize() {
> @@ -187,8 +141,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
>  			state="$TERM_GOOD"
>  		fi
>
> -		# We have to use a subshell because "bisect_state" can exit.
> -		( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> +		git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
>  		res=$?
>
>  		cat "$GIT_DIR/BISECT_RUN"
> @@ -203,7 +156,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
>  		if [ $res -ne 0 ]
>  		then
>  			eval_gettextln "bisect run failed:
> -'bisect_state \$state' exited with error code \$res" >&2
> +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2

I don't think we should include implementation details such as the use of `bisect--helper` in the error message. Personally, I would either keep `bisect_state` or paraphrase what failed in English to begin with.

Thank you, Johannes

Show 16 quoted lines
>  			exit $res
>  		fi
>
> @@ -244,7 +197,7 @@ case "$#" in
>  	start)
>  		git bisect--helper --bisect-start "$@" ;;
>  	bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
> -		bisect_state "$cmd" "$@" ;;
> +		git bisect--helper --bisect-state "$cmd" "$@" ;;
>  	skip)
>  		bisect_skip "$@" ;;
>  	next)
> --
> 2.25.0
>
>
Previous: Miriam RubioNext: Miriam Rubio
Message 13 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.