Re: [PATCH v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C

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

Hi Miriam,

On Thu, 23 Apr 2020, Miriam Rubio wrote:

> 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 | 70 +++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            | 55 +++----------------------------
>  2 files changed, 73 insertions(+), 52 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 2d8660c79f..9db72f5891 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
>  };
>
> @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)
>  	return bisect_start(terms, 0, NULL, 0);
>  }
>
> +static int bisect_head(struct object_id *oid)
> +{
> +	if (!file_exists(git_path_bisect_head()))
> +		return get_oid("HEAD", oid);
> +
> +	return get_oid("BISECT_HEAD", oid);

This can be easily reduced to

	return get_oid(file_exists(git_path_bisect_head()) ?
		       "BISECT_HEAD" : "HEAD", oid);

At the same time, it is wrong, just like the shell script version was
wrong: in particular in light of the `hn/reftable` effort, we do _not_
want to assume that all refs are backed by files!

So really, what this should do instead is this:

	enum get_oid_result res = get_oid("BISECT_HEAD", oid);

	if (res == MISSING_OBJECT)
		res = get_oid("HEAD", oid);

Given that this is still only three lines long, the overhead of having it
in its own function for just a _single_ call seems excessive. I'd prefer
it to be inlined in `bisect_state()`.

> +}
> +
> +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
> +			int argc)
> +{

I offered a lengthy discussion about this function in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/

It does not look, however, as if v3 benefitted from the entirety of my
analysis: All the `check_expected_revs()` function does is to verify that
the passed list of revs matches exactly the contents of the
`BISECT_EXPECTED_REV` file.

That can be done in a much simpler way, though, by first reading the file
and parsing the contents into an OID, and then comparing to that parsed
OID instead.

Besides, `check_expected_revs()` is only used to check one rev at a time.

In other words, it could be simplified to something like this:

static void check_expected_rev(struct object_id *oid)
{
	struct object_id expected;
	struct strbuf buf = STRBUF_INIT;

        if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)
	      < the_hash_algo->hexsz ||
	    get_oid_hex(buf.buf, &expected) < 0)
		return; /* Ignore invalid file contents */

	if (!oideq(oid, &expected)) {
		... unlink ...
		return;
	}
}

But even that would be wasteful, as we would read the file over and over
and over again.

The good news is that we do not even _need_ `check_expected_rev()`.
Because we do not need to have two call sites, we can simplify the code
much further. See below:

> +	const char *state;
> +	const char *hex;
> +	int i;
> +	struct oid_array revs = OID_ARRAY_INIT;
> +	struct object_id oid;
> +
> +	if (!argc)
> +		return error(_("Please call `--bisect-state` with at least one argument"));
> +	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 (!strcmp(state, terms->term_bad) && (argc > 1))
> +		return error(_("'git bisect %s' can take only one argument."),terms->term_bad);
> +	if (argc == 0) {
> +		if (bisect_head(&oid))
> +			return error(_("Bad bisect_head rev input"));
> +		hex = oid_to_hex(&oid);
> +		if (bisect_write(state, hex, terms, 0))
> +			return BISECT_FAILED;
> +		check_expected_revs(&hex, 1);
> +		return bisect_auto_next(terms, NULL);
> +	}
> +
> +	/* Here argc > 0 */
> +	for (; argc; argc--, argv++) {
> +		struct object_id oid;
> +		if (get_oid(*argv, &oid))
> +			return error(_("Bad rev input: %s"), *argv);
> +		oid_array_append(&revs, &oid);
> +	}

It really does not make sense to parse the arguments into an OID array,
_then_ iterate over the array once, and then immediately releasing it.
That OID array is not needed at all.

So we'll end up with this loop in case `argc > 0` (where we now call
`get_oid()`, too), and note how the loop body looks _eerily_ similar to
the conditional `argc == 0` code block above?

> +
> +	for (i = 0; i < revs.nr; i++) {
> +		hex = oid_to_hex(&revs.oid[i]);
> +		if (bisect_write(state, hex, terms, 0)) {
> +			oid_array_clear(&revs);
> +			return BISECT_FAILED;
> +		}
> +		check_expected_revs(&hex, 1);
> +	}
> +
> +	oid_array_clear(&revs);
> +	return bisect_auto_next(terms, NULL);
> +}

So really, this function pretty much _wants_ to look this way (modulo
bugs, as I did not even test-compile the code):

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;

	if (!argc)
		return error(_("Please call `--bisect-state` with at least one argument"));

	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 (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 */


	for (i = 0; i < argc + !argc; i++) {
		if (argc) {
			if (get_oid(argv[i], &oid)) {
				error(_("Bad rev input: %s"), *argv);
				return BISECT_FAILED;
			}
		} else {
			enum get_oid_result res = get_oid("BISECT_HEAD", &oid);

			if (res == MISSING_OBJECT)
				res = get_oid("HEAD", &oid);
			if (res) {
				error(_("Bad bisect_head rev input"));
				return BISECT_FAILED;
			}
		}

		if (bisect_write(state, oid_to_hex(&oid), terms, 0))
			return BISECT_FAILED;

		if (verify_expected && !oideq(&oid, &expected)) {
			unlink_or_warn(git_path_bisect_ancestors_ok());
			unlink_or_warn(git_path_bisect_expected_rev());
			verify_expected = 0;
		}
	}

	return bisect_auto_next(terms, NULL);
}

There, not bad, is it?

> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -847,7 +907,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 no_checkout = 0, res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -873,6 +934,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 BISECT_START is empty or missing"), BISECT_AUTOSTART),
> +		OPT_CMDMODE(0, "bisect-state", &cmdmode,
> +			 N_("mark the state of ref (or refs)"), BISECT_STATE),
>  		OPT_BOOL(0, "no-checkout", &no_checkout,
>  			 N_("update BISECT_HEAD instead of checking out the current commit")),
>  		OPT_BOOL(0, "no-log", &nolog,
> @@ -945,6 +1008,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", (int)cmdmode);
>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 049ffacdff..2da0810b1a 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 test -f "$GIT_DIR/BISECT_HEAD"
> -	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
>  }
>
>  bisect_visualize() {
> @@ -185,8 +139,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"
> @@ -201,7 +154,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

This is not your fault, of course, but it does make me shudder to see such
an obvious implementation detail in a user-facing error message.

Maybe something to fix up in a follow-up?

Ciao,
Dscho

>  			exit $res
>  		fi
>
> @@ -242,7 +195,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
>
>