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

Re: [PATCH v4 5/6] bisect--helper: reimplement `bisect_run` shell function in C

From
Johannes Schindelin <johannes.schindelin@gmx.de>
Date
Aug 17, 2021, 11:42 UTC
Message-ID
<nycvar.QRO.7.76.6.2108171332370.55@tvgsbejvaqbjf.bet>
In-Reply-To
<20210817081458.53136-6-mirucam@gmail.com>
Hi Miriam,
On Tue, 17 Aug 2021, Miriam Rubio wrote:
Show 45 quoted lines
> From: Tanushree Tumane <tanushreetumane@gmail.com>
>
> Reimplement the `bisect_run()` shell function
> in C and also add `--bisect-run` subcommand to
> `git bisect--helper` to call it from git-bisect.sh.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>  builtin/bisect--helper.c | 75 ++++++++++++++++++++++++++++++++++++++++
>  git-bisect.sh            | 62 +--------------------------------
>  2 files changed, 76 insertions(+), 61 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 4258429c1c..852e0a30fb 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -31,6 +31,7 @@ static const char * const git_bisect_helper_usage[] = {
>  	N_("git bisect--helper --bisect-replay <filename>"),
>  	N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
>  	N_("git bisect--helper --bisect-visualize"),
> +	N_("git bisect--helper --bisect-run <cmd>..."),
>  	NULL
>  };
>
> @@ -1075,6 +1076,71 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a
>  	return res;
>  }
>
> +static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
> +{
> +	int res = BISECT_OK;
> +	struct strbuf command = STRBUF_INIT;
> +	struct strvec args = STRVEC_INIT;
> +	struct strvec run_args = STRVEC_INIT;
> +	int exit = 0;
> +
> +	if (bisect_next_check(terms, NULL))
> +		return BISECT_FAILED;
> +
> +	if (argc)
> +		sq_quote_argv(&command, argv);
> +	else
> +		return BISECT_FAILED;

Do we want to say something helpful here, e.g. _("bisect run failed: no command provided.")?

Show 25 quoted lines
> +
> +	strvec_push(&run_args, command.buf);
> +
> +	while (1) {
> +		strvec_clear(&args);
> +		exit = 1;
> +
> +		printf(_("running %s"), command.buf);
> +		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
> +
> +		if (res < 0 || 128 <= res) {
> +			error(_("bisect run failed: exit code %d from"
> +				" '%s' is < 0 or >= 128"), res, command.buf);
> +			strbuf_release(&command);
> +			return res;
> +		}
> +
> +		if (res == 125)
> +			strvec_push(&args, "skip");
> +		else if (res > 0)
> +			strvec_push(&args, terms->term_bad);
> +		else
> +			strvec_push(&args, terms->term_good);
> +
> +		res = bisect_state(terms, args.v, args.nr);

Since `args.nr` will always be 1, it would probably be better to use something like this:

		const char *new_state;
		[...]
		if (res == 125)
			new_state = "skip";
		else
			new_state = res > 0 ?
				terms->term_bad : terms->term_good;
		res = bisect_state(terms, &new_state, 1);
Also: I think at this stage, an equivalent to `cat "$GIT_DIR/BISECT_RUN"`
is missing.
Show 14 quoted lines
> +
> +		if (res == BISECT_ONLY_SKIPPED_LEFT)
> +			error(_("bisect run cannot continue any more"));
> +		else if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {
> +			printf(_("bisect run success"));
> +			res = BISECT_OK;
> +		} else if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
> +			printf(_("bisect found first bad commit"));
> +			res = BISECT_OK;
> +		} else if (res) {
> +			error(_("bisect run failed:'git bisect--helper --bisect-state"
> +			" %s' exited with error code %d"), args.v[0], res);
> +		} else {
> +			exit = 0;

Since the only purpose of `exit` seems to be that the loop should continue if `exit` is set to 0, and it is only set here, how about doing away with the variable altogether and writing `continue;` instead of `exit = 0;`? Then the conditional block below does not need to be conditional.

Other than that: well done!

Ciao, Dscho

Show 128 quoted lines
> +		}
> +
> +		if (exit) {
> +			strbuf_release(&command);
> +			strvec_clear(&args);
> +			strvec_clear(&run_args);
> +			return res;
> +		}
> +	}
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -1089,6 +1155,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		BISECT_REPLAY,
>  		BISECT_SKIP,
>  		BISECT_VISUALIZE,
> +		BISECT_RUN,
>  	} cmdmode = 0;
>  	int res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -1112,6 +1179,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  			 N_("skip some commits for checkout"), BISECT_SKIP),
>  		OPT_CMDMODE(0, "bisect-visualize", &cmdmode,
>  			 N_("visualize the bisection"), BISECT_VISUALIZE),
> +		OPT_CMDMODE(0, "bisect-run", &cmdmode,
> +			 N_("use <cmd>... to automatically bisect."), BISECT_RUN),
>  		OPT_BOOL(0, "no-log", &nolog,
>  			 N_("no log for BISECT_WRITE")),
>  		OPT_END()
> @@ -1177,6 +1246,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		get_terms(&terms);
>  		res = bisect_visualize(&terms, argv, argc);
>  		break;
> +	case BISECT_RUN:
> +		if (!argc)
> +			return error(_("bisect run failed: no command provided."));
> +		get_terms(&terms);
> +		res = bisect_run(&terms, argv, argc);
> +		break;
>  	default:
>  		BUG("unknown subcommand %d", cmdmode);
>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 95f7f3fb8c..e83d011e17 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -39,66 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
>  TERM_BAD=bad
>  TERM_GOOD=good
>
> -bisect_run () {
> -	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
> -
> -	test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
> -
> -	while true
> -	do
> -		command="$@"
> -		eval_gettextln "running \$command"
> -		"$@"
> -		res=$?
> -
> -		# Check for really bad run error.
> -		if [ $res -lt 0 -o $res -ge 128 ]
> -		then
> -			eval_gettextln "bisect run failed:
> -exit code \$res from '\$command' is < 0 or >= 128" >&2
> -			exit $res
> -		fi
> -
> -		# Find current state depending on run success or failure.
> -		# A special exit code of 125 means cannot test.
> -		if [ $res -eq 125 ]
> -		then
> -			state='skip'
> -		elif [ $res -gt 0 ]
> -		then
> -			state="$TERM_BAD"
> -		else
> -			state="$TERM_GOOD"
> -		fi
> -
> -		git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
> -		res=$?
> -
> -		cat "$GIT_DIR/BISECT_RUN"
> -
> -		if sane_grep "first $TERM_BAD commit could be any of" "$GIT_DIR/BISECT_RUN" \
> -			>/dev/null
> -		then
> -			gettextln "bisect run cannot continue any more" >&2
> -			exit $res
> -		fi
> -
> -		if [ $res -ne 0 ]
> -		then
> -			eval_gettextln "bisect run failed:
> -'bisect-state \$state' exited with error code \$res" >&2
> -			exit $res
> -		fi
> -
> -		if sane_grep "is the first $TERM_BAD commit" "$GIT_DIR/BISECT_RUN" >/dev/null
> -		then
> -			gettextln "bisect run success"
> -			exit 0;
> -		fi
> -
> -	done
> -}
> -
>  get_terms () {
>  	if test -s "$GIT_DIR/BISECT_TERMS"
>  	then
> @@ -137,7 +77,7 @@ case "$#" in
>  	log)
>  		git bisect--helper --bisect-log || exit ;;
>  	run)
> -		bisect_run "$@" ;;
> +		git bisect--helper --bisect-run "$@" || exit;;
>  	terms)
>  		git bisect--helper --bisect-terms "$@" || exit;;
>  	*)
> --
> 2.29.2
>
>
Previous: Miriam RubioNext: Miriam R.
Message 14 of 20 in “Finish converting git bisect to C part 4”
  1. 0/6 Finish converting git bisect to C part 4Miriam Rubio, Aug 17, 2021
  2. 1/6 t6030-bisect-porcelain: add tests to control bisect run exit casesMiriam Rubio, Aug 17, 2021
  3. Bagas SanjayaAug 17, 2021
  4. Christian CouderAug 17, 2021
  5. Miriam R.Aug 17, 2021
  6. 2/6 t6030-bisect-porcelain: add test for bisect visualizeMiriam Rubio, Aug 17, 2021
  7. Bagas SanjayaAug 17, 2021
  8. Miriam R.Aug 17, 2021
  9. 3/6 run-command: make `exists_in_PATH()` non-staticMiriam Rubio, Aug 17, 2021
  10. 4/6 bisect--helper: reimplement `bisect_visualize()`shell function in CMiriam Rubio, Aug 17, 2021
  11. Johannes SchindelinAug 17, 2021
  12. Miriam R.Aug 17, 2021
  13. 5/6 bisect--helper: reimplement `bisect_run` shell function in CMiriam Rubio, Aug 17, 2021
  14. Johannes SchindelinAug 17, 2021
  15. Miriam R.Aug 17, 2021
  16. Johannes SchindelinAug 17, 2021
  17. Christian CouderAug 18, 2021
  18. Miriam R.Aug 18, 2021
  19. 6/6 bisect--helper: retire `--bisect-next-check` subcommandMiriam Rubio, Aug 17, 2021
  20. Johannes SchindelinAug 17, 2021

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.