[PATCH v2 0/3] bisect: add --auto-reset to leave when done

["Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com>]

Add a --auto-reset option to git bisect that resets the bisect session when
culprit is found.

Changes in v2:

 * Add option --auto-reset[=<where>] with option to go to final commit as
   well as original.
 * Refactored tests.

Harald Nordgren (3):
  bisect: read run output from the open descriptor
  bisect: let bisect_reset() optionally check out quietly
  bisect: add --auto-reset to leave when done

 Documentation/git-bisect.adoc |  14 +++-
 bisect.c                      |   2 +
 builtin/bisect.c              | 132 +++++++++++++++++++++++++++++-----
 t/t6030-bisect-porcelain.sh   | 107 +++++++++++++++++++++++++++
 4 files changed, 234 insertions(+), 21 deletions(-)


base-commit: 44de1520f08d1dfebc3ab2d9f644208eaa5ac925
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2335%2FHaraldNordgren%2Fbisect-auto-reset-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2335/HaraldNordgren/bisect-auto-reset-v2
Pull-Request: https://github.com/git/git/pull/2335

Range-diff vs v1:

 1:  3fd365835d = 1:  0de8b12f65 bisect: read run output from the open descriptor
 2:  a7670baafc = 2:  8a2dcdf305 bisect: let bisect_reset() optionally check out quietly
 3:  a9194b1d00 ! 3:  5b3704fbd4 bisect: add --auto-reset to leave when done
     @@ Metadata
       ## Commit message ##
          bisect: add --auto-reset to leave when done
      
     -    When a bisection finished, "git bisect" reported the first bad commit
     -    but left the session active until "git bisect reset" was run by hand.
     +    When a bisection finishes, "git bisect" reports the first bad commit
     +    but leaves the session active until "git bisect reset" is run by hand.
      
     -    Add an "--auto-reset" option, accepted by both "git bisect start" and
     -    "git bisect run", that resets as soon as the first bad commit is found,
     -    returning to the commit checked out before "git bisect start". The flag
     -    is persisted in a BISECT_AUTO_RESET state file and the restoring
     -    checkout is done quietly.
     +    Add an "--auto-reset[=<where>]" option, accepted by both "git bisect
     +    start" and "git bisect run", that resets as soon as the first bad commit
     +    is found. The "original" value returns to the commit checked out before
     +    "git bisect start", while "found" leaves the first bad commit checked
     +    out; omitting the value defaults to "original".
     +
     +    Persist the selected target in a BISECT_AUTO_RESET state file and perform
     +    the reset quietly. Reject this option together with "--no-checkout",
     +    since that mode must not check out either target.
      
          Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>
      
     @@ Documentation/git-bisect.adoc: SYNOPSIS
       [synopsis]
       git bisect start [--term-(bad|new)=<term-new> --term-(good|old)=<term-old>]
      -		 [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...]
     -+		 [--no-checkout] [--first-parent] [--auto-reset] [<bad> [<good>...]] [--] [<pathspec>...]
     ++		 [--no-checkout] [--first-parent] [--auto-reset[=<where>]] [<bad> [<good>...]] [--] [<pathspec>...]
       git bisect (bad|new|<term-new>) [<rev>]
       git bisect (good|old|<term-old>) [<rev>...]
       git bisect terms [--term-(good|old) | --term-(bad|new)]
     @@ Documentation/git-bisect.adoc: git bisect reset [<commit>]
       git bisect replay <logfile>
       git bisect log
      -git bisect run <cmd> [<arg>...]
     -+git bisect run [--auto-reset] <cmd> [<arg>...]
     ++git bisect run [--auto-reset[=<where>]] <cmd> [<arg>...]
       git bisect help
       
       DESCRIPTION
     @@ Documentation/git-bisect.adoc: ignored.
       This option is particularly useful in avoiding false positives when a merged
       branch contained broken or non-buildable commits, but the merge itself was OK.
       
     -+`--auto-reset`::
     -+	Once the first bad commit is found, clean up the bisection state and
     -+	return to the commit that was checked out before `git bisect start`,
     -+	as if `git bisect reset` had been run. The first bad commit is still
     -+	reported before resetting.
     ++`--auto-reset[=<where>]`::
     ++	Once the first bad commit is found, report it and clean up the
     ++	bisection state. `<where>` may be `original` to return to the commit
     ++	checked out before `git bisect start`, or `found` to leave the first
     ++	bad commit checked out. If `<where>` is omitted, it defaults to
     ++	`original`.
      ++
     -+This option may be given to `git bisect start` or to `git bisect run`.
     ++This option may be given to `git bisect start` or to `git bisect run`. It
     ++cannot be used for a bisection started with `--no-checkout`.
      +
       EXAMPLES
       --------
     @@ builtin/bisect.c: static GIT_PATH_FUNC(git_path_bisect_start, "BISECT_START")
       #define BUILTIN_GIT_BISECT_START_USAGE \
       	N_("git bisect start [--term-(bad|new)=<term-new> --term-(good|old)=<term-old>]\n" \
      -	   "                 [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...]")
     -+	   "                 [--no-checkout] [--first-parent] [--auto-reset] [<bad> [<good>...]] [--] [<pathspec>...]")
     ++	   "                 [--no-checkout] [--first-parent] [--auto-reset[=<where>]] [<bad> [<good>...]] [--] [<pathspec>...]")
       #define BUILTIN_GIT_BISECT_BAD_USAGE \
       	N_("git bisect (bad|new|<term-new>) [<rev>]")
       #define BUILTIN_GIT_BISECT_GOOD_USAGE \
     @@ builtin/bisect.c: static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
       	"git bisect log"
       #define BUILTIN_GIT_BISECT_RUN_USAGE \
      -	N_("git bisect run <cmd> [<arg>...]")
     -+	N_("git bisect run [--auto-reset] <cmd> [<arg>...]")
     ++	N_("git bisect run [--auto-reset[=<where>]] <cmd> [<arg>...]")
       #define BUILTIN_GIT_BISECT_HELP_USAGE \
       	"git bisect help"
       
     +@@ builtin/bisect.c: static const char * const git_bisect_usage[] = {
     + 	NULL
     + };
     + 
     ++enum auto_reset_mode {
     ++	AUTO_RESET_NONE,
     ++	AUTO_RESET_ORIGINAL,
     ++	AUTO_RESET_FOUND,
     ++};
     ++
     + struct add_bisect_ref_data {
     + 	struct rev_info *revs;
     + 	unsigned int object_flags;
     +@@ builtin/bisect.c: static int bisect_reset(const char *commit, int quiet)
     + 	return bisect_clean_state();
     + }
     + 
     ++static int parse_auto_reset(const char *value, enum auto_reset_mode *mode)
     ++{
     ++	if (!strcmp(value, "original"))
     ++		*mode = AUTO_RESET_ORIGINAL;
     ++	else if (!strcmp(value, "found"))
     ++		*mode = AUTO_RESET_FOUND;
     ++	else
     ++		return error(_("invalid value for '--auto-reset': '%s'"), value);
     ++
     ++	return 0;
     ++}
     ++
     ++static const char *auto_reset_mode_name(enum auto_reset_mode mode)
     ++{
     ++	switch (mode) {
     ++	case AUTO_RESET_ORIGINAL:
     ++		return "original";
     ++	case AUTO_RESET_FOUND:
     ++		return "found";
     ++	case AUTO_RESET_NONE:
     ++		BUG("no name for unset auto-reset mode");
     ++	}
     ++	BUG("unknown auto-reset mode %d", mode);
     ++}
     ++
     ++static int bisect_auto_reset(struct bisect_terms *terms)
     ++{
     ++	struct strbuf value = STRBUF_INIT;
     ++	enum auto_reset_mode mode;
     ++	char *commit = NULL;
     ++	int res;
     ++
     ++	if (strbuf_read_file(&value, git_path_bisect_auto_reset(), 0) < 0) {
     ++		res = error_errno(_("could not read '%s'"),
     ++				  git_path_bisect_auto_reset());
     ++		goto cleanup;
     ++	}
     ++	strbuf_trim(&value);
     ++	if (parse_auto_reset(value.buf, &mode)) {
     ++		res = -1;
     ++		goto cleanup;
     ++	}
     ++
     ++	if (mode == AUTO_RESET_FOUND)
     ++		commit = xstrfmt("refs/bisect/%s", terms->term_bad);
     ++	res = bisect_reset(commit, 1);
     ++
     ++cleanup:
     ++	free(commit);
     ++	strbuf_release(&value);
     ++	return res;
     ++}
     ++
     + static void log_commit(FILE *fp,
     + 		       const char *fmt, const char *state,
     + 		       struct commit *commit)
      @@ builtin/bisect.c: static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
       
       	if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
       		res = bisect_successful(terms);
      +		if (!res && !is_empty_or_missing_file(git_path_bisect_auto_reset()))
     -+			res = bisect_reset(NULL, 1);
     ++			res = bisect_auto_reset(terms);
       		return res ? res : BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;
       	} else if (res == BISECT_ONLY_SKIPPED_LEFT) {
       		res = bisect_skipped_commits(terms);
     @@ builtin/bisect.c: static enum bisect_error bisect_start(struct bisect_terms *ter
       {
       	int no_checkout = 0;
       	int first_parent_only = 0;
     -+	int auto_reset = 0;
     ++	enum auto_reset_mode auto_reset = AUTO_RESET_NONE;
       	int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
       	int flags, pathspec_pos;
       	enum bisect_error res = BISECT_OK;
     @@ builtin/bisect.c: static enum bisect_error bisect_start(struct bisect_terms *ter
       		} else if (!strcmp(arg, "--first-parent")) {
       			first_parent_only = 1;
      +		} else if (!strcmp(arg, "--auto-reset")) {
     -+			auto_reset = 1;
     ++			auto_reset = AUTO_RESET_ORIGINAL;
     ++		} else if (skip_prefix(arg, "--auto-reset=", &arg)) {
     ++			if (parse_auto_reset(arg, &auto_reset)) {
     ++				res = BISECT_FAILED;
     ++				goto finish;
     ++			}
       		} else if (!strcmp(arg, "--term-good") ||
       			 !strcmp(arg, "--term-old")) {
       			i++;
     +@@ builtin/bisect.c: static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
     + 			break;
     + 		}
     + 	}
     ++	if (auto_reset != AUTO_RESET_NONE && no_checkout) {
     ++		res = error(_("'--auto-reset' cannot be used with '--no-checkout'"));
     ++		goto finish;
     ++	}
     + 	pathspec_pos = i;
     + 
     + 	/*
      @@ builtin/bisect.c: static enum bisect_error bisect_start(struct bisect_terms *terms, int argc,
       	if (first_parent_only)
       		write_file(git_path_bisect_first_parent(), "\n");
       
     -+	if (auto_reset)
     -+		write_file(git_path_bisect_auto_reset(), "\n");
     ++	if (auto_reset != AUTO_RESET_NONE)
     ++		write_file(git_path_bisect_auto_reset(), "%s\n",
     ++			   auto_reset_mode_name(auto_reset));
      +
       	if (no_checkout) {
       		if (repo_get_oid(the_repository, start_head.buf, &oid) < 0) {
       			res = error(_("invalid ref: '%s'"), start_head.buf);
     +@@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, int argc, const char **argv)
     + {
     + 	int res = BISECT_OK;
     + 	struct strbuf command = STRBUF_INIT;
     ++	enum auto_reset_mode auto_reset = AUTO_RESET_NONE;
     ++	const char *auto_reset_arg;
     + 	const char *new_state;
     + 	int temporary_stdout_fd, saved_stdout;
     + 	int is_first_run = 1;
      @@ builtin/bisect.c: 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], "--auto-reset")) {
     -+		write_file(git_path_bisect_auto_reset(), "\n");
     ++	if (argc && !strcmp(argv[0], "--auto-reset"))
     ++		auto_reset = AUTO_RESET_ORIGINAL;
     ++	else if (argc && skip_prefix(argv[0], "--auto-reset=", &auto_reset_arg)) {
     ++		if (parse_auto_reset(auto_reset_arg, &auto_reset))
     ++			return BISECT_FAILED;
     ++	}
     ++
     ++	if (auto_reset != AUTO_RESET_NONE) {
     ++		if (refs_ref_exists(get_main_ref_store(the_repository), "BISECT_HEAD"))
     ++			return error(_("'--auto-reset' cannot be used with '--no-checkout'"));
     ++		write_file(git_path_bisect_auto_reset(), "%s\n",
     ++			   auto_reset_mode_name(auto_reset));
      +		argc--;
      +		argv++;
      +	}
     @@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, int argc, co
       		return BISECT_FAILED;
      
       ## t/t6030-bisect-porcelain.sh ##
     +@@ t/t6030-bisect-porcelain.sh: 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")"
     ++}
     ++
     ++bisect_start_and_finish () {
     ++	git bisect start "$1" $HASH4 $HASH2 &&
     ++	git bisect bad
     ++}
     ++
     ++bisect_run_auto_reset () {
     ++	write_script test_script.sh <<-\EOF &&
     ++	! grep Another hello >/dev/null
     ++	EOF
     ++	git bisect start $HASH4 $HASH2 &&
     ++	git bisect run "$1" ./test_script.sh >my_bisect_log.txt &&
     ++	test_grep "$HASH3 is the first .bad. commit" my_bisect_log.txt
     ++}
     ++
     ++test_auto_reset_fails () {
     ++	local pattern="$1" &&
     ++	local state_file="$2" &&
     ++	shift 2 &&
     ++	test_must_fail "$@" 2>err &&
     ++	test_grep -- "$pattern" err &&
     ++	test_bisect_state_missing "$state_file"
     ++}
     ++
     + test_expect_success 'bisect usage' "
     + 	test_bisect_usage 1 git bisect reset extra1 extra2 <<-\EOF &&
     + 	error: 'git bisect reset' requires either no argument or a commit
      @@ t/t6030-bisect-porcelain.sh: test_expect_success '"git bisect run" simple case' '
       	git bisect reset
       '
       
     -+test_expect_success '"git bisect start --auto-reset" leaves the bisection' '
     -+	test_when_finished "git bisect reset" &&
     -+	git bisect start --auto-reset $HASH4 $HASH2 &&
     -+	git bisect bad &&
     -+	test_path_is_missing "$(git rev-parse --git-path BISECT_START)"
     ++test_expect_success '"git bisect start --auto-reset" defaults to original' '
     ++	test_when_finished "git bisect reset; git checkout main" &&
     ++	git checkout main &&
     ++	bisect_start_and_finish --auto-reset &&
     ++	test "$HASH4" = "$(git rev-parse HEAD)" &&
     ++	test main = "$(git branch --show-current)" &&
     ++	test_bisect_state_missing BISECT_START &&
     ++
     ++	bisect_start_and_finish --auto-reset=original &&
     ++	test "$HASH4" = "$(git rev-parse HEAD)" &&
     ++	test main = "$(git branch --show-current)" &&
     ++	test_bisect_state_missing BISECT_START
      +'
      +
     -+test_expect_success '"git bisect run --auto-reset" leaves the bisection' '
     -+	test_when_finished "git bisect reset" &&
     -+	write_script test_script.sh <<-\EOF &&
     -+	! grep Another hello >/dev/null
     -+	EOF
     ++test_expect_success '"git bisect start --auto-reset=found" leaves first bad checked out' '
     ++	test_when_finished "git bisect reset; git checkout main" &&
     ++	bisect_start_and_finish --auto-reset=found &&
     ++	test "$HASH3" = "$(git rev-parse HEAD)" &&
     ++	test_bisect_state_missing BISECT_START
     ++'
     ++
     ++test_expect_success '"git bisect run --auto-reset" defaults to original' '
     ++	test_when_finished "git bisect reset; git checkout main" &&
     ++	bisect_run_auto_reset --auto-reset &&
     ++	test "$HASH4" = "$(git rev-parse HEAD)" &&
     ++	test main = "$(git branch --show-current)" &&
     ++	test_bisect_state_missing BISECT_START
     ++'
     ++
     ++test_expect_success '"git bisect run --auto-reset=found" leaves first bad checked out' '
     ++	test_when_finished "git bisect reset; git checkout main" &&
     ++	bisect_run_auto_reset --auto-reset=found &&
     ++	test "$HASH3" = "$(git rev-parse HEAD)" &&
     ++	test_bisect_state_missing BISECT_START
     ++'
     ++
     ++test_expect_success '--auto-reset rejects an unknown reset target' '
     ++	test_when_finished "git bisect reset; git checkout main" &&
     ++	test_auto_reset_fails \
     ++		"invalid value for.*--auto-reset.*unknown" BISECT_START \
     ++		git bisect start --auto-reset=unknown $HASH4 $HASH2 &&
     ++
      +	git bisect start $HASH4 $HASH2 &&
     -+	git bisect run --auto-reset ./test_script.sh >my_bisect_log.txt &&
     -+	grep "$HASH3 is the first .bad. commit" my_bisect_log.txt &&
     -+	test_path_is_missing "$(git rev-parse --git-path BISECT_START)"
     ++	test_auto_reset_fails \
     ++		"invalid value for.*--auto-reset.*unknown" BISECT_AUTO_RESET \
     ++		git bisect run --auto-reset=unknown true
     ++'
     ++
     ++test_expect_success '--auto-reset cannot be used with --no-checkout' '
     ++	test_when_finished "git bisect reset" &&
     ++	test_auto_reset_fails \
     ++		"cannot be used with.*--no-checkout" BISECT_START \
     ++		git bisect start --auto-reset=original --no-checkout $HASH4 $HASH2 &&
     ++
     ++	git bisect start --no-checkout $HASH4 $HASH2 &&
     ++	test_auto_reset_fails \
     ++		"cannot be used with.*--no-checkout" BISECT_AUTO_RESET \
     ++		git bisect run --auto-reset=found true
      +'
      +
      +test_expect_success 'without --auto-reset the bisection state is kept' '
      +	test_when_finished "git bisect reset" &&
      +	git bisect start $HASH4 $HASH2 &&
      +	git bisect bad &&
     -+	test_path_is_file "$(git rev-parse --git-path BISECT_START)"
     ++	test_bisect_state_file BISECT_START
      +'
      +
      +test_expect_success '--auto-reset does not leak into a later bisection' '
     -+	test_when_finished "git bisect reset" &&
     -+	git bisect start --auto-reset $HASH4 $HASH2 &&
     -+	git bisect bad &&
     ++	test_when_finished "git bisect reset; git checkout main" &&
     ++	bisect_start_and_finish --auto-reset &&
     ++
      +	git bisect start $HASH4 $HASH2 &&
      +	git bisect bad &&
     -+	test_path_is_file "$(git rev-parse --git-path BISECT_START)"
     ++	test_bisect_state_file BISECT_START
      +'
      +
       # We want to automatically find the commit that

-- 
gitgitgadget