Re: [PATCH v3 2/2] bisect: add --reset-when-found to leave when done

[Johannes Sixt <j6t@kdbg.org>]

Am 20.07.26 um 11:10 schrieb Harald Nordgren via GitGitGadget:
> @@ -784,6 +859,10 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
>  			break;
>  		}
>  	}
> +	if (reset_when_found != RESET_WHEN_FOUND_NONE && no_checkout) {
> +		res = error(_("'--reset-when-found' cannot be used with '--no-checkout'"));

We have a boilerplate text for this kind of error that saves a translation:

		res = error(_("options '%s' and '%s' cannot be used together"),
"--reset-when-found", "--no-checkout");

> +		goto finish;
> +	}
>  	pathspec_pos = i;
>  
>  	/*

> @@ -1246,6 +1331,23 @@ static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
>  	if (bisect_next_check(terms, NULL))
>  		return BISECT_FAILED;
>  
> +	if (argc && !strcmp(argv[0], "--reset-when-found"))
> +		reset_when_found = RESET_WHEN_FOUND_TO_ORIGINAL;
> +	else if (argc && skip_prefix(argv[0], "--reset-when-found=",
> +				    &reset_when_found_arg)) {
> +		if (parse_reset_when_found(reset_when_found_arg, &reset_when_found))
> +			return BISECT_FAILED;
> +	}
> +
> +	if (reset_when_found != RESET_WHEN_FOUND_NONE) {
> +		if (refs_ref_exists(get_main_ref_store(the_repository), "BISECT_HEAD"))
> +			return error(_("'--reset-when-found' cannot be used with '--no-checkout'"));

Ditto.

> +		write_file(git_path_bisect_reset_when_found(), "%s\n",
> +			   reset_when_found_mode_name(reset_when_found));
> +		argc--;
> +		argv++;
> +	}
> +
>  	if (!argc) {
>  		error(_("bisect run failed: no command provided."));
>  		return BISECT_FAILED;

> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 081116220a..7dfb871ab9 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -43,6 +43,38 @@ test_bisect_usage () {
>  	test_cmp expect actual
>  }
>  
> +test_bisect_state_file () {
> +	test_path_is_file "$(git rev-parse --git-path "$1")"
> +}
> +
> +test_bisect_state_missing () {
> +	test_path_is_missing "$(git rev-parse --git-path "$1")"
> +}

These should not use `git` in a $( ) subshell to avoid a case of "ignore
failure in upstream of pipe". Note that

	local file=$(git rev-parse ...) &&
	test_path...

would be wrong, too, for the same reason. But

	local file
	file=$(git rev-parse ...) &&
	test_path...

works as desired.

> +test_expect_success '"git bisect start --reset-when-found" defaults to original' '
> +	test_when_finished "git bisect reset; git checkout main" &&

Looking at other cases where more than one git command is invoked by
test_when_finished, it seems that they are chained with '&&'. `git grepc
"&& git checkout main"` does find a few hits.

> +	git checkout main &&
> +	bisect_start_and_finish --reset-when-found &&
> +	test "$HASH4" = "$(git rev-parse HEAD)" &&
> +	test main = "$(git branch --show-current)" &&
> +	test_bisect_state_missing BISECT_START &&
> +
> +	bisect_start_and_finish --reset-when-found=original &&
> +	test "$HASH4" = "$(git rev-parse HEAD)" &&
> +	test main = "$(git branch --show-current)" &&
> +	test_bisect_state_missing BISECT_START
> +'

More cases of `git` in a subshell above and below. I notice that you are
mimicking existing practice in this file. I'm torn whether to change
this or not. After all, there are also a lot of cases in the file that
uses the correct pattern where the subshell is in a variable assignment.

> +
> +test_expect_success '"git bisect start --reset-when-found=found" leaves first bad checked out' '
> +	test_when_finished "git bisect reset; git checkout main" &&
> +	bisect_start_and_finish --reset-when-found=found &&
> +	test "$HASH3" = "$(git rev-parse HEAD)" &&
> +	test_bisect_state_missing BISECT_START
> +'
> +
> +test_expect_success '"git bisect run --reset-when-found" defaults to original' '
> +	test_when_finished "git bisect reset; git checkout main" &&
> +	bisect_run_reset_when_found --reset-when-found &&
> +	test "$HASH4" = "$(git rev-parse HEAD)" &&
> +	test main = "$(git branch --show-current)" &&
> +	test_bisect_state_missing BISECT_START
> +'
> +
> +test_expect_success '"git bisect run --reset-when-found=found" leaves first bad checked out' '
> +	test_when_finished "git bisect reset; git checkout main" &&
> +	bisect_run_reset_when_found --reset-when-found=found &&
> +	test "$HASH3" = "$(git rev-parse HEAD)" &&
> +	test_bisect_state_missing BISECT_START
> +'
-- Hannes