[Outreachy][PATCH 00/10] Finish converting git bisect to C part 2

[Outreachy][PATCH 00/10] Finish converting git bisect to C part 2

[Miriam Rubio]

These patches correspond to a second part of patch series 
of Outreachy project "Finish converting `git bisect` from shell to C" 
started by Pranit Bauva and Tanushree Tumane
(https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
continued by me.

This second part depends on mr/bisect-in-c-1 and it is formed by 
reimplementations of some `git bisect` subcommands and removal of some
temporary subcommands.

These patch series emails were generated from:
https://gitlab.com/mirucam/git/commits/git-bisect-work-part2.

General changes
---------------

* Rebase on next branch: 5900a2a8f9 (Sync with master, 2020-02-25).
* Improve commit messages.
* Amend patch series titles.
* Reorder commits.

Specific changes
----------------

[1/10] bisect--helper: introduce new `write_in_file()` function

* New patch to refactor code in `write_in_file()` function and use it in
`write_terms()`.

--

[2/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

* Fix `repo_init_revisions()` and bisect_next_all` calls after 
rebase on master.
* Add blank line.
* Remove goto statements in `bisect_skipped_commits()`.
* Add `clear_commit_marks()` in `process_skipped_commits()`.
* Use `write_in_file()` in `bisect_successful()`.
* Change header return type and internal returns to enum in 
`bisect_next()` and `bisect_auto_next()`.
* Improve code comments.

--

[3/10] bisect--helper: finish porting `bisect_start()` to C

* Change header return type and internal returns to enum in `bisect_start()`.
* Improve code comments. 

--

[6/10] bisect--helper: reimplement `bisect_autostart` shell function in C

* Change an internal return to enum in `bisect_autostart()`.

--

[7/10] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C

* Change header return type and internal returns to enum in `bisect_state()`.

--

Miriam Rubio (1):
  bisect--helper: introduce new `write_in_file()` function

Pranit Bauva (9):
  bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
    functions in C
  bisect--helper: finish porting `bisect_start()` to C
  bisect--helper: retire `--bisect-clean-state` subcommand
  bisect--helper: retire `--next-all` subcommand
  bisect--helper: reimplement `bisect_autostart` shell function in C
  bisect--helper: reimplement `bisect_state` & `bisect_head` shell
    functions in C
  bisect--helper: retire `--check-expected-revs` subcommand
  bisect--helper: retire `--write-terms` subcommand
  bisect--helper: retire `--bisect-autostart` subcommand

 bisect.c                 |  11 ++
 builtin/bisect--helper.c | 388 ++++++++++++++++++++++++++++++++++-----
 git-bisect.sh            | 145 +--------------
 3 files changed, 358 insertions(+), 186 deletions(-)

-- 
2.25.0

[PATCH 01/10] bisect--helper: introduce new `write_in_file()` function

[Miriam Rubio]

Let's refactor code adding a new `write_in_file()` function
that opens a file for writing a message and closes it.

This removes some duplicated code and makes the code simpler,
clearer and easier to understand.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c1c40b516d..ee1be630da 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -74,6 +74,19 @@ static int one_of(const char *term, ...)
 	return res;
 }
 
+static int write_in_file(const char *filepath, const char *content, int append)
+{
+	FILE *fp = NULL;
+	const char *mode = append ? "a" : "w";
+
+	fp = fopen(filepath, mode);
+	if (!fp)
+		return error_errno(_("could not open the file '%s'"), filepath);
+	if (!fprintf(fp, "%s\n", content))
+		return error_errno(_("could not write in file '%s'"), filepath);
+	return fclose(fp);
+}
+
 static int check_term_format(const char *term, const char *orig_term)
 {
 	int res;
@@ -104,7 +117,7 @@ static int check_term_format(const char *term, const char *orig_term)
 
 static int write_terms(const char *bad, const char *good)
 {
-	FILE *fp = NULL;
+	char *content = xstrfmt("%s\n%s", bad, good);
 	int res;
 
 	if (!strcmp(bad, good))
@@ -113,12 +126,9 @@ static int write_terms(const char *bad, const char *good)
 	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
 		return -1;
 
-	fp = fopen(git_path_bisect_terms(), "w");
-	if (!fp)
-		return error_errno(_("could not open the file BISECT_TERMS"));
+	res = write_in_file(git_path_bisect_terms(), content, 0);
+	free(content);
 
-	res = fprintf(fp, "%s\n%s\n", bad, good);
-	res |= fclose(fp);
 	return (res < 0) ? -1 : 0;
 }
 
-- 
2.25.0

[PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Miriam Rubio]

From: Pranit Bauva <pranit.bauva@gmail.com>

Reimplement the `bisect_next()` and the `bisect_auto_next()` shell functions
in C and add the subcommands to `git bisect--helper` to call them from
git-bisect.sh .

Using `--bisect-next` and `--bisect-auto-start` subcommands is a
temporary measure to port shell function to C so as to use the existing
test suite. As more functions are ported, `--bisect-auto-start`
subcommand will be retired and will be called by some other methods.

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>
---
 bisect.c                 |  11 +++
 builtin/bisect--helper.c | 172 ++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            |  47 +----------
 3 files changed, 186 insertions(+), 44 deletions(-)

diff --git a/bisect.c b/bisect.c
index 9154f810f7..a50278ea7e 100644
--- a/bisect.c
+++ b/bisect.c
@@ -635,6 +635,11 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
 	struct argv_array rev_argv = ARGV_ARRAY_INIT;
 	int i;
 
+	/*
+	 * `revs` could has been used before.
+	 * Thus we first need to reset it.
+	 */
+	reset_revision_walk();
 	repo_init_revisions(r, revs, prefix);
 	revs->abbrev = 0;
 	revs->commit_format = CMIT_FMT_UNSPECIFIED;
@@ -980,6 +985,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
  * the bisection process finished successfully.
  * In this case the calling function or command should not turn a
  * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.
+ *
+ * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND
+ * in bisect_helper::bisect_next() and only transforming it to 0 at
+ * the end of bisect_helper::cmd_bisect__helper() helps bypassing
+ * all the code related to finding a commit to test.
+ *
  * If no_checkout is non-zero, the bisection process does not
  * checkout the trial commit but instead simply updates BISECT_HEAD.
  */
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index ee1be630da..c82ffe9c1c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -8,6 +8,7 @@
 #include "run-command.h"
 #include "prompt.h"
 #include "quote.h"
+#include "revision.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
 	N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
 					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
+	N_("git bisect--helper --bisect-next"),
+	N_("git bisect--helper --bisect-auto-next"),
 	NULL
 };
 
@@ -431,6 +434,155 @@ static int bisect_append_log_quoted(const char **argv)
 	return res;
 }
 
+static int register_good_ref(const char *refname,
+			     const struct object_id *oid, int flags,
+			     void *cb_data)
+{
+	struct string_list *good_refs = cb_data;
+
+	string_list_append(good_refs, oid_to_hex(oid));
+	return 0;
+}
+
+static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
+{
+	struct string_list good_revs = STRING_LIST_INIT_DUP;
+	char *term_good = xstrfmt("%s-*", terms->term_good);
+
+	for_each_glob_ref_in(register_good_ref, term_good,
+			     "refs/bisect/", &good_revs);
+
+	argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
+	for (int i = 0; i < good_revs.nr; i++)
+		argv_array_push(rev_argv, good_revs.items[i].string);
+
+	string_list_clear(&good_revs, 0);
+	free(term_good);
+}
+
+static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
+{
+	int res = 0;
+	struct argv_array rev_argv = ARGV_ARRAY_INIT;
+
+	prepare_rev_argv(terms, &rev_argv);
+
+	/*
+	 * It is important to reset the flags used by revision walks
+	 * as the previous call to bisect_next_all() in turn
+	 * setups a revision walk.
+	 */
+	reset_revision_walk();
+	init_revisions(revs, NULL);
+	rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);
+	if (prepare_revision_walk(revs))
+		res = error(_("revision walk setup failed\n"));
+
+	argv_array_clear(&rev_argv);
+	return res;
+}
+
+static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)
+{
+	struct commit *commit;
+	struct pretty_print_context pp = {0};
+
+	if (fprintf(fp, "# only skipped commits left to test\n") < 1)
+		return -1;
+
+	while ((commit = get_revision(revs)) != NULL) {
+		struct strbuf commit_name = STRBUF_INIT;
+		format_commit_message(commit, "%s",
+				      &commit_name, &pp);
+		fprintf(fp, "# possible first %s commit: [%s] %s\n",
+			terms->term_bad, oid_to_hex(&commit->object.oid),
+			commit_name.buf);
+		strbuf_release(&commit_name);
+		clear_commit_marks(commit, ALL_REV_FLAGS);
+	}
+
+	return 0;
+}
+
+static int bisect_skipped_commits(struct bisect_terms *terms)
+{
+	int res = 0;
+	FILE *fp = NULL;
+	struct rev_info revs;
+
+	fp = fopen(git_path_bisect_log(), "a");
+	if (!fp)
+		return error_errno(_("could not open '%s' for appending"),
+				  git_path_bisect_log());
+
+	res = prepare_revs(terms, &revs);
+
+	if (!res)
+		res = process_skipped_commits(fp, terms, &revs);
+
+	fclose(fp);
+	return res;
+}
+
+static int bisect_successful(struct bisect_terms *terms)
+{
+	struct object_id oid;
+	struct commit *commit;
+	struct pretty_print_context pp = {0};
+	struct strbuf commit_name = STRBUF_INIT;
+	char *bad_ref = xstrfmt("refs/bisect/%s",
+				terms->term_bad);
+	char *content;
+	int res;
+
+	read_ref(bad_ref, &oid);
+	printf("%s\n", bad_ref);
+	commit = lookup_commit_reference(the_repository, &oid);
+	format_commit_message(commit, "%s", &commit_name, &pp);
+
+	content = xstrfmt("# first %s commit: [%s] %s",
+	terms->term_bad, oid_to_hex(&oid),
+	commit_name.buf);
+
+	res = write_in_file(git_path_bisect_log(), content, 1);
+
+	strbuf_release(&commit_name);
+	free(bad_ref);
+	free(content);
+	return res;
+}
+
+static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
+{
+	int no_checkout;
+	enum bisect_error res;
+
+	if (bisect_next_check(terms, terms->term_good))
+		return BISECT_FAILED;
+
+	no_checkout = !is_empty_or_missing_file(git_path_bisect_head());
+
+	/* Perform all bisection computation, display and checkout */
+	res = bisect_next_all(the_repository, prefix, no_checkout);
+
+	if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
+		res = bisect_successful(terms);
+		return res ? res : BISECT_INTERNAL_SUCCESS_MERGE_BASE;
+	} else if (res == BISECT_ONLY_SKIPPED_LEFT) {
+		res = bisect_skipped_commits(terms);
+		return res ? res : BISECT_ONLY_SKIPPED_LEFT;
+	}
+	return res;
+}
+
+static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
+{
+	if (bisect_next_check(terms, NULL))
+		return BISECT_OK;
+
+	return bisect_next(terms, prefix);
+}
+
 static int bisect_start(struct bisect_terms *terms, int no_checkout,
 			const char **argv, int argc)
 {
@@ -635,7 +787,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		CHECK_AND_SET_TERMS,
 		BISECT_NEXT_CHECK,
 		BISECT_TERMS,
-		BISECT_START
+		BISECT_START,
+		BISECT_NEXT,
+		BISECT_AUTO_NEXT,
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
@@ -659,6 +813,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("print out the bisect terms"), BISECT_TERMS),
 		OPT_CMDMODE(0, "bisect-start", &cmdmode,
 			 N_("start the bisect session"), BISECT_START),
+		OPT_CMDMODE(0, "bisect-next", &cmdmode,
+			 N_("find the next bisection commit"), BISECT_NEXT),
+		OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
+			 N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_BOOL(0, "no-log", &nolog,
@@ -720,6 +878,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		set_terms(&terms, "bad", "good");
 		res = bisect_start(&terms, no_checkout, argv, argc);
 		break;
+	case BISECT_NEXT:
+		if (argc)
+			return error(_("--bisect-next requires 0 arguments"));
+		get_terms(&terms);
+		res = bisect_next(&terms, prefix);
+		break;
+	case BISECT_AUTO_NEXT:
+		if (argc)
+			return error(_("--bisect-auto-next requires 0 arguments"));
+		get_terms(&terms);
+		res = bisect_auto_next(&terms, prefix);
+		break;
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index efee12b8b1..e03f210d55 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -86,8 +86,7 @@ bisect_start() {
 	#
 	# Check if we can proceed to the next bisect state.
 	#
-	get_terms
-	bisect_auto_next
+	git bisect--helper --bisect-auto-next || exit
 
 	trap '-' 0
 }
@@ -140,45 +139,7 @@ bisect_state() {
 	*)
 		usage ;;
 	esac
-	bisect_auto_next
-}
-
-bisect_auto_next() {
-	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :
-}
-
-bisect_next() {
-	case "$#" in 0) ;; *) usage ;; esac
-	bisect_autostart
-	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
-
-	# Perform all bisection computation, display and checkout
-	git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo --no-checkout)
-	res=$?
-
-	# Check if we should exit because bisection is finished
-	if test $res -eq 10
-	then
-		bad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)
-		bad_commit=$(git show-branch $bad_rev)
-		echo "# first $TERM_BAD commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
-		exit 0
-	elif test $res -eq 2
-	then
-		echo "# only skipped commits left to test" >>"$GIT_DIR/BISECT_LOG"
-		good_revs=$(git for-each-ref --format="%(objectname)" "refs/bisect/$TERM_GOOD-*")
-		for skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)
-		do
-			skipped_commit=$(git show-branch $skipped)
-			echo "# possible first $TERM_BAD commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG"
-		done
-		exit $res
-	fi
-
-	# Check for an error in the bisection process
-	test $res -ne 0 && exit $res
-
-	return 0
+	git bisect--helper --bisect-auto-next
 }
 
 bisect_visualize() {
@@ -232,7 +193,7 @@ bisect_replay () {
 			die "$(gettext "?? what are you talking about?")" ;;
 		esac
 	done <"$file"
-	bisect_auto_next
+	git bisect--helper --bisect-auto-next
 }
 
 bisect_run () {
@@ -329,7 +290,7 @@ case "$#" in
 		bisect_skip "$@" ;;
 	next)
 		# Not sure we want "next" at the UI level anymore.
-		bisect_next "$@" ;;
+		git bisect--helper --bisect-next "$@" || exit ;;
 	visualize|view)
 		bisect_visualize "$@" ;;
 	reset)
-- 
2.25.0

[PATCH 03/10] bisect--helper: finish porting `bisect_start()` to C

[Miriam Rubio]

From: Pranit Bauva <pranit.bauva@gmail.com>

Add the subcommand to `git bisect--helper` and call it from
git-bisect.sh.

With the conversion of `bisect_auto_next()` from shell to C in a
previous commit, `bisect_start()` can now be fully ported to C.

So let's complete the `--bisect-start` subcommand of
`git bisect--helper` so that it fully implements `bisect_start()`,
and let's use this subcommand in `git-bisect.sh` instead of
`bisect_start()`.

This removes the signal handling we had in `bisect_start()` as we
don't really need it. While at it, "trap" is changed to "handle".
As "trap" is a reference to the shell "trap" builtin, which isn't
used anymore.

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 | 54 +++++++++++++++++++++++++++++++---------
 git-bisect.sh            | 28 +++------------------
 2 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c82ffe9c1c..e75e7a0837 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -583,11 +583,12 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char
 	return bisect_next(terms, prefix);
 }
 
-static int bisect_start(struct bisect_terms *terms, int no_checkout,
+static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkout,
 			const char **argv, int argc)
 {
 	int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
-	int flags, pathspec_pos, res = 0;
+	int flags, pathspec_pos;
+	enum bisect_error res = BISECT_OK;
 	struct string_list revs = STRING_LIST_INIT_DUP;
 	struct string_list states = STRING_LIST_INIT_DUP;
 	struct strbuf start_head = STRBUF_INIT;
@@ -640,9 +641,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 			return error(_("unrecognized option: '%s'"), arg);
 		} else {
 			char *commit_id = xstrfmt("%s^{commit}", arg);
-			if (get_oid(commit_id, &oid) && has_double_dash)
-				die(_("'%s' does not appear to be a valid "
-				      "revision"), arg);
+			if (get_oid(commit_id, &oid) && has_double_dash) {
+				error(_("'%s' does not appear to be a valid "
+					"revision"), arg);
+				free(commit_id);
+				return BISECT_FAILED;
+			}
 
 			string_list_append(&revs, oid_to_hex(&oid));
 			free(commit_id);
@@ -720,12 +724,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 	 * Get rid of any old bisect state.
 	 */
 	if (bisect_clean_state())
-		return -1;
+		return BISECT_FAILED;
 
 	/*
-	 * In case of mistaken revs or checkout error, or signals received,
+	 * In case of mistaken revs or checkout error,
 	 * "bisect_auto_next" below may exit or misbehave.
-	 * We have to trap this to be able to clean up using
+	 * We have to handle this to be able to clean up using
 	 * "bisect_clean_state".
 	 */
 
@@ -741,7 +745,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 		}
 		if (update_ref(NULL, "BISECT_HEAD", &oid, NULL, 0,
 			       UPDATE_REFS_MSG_ON_ERR)) {
-			res = -1;
+			res = BISECT_FAILED;
 			goto finish;
 		}
 	}
@@ -753,25 +757,51 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 	for (i = 0; i < states.nr; i++)
 		if (bisect_write(states.items[i].string,
 				 revs.items[i].string, terms, 1)) {
-			res = -1;
+			res = BISECT_FAILED;
 			goto finish;
 		}
 
 	if (must_write_terms && write_terms(terms->term_bad,
 					    terms->term_good)) {
-		res = -1;
+		res = BISECT_FAILED;
 		goto finish;
 	}
 
 	res = bisect_append_log_quoted(argv);
 	if (res)
-		res = -1;
+		res = BISECT_FAILED;
 
 finish:
 	string_list_clear(&revs, 0);
 	string_list_clear(&states, 0);
 	strbuf_release(&start_head);
 	strbuf_release(&bisect_names);
+	if (res)
+		return res;
+
+	res = bisect_auto_next(terms, NULL);
+	/*
+	 * In case of mistaken revs or checkout error, or signals received,
+	 * "bisect_auto_next" below may exit or misbehave.
+	 * We have to handle this to be able to clean up using
+	 * "bisect_clean_state".
+	 * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE is special code
+	 * that indicates special success.
+	 *	-> bisect_start()
+	 *	   . res = bisect_auto_next()
+	 *	    -> bisect_auto_next()
+	 *	       . return bisect_next()
+	 *	       -> bisect_next()
+	 *		  . res = bisect_next_all()
+	 *		  -> bisect_next_all()
+	 *		     . res = check_good_are_ancestors_of_bad()
+	 *		     -> check_good_are_ancestors_of_bad()
+	 *			. res = check_merge_bases()
+	 *			-> check_merge_bases()
+	 *			   . res = BISECT_INTERNAL_SUCCESS_MERGE_BASE
+	 */
+	if (res && res != BISECT_INTERNAL_SUCCESS_MERGE_BASE)
+		bisect_clean_state();
 	return res;
 }
 
diff --git a/git-bisect.sh b/git-bisect.sh
index e03f210d55..166f6a64dd 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -63,34 +63,13 @@ bisect_autostart() {
 			[Nn]*)
 				exit ;;
 			esac
-			bisect_start
+			git bisect--helper --bisect-start
 		else
 			exit 1
 		fi
 	}
 }
 
-bisect_start() {
-	git bisect--helper --bisect-start $@ || exit
-
-	#
-	# Change state.
-	# In case of mistaken revs or checkout error, or signals received,
-	# "bisect_auto_next" below may exit or misbehave.
-	# We have to trap this to be able to clean up using
-	# "bisect_clean_state".
-	#
-	trap 'git bisect--helper --bisect-clean-state' 0
-	trap 'exit 255' 1 2 3 15
-
-	#
-	# Check if we can proceed to the next bisect state.
-	#
-	git bisect--helper --bisect-auto-next || exit
-
-	trap '-' 0
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -183,8 +162,7 @@ bisect_replay () {
 		get_terms
 		case "$command" in
 		start)
-			cmd="bisect_start $rev"
-			eval "$cmd" ;;
+			eval "git bisect--helper --bisect-start $rev" ;;
 		"$TERM_GOOD"|"$TERM_BAD"|skip)
 			git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
 		terms)
@@ -283,7 +261,7 @@ case "$#" in
 	help)
 		git bisect -h ;;
 	start)
-		bisect_start "$@" ;;
+		git bisect--helper --bisect-start "$@" ;;
 	bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
 		bisect_state "$cmd" "$@" ;;
 	skip)
-- 
2.25.0

[PATCH 04/10] bisect--helper: retire `--bisect-clean-state` subcommand

[Miriam Rubio]

From: Pranit Bauva <pranit.bauva@gmail.com>

The `--bisect-clean-state` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function
`bisect_clean_state()` is directly called from the C
implementation.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
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 | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index e75e7a0837..1792751179 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
 	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
-	N_("git bisect--helper --bisect-clean-state"),
 	N_("git bisect--helper --bisect-reset [<commit>]"),
 	N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
 	N_("git bisect--helper --bisect-check-and-set-terms <command> <good_term> <bad_term>"),
@@ -810,7 +809,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
-		BISECT_CLEAN_STATE,
 		CHECK_EXPECTED_REVS,
 		BISECT_RESET,
 		BISECT_WRITE,
@@ -827,8 +825,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_CMDMODE(0, "write-terms", &cmdmode,
 			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
-		OPT_CMDMODE(0, "bisect-clean-state", &cmdmode,
-			 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
 		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
 			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
 		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
@@ -870,10 +866,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 2)
 			return error(_("--write-terms requires two arguments"));
 		return write_terms(argv[0], argv[1]);
-	case BISECT_CLEAN_STATE:
-		if (argc != 0)
-			return error(_("--bisect-clean-state requires no arguments"));
-		return bisect_clean_state();
 	case CHECK_EXPECTED_REVS:
 		check_expected_revs(argv, argc);
 		return 0;
-- 
2.25.0

[PATCH 05/10] bisect--helper: retire `--next-all` subcommand

[Miriam Rubio]

From: Pranit Bauva <pranit.bauva@gmail.com>

The `--next-all` subcommand is no longer used from the git-bisect.sh
shell script. Instead the function `bisect_next_all()` is called from
the C implementation of `bisect_next()`.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
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 | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1792751179..c3bb936a40 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
-	N_("git bisect--helper --next-all [--no-checkout]"),
 	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	N_("git bisect--helper --bisect-reset [<commit>]"),
 	N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
@@ -807,8 +806,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkou
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
-		NEXT_ALL = 1,
-		WRITE_TERMS,
+		WRITE_TERMS = 1,
 		CHECK_EXPECTED_REVS,
 		BISECT_RESET,
 		BISECT_WRITE,
@@ -821,8 +819,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
-		OPT_CMDMODE(0, "next-all", &cmdmode,
-			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_CMDMODE(0, "write-terms", &cmdmode,
 			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
@@ -859,9 +855,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_bisect_helper_usage, options);
 
 	switch (cmdmode) {
-	case NEXT_ALL:
-		res = bisect_next_all(the_repository, prefix, no_checkout);
-		break;
 	case WRITE_TERMS:
 		if (argc != 2)
 			return error(_("--write-terms requires two arguments"));
-- 
2.25.0

[PATCH 06/10] bisect--helper: reimplement `bisect_autostart` shell function in C

[Miriam Rubio]

From: Pranit Bauva <pranit.bauva@gmail.com>

Reimplement the `bisect_autostart()` shell function in C and add the
C implementation from `bisect_next()` which was previously left
uncovered. Also add a subcommand `--bisect-autostart` to
`git bisect--helper` be called from `bisect_state()` from
git-bisect.sh .

Using `--bisect-autostart` 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
bisect_autostart() will be called directly by `bisect_state()`.

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 | 39 +++++++++++++++++++++++++++++++++++++++
 git-bisect.sh            | 23 +----------------------
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c3bb936a40..f9b04bee23 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {
 					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
 	N_("git bisect--helper --bisect-next"),
 	N_("git bisect--helper --bisect-auto-next"),
+	N_("git bisect--helper --bisect-autostart"),
 	NULL
 };
 
@@ -56,6 +57,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,
 static const char vocab_bad[] = "bad|new";
 static const char vocab_good[] = "good|old";
 
+static int bisect_autostart(struct bisect_terms *terms);
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -555,6 +558,7 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
 	int no_checkout;
 	enum bisect_error res;
 
+	bisect_autostart(terms);
 	if (bisect_next_check(terms, terms->term_good))
 		return BISECT_FAILED;
 
@@ -803,6 +807,32 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkou
 	return res;
 }
 
+static int bisect_autostart(struct bisect_terms *terms)
+{
+	if (is_empty_or_missing_file(git_path_bisect_start())) {
+		const char *yesno;
+		const char *argv[] = {NULL};
+		fprintf(stderr, _("You need to start by \"git bisect "
+				  "start\"\n"));
+
+		if (!isatty(STDIN_FILENO))
+			return 1;
+
+		/*
+		 * TRANSLATORS: Make sure to include [Y] and [n] in your
+		 * translation. The program will only accept English input
+		 * at this point.
+		 */
+		yesno = git_prompt(_("Do you want me to do it for you "
+				     "[Y/n]? "), PROMPT_ECHO);
+		if (starts_with(yesno, _("n")) || starts_with(yesno, _("N")))
+			return 1;
+
+		return bisect_start(terms, 0, argv, 0);
+	}
+	return BISECT_OK;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -816,6 +846,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_START,
 		BISECT_NEXT,
 		BISECT_AUTO_NEXT,
+		BISECT_AUTOSTART,
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
@@ -839,6 +870,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("find the next bisection commit"), BISECT_NEXT),
 		OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
 			 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_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_BOOL(0, "no-log", &nolog,
@@ -905,6 +938,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		get_terms(&terms);
 		res = bisect_auto_next(&terms, prefix);
 		break;
+	case BISECT_AUTOSTART:
+		if (argc)
+			return error(_("--bisect-autostart requires 0 arguments"));
+		set_terms(&terms, "bad", "good");
+		res = bisect_autostart(&terms);
+		break;
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 166f6a64dd..049ffacdff 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -49,27 +49,6 @@ bisect_head()
 	fi
 }
 
-bisect_autostart() {
-	test -s "$GIT_DIR/BISECT_START" || {
-		gettextln "You need to start by \"git bisect start\"" >&2
-		if test -t 0
-		then
-			# TRANSLATORS: Make sure to include [Y] and [n] in your
-			# translation. The program will only accept English input
-			# at this point.
-			gettext "Do you want me to do it for you [Y/n]? " >&2
-			read yesno
-			case "$yesno" in
-			[Nn]*)
-				exit ;;
-			esac
-			git bisect--helper --bisect-start
-		else
-			exit 1
-		fi
-	}
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -86,7 +65,7 @@ bisect_skip() {
 }
 
 bisect_state() {
-	bisect_autostart
+	git bisect--helper --bisect-autostart
 	state=$1
 	git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
 	get_terms
-- 
2.25.0

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

[Miriam Rubio]

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 | 80 ++++++++++++++++++++++++++++++++++++++++
 git-bisect.sh            | 55 ++-------------------------
 2 files changed, 84 insertions(+), 51 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index f9b04bee23..fedcad4d9b 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
 };
 
@@ -833,6 +835,74 @@ static int bisect_autostart(struct bisect_terms *terms)
 	return BISECT_OK;
 }
 
+static char *bisect_head(void)
+{
+	if (is_empty_or_missing_file(git_path_bisect_head()))
+		return "HEAD";
+	else
+		return "BISECT_HEAD";
+}
+
+static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
+			int argc)
+{
+	const char *state = argv[0];
+
+	if (check_and_set_terms(terms, state))
+		return BISECT_FAILED;
+
+	if (!argc)
+		return error(_("Please call `--bisect-state` with at least one argument"));
+
+	if (argc == 1 && one_of(state, terms->term_good,
+	    terms->term_bad, "skip", NULL)) {
+		const char *bisected_head = xstrdup(bisect_head());
+		const char *hex[1];
+		struct object_id oid;
+
+		if (get_oid(bisected_head, &oid))
+			return error(_("Bad rev input: %s"), bisected_head);
+		if (bisect_write(state, oid_to_hex(&oid), terms, 0))
+			return BISECT_FAILED;
+
+		*hex = xstrdup(oid_to_hex(&oid));
+		check_expected_revs(hex, 1);
+		return bisect_auto_next(terms, NULL);
+	}
+
+	if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
+			one_of(state, terms->term_good, "skip", NULL)) {
+		int i;
+		struct string_list hex = STRING_LIST_INIT_DUP;
+
+		for (i = 1; i < argc; i++) {
+			struct object_id oid;
+
+			if (get_oid(argv[i], &oid)) {
+				string_list_clear(&hex, 0);
+				return error(_("Bad rev input: %s"), argv[i]);
+			}
+			string_list_append(&hex, oid_to_hex(&oid));
+		}
+		for (i = 0; i < hex.nr; i++) {
+			const char **hex_string = (const char **) &hex.items[i].string;
+			if (bisect_write(state, *hex_string, terms, 0)) {
+				string_list_clear(&hex, 0);
+				return BISECT_FAILED;
+			}
+			check_expected_revs(hex_string, 1);
+		}
+		string_list_clear(&hex, 0);
+		return bisect_auto_next(terms, NULL);
+	}
+
+	if (!strcmp(state, terms->term_bad))
+		return error(_("'git bisect %s' can take only one argument."),
+		      terms->term_bad);
+
+	return BISECT_FAILED;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -847,6 +917,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_NEXT,
 		BISECT_AUTO_NEXT,
 		BISECT_AUTOSTART,
+		BISECT_STATE
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
@@ -872,6 +943,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,
@@ -944,6 +1017,13 @@ 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:
+		if (!argc)
+			return error(_("--bisect-state requires at least one revision"));
+		set_terms(&terms, "bad", "good");
+		get_terms(&terms);
+		res = bisect_state(&terms, argv, argc);
+		break;
 	default:
 		return error("BUG: unknown subcommand '%d'", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 049ffacdff..7f10187055 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
 			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

[PATCH 08/10] bisect--helper: retire `--check-expected-revs` subcommand

[Miriam Rubio]

From: Pranit Bauva <pranit.bauva@gmail.com>

The `--check-expected-revs` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function
`check_expected_revs()` is called from the C implementation of
`bisect-next()`.

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 | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index fedcad4d9b..c2fd3ee1b3 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -907,7 +907,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		WRITE_TERMS = 1,
-		CHECK_EXPECTED_REVS,
 		BISECT_RESET,
 		BISECT_WRITE,
 		CHECK_AND_SET_TERMS,
@@ -923,8 +922,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_CMDMODE(0, "write-terms", &cmdmode,
 			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
-		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
-			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
 		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
 			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_CMDMODE(0, "bisect-write", &cmdmode,
@@ -965,9 +962,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 2)
 			return error(_("--write-terms requires two arguments"));
 		return write_terms(argv[0], argv[1]);
-	case CHECK_EXPECTED_REVS:
-		check_expected_revs(argv, argc);
-		return 0;
 	case BISECT_RESET:
 		if (argc > 1)
 			return error(_("--bisect-reset requires either no argument or a commit"));
-- 
2.25.0

[PATCH 09/10] bisect--helper: retire `--write-terms` subcommand

[Miriam Rubio]

From: Pranit Bauva <pranit.bauva@gmail.com>

The `--write-terms` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function `write_terms()`
is called from the C implementation of `set_terms()` and
`bisect_start()`.

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 | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c2fd3ee1b3..650ceefcd7 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
-	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	N_("git bisect--helper --bisect-reset [<commit>]"),
 	N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
 	N_("git bisect--helper --bisect-check-and-set-terms <command> <good_term> <bad_term>"),
@@ -906,8 +905,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
-		WRITE_TERMS = 1,
-		BISECT_RESET,
+		BISECT_RESET = 1,
 		BISECT_WRITE,
 		CHECK_AND_SET_TERMS,
 		BISECT_NEXT_CHECK,
@@ -920,8 +918,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
-		OPT_CMDMODE(0, "write-terms", &cmdmode,
-			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
 			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_CMDMODE(0, "bisect-write", &cmdmode,
@@ -958,10 +954,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_bisect_helper_usage, options);
 
 	switch (cmdmode) {
-	case WRITE_TERMS:
-		if (argc != 2)
-			return error(_("--write-terms requires two arguments"));
-		return write_terms(argv[0], argv[1]);
 	case BISECT_RESET:
 		if (argc > 1)
 			return error(_("--bisect-reset requires either no argument or a commit"));
-- 
2.25.0

[PATCH 10/10] bisect--helper: retire `--bisect-autostart` subcommand

[Miriam Rubio]

From: Pranit Bauva <pranit.bauva@gmail.com>

The `--bisect-autostart` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function
`bisect_autostart()` is directly called from the C implementation.

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 | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 650ceefcd7..c692c660a4 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -29,7 +29,6 @@ static const char * const git_bisect_helper_usage[] = {
 					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
 	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
@@ -913,7 +912,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_START,
 		BISECT_NEXT,
 		BISECT_AUTO_NEXT,
-		BISECT_AUTOSTART,
 		BISECT_STATE
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
@@ -934,8 +932,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("find the next bisection commit"), BISECT_NEXT),
 		OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
 			 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,
@@ -997,12 +993,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		get_terms(&terms);
 		res = bisect_auto_next(&terms, prefix);
 		break;
-	case BISECT_AUTOSTART:
-		if (argc)
-			return error(_("--bisect-autostart requires 0 arguments"));
-		set_terms(&terms, "bad", "good");
-		res = bisect_autostart(&terms);
-		break;
 	case BISECT_STATE:
 		if (!argc)
 			return error(_("--bisect-state requires at least one revision"));
-- 
2.25.0

Re: [PATCH 01/10] bisect--helper: introduce new `write_in_file()` function

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> Let's refactor code adding a new `write_in_file()` function
> that opens a file for writing a message and closes it.
>
> This removes some duplicated code and makes the code simpler,
> clearer and easier to understand.

I find it a bit too much self promotion ;-)  With what we see only in
this step, the code is not so much simpler nor easier to understand.
Perhaps when we see more callers in the remainder of the series, we
may start to appreciate it, but if that is the case, we should
clarify it here, e.g.

    This helper will be used in later steps and makes the code
    simpler, ...

> +static int write_in_file(const char *filepath, const char *content, int append)
> +{
> +	FILE *fp = NULL;
> +	const char *mode = append ? "a" : "w";
> +
> +	fp = fopen(filepath, mode);
> +	if (!fp)
> +		return error_errno(_("could not open the file '%s'"), filepath);
> +	if (!fprintf(fp, "%s\n", content))
> +		return error_errno(_("could not write in file '%s'"), filepath);

Use and non-use of definite article being inconstent between "open
the file X" vs "write in file X" bothers me as a reader here.

Wouldn't it be more common to say "write TO" here, not "write IN"?

> +	return fclose(fp);

We didn't use to say error_errno(_("could not close ...")) in the
original, so we stay silent and return an error (EOF, which is a
negative integer) from here.

> +}
> +
>  static int check_term_format(const char *term, const char *orig_term)
>  {
>  	int res;
> @@ -104,7 +117,7 @@ static int check_term_format(const char *term, const char *orig_term)
>  
>  static int write_terms(const char *bad, const char *good)
>  {
> -	FILE *fp = NULL;
> +	char *content = xstrfmt("%s\n%s", bad, good);
>  	int res;
>  
>  	if (!strcmp(bad, good))
> @@ -113,12 +126,9 @@ static int write_terms(const char *bad, const char *good)
>  	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
>  		return -1;
>  
> -	fp = fopen(git_path_bisect_terms(), "w");
> -	if (!fp)
> -		return error_errno(_("could not open the file BISECT_TERMS"));
> +	res = write_in_file(git_path_bisect_terms(), content, 0);
> +	free(content);
>  
> -	res = fprintf(fp, "%s\n%s\n", bad, good);

With just this one callsite, I do not think it is a good idea to
lose the "write a formatted output without an extra allcoation" 
by introducing this wrapper.  The equation may become different if
we see more (a lot more) callsites that already have strings ready
to be written that can use this helper, though.

> -	res |= fclose(fp);
>  	return (res < 0) ? -1 : 0;

The return value is a bit strange here, but it is inherited from the
original, so I'll let it pass.

>  }

Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> +static int register_good_ref(const char *refname,
> +			     const struct object_id *oid, int flags,
> +			     void *cb_data)
> +{
> +	struct string_list *good_refs = cb_data;
> +
> +	string_list_append(good_refs, oid_to_hex(oid));
> +	return 0;
> +}
> +
> +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
> +{
> +	struct string_list good_revs = STRING_LIST_INIT_DUP;
> +	char *term_good = xstrfmt("%s-*", terms->term_good);
> +
> +	for_each_glob_ref_in(register_good_ref, term_good,
> +			     "refs/bisect/", &good_revs);
> +
> +	argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
> +	for (int i = 0; i < good_revs.nr; i++)
> +		argv_array_push(rev_argv, good_revs.items[i].string);
> +
> +	string_list_clear(&good_revs, 0);
> +	free(term_good);

Why do you need good_revs string_list in the first place?  Wouldn't
it be simpler and easier to understand to pass in the argv as part
of the callback data and push the good refs in the callback function?

> +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
> +{
> +	int res = 0;
> +	struct argv_array rev_argv = ARGV_ARRAY_INIT;
> +
> +	prepare_rev_argv(terms, &rev_argv);
> +
> +	/*
> +	 * It is important to reset the flags used by revision walks
> +	 * as the previous call to bisect_next_all() in turn
> +	 * setups a revision walk.

"setup" is not a verb X-<.  "... in turn sets up a ..." would be OK.

> +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)
> +{
> +	struct commit *commit;
> +	struct pretty_print_context pp = {0};
> +
> +	if (fprintf(fp, "# only skipped commits left to test\n") < 1)
> +		return -1;
> +
> +	while ((commit = get_revision(revs)) != NULL) {
> +		struct strbuf commit_name = STRBUF_INIT;
> +		format_commit_message(commit, "%s",
> +				      &commit_name, &pp);
> +		fprintf(fp, "# possible first %s commit: [%s] %s\n",
> +			terms->term_bad, oid_to_hex(&commit->object.oid),
> +			commit_name.buf);
> +		strbuf_release(&commit_name);
> +		clear_commit_marks(commit, ALL_REV_FLAGS);

clear_commit_marks() is to clear the given flag bits from the named
commit *AND* all of its ancestors (recursively) as long as they have
these bits on, and it typically is used in order to clean up the
state bits left on objects _after_ a revision walk is _done_.

Calling it, especially to clear ALL_REV_FLAGS that contains crucial
flag bits necessary to drive get_revision(), inside a loop that is
still walking commits one by one by calling get_revision(), is
extremely unusual.

It would be surprising if this code were correct.  It may be that it
happens to (appear to) work because parents of the commit hasn't
been painted and calling the helper only clears the mark from the
commit (so we won't see repeated "painting down to the root commit")
in which case this might be an extremely expensive looking variant
of

	commit->object.flags &= ~ALL_REV_FLAGS;

that only confuses the readers.

Even then, I think by clearing bits like SEEN from commit, it breaks
the revision traversal machinery---for example, doesn't this mean
that the commit we just processed can be re-visited by
get_revision() without deduping in a history with forks and merges?

Has this been shown to any of your mentors before sending it to the
list?

> +static int bisect_successful(struct bisect_terms *terms)
> +{
> +	struct object_id oid;
> +	struct commit *commit;
> +	struct pretty_print_context pp = {0};
> +	struct strbuf commit_name = STRBUF_INIT;
> +	char *bad_ref = xstrfmt("refs/bisect/%s",
> +				terms->term_bad);
> +	char *content;
> +	int res;
> +
> +	read_ref(bad_ref, &oid);
> +	printf("%s\n", bad_ref);
> +	commit = lookup_commit_reference(the_repository, &oid);
> +	format_commit_message(commit, "%s", &commit_name, &pp);
> +
> +	content = xstrfmt("# first %s commit: [%s] %s",
> +	terms->term_bad, oid_to_hex(&oid),
> +	commit_name.buf);

Strange indentation.

> +	res = write_in_file(git_path_bisect_log(), content, 1);

So this is a new use of the helper introduced in [01/10].  It is
true that use of it lets this caller not to worry about opening,
writing and closing, but it needs an extra allocation to prepare
"content".

If the calling convention were more like this:

	res = write_to_file(git_path_bisect_log(), "a",
			    "# first %s commit: [%s] %s",
			    terms->term_bad, oid_to_hex(&oid), commit_name.buf,
			    NULL);

this callsite and the callsite in [01/10] wouldn't have had to make
an unnecessary allocation, perhaps?

Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Miriam R.]

Hi Junio,

El mié., 26 feb. 2020 a las 20:34, Junio C Hamano
(<gitster@pobox.com>) escribió:
>
> Miriam Rubio <mirucam@gmail.com> writes:
>
> > +static int register_good_ref(const char *refname,
> > +                          const struct object_id *oid, int flags,
> > +                          void *cb_data)
> > +{
> > +     struct string_list *good_refs = cb_data;
> > +
> > +     string_list_append(good_refs, oid_to_hex(oid));
> > +     return 0;
> > +}
> > +
> > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
> > +{
> > +     struct string_list good_revs = STRING_LIST_INIT_DUP;
> > +     char *term_good = xstrfmt("%s-*", terms->term_good);
> > +
> > +     for_each_glob_ref_in(register_good_ref, term_good,
> > +                          "refs/bisect/", &good_revs);
> > +
> > +     argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
> > +     for (int i = 0; i < good_revs.nr; i++)
> > +             argv_array_push(rev_argv, good_revs.items[i].string);
> > +
> > +     string_list_clear(&good_revs, 0);
> > +     free(term_good);
>
> Why do you need good_revs string_list in the first place?  Wouldn't
> it be simpler and easier to understand to pass in the argv as part
> of the callback data and push the good refs in the callback function?
>
Ok, I will do it that way.

> > +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
> > +{
> > +     int res = 0;
> > +     struct argv_array rev_argv = ARGV_ARRAY_INIT;
> > +
> > +     prepare_rev_argv(terms, &rev_argv);
> > +
> > +     /*
> > +      * It is important to reset the flags used by revision walks
> > +      * as the previous call to bisect_next_all() in turn
> > +      * setups a revision walk.
>
> "setup" is not a verb X-<.  "... in turn sets up a ..." would be OK.
>
Noted.
> > +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)
> > +{
> > +     struct commit *commit;
> > +     struct pretty_print_context pp = {0};
> > +
> > +     if (fprintf(fp, "# only skipped commits left to test\n") < 1)
> > +             return -1;
> > +
> > +     while ((commit = get_revision(revs)) != NULL) {
> > +             struct strbuf commit_name = STRBUF_INIT;
> > +             format_commit_message(commit, "%s",
> > +                                   &commit_name, &pp);
> > +             fprintf(fp, "# possible first %s commit: [%s] %s\n",
> > +                     terms->term_bad, oid_to_hex(&commit->object.oid),
> > +                     commit_name.buf);
> > +             strbuf_release(&commit_name);
> > +             clear_commit_marks(commit, ALL_REV_FLAGS);
>
> clear_commit_marks() is to clear the given flag bits from the named
> commit *AND* all of its ancestors (recursively) as long as they have
> these bits on, and it typically is used in order to clean up the
> state bits left on objects _after_ a revision walk is _done_.
>
> Calling it, especially to clear ALL_REV_FLAGS that contains crucial
> flag bits necessary to drive get_revision(), inside a loop that is
> still walking commits one by one by calling get_revision(), is
> extremely unusual.
>
> It would be surprising if this code were correct.  It may be that it
> happens to (appear to) work because parents of the commit hasn't
> been painted and calling the helper only clears the mark from the
> commit (so we won't see repeated "painting down to the root commit")
> in which case this might be an extremely expensive looking variant
> of
>
>         commit->object.flags &= ~ALL_REV_FLAGS;
>
> that only confuses the readers.
>
> Even then, I think by clearing bits like SEEN from commit, it breaks
> the revision traversal machinery---for example, doesn't this mean
> that the commit we just processed can be re-visited by
> get_revision() without deduping in a history with forks and merges?
>
> Has this been shown to any of your mentors before sending it to the
> list?

Adding clear_commit_marks() was a suggestion of a previous review of this patch:
https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/

And of course, my mentor always reviews my patch series before sending.

>
> > +static int bisect_successful(struct bisect_terms *terms)
> > +{
> > +     struct object_id oid;
> > +     struct commit *commit;
> > +     struct pretty_print_context pp = {0};
> > +     struct strbuf commit_name = STRBUF_INIT;
> > +     char *bad_ref = xstrfmt("refs/bisect/%s",
> > +                             terms->term_bad);
> > +     char *content;
> > +     int res;
> > +
> > +     read_ref(bad_ref, &oid);
> > +     printf("%s\n", bad_ref);
> > +     commit = lookup_commit_reference(the_repository, &oid);
> > +     format_commit_message(commit, "%s", &commit_name, &pp);
> > +
> > +     content = xstrfmt("# first %s commit: [%s] %s",
> > +     terms->term_bad, oid_to_hex(&oid),
> > +     commit_name.buf);
>
> Strange indentation.
Noted.
>
> > +     res = write_in_file(git_path_bisect_log(), content, 1);
>
> So this is a new use of the helper introduced in [01/10].  It is
> true that use of it lets this caller not to worry about opening,
> writing and closing, but it needs an extra allocation to prepare
> "content".
>
> If the calling convention were more like this:
>
>         res = write_to_file(git_path_bisect_log(), "a",
>                             "# first %s commit: [%s] %s",
>                             terms->term_bad, oid_to_hex(&oid), commit_name.buf,
>                             NULL);
>
> this callsite and the callsite in [01/10] wouldn't have had to make
> an unnecessary allocation, perhaps?
>
Aha. I will change it.

This helper function was also a suggestion of the previous reviewer:
https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/

Thank you very much for reviewing!.

Best,
Miriam

Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Junio C Hamano]

"Miriam R." <mirucam@gmail.com> writes:

>> It would be surprising if this code were correct.  It may be that it
>> happens to (appear to) work because parents of the commit hasn't
>> been painted and calling the helper only clears the mark from the
>> commit (so we won't see repeated "painting down to the root commit")
>> in which case this might be an extremely expensive looking variant
>> of
>>
>>         commit->object.flags &= ~ALL_REV_FLAGS;
>>
>> that only confuses the readers.
>>
>> Even then, I think by clearing bits like SEEN from commit, it breaks
>> the revision traversal machinery---for example, doesn't this mean
>> that the commit we just processed can be re-visited by
>> get_revision() without deduping in a history with forks and merges?
>>
>> Has this been shown to any of your mentors before sending it to the
>> list?
>
> Adding clear_commit_marks() was a suggestion of a previous review of this patch:
> https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/
>
> And of course, my mentor always reviews my patch series before sending.

OK, I just didn't know how you and your mentors work.  Some leave
their door open for mentees and help them when asked but otherwise
act as an ordinary reviewer who somehow prioritises reviewing the
work by their mentees.  So your mentors may be a better source of
information why this piece of code, which I still do not know how it
could be correct, is supposed to work.  Good.

After reading the above URL, I think you may have misread the
suggestion you were given.  Resetting using clear_commit_marks() may
be necessary, but you do so when you finished walking so that you
can do unrelated revision walk later.  For that, you clear the flag
bits after the while() loop that asks get_revision() to yield
commits are done, using the initial set of commits that you used to
start iteration.

That is how bisect.c::check_ancestors() work, that is

 - it initializes a rev_info 'revs' from an array of commit rev[]

 - it lets bisect_common() use the 'revs', which is allowed to
   smudge the flag bits of commit objects.

 - it uses clear_commit_marks_many() to clear the flags of the
   commits whose flag bits may have been smudged and their
   ancestors, recursively.  In order to use as the starting points,
   the original array of commit rev[] that started the revision
   traversal is used.

Re: [PATCH 06/10] bisect--helper: reimplement `bisect_autostart` shell function in C

[Johannes Schindelin]

Hi,

On Wed, 26 Feb 2020, Miriam Rubio wrote:

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c3bb936a40..f9b04bee23 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -56,6 +57,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,
>  static const char vocab_bad[] = "bad|new";
>  static const char vocab_good[] = "good|old";
>
> +static int bisect_autostart(struct bisect_terms *terms);
> +

Can we move the definition of said function here? If so, we can save
ourselves the forward-declaration of it.

>  /*
>   * Check whether the string `term` belongs to the set of strings
>   * included in the variable arguments.
> @@ -555,6 +558,7 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
>  	int no_checkout;
>  	enum bisect_error res;
>
> +	bisect_autostart(terms);
>  	if (bisect_next_check(terms, terms->term_good))
>  		return BISECT_FAILED;
>
> @@ -803,6 +807,32 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkou
>  	return res;
>  }
>
> +static int bisect_autostart(struct bisect_terms *terms)
> +{
> +	if (is_empty_or_missing_file(git_path_bisect_start())) {

As a rule, we do prefer to handle early exits first. In other words, I
would suggest starting this function by:

	if (!is_empty_or_missing_file(git_path_bisect_start()))
		return BISECT_OK;

That way, not only can you save on indentation, but it also frees one slot
of the readers' working memory.

(Of course you would still need to ove the declarations before this
statement.)

> +		const char *yesno;
> +		const char *argv[] = {NULL};

Please separate declarations and statements with an empty line. I would
also prefer spaces around the curly brackets, i.e. `{ NULL }` instead of
`{NULL}`.

> +		fprintf(stderr, _("You need to start by \"git bisect "
> +				  "start\"\n"));
> +
> +		if (!isatty(STDIN_FILENO))
> +			return 1;
> +
> +		/*
> +		 * TRANSLATORS: Make sure to include [Y] and [n] in your
> +		 * translation. The program will only accept English input
> +		 * at this point.
> +		 */
> +		yesno = git_prompt(_("Do you want me to do it for you "
> +				     "[Y/n]? "), PROMPT_ECHO);
> +		if (starts_with(yesno, _("n")) || starts_with(yesno, _("N")))
> +			return 1;
> +
> +		return bisect_start(terms, 0, argv, 0);

Do we really need this `argv`, or can we pass `NULL` directly? We say that
there are zero arguments, after all, therefore `bisect_start()` should not
even look at `argv`.

The rest of the patch looks good to me.

Thank you,
Dscho

> +	}
> +	return BISECT_OK;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -816,6 +846,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		BISECT_START,
>  		BISECT_NEXT,
>  		BISECT_AUTO_NEXT,
> +		BISECT_AUTOSTART,
>  	} cmdmode = 0;
>  	int no_checkout = 0, res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -839,6 +870,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  			 N_("find the next bisection commit"), BISECT_NEXT),
>  		OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
>  			 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_BOOL(0, "no-checkout", &no_checkout,
>  			 N_("update BISECT_HEAD instead of checking out the current commit")),
>  		OPT_BOOL(0, "no-log", &nolog,
> @@ -905,6 +938,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		get_terms(&terms);
>  		res = bisect_auto_next(&terms, prefix);
>  		break;
> +	case BISECT_AUTOSTART:
> +		if (argc)
> +			return error(_("--bisect-autostart requires 0 arguments"));
> +		set_terms(&terms, "bad", "good");
> +		res = bisect_autostart(&terms);
> +		break;
>  	default:
>  		return error("BUG: unknown subcommand '%d'", cmdmode);
>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 166f6a64dd..049ffacdff 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -49,27 +49,6 @@ bisect_head()
>  	fi
>  }
>
> -bisect_autostart() {
> -	test -s "$GIT_DIR/BISECT_START" || {
> -		gettextln "You need to start by \"git bisect start\"" >&2
> -		if test -t 0
> -		then
> -			# TRANSLATORS: Make sure to include [Y] and [n] in your
> -			# translation. The program will only accept English input
> -			# at this point.
> -			gettext "Do you want me to do it for you [Y/n]? " >&2
> -			read yesno
> -			case "$yesno" in
> -			[Nn]*)
> -				exit ;;
> -			esac
> -			git bisect--helper --bisect-start
> -		else
> -			exit 1
> -		fi
> -	}
> -}
> -
>  bisect_skip() {
>  	all=''
>  	for arg in "$@"
> @@ -86,7 +65,7 @@ bisect_skip() {
>  }
>
>  bisect_state() {
> -	bisect_autostart
> +	git bisect--helper --bisect-autostart
>  	state=$1
>  	git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
>  	get_terms
> --
> 2.25.0
>
>

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

[Johannes Schindelin]

Hi,

On Wed, 26 Feb 2020, Miriam Rubio wrote:

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index f9b04bee23..fedcad4d9b 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -833,6 +835,74 @@ static int bisect_autostart(struct bisect_terms *terms)
>  	return BISECT_OK;
>  }
>
> +static char *bisect_head(void)
> +{
> +	if (is_empty_or_missing_file(git_path_bisect_head()))
> +		return "HEAD";
> +	else
> +		return "BISECT_HEAD";

It is relatively common, and makes it easier to read at least to me, to
omit the `else` here: the conditional `return` already leaves the function
early.

> +}
> +
> +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
> +			int argc)
> +{
> +	const char *state = argv[0];
> +
> +	if (check_and_set_terms(terms, state))
> +		return BISECT_FAILED;
> +
> +	if (!argc)
> +		return error(_("Please call `--bisect-state` with at least one argument"));
> +
> +	if (argc == 1 && one_of(state, terms->term_good,
> +	    terms->term_bad, "skip", NULL)) {
> +		const char *bisected_head = xstrdup(bisect_head());

Do we really want to `strdup()` it? That would leak memory, no? And I
don't think that we need this to be allocated anywhere. It's not like we
are trying to modify the characters or that we need to take custody of the
string: `bisect_head()` will return a static, immutable string.

But then, it is not very natural for C code to let `bisect_head()` return
a string. Why not return the already-parsed object ID? That would also
make that function more robust: instead of expecting `BISECT_HEAD` to be
stored in the file `BISECT_HEAD` in the `.git/` directory, you could
simply call `get_oid("BISECT_HEAD", &oid)` and if that fails, fall back to
do the same with `HEAD`. That way, you tap into the refs machinery and the
bisect code would be less dependent on implementation details.

> +		const char *hex[1];
> +		struct object_id oid;
> +
> +		if (get_oid(bisected_head, &oid))
> +			return error(_("Bad rev input: %s"), bisected_head);
> +		if (bisect_write(state, oid_to_hex(&oid), terms, 0))
> +			return BISECT_FAILED;
> +
> +		*hex = xstrdup(oid_to_hex(&oid));
> +		check_expected_revs(hex, 1);

Hmm. Do we expect `check_expected_revs()` to modify what is passed into
it? If not, we could probably simplify this a lot (and avoid another
memory leak) by something like this:

		const char *hex;

		[...]
		hex = oid_to_hex(&oid);
		check_expected_revs(&hex, 1);

However, taking a step back, I wonder whether it makes sense for
`check_expected_revs()` to accept `const char **revs` instead of `struct
object_id *oids`.

Looking at the definition of `check_expected_revs()`, I would think that
it would be better to pass object IDs in, rather than strings:

static int is_expected_rev(const char *expected_hex)
{
        struct strbuf actual_hex = STRBUF_INIT;
        int res = 0;
        if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
                strbuf_trim(&actual_hex);
                res = !strcmp(actual_hex.buf, expected_hex);
        }
        strbuf_release(&actual_hex);
        return res;
}

static void check_expected_revs(const char **revs, int rev_nr)
{
        int i;

        for (i = 0; i < rev_nr; i++) {
                if (!is_expected_rev(revs[i])) {
                        unlink_or_warn(git_path_bisect_ancestors_ok());
                        unlink_or_warn(git_path_bisect_expected_rev());
                }
        }
}

There are a couple of observation that immediately come to my mind:

- Reading the bisect_expected_rev file for each rev, only to immediately
  release it before the next iteration, is wasteful

- We can break out of the loop immediately once `is_expected_rev()`
  returns 0 because we are then unlinking the very file that
  `is_expected_rev()` checks against.

- As I suspected above, a much cleaner interface would use `struct
  object_id **revs`.

- Keeping the code of `is_expected_rev()` separate from
  `check_expected_revs()` does not make the code more readable for me, but
  actually less readable instead.

- Why are we hard-coding the 40? At this time and age, we've _got_ to use
  `the_hash_algo->hexsz` instead.

In other words, I would do something like this instead:

static void check_expected_revs(struct object_id **revs, int rev_nr)
{
	struct object_id expected;
	struct strbuf buf = STRBUF_INIT;
	int i;

	if (!rev_nr)
		return;

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

	for (i = 0; i < rev_nr; i++)
		if (!oideq(revs[i], &expected)) {
			... unlink ...
			return;
		}
}

However, taking _yet_ another step back, it seems a bit silly to compare
many revs (that need to be provided as 40-digit hex strings) to the
contents of a file that we expect to contain exactly one 40-digit hex
string. Surely, we do not want to take an arbitrary number of revisions,
then expect all of them to be identical to the _same_ rev, and otherwise
delete that file from where that rev came from?

We could even skip reading that file if two or more revisions were passed
to `check_expected_revs()` unless _all_ of them are identical!

So let's look at the actual caller of this thing:

bisect_state() {
        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
        bisect_auto_next
}

So there are actually two callers: the first one in the 1,* arm, the
second one in the 2,* arm. The first one is easy: it only passes a single
rev to `git bisect--helper --check-expected-revs`.

The second caller indeed can pass multiple arguments to
`check_expected_revs()`, but only if the first argument was not `bad`.

But this whole shell function is super ugly, and I do not believe that
we're served well by translating it verbatim to even super uglier C code.

I propose to instead rewrite it substantially, in the following way:

- Handle the `argc == 0` case first thing: in this case, die, complaining
  that we need at least one argument.

  (You already did that. Although `state` was already assigned to
   `argv[0]` at that stage, and I think we don't really want that, just in
   case that _one_ call chain ends up using `NULL, 0` for `argv, argc`.)

- Next, assign `state = argv[0]` and verify that it is one of `good`,
  `bad` or `skip`, otherwise erroring out.

- At this stage, we should do the equivalent of a `shift`: `argv++;
  argc--;`

- Now, we probably need to special-case the `bad` case and verify that
  `argc <= 1`, otherwise error out.

  Side note: I do _not_ understand this restriction. Why shouldn't it be
  valid to pass several revisions to `git bisect bad <revisions>...`?

- At this point, if `argc == 0`, we should parse `bisect_head()` into a
  `struct object_id` and use that as `revs`, setting `argc = 0`.
  Otherwise, if `argc > 0`, we should populate a `struct object_array` and
  use that as `revs`.

- And finally, we should iterate over these revs and call `bisect_write()`
  with them, and in the same loop we can also take care of the expected
  rev. It does not make sense to keep those separate from one another
  (i.e. to have _two_ loops).

That would be a much more natural, and readable, flow, at least to me.

> +	}
> +
> +	if ((argc == 2 && !strcmp(state, terms->term_bad)) ||
> +			one_of(state, terms->term_good, "skip", NULL)) {
> +		int i;
> +		struct string_list hex = STRING_LIST_INIT_DUP;
> +
> +		for (i = 1; i < argc; i++) {
> +			struct object_id oid;
> +
> +			if (get_oid(argv[i], &oid)) {

The original does `git rev-parse --verify "$rev^{commit}"` here, so I
think we really want `get_oid_commit()` here, not just `get_oid()`.

> +				string_list_clear(&hex, 0);
> +				return error(_("Bad rev input: %s"), argv[i]);
> +			}
> +			string_list_append(&hex, oid_to_hex(&oid));

Converting the parsed object ID to hex and storing _that_ as a string and
throwing away the parsed object ID looks not very much like C, but much
more like shell script. Let's not do that. Let's store the parsed data in
a proper `struct object_array`, and only resort to `oid_to_hex()` when we
_have_ to.

> +		}
> +		for (i = 0; i < hex.nr; i++) {
> +			const char **hex_string = (const char **) &hex.items[i].string;
> +			if (bisect_write(state, *hex_string, terms, 0)) {
> +				string_list_clear(&hex, 0);
> +				return BISECT_FAILED;
> +			}
> +			check_expected_revs(hex_string, 1);
> +		string_list_clear(&hex, 0);
> +		return bisect_auto_next(terms, NULL);
> +	}
> +
> +	if (!strcmp(state, terms->term_bad))
> +		return error(_("'git bisect %s' can take only one argument."),
> +		      terms->term_bad);

That is an awful late time in the function for a reviewer to read about a
condition that should be an early error.

> +
> +	return BISECT_FAILED;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> [...]
> @@ -944,6 +1017,13 @@ 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:
> +		if (!argc)
> +			return error(_("--bisect-state requires at least one revision"));

Wait, we _already_ test for that here? Then we do not need the very same
check in `bisect_state()`.

I'd rather have it in `bisect_state()`.

Besides, we're in a `cmd_*()` function, so we cannot return `error()`
because that translates to `-1` and `cmd_*()` functions need to return
exit codes, i.e. numbers between 0 and 127.

> +		set_terms(&terms, "bad", "good");
> +		get_terms(&terms);
> +		res = bisect_state(&terms, argv, argc);
> +		break;
>  	default:
>  		return error("BUG: unknown subcommand '%d'", cmdmode);

Granted, this incorrect `return error()` in a `cmd_*()` function was there
before your patch. It is just as incorrect, though.

>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 049ffacdff..7f10187055 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> [...]
> @@ -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" )

This subshell looks pointless. Before, we wanted to be able to catch the
`die` in `bisect_state`. But since the `bisect--helper` cannot cause the
_shell script_ to exit, we can just drop the subshell here.

Phew, that was quite a lot, and the review of this patch also took two
hours. I'll have to stop here, once again, and hope that I will find time
soon to continue the review.

I left you quite a bit of work here, but I hope that all of my suggestions
are clear enough to act on. If not, please holler.

Ciao,
Dscho

>  		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
>  			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
>
>

Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Miriam R.]

Hi Junio,

El jue., 27 feb. 2020 a las 17:41, Junio C Hamano
(<gitster@pobox.com>) escribió:
>
> "Miriam R." <mirucam@gmail.com> writes:
>
> >> It would be surprising if this code were correct.  It may be that it
> >> happens to (appear to) work because parents of the commit hasn't
> >> been painted and calling the helper only clears the mark from the
> >> commit (so we won't see repeated "painting down to the root commit")
> >> in which case this might be an extremely expensive looking variant
> >> of
> >>
> >>         commit->object.flags &= ~ALL_REV_FLAGS;
> >>
> >> that only confuses the readers.
> >>
> >> Even then, I think by clearing bits like SEEN from commit, it breaks
> >> the revision traversal machinery---for example, doesn't this mean
> >> that the commit we just processed can be re-visited by
> >> get_revision() without deduping in a history with forks and merges?
> >>
> >> Has this been shown to any of your mentors before sending it to the
> >> list?
> >
> > Adding clear_commit_marks() was a suggestion of a previous review of this patch:
> > https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/
> >
> > And of course, my mentor always reviews my patch series before sending.
>
> OK, I just didn't know how you and your mentors work.  Some leave
> their door open for mentees and help them when asked but otherwise
> act as an ordinary reviewer who somehow prioritises reviewing the
> work by their mentees.  So your mentors may be a better source of
> information why this piece of code, which I still do not know how it
> could be correct, is supposed to work.  Good.
>
> After reading the above URL, I think you may have misread the
> suggestion you were given.  Resetting using clear_commit_marks() may
> be necessary, but you do so when you finished walking so that you
> can do unrelated revision walk later.  For that, you clear the flag
> bits after the while() loop that asks get_revision() to yield
> commits are done, using the initial set of commits that you used to
> start iteration.
>
> That is how bisect.c::check_ancestors() work, that is
>
>  - it initializes a rev_info 'revs' from an array of commit rev[]
>
>  - it lets bisect_common() use the 'revs', which is allowed to
>    smudge the flag bits of commit objects.
>
>  - it uses clear_commit_marks_many() to clear the flags of the
>    commits whose flag bits may have been smudged and their
>    ancestors, recursively.  In order to use as the starting points,
>    the original array of commit rev[] that started the revision
>    traversal is used.

Thank you for your explanation.

To my understanding, it looks like calling reset_revision_walk() after
the while() loop should be enough. Am I right or am I missing
something?

Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Junio C Hamano]

"Miriam R." <mirucam@gmail.com> writes:

> To my understanding, it looks like calling reset_revision_walk() after
> the while() loop should be enough. Am I right or am I missing
> something?

I suspect that reset_revision_walk() may be too-big a hammer, as it
clears everything, regardless of the reason why the flag bits were
set.  On the other hand, the clearly strategy that uses
clear_commit_marks() is to clear only the flag bits that were set
during the previous revision walk from only the commits that were
walked during the previous revision walk.

I offhand do not know what flag bits on what objects that were not
involved in the previous revision walk are still necessary at the
point of the call made by the caller (that's a question for your
mentors who volunteered their expertise on the program in question),
so if there isn't any, reset_revision_walk() may be an easy way out.
I just do not know if it clears too much to break the code that
comes after the function returns.

Thanks.

Re: [PATCH 02/10] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Christian Couder]

On Fri, Mar 6, 2020 at 8:06 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Miriam R." <mirucam@gmail.com> writes:
>
> > To my understanding, it looks like calling reset_revision_walk() after
> > the while() loop should be enough. Am I right or am I missing
> > something?
>
> I suspect that reset_revision_walk() may be too-big a hammer, as it
> clears everything, regardless of the reason why the flag bits were
> set.  On the other hand, the clearly strategy that uses
> clear_commit_marks() is to clear only the flag bits that were set
> during the previous revision walk from only the commits that were
> walked during the previous revision walk.
>
> I offhand do not know what flag bits on what objects that were not
> involved in the previous revision walk are still necessary at the
> point of the call made by the caller (that's a question for your
> mentors who volunteered their expertise on the program in question),
> so if there isn't any, reset_revision_walk() may be an easy way out.
> I just do not know if it clears too much to break the code that
> comes after the function returns.

process_skipped_commits(), the function that does this revision walk,
is called by bisect_skipped_commits() to print the possible first bad
commits when there are only skipped commits left to test and we
therefore cannot bisect more. This can be seen in bisect_next() which
does basically the following:

bisect_next()
{
       ...

       /* Perform all bisection computation, display and checkout */
       res = bisect_next_all(the_repository, prefix, no_checkout);

       if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
               ...
       } else if (res == BISECT_ONLY_SKIPPED_LEFT) {
               res = bisect_skipped_commits(terms);
               return res ? res : BISECT_ONLY_SKIPPED_LEFT;
       }
       return res;
}

BISECT_ONLY_SKIPPED_LEFT is an error code (-2) so bisect_next() will
always return an error in this case.

This means that the revision walk in process_skipped_commits() is very
likely to be the last revision walk performed by the command. So my
opinion is that not clearing anything at the end of that revision walk
is fine.

If we are worried about what could happen one day, when people might
be interested in actually doing another revision walk after this one,
then as we don't know what they will want to do and might be
interested in, cleaning everything with reset_revision_walk() might be
the safest thing to do and is probably cheap enough that it's ok to
use it right now.

Thanks for your review,
Christian.