[PATCH v3 00/12] Finish converting git bisect to C part 2

[PATCH v3 00/12] 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.

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

I would like to thank Junio Hamano for reviewing this patch series and
Christian Couder for his help.

--- Changes since v2 Finish converting git bisect to C part 2 patch series ---

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

* Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)

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

[1/12] bisect--helper: fix `cmd_*()` function switch default return

* Use `BUG()` instead of `return error()` in default switch.

--

[2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return

* New patch: use '-res' instead of 'abs(res)'.

--

[3/12] bisect--helper: introduce new `write_in_file()` function

* Rename input parameter `filepath` to `path`.
* Change `error_errno()` to `error()` in mode checking.
* Change error message when file cannot be opened.
* Add `fclose()` before error return.

--

[4/12] bisect--helper: reimplement `bisect_autostart` shell function in C

* Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`
shell functions in C` to use `bisect_autostart()` function in 
`bisect_append_log_quoted()`.

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

* Amend commit message.
* Add `clear_commit_marks()` at the end of bisect_next_all() and remove it from 
`bisect_rev_setup()`.
* Fix if condition.
* Add `bisect_autostart()` in `bisect_append_log_quoted()`.
* Check `git_path_bisect_head()` with `file_exist()` instead of 
`is_empty_or_missing_file()`.
* Fix return with BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
* Add conversion in `cmd_*()` to BISECT_OK when return is 
BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.

* Note to previous reviewer: suggestion to change `bisect_auto_next()` function
to return void cannot be done because errors from `bisect_next()` are not 
ignored.

--

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

* Check `git_path_bisect_head()` with `file_exist()` instead of 
`is_empty_or_missing_file()`.

--

Miriam Rubio (3):
  bisect--helper: fix `cmd_*()` function switch default return
  bisect--helper: use '-res' in 'cmd_bisect__helper' return
  bisect--helper: introduce new `write_in_file()` function

Pranit Bauva (9):
  bisect--helper: reimplement `bisect_autostart` shell function in C
  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_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                 |   8 +
 builtin/bisect--helper.c | 383 +++++++++++++++++++++++++++++++++------
 git-bisect.sh            | 145 +--------------
 3 files changed, 347 insertions(+), 189 deletions(-)

-- 
2.25.0

[PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return

[Miriam Rubio]

In a `cmd_*()` function, return `error()` cannot be used
because that translates to `-1` and `cmd_*()` functions need
to return exit codes.

Let's fix switch default return.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c1c40b516d..a81a2c76ff 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		res = bisect_start(&terms, no_checkout, argv, argc);
 		break;
 	default:
-		return error("BUG: unknown subcommand '%d'", cmdmode);
+		BUG("unknown subcommand %d", (int)cmdmode);
 	}
 	free_terms(&terms);
 
-- 
2.25.0

Re: [PATCH v3 01/12] bisect--helper: fix `cmd_*()` function switch default return

[Johannes Schindelin]

Hi Miriam,

On Thu, 23 Apr 2020, Miriam Rubio wrote:

> In a `cmd_*()` function, return `error()` cannot be used
> because that translates to `-1` and `cmd_*()` functions need
> to return exit codes.
>
> Let's fix switch default return.

I think there is an even stronger argument for not returning here: it
actually _would_ be a bug, not a user error, if the `default` branch was
reached.

The patch looks good.

Ciao,
Dscho

>
> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c1c40b516d..a81a2c76ff 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		res = bisect_start(&terms, no_checkout, argv, argc);
>  		break;
>  	default:
> -		return error("BUG: unknown subcommand '%d'", cmdmode);
> +		BUG("unknown subcommand %d", (int)cmdmode);
>  	}
>  	free_terms(&terms);
>
> --
> 2.25.0
>
>

[PATCH v3 02/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return

[Miriam Rubio]

Following 'enum bisect_error' vocabulary, return variable 'res' is
always non-positive.
Let's use '-res' instead of 'abs(res)' to make the code clearer.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/bisect--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index a81a2c76ff..0fbd924aac 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -722,5 +722,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
 		res = BISECT_OK;
 
-	return abs(res);
+	return -res;
 }
-- 
2.25.0

Re: [PATCH v3 02/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return

[Johannes Schindelin]

Hi Miriam,

On Thu, 23 Apr 2020, Miriam Rubio wrote:

> Following 'enum bisect_error' vocabulary, return variable 'res' is
> always non-positive.
> Let's use '-res' instead of 'abs(res)' to make the code clearer.

I agree with this reasoning, and with the patch.

Thank you,
Dscho

>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> ---
>  builtin/bisect--helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index a81a2c76ff..0fbd924aac 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -722,5 +722,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  	if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
>  		res = BISECT_OK;
>
> -	return abs(res);
> +	return -res;
>  }
> --
> 2.25.0
>
>

[PATCH v3 03/12] 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 helper will be used in later steps and makes the code
simpler 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 | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 0fbd924aac..d3b2b33df0 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -74,6 +74,28 @@ static int one_of(const char *term, ...)
 	return res;
 }
 
+static int write_in_file(const char *path, const char *mode, const char *format,...)
+{
+	FILE *fp = NULL;
+	va_list args;
+	int res = 0;
+
+	if (!strcmp(mode, "a") && !strcmp(mode, "w"))
+		return error(_("wrong writing mode '%s'"), mode);
+	fp = fopen(path, mode);
+	if (!fp)
+		return error_errno(_("cannot open file '%s' in mode '%s'"), path, mode);
+	va_start(args, format);
+	res = vfprintf(fp, format, args);
+	va_end(args);
+	if (!res) {
+		fclose(fp);
+		return error_errno(_("could not write to file '%s'"), path);
+	}
+
+	return fclose(fp);
+}
+
 static int check_term_format(const char *term, const char *orig_term)
 {
 	int res;
@@ -104,7 +126,6 @@ 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;
 	int res;
 
 	if (!strcmp(bad, good))
@@ -113,12 +134,8 @@ 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(), "w", "%s\n%s\n", bad, good);
 
-	res = fprintf(fp, "%s\n%s\n", bad, good);
-	res |= fclose(fp);
 	return (res < 0) ? -1 : 0;
 }
 
-- 
2.25.0

Re: [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function

[Johannes Schindelin]

Hi Miriam,

On Thu, 23 Apr 2020, Miriam Rubio wrote:

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

Don't we have that already?
https://github.com/git/git/blob/v2.27.0-rc1/wrapper.c#L625-L638

If that function does not meet the bill, it would make more sense to
extend it (or refactor it, providing a new global function that does meet
the bill).

> This helper will be used in later steps and makes the code
> simpler and easier to understand.

Makes a lot of sense to me!

> 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 | 29 +++++++++++++++++++++++------
>  1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 0fbd924aac..d3b2b33df0 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -74,6 +74,28 @@ static int one_of(const char *term, ...)
>  	return res;
>  }
>
> +static int write_in_file(const char *path, const char *mode, const char *format,...)

I wonder whether there is _ever_ a case where we want to set `mode` to
anything else than `"w"`? Ah...

> +{
> +	FILE *fp = NULL;
> +	va_list args;
> +	int res = 0;
> +
> +	if (!strcmp(mode, "a") && !strcmp(mode, "w"))
> +		return error(_("wrong writing mode '%s'"), mode);

... rather than doing this, I would prefer a pair of functions (that can
call the same helper function doing the actual work): `write_to_file()`
and `append_to_file()`.

Apart from that, the patch looks good to me,
Dscho

> +	fp = fopen(path, mode);
> +	if (!fp)
> +		return error_errno(_("cannot open file '%s' in mode '%s'"), path, mode);
> +	va_start(args, format);
> +	res = vfprintf(fp, format, args);
> +	va_end(args);
> +	if (!res) {
> +		fclose(fp);
> +		return error_errno(_("could not write to file '%s'"), path);
> +	}
> +
> +	return fclose(fp);
> +}
> +
>  static int check_term_format(const char *term, const char *orig_term)
>  {
>  	int res;
> @@ -104,7 +126,6 @@ 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;
>  	int res;
>
>  	if (!strcmp(bad, good))
> @@ -113,12 +134,8 @@ 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(), "w", "%s\n%s\n", bad, good);
>
> -	res = fprintf(fp, "%s\n%s\n", bad, good);
> -	res |= fclose(fp);
>  	return (res < 0) ? -1 : 0;
>  }
>
> --
> 2.25.0
>
>

Re: [PATCH v3 03/12] bisect--helper: introduce new `write_in_file()` function

[Đoàn Trần Công Danh]

On 2020-04-23 09:06:55+0200, Miriam Rubio <mirucam@gmail.com> wrote:
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 0fbd924aac..d3b2b33df0 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -74,6 +74,28 @@ static int one_of(const char *term, ...)
>  	return res;
>  }
>  
> +static int write_in_file(const char *path, const char *mode, const char *format,...)

Nit: It looks like we want to add a space in the end ", ...)"
Since "one_of" above also has that space.

Otherwise, look good.

-- 
Danh

[PATCH v3 04/12] 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 | 40 +++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 25 ++-----------------------
 2 files changed, 41 insertions(+), 24 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index d3b2b33df0..9df69800e3 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -29,6 +29,7 @@ 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-autostart"),
 	NULL
 };
 
@@ -55,6 +56,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.
@@ -630,6 +633,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 	return res;
 }
 
+static int bisect_autostart(struct bisect_terms *terms)
+{
+	const char *yesno;
+
+	if (!is_empty_or_missing_file(git_path_bisect_start()))
+		return 0;
+
+	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, NULL, 0);
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -642,7 +671,8 @@ 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_AUTOSTART,
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
@@ -666,6 +696,8 @@ 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-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,
@@ -727,6 +759,12 @@ 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_AUTOSTART:
+		if (argc)
+			return error(_("--bisect-autostart requires 0 arguments"));
+		set_terms(&terms, "bad", "good");
+		res = bisect_autostart(&terms);
+		break;
 	default:
 		BUG("unknown subcommand %d", (int)cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index efee12b8b1..426d443e7e 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
-			bisect_start
-		else
-			exit 1
-		fi
-	}
-}
-
 bisect_start() {
 	git bisect--helper --bisect-start $@ || exit
 
@@ -108,7 +87,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
@@ -149,7 +128,7 @@ bisect_auto_next() {
 
 bisect_next() {
 	case "$#" in 0) ;; *) usage ;; esac
-	bisect_autostart
+	git bisect--helper --bisect-autostart
 	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
 
 	# Perform all bisection computation, display and checkout
-- 
2.25.0

Re: [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C

[Johannes Schindelin]

Hi Miriam,

On Thu, 23 Apr 2020, Miriam Rubio wrote:

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

I know you did not author this, but maybe I can still ask you to address a
couple of concerns I have? I kind of stumble when I read "Also add a
subcommand ... be called from ... from ...".

Also, why the funny space before the full stop?

> Using `--bisect-autostart` subcommand is a temporary measure to port
> shell function to C so as to use the existing test suite. As more

Likewise, "... to port shell function..." might benefit from inserting
"the" in the middle?

> 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 | 40 +++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            | 25 ++-----------------------
>  2 files changed, 41 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index d3b2b33df0..9df69800e3 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -29,6 +29,7 @@ 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-autostart"),
>  	NULL
>  };
>
> @@ -55,6 +56,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);
> +

Why forward-declare it? There is no reference before it is defined below,
so I think this forward-declaration can be removed without any problem.

>  /*
>   * Check whether the string `term` belongs to the set of strings
>   * included in the variable arguments.
> @@ -630,6 +633,32 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
>  	return res;
>  }
>
> +static int bisect_autostart(struct bisect_terms *terms)
> +{
> +	const char *yesno;
> +
> +	if (!is_empty_or_missing_file(git_path_bisect_start()))
> +		return 0;

This is a bit more convoluted to read than the shell script:

	bisect_autostart() {
		test -s "$GIT_DIR/BISECT_START" || {
			gettextln "You need to start by \"git bisect start\"" >&2
			[...]
		}
	}

The `test -s <file>` command succeeds if the file exists and is non-empty.
Which makes sense: either the file exists and is non-empty, or we need to
do something.

When I read the C version, it takes me a bit more time to figure out what
is going on here. Maybe we could introduce a helper function like this and
use it here?

	static inline int file_is_not_empty(const char *path)
	{
		return !is_empty_or_missing_file(path);
	}

> +
> +	fprintf(stderr, _("You need to start by \"git bisect "
> +			  "start\"\n"));

If you use `fprintf_ln()` here, you can keep the string identical to the
shell script version, i.e. without the trailing newline. That will allow
the translations to be re-used, lessening the burden of the i18n team.

> +
> +	if (!isatty(STDIN_FILENO))
> +		return 1;

I would prefer it if only `cmd_*()` functions return positive values for
the error case.
> +
> +	/*
> +	 * 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);

Technically, `PROMPT_ECHO` is not required here as `git_prompt()` adds
that flag implicitly unless you pass in `PASS_ASKPASS`, but I guess it
does not hurt here.

However, I cannot fail to note that this constitutes a change of behavior,
as `git_prompt()` does _not_ read from `stdin`.

Maybe we should call `git_read_line_interactively()` instead, after
writing the prompt to `stderr` explicitly?

> +	if (starts_with(yesno, _("n")) || starts_with(yesno, _("N")))

This is wrong: "n" and "N" should not be translated, as they are not
translated in the shell script version, either:

			case "$yesno" in
			[Nn]*)
				exit ;;
			esac

In light of this, a much, much shorter version would be:

	if (tolower(*yesno) == 'n')
		return -1;

(I use `-1` here because this is not within a `cmd_*()` function.)

However, I still think that we need to call `free(yesno);` or we leak
memory. My preferred way would be:

		int ret;
		[...]

		ret = tolower(*yesno) == 'n' ?
			-1 : bisect_start(terms, 0, NULL, 0);

		free(yesno);
		return ret;

> +		return 1;
> +
> +	return bisect_start(terms, 0, NULL, 0);
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -642,7 +671,8 @@ 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_AUTOSTART,
>  	} cmdmode = 0;
>  	int no_checkout = 0, res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -666,6 +696,8 @@ 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-autostart", &cmdmode,
> +			 N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),

Not that it matters a lot because this command mode will go away soon
thereafter, but this describes an implementation detail rather than the
purpose of the function. I would much rather read "start the bisection if
it has not yet been started" instead.

>  		OPT_BOOL(0, "no-checkout", &no_checkout,
>  			 N_("update BISECT_HEAD instead of checking out the current commit")),
>  		OPT_BOOL(0, "no-log", &nolog,
> @@ -727,6 +759,12 @@ 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_AUTOSTART:
> +		if (argc)
> +			return error(_("--bisect-autostart requires 0 arguments"));

Maybe "--bisect-autostart does not accept arguments" instead?

Ciao,
Dscho

> +		set_terms(&terms, "bad", "good");
> +		res = bisect_autostart(&terms);
> +		break;
>  	default:
>  		BUG("unknown subcommand %d", (int)cmdmode);
>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index efee12b8b1..426d443e7e 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
> -			bisect_start
> -		else
> -			exit 1
> -		fi
> -	}
> -}
> -
>  bisect_start() {
>  	git bisect--helper --bisect-start $@ || exit
>
> @@ -108,7 +87,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
> @@ -149,7 +128,7 @@ bisect_auto_next() {
>
>  bisect_next() {
>  	case "$#" in 0) ;; *) usage ;; esac
> -	bisect_autostart
> +	git bisect--helper --bisect-autostart
>  	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
>
>  	# Perform all bisection computation, display and checkout
> --
> 2.25.0
>
>

Re: [PATCH v3 04/12] bisect--helper: reimplement `bisect_autostart` shell function in C

[Johannes Schindelin]

Hi Miriam,

On Fri, 22 May 2020, Johannes Schindelin wrote:

> On Thu, 23 Apr 2020, Miriam Rubio wrote:
>
> > +	/*
> > +	 * 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);
>
> [...]
>
> I still think that we need to call `free(yesno);` or we leak memory.

Never mind. I got fooled by the fact that `git_prompt()` returns `char *`.
It actually returns the pointer `buf.buf` of a `static `strbuf buf`, so
there is nothing to `free()` here.

Sorry for the noise,
Dscho

[PATCH v3 05/12] 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 .

bisect_auto_next() function returns an enum bisect_error type as whole
`git bisect` can exit with an error code when bisect_next() does.

Using `--bisect-next` and `--bisect-auto-next` 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-next`
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                 |   8 ++
 builtin/bisect--helper.c | 169 ++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            |  47 +----------
 3 files changed, 179 insertions(+), 45 deletions(-)

diff --git a/bisect.c b/bisect.c
index 9154f810f7..31cab3f0b0 100644
--- a/bisect.c
+++ b/bisect.c
@@ -980,6 +980,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.
  */
@@ -1064,6 +1070,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
 		  "Bisecting: %d revisions left to test after this %s\n",
 		  nr), nr, steps_msg);
 	free(steps_msg);
+	/* Clean up objects used, as they will be reused. */
+	clear_commit_marks_all(ALL_REV_FLAGS);
 
 	return bisect_checkout(bisect_rev, no_checkout);
 }
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 9df69800e3..c6aaa8eb15 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"),
 	N_("git bisect--helper --bisect-autostart"),
 	NULL
 };
@@ -441,6 +444,150 @@ 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 argv_array *rev_argv = cb_data;
+
+	argv_array_push(rev_argv, oid_to_hex(oid));
+	return 0;
+}
+
+static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
+{
+	char *term_good = xstrfmt("%s-*", terms->term_good);
+
+	argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
+	for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv);
+
+	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
+	 * sets up 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") < 0)
+		return error_errno(_("failed to write to '%s'"), git_path_bisect_log());
+
+	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);
+	}
+
+	/*
+	 * Reset the flags used by revision walks in case
+	 * there is another revision walk after this one.
+	 */
+	reset_revision_walk();
+
+	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);
+	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);
+
+	res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n",
+			    terms->term_bad, oid_to_hex(&oid),
+			    commit_name.buf);
+
+	strbuf_release(&commit_name);
+	free(bad_ref);
+	return res;
+}
+
+static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
+{
+	int no_checkout;
+	enum bisect_error res;
+
+	bisect_autostart(terms);
+	if (bisect_next_check(terms, terms->term_good))
+		return BISECT_FAILED;
+
+	no_checkout = file_exists(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_1ST_BAD_FOUND;
+	} 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)
 {
@@ -672,7 +819,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_NEXT_CHECK,
 		BISECT_TERMS,
 		BISECT_START,
-		BISECT_AUTOSTART,
+		BISECT_NEXT,
+		BISECT_AUTO_NEXT,
+		BISECT_AUTOSTART
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
@@ -696,6 +845,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_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,
@@ -759,6 +912,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;
 	case BISECT_AUTOSTART:
 		if (argc)
 			return error(_("--bisect-autostart requires 0 arguments"));
@@ -774,7 +939,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	 * Handle early success
 	 * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
 	 */
-	if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
+	if ((res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) || (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND))
 		res = BISECT_OK;
 
 	return -res;
diff --git a/git-bisect.sh b/git-bisect.sh
index 426d443e7e..897825b675 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -65,8 +65,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
 }
@@ -119,45 +118,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
-	git bisect--helper --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() {
@@ -211,7 +172,7 @@ bisect_replay () {
 			die "$(gettext "?? what are you talking about?")" ;;
 		esac
 	done <"$file"
-	bisect_auto_next
+	git bisect--helper --bisect-auto-next
 }
 
 bisect_run () {
@@ -308,7 +269,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

Re: [PATCH v3 05/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Johannes Schindelin]

Hi Miriam,

On Thu, 23 Apr 2020, Miriam Rubio wrote:

> 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 .
>
> bisect_auto_next() function returns an enum bisect_error type as whole
> `git bisect` can exit with an error code when bisect_next() does.
>
> Using `--bisect-next` and `--bisect-auto-next` 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-next`
> 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                 |   8 ++
>  builtin/bisect--helper.c | 169 ++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            |  47 +----------
>  3 files changed, 179 insertions(+), 45 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index 9154f810f7..31cab3f0b0 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -980,6 +980,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.

Nice!

> + *
>   * If no_checkout is non-zero, the bisection process does not
>   * checkout the trial commit but instead simply updates BISECT_HEAD.
>   */
> @@ -1064,6 +1070,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int
>  		  "Bisecting: %d revisions left to test after this %s\n",
>  		  nr), nr, steps_msg);
>  	free(steps_msg);
> +	/* Clean up objects used, as they will be reused. */
> +	clear_commit_marks_all(ALL_REV_FLAGS);

That strikes me as a change that is orthogonal to the purpose claimed by
the commit message. As such, I think it wants to live in its own commit.

>
>  	return bisect_checkout(bisect_rev, no_checkout);
>  }
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 9df69800e3..c6aaa8eb15 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"),
>  	N_("git bisect--helper --bisect-autostart"),
>  	NULL
>  };
> @@ -441,6 +444,150 @@ 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 argv_array *rev_argv = cb_data;
> +
> +	argv_array_push(rev_argv, oid_to_hex(oid));
> +	return 0;
> +}
> +
> +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
> +{
> +	char *term_good = xstrfmt("%s-*", terms->term_good);

This could be `xstrfmt("refs/bisect/%s-*", terms->term_good)` instead, and
that would have avoided some head-scratching on my side. The
"refs/bisect/" prefix passed to `for_each_glob_ref_in()` makes this
correct, of course.

> +
> +	argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);

This hard-codes `bad` where the shell script version uses
`refs/bisect/$TERM_BAD`.

> +	for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv);
> +
> +	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
> +	 * sets up a revision walk.
> +	 */
> +	reset_revision_walk();
> +	init_revisions(revs, NULL);
> +	rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);

I have not heard any valid argument against my suggestion in
https://lore.kernel.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/
to render into the pending objects directly. Yes, there was a hand-waving
"but the shell script version does not do that" but that's just not a
valid argument because if the shell script version could, it would, but it
cannot: it's shell script, for crying out loud. It has no way to use the
appropriate data structures. C code does. And should. We're not in the
business of converting OIDs into strings that are appended to string lists
so that they can be re-parsed into OIDs.

Therefore, I would suggest (and now that almost four months have passed
since my initial suggestion, rather strongly) to do something like this:

	struct add_bisect_ref_data {
		struct rev_info *revs;
		int object_flags;
	};

	static int add_bisect_ref(const char *refname, const struct object_id *oid,
				  int flags, void *cb)
	{
		struct add_bisect_ref_data *data = cb;

		add_pending_oid(data->revs, refname, oid, data->object_flags);

		return 0;
	}

	[...]
		struct add_bisect_ref_data cb = { &revs };
		char *good = xstrfmt("%s-*", terms->term_good);

		[...]
		setup_revisions(0, NULL, revs, NULL);
		for_each_glob_ref_in(add_bisect_ref, terms->term_bad, "refs/bisect/", &cb);
		cb.object_flags = UNINTERESTING;
		for_each_glob_ref_in(add_bisect_ref, good, "refs/bisect/", &cb);

This would remove one (unnecessary) level of indirection, and converting
OIDs from/to/from strings.

> +	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") < 0)
> +		return error_errno(_("failed to write to '%s'"), git_path_bisect_log());
> +
> +	while ((commit = get_revision(revs)) != NULL) {
> +		struct strbuf commit_name = STRBUF_INIT;

I missed this in my review end of January: it is an anti-pattern to create
a new `strbuf` and releasing it right away in the same loop iteration.
Let's instead move the declaration of `commit_name` to the top of the
function, then call `strbuf_reset()` at the beginning of the loop, and
finally call `strbuf_release()` only _once_, at the end of the function.

> +		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);
> +	}
> +
> +	/*
> +	 * Reset the flags used by revision walks in case
> +	 * there is another revision walk after this one.
> +	 */
> +	reset_revision_walk();
> +
> +	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;

It would make the code easier to follow if `prepare_revs()` was called
before opening the file and returning early in case of an error.

Further, I do not see the point of having the code in
`process_skipped_commits()` there is only this caller, in a tiny function.
It would be better to fold the code into a single function.

I did point that out already in my review at the end of this past January.

> +}
> +
> +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);
> +	int res;
> +
> +	read_ref(bad_ref, &oid);
> +	printf("%s\n", bad_ref);

I missed this in January: is this a left-over debug printf? I cannot find
the equivalent in the shell script version.

> +	commit = lookup_commit_reference(the_repository, &oid);

There is a relatively recent convention to initialize a local variable
`struct repository *r = the_repository;` at the beginning of the function
and then using `r` throughout the function. I think I'd like to use that
here, too, even if it is only used once.

Any reason not to use `lookup_commit_reference_by_name(r, bad_ref)`
directly instead? You can still reference `commit->object.oid` instead of
`oid` later on.

> +	format_commit_message(commit, "%s", &commit_name, &pp);
> +
> +	res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n",
> +			    terms->term_bad, oid_to_hex(&oid),
> +			    commit_name.buf);
> +
> +	strbuf_release(&commit_name);
> +	free(bad_ref);
> +	return res;
> +}
> +
> +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
> +{
> +	int no_checkout;
> +	enum bisect_error res;
> +
> +	bisect_autostart(terms);
> +	if (bisect_next_check(terms, terms->term_good))
> +		return BISECT_FAILED;
> +
> +	no_checkout = file_exists(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_1ST_BAD_FOUND;
> +	} else if (res == BISECT_ONLY_SKIPPED_LEFT) {
> +		res = bisect_skipped_commits(terms);
> +		return res ? res : BISECT_ONLY_SKIPPED_LEFT;
> +	}

This looks much, much nicer than the equivalent code I reviewed almost
four months ago.

Personally, I would have used the idiom

		if (!res)
			return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND;

but I can live with the current version.

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

This is a straight-forward translation from the shell code.

I would like to take a step back, though: the only two differences between the
`bisect_next_check()` call here and the one in `bisect_next()` are:

- the return value if `bisect_next_check()` failed: the "auto" version
  seems to report success, the non-"auto" version failure

- the "auto" version seems not to accept missing "good" commits

Seeing this, it does strike me odd that `bisect_next()` is called from
`bisect_auto_next()`, and I would have rather expected an `int auto`
parameter to the `bisect_next()` function, where the `bisect_autostart()`
call is skipped in case `auto != 0`.

The rest of the patch looks fine to me.

Ciao,
Dscho

> +
> +	return bisect_next(terms, prefix);
> +}
> +
>  static int bisect_start(struct bisect_terms *terms, int no_checkout,
>  			const char **argv, int argc)
>  {
> @@ -672,7 +819,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		BISECT_NEXT_CHECK,
>  		BISECT_TERMS,
>  		BISECT_START,
> -		BISECT_AUTOSTART,
> +		BISECT_NEXT,
> +		BISECT_AUTO_NEXT,
> +		BISECT_AUTOSTART
>  	} cmdmode = 0;
>  	int no_checkout = 0, res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -696,6 +845,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_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,
> @@ -759,6 +912,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;
>  	case BISECT_AUTOSTART:
>  		if (argc)
>  			return error(_("--bisect-autostart requires 0 arguments"));
> @@ -774,7 +939,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  	 * Handle early success
>  	 * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
>  	 */
> -	if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
> +	if ((res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) || (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND))
>  		res = BISECT_OK;
>
>  	return -res;
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 426d443e7e..897825b675 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -65,8 +65,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
>  }
> @@ -119,45 +118,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
> -	git bisect--helper --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() {
> @@ -211,7 +172,7 @@ bisect_replay () {
>  			die "$(gettext "?? what are you talking about?")" ;;
>  		esac
>  	done <"$file"
> -	bisect_auto_next
> +	git bisect--helper --bisect-auto-next
>  }
>
>  bisect_run () {
> @@ -308,7 +269,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 v3 06/12] 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            | 26 ++-----------------
 2 files changed, 44 insertions(+), 36 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index c6aaa8eb15..f2ec7f89e4 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -588,11 +588,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;
@@ -645,9 +646,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);
@@ -725,12 +729,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".
 	 */
 
@@ -746,7 +750,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;
 		}
 	}
@@ -758,25 +762,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 897825b675..049ffacdff 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -49,27 +49,6 @@ bisect_head()
 	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 "$@"
@@ -162,8 +141,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)
@@ -262,7 +240,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

Re: [PATCH v3 06/12] bisect--helper: finish porting `bisect_start()` to C

[Johannes Schindelin]

Hi Miriam,

On Thu, 23 Apr 2020, Miriam Rubio wrote:

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

I do not see any "handle" in the diff...

> As "trap" is a reference to the shell "trap" builtin, which isn't
> used anymore.

Otherwise, the commit message looks good to me!

>
> 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            | 26 ++-----------------
>  2 files changed, 44 insertions(+), 36 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c6aaa8eb15..f2ec7f89e4 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -588,11 +588,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;
> @@ -645,9 +646,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;
> +			}

Makes sense.

>
>  			string_list_append(&revs, oid_to_hex(&oid));
>  			free(commit_id);
> @@ -725,12 +729,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".
>  	 */
>
> @@ -746,7 +750,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;
>  		}
>  	}
> @@ -758,25 +762,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.

This "below" was true in the shell script version, but now it should be
"above".

> +	 * 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.

Maybe we should have a helper function and use it here:

	static int is_bisect_success(enum bisect_error res)
	{
		return !res ||
			res == BISECT_INTERNAL_1ST_BAD_FOUND ||
			res == BISECT_INTERNAL_SUCCESS_MERGE_BASE;
	}

(Note that I added the `_1ST_BAD_FOUND` case here.)

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

While this might be an interesting tidbit for the commit message, I do not
think that it is a good idea to keep this as a code comment; It is prone
to become stale/incorrect over time.

> +	 */
> +	if (res && res != BISECT_INTERNAL_SUCCESS_MERGE_BASE)
> +		bisect_clean_state();
>  	return res;
>  }
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 897825b675..049ffacdff 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -49,27 +49,6 @@ bisect_head()
>  	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 "$@"
> @@ -162,8 +141,7 @@ bisect_replay () {
>  		get_terms
>  		case "$command" in
>  		start)
> -			cmd="bisect_start $rev"
> -			eval "$cmd" ;;
> +			eval "git bisect--helper --bisect-start $rev" ;;

The original code _had_ to use `eval` because `bisect_start` could cause
the code to `exit` and we did _not_ want to let that exit the `replay`
process.

However, we are calling `git bisect--helper` here, which runs in its own
process anyway, so the `eval` should be dropped now.

Otherwise, the patch looks fine to me,
Dscho

>  		"$TERM_GOOD"|"$TERM_BAD"|skip)
>  			git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
>  		terms)
> @@ -262,7 +240,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 v3 07/12] 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 f2ec7f89e4..1b4d0a0f87 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>"),
@@ -841,7 +840,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,
@@ -859,8 +857,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,
@@ -904,10 +900,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 v3 08/12] 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 1b4d0a0f87..2d8660c79f 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>"),
@@ -838,8 +837,7 @@ static int bisect_autostart(struct bisect_terms *terms)
 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,
@@ -853,8 +851,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,
@@ -893,9 +889,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 v3 09/12] 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 | 70 +++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 55 +++----------------------------
 2 files changed, 73 insertions(+), 52 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 2d8660c79f..9db72f5891 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-next"),
 	N_("git bisect--helper --bisect-auto-next"),
 	N_("git bisect--helper --bisect-autostart"),
+	N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
+	N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
 	NULL
 };
 
@@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)
 	return bisect_start(terms, 0, NULL, 0);
 }
 
+static int bisect_head(struct object_id *oid)
+{
+	if (!file_exists(git_path_bisect_head()))
+		return get_oid("HEAD", oid);
+
+	return get_oid("BISECT_HEAD", oid);
+}
+
+static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
+			int argc)
+{
+	const char *state;
+	const char *hex;
+	int i;
+	struct oid_array revs = OID_ARRAY_INIT;
+	struct object_id oid;
+
+	if (!argc)
+		return error(_("Please call `--bisect-state` with at least one argument"));
+	state = argv[0];
+	if (check_and_set_terms(terms, state) ||
+	    !one_of(state, terms->term_good,terms->term_bad, "skip", NULL))
+		return BISECT_FAILED;
+	argv++;
+	argc--;
+	if (!strcmp(state, terms->term_bad) && (argc > 1))
+		return error(_("'git bisect %s' can take only one argument."),terms->term_bad);
+	if (argc == 0) {
+		if (bisect_head(&oid))
+			return error(_("Bad bisect_head rev input"));
+		hex = oid_to_hex(&oid);
+		if (bisect_write(state, hex, terms, 0))
+			return BISECT_FAILED;
+		check_expected_revs(&hex, 1);
+		return bisect_auto_next(terms, NULL);
+	}
+
+	/* Here argc > 0 */
+	for (; argc; argc--, argv++) {
+		struct object_id oid;
+		if (get_oid(*argv, &oid))
+			return error(_("Bad rev input: %s"), *argv);
+		oid_array_append(&revs, &oid);
+	}
+
+	for (i = 0; i < revs.nr; i++) {
+		hex = oid_to_hex(&revs.oid[i]);
+		if (bisect_write(state, hex, terms, 0)) {
+			oid_array_clear(&revs);
+			return BISECT_FAILED;
+		}
+		check_expected_revs(&hex, 1);
+	}
+
+	oid_array_clear(&revs);
+	return bisect_auto_next(terms, NULL);
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_START,
 		BISECT_NEXT,
 		BISECT_AUTO_NEXT,
-		BISECT_AUTOSTART
+		BISECT_AUTOSTART,
+		BISECT_STATE
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
@@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
 		OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
 			 N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
+		OPT_CMDMODE(0, "bisect-state", &cmdmode,
+			 N_("mark the state of ref (or refs)"), BISECT_STATE),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_BOOL(0, "no-log", &nolog,
@@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		set_terms(&terms, "bad", "good");
 		res = bisect_autostart(&terms);
 		break;
+	case BISECT_STATE:
+		set_terms(&terms, "bad", "good");
+		get_terms(&terms);
+		res = bisect_state(&terms, argv, argc);
+		break;
 	default:
 		BUG("unknown subcommand %d", (int)cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 049ffacdff..2da0810b1a 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
 TERM_BAD=bad
 TERM_GOOD=good
 
-bisect_head()
-{
-	if test -f "$GIT_DIR/BISECT_HEAD"
-	then
-		echo BISECT_HEAD
-	else
-		echo HEAD
-	fi
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -61,43 +51,7 @@ bisect_skip() {
 		esac
 		all="$all $revs"
 	done
-	eval bisect_state 'skip' $all
-}
-
-bisect_state() {
-	git bisect--helper --bisect-autostart
-	state=$1
-	git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
-	get_terms
-	case "$#,$state" in
-	0,*)
-		die "Please call 'bisect_state' with at least one argument." ;;
-	1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
-		bisected_head=$(bisect_head)
-		rev=$(git rev-parse --verify "$bisected_head") ||
-			die "$(eval_gettext "Bad rev input: \$bisected_head")"
-		git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
-		git bisect--helper --check-expected-revs "$rev" ;;
-	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
-		shift
-		hash_list=''
-		for rev in "$@"
-		do
-			sha=$(git rev-parse --verify "$rev^{commit}") ||
-				die "$(eval_gettext "Bad rev input: \$rev")"
-			hash_list="$hash_list $sha"
-		done
-		for rev in $hash_list
-		do
-			git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
-		done
-		git bisect--helper --check-expected-revs $hash_list ;;
-	*,"$TERM_BAD")
-		die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
-	*)
-		usage ;;
-	esac
-	git bisect--helper --bisect-auto-next
+	eval git bisect--helper --bisect-state 'skip' $all
 }
 
 bisect_visualize() {
@@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
 			state="$TERM_GOOD"
 		fi
 
-		# We have to use a subshell because "bisect_state" can exit.
-		( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
+		git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
 		res=$?
 
 		cat "$GIT_DIR/BISECT_RUN"
@@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
 		if [ $res -ne 0 ]
 		then
 			eval_gettextln "bisect run failed:
-'bisect_state \$state' exited with error code \$res" >&2
+'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2
 			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 v3 09/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C

[Johannes Schindelin]

Hi Miriam,

On Thu, 23 Apr 2020, Miriam Rubio wrote:

> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Reimplement the `bisect_state()` shell functions in C and also add a
> subcommand `--bisect-state` to `git-bisect--helper` to call them from
> git-bisect.sh .
>
> Using `--bisect-state` subcommand is a temporary measure to port shell
> function to C so as to use the existing test suite. As more functions
> are ported, this subcommand will be retired and will be called by some
> other methods.
>
> `bisect_head()` is only called from `bisect_state()`, thus it is not
> required to introduce another subcommand.
>
> Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>  builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            | 55 +++----------------------------
>  2 files changed, 73 insertions(+), 52 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 2d8660c79f..9db72f5891 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
>  	N_("git bisect--helper --bisect-next"),
>  	N_("git bisect--helper --bisect-auto-next"),
>  	N_("git bisect--helper --bisect-autostart"),
> +	N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
> +	N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
>  	NULL
>  };
>
> @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)
>  	return bisect_start(terms, 0, NULL, 0);
>  }
>
> +static int bisect_head(struct object_id *oid)
> +{
> +	if (!file_exists(git_path_bisect_head()))
> +		return get_oid("HEAD", oid);
> +
> +	return get_oid("BISECT_HEAD", oid);

This can be easily reduced to

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

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

So really, what this should do instead is this:

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

static enum bisect_error bisect_state(struct bisect_terms *terms,
				      const char **argv, int argc)
{
	const char *state;
	int i, verify_expected = 1;
	struct object_id oid, expected;
	struct strbuf buf = STRBUF_INIT;

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

	state = argv[0];
	if (check_and_set_terms(terms, state) ||
	    !one_of(state, terms->term_good, terms->term_bad, "skip", NULL))
		return BISECT_FAILED;

	argv++;
	argc--;
	if (argc > 1 && !strcmp(state, terms->term_bad))
		return error(_("'git bisect %s' can take only one argument."), terms->term_bad);

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


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

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

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

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

	return bisect_auto_next(terms, NULL);
}

There, not bad, is it?

> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		BISECT_START,
>  		BISECT_NEXT,
>  		BISECT_AUTO_NEXT,
> -		BISECT_AUTOSTART
> +		BISECT_AUTOSTART,
> +		BISECT_STATE
>  	} cmdmode = 0;
>  	int no_checkout = 0, res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  			 N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
>  		OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
>  			 N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
> +		OPT_CMDMODE(0, "bisect-state", &cmdmode,
> +			 N_("mark the state of ref (or refs)"), BISECT_STATE),
>  		OPT_BOOL(0, "no-checkout", &no_checkout,
>  			 N_("update BISECT_HEAD instead of checking out the current commit")),
>  		OPT_BOOL(0, "no-log", &nolog,
> @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		set_terms(&terms, "bad", "good");
>  		res = bisect_autostart(&terms);
>  		break;
> +	case BISECT_STATE:
> +		set_terms(&terms, "bad", "good");
> +		get_terms(&terms);
> +		res = bisect_state(&terms, argv, argc);
> +		break;
>  	default:
>  		BUG("unknown subcommand %d", (int)cmdmode);
>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 049ffacdff..2da0810b1a 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
>  TERM_BAD=bad
>  TERM_GOOD=good
>
> -bisect_head()
> -{
> -	if test -f "$GIT_DIR/BISECT_HEAD"
> -	then
> -		echo BISECT_HEAD
> -	else
> -		echo HEAD
> -	fi
> -}
> -
>  bisect_skip() {
>  	all=''
>  	for arg in "$@"
> @@ -61,43 +51,7 @@ bisect_skip() {
>  		esac
>  		all="$all $revs"
>  	done
> -	eval bisect_state 'skip' $all
> -}
> -
> -bisect_state() {
> -	git bisect--helper --bisect-autostart
> -	state=$1
> -	git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> -	get_terms
> -	case "$#,$state" in
> -	0,*)
> -		die "Please call 'bisect_state' with at least one argument." ;;
> -	1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
> -		bisected_head=$(bisect_head)
> -		rev=$(git rev-parse --verify "$bisected_head") ||
> -			die "$(eval_gettext "Bad rev input: \$bisected_head")"
> -		git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> -		git bisect--helper --check-expected-revs "$rev" ;;
> -	2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
> -		shift
> -		hash_list=''
> -		for rev in "$@"
> -		do
> -			sha=$(git rev-parse --verify "$rev^{commit}") ||
> -				die "$(eval_gettext "Bad rev input: \$rev")"
> -			hash_list="$hash_list $sha"
> -		done
> -		for rev in $hash_list
> -		do
> -			git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> -		done
> -		git bisect--helper --check-expected-revs $hash_list ;;
> -	*,"$TERM_BAD")
> -		die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
> -	*)
> -		usage ;;
> -	esac
> -	git bisect--helper --bisect-auto-next
> +	eval git bisect--helper --bisect-state 'skip' $all
>  }
>
>  bisect_visualize() {
> @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
>  			state="$TERM_GOOD"
>  		fi
>
> -		# We have to use a subshell because "bisect_state" can exit.
> -		( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> +		git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
>  		res=$?
>
>  		cat "$GIT_DIR/BISECT_RUN"
> @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
>  		if [ $res -ne 0 ]
>  		then
>  			eval_gettextln "bisect run failed:
> -'bisect_state \$state' exited with error code \$res" >&2
> +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2

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

Maybe something to fix up in a follow-up?

Ciao,
Dscho

>  			exit $res
>  		fi
>
> @@ -242,7 +195,7 @@ case "$#" in
>  	start)
>  		git bisect--helper --bisect-start "$@" ;;
>  	bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
> -		bisect_state "$cmd" "$@" ;;
> +		git bisect--helper --bisect-state "$cmd" "$@" ;;
>  	skip)
>  		bisect_skip "$@" ;;
>  	next)
> --
> 2.25.0
>
>

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

[Miriam R.]

Hi Johannes,
I'm finishing the next patch series version and I have an issue about
one of your suggestions:

El sáb., 23 may. 2020 a las 0:06, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Thu, 23 Apr 2020, Miriam Rubio wrote:
>
> > From: Pranit Bauva <pranit.bauva@gmail.com>
> >
> > Reimplement the `bisect_state()` shell functions in C and also add a
> > subcommand `--bisect-state` to `git-bisect--helper` to call them from
> > git-bisect.sh .
> >
> > Using `--bisect-state` subcommand is a temporary measure to port shell
> > function to C so as to use the existing test suite. As more functions
> > are ported, this subcommand will be retired and will be called by some
> > other methods.
> >
> > `bisect_head()` is only called from `bisect_state()`, thus it is not
> > required to introduce another subcommand.
> >
> > Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> > Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> > ---
> >  builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-
> >  git-bisect.sh            | 55 +++----------------------------
> >  2 files changed, 73 insertions(+), 52 deletions(-)
> >
> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > index 2d8660c79f..9db72f5891 100644
> > --- a/builtin/bisect--helper.c
> > +++ b/builtin/bisect--helper.c
> > @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
> >       N_("git bisect--helper --bisect-next"),
> >       N_("git bisect--helper --bisect-auto-next"),
> >       N_("git bisect--helper --bisect-autostart"),
> > +     N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
> > +     N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
> >       NULL
> >  };
> >
> > @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)
> >       return bisect_start(terms, 0, NULL, 0);
> >  }
> >
> > +static int bisect_head(struct object_id *oid)
> > +{
> > +     if (!file_exists(git_path_bisect_head()))
> > +             return get_oid("HEAD", oid);
> > +
> > +     return get_oid("BISECT_HEAD", oid);
>
> This can be easily reduced to
>
>         return get_oid(file_exists(git_path_bisect_head()) ?
>                        "BISECT_HEAD" : "HEAD", oid);
>
> At the same time, it is wrong, just like the shell script version was
> wrong: in particular in light of the `hn/reftable` effort, we do _not_
> want to assume that all refs are backed by files!
>
> So really, what this should do instead is this:
>
>         enum get_oid_result res = get_oid("BISECT_HEAD", oid);
>
>         if (res == MISSING_OBJECT)
>                 res = get_oid("HEAD", oid);
>
> Given that this is still only three lines long, the overhead of having it
> in its own function for just a _single_ call seems excessive. I'd prefer
> it to be inlined in `bisect_state()`.
>
> > +}
> > +
> > +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
> > +                     int argc)
> > +{
>
> I offered a lengthy discussion about this function in
> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/
>
> It does not look, however, as if v3 benefitted from the entirety of my
> analysis: All the `check_expected_revs()` function does is to verify that
> the passed list of revs matches exactly the contents of the
> `BISECT_EXPECTED_REV` file.
>
> That can be done in a much simpler way, though, by first reading the file
> and parsing the contents into an OID, and then comparing to that parsed
> OID instead.
>
> Besides, `check_expected_revs()` is only used to check one rev at a time.
>
> In other words, it could be simplified to something like this:
>
> static void check_expected_rev(struct object_id *oid)
> {
>         struct object_id expected;
>         struct strbuf buf = STRBUF_INIT;
>
>         if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)
>               < the_hash_algo->hexsz ||
>             get_oid_hex(buf.buf, &expected) < 0)
>                 return; /* Ignore invalid file contents */
>
>         if (!oideq(oid, &expected)) {
>                 ... unlink ...
>                 return;
>         }
> }
>
> But even that would be wasteful, as we would read the file over and over
> and over again.
>
> The good news is that we do not even _need_ `check_expected_rev()`.
> Because we do not need to have two call sites, we can simplify the code
> much further. See below:
>
> > +     const char *state;
> > +     const char *hex;
> > +     int i;
> > +     struct oid_array revs = OID_ARRAY_INIT;
> > +     struct object_id oid;
> > +
> > +     if (!argc)
> > +             return error(_("Please call `--bisect-state` with at least one argument"));
> > +     state = argv[0];
> > +     if (check_and_set_terms(terms, state) ||
> > +         !one_of(state, terms->term_good,terms->term_bad, "skip", NULL))
> > +             return BISECT_FAILED;
> > +     argv++;
> > +     argc--;
> > +     if (!strcmp(state, terms->term_bad) && (argc > 1))
> > +             return error(_("'git bisect %s' can take only one argument."),terms->term_bad);
> > +     if (argc == 0) {
> > +             if (bisect_head(&oid))
> > +                     return error(_("Bad bisect_head rev input"));
> > +             hex = oid_to_hex(&oid);
> > +             if (bisect_write(state, hex, terms, 0))
> > +                     return BISECT_FAILED;
> > +             check_expected_revs(&hex, 1);
> > +             return bisect_auto_next(terms, NULL);
> > +     }
> > +
> > +     /* Here argc > 0 */
> > +     for (; argc; argc--, argv++) {
> > +             struct object_id oid;
> > +             if (get_oid(*argv, &oid))
> > +                     return error(_("Bad rev input: %s"), *argv);
> > +             oid_array_append(&revs, &oid);
> > +     }
>
> It really does not make sense to parse the arguments into an OID array,
> _then_ iterate over the array once, and then immediately releasing it.
> That OID array is not needed at all.
>
> So we'll end up with this loop in case `argc > 0` (where we now call
> `get_oid()`, too), and note how the loop body looks _eerily_ similar to
> the conditional `argc == 0` code block above?
>
> > +
> > +     for (i = 0; i < revs.nr; i++) {
> > +             hex = oid_to_hex(&revs.oid[i]);
> > +             if (bisect_write(state, hex, terms, 0)) {
> > +                     oid_array_clear(&revs);
> > +                     return BISECT_FAILED;
> > +             }
> > +             check_expected_revs(&hex, 1);
> > +     }
> > +
> > +     oid_array_clear(&revs);
> > +     return bisect_auto_next(terms, NULL);
> > +}
>
> So really, this function pretty much _wants_ to look this way (modulo
> bugs, as I did not even test-compile the code):
>
> static enum bisect_error bisect_state(struct bisect_terms *terms,
>                                       const char **argv, int argc)
> {
>         const char *state;
>         int i, verify_expected = 1;
>         struct object_id oid, expected;
>         struct strbuf buf = STRBUF_INIT;
>
>         if (!argc)
>                 return error(_("Please call `--bisect-state` with at least one argument"));
>
>         state = argv[0];
>         if (check_and_set_terms(terms, state) ||
>             !one_of(state, terms->term_good, terms->term_bad, "skip", NULL))
>                 return BISECT_FAILED;
>
>         argv++;
>         argc--;
>         if (argc > 1 && !strcmp(state, terms->term_bad))
>                 return error(_("'git bisect %s' can take only one argument."), terms->term_bad);
>
>         if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
>             get_oid_hex(buf.buf, &expected) < 0)
>                 verify_expected = 0; /* Ignore invalid file contents */
>
>
>         for (i = 0; i < argc + !argc; i++) {
>                 if (argc) {
>                         if (get_oid(argv[i], &oid)) {
>                                 error(_("Bad rev input: %s"), *argv);
>                                 return BISECT_FAILED;
>                         }
>                 } else {
>                         enum get_oid_result res = get_oid("BISECT_HEAD", &oid);
>
>                         if (res == MISSING_OBJECT)
>                                 res = get_oid("HEAD", &oid);
>                         if (res) {
>                                 error(_("Bad bisect_head rev input"));
>                                 return BISECT_FAILED;
>                         }
>                 }
>
>                 if (bisect_write(state, oid_to_hex(&oid), terms, 0))
>                         return BISECT_FAILED;
>
>                 if (verify_expected && !oideq(&oid, &expected)) {
>                         unlink_or_warn(git_path_bisect_ancestors_ok());
>                         unlink_or_warn(git_path_bisect_expected_rev());
>                         verify_expected = 0;
>                 }
>         }
>
>         return bisect_auto_next(terms, NULL);
> }
>
> There, not bad, is it?
>
 After implementing this solution some tests failed. After debugging
them, I found that with Pranit's solution, that arguments were parsed
into an OID array, if bisect received some junk rev the function
returned and bisect_write() was not executed.
With the new solution, if junk rev is received after a valid rev,
bisect_write() was executed for the valid and the function returned with
the junk rev.
So, there is garbage in the file and when for example bisect-porcelain
test number 5 - 'bisect fails if given any junk instead of revs'
executes 'test -z' fails.

Should I keep the original patch and add a comment in the code that
explains why we use an oid array?.

(I also have implemented an alternative solution that when some junk
rev is found, I delete all refs written, but maybe is too complicated
or not totally correct:
https://gitlab.com/mirucam/git/-/commit/93f669877b87d09a30a5d07f0967667b22026511
)


> > +
> >  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> >  {
> >       enum {
> > @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> >               BISECT_START,
> >               BISECT_NEXT,
> >               BISECT_AUTO_NEXT,
> > -             BISECT_AUTOSTART
> > +             BISECT_AUTOSTART,
> > +             BISECT_STATE
> >       } cmdmode = 0;
> >       int no_checkout = 0, res = 0, nolog = 0;
> >       struct option options[] = {
> > @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> >                        N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
> >               OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> >                        N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
> > +             OPT_CMDMODE(0, "bisect-state", &cmdmode,
> > +                      N_("mark the state of ref (or refs)"), BISECT_STATE),
> >               OPT_BOOL(0, "no-checkout", &no_checkout,
> >                        N_("update BISECT_HEAD instead of checking out the current commit")),
> >               OPT_BOOL(0, "no-log", &nolog,
> > @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> >               set_terms(&terms, "bad", "good");
> >               res = bisect_autostart(&terms);
> >               break;
> > +     case BISECT_STATE:
> > +             set_terms(&terms, "bad", "good");
> > +             get_terms(&terms);
> > +             res = bisect_state(&terms, argv, argc);
> > +             break;
> >       default:
> >               BUG("unknown subcommand %d", (int)cmdmode);
> >       }
> > diff --git a/git-bisect.sh b/git-bisect.sh
> > index 049ffacdff..2da0810b1a 100755
> > --- a/git-bisect.sh
> > +++ b/git-bisect.sh
> > @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
> >  TERM_BAD=bad
> >  TERM_GOOD=good
> >
> > -bisect_head()
> > -{
> > -     if test -f "$GIT_DIR/BISECT_HEAD"
> > -     then
> > -             echo BISECT_HEAD
> > -     else
> > -             echo HEAD
> > -     fi
> > -}
> > -
> >  bisect_skip() {
> >       all=''
> >       for arg in "$@"
> > @@ -61,43 +51,7 @@ bisect_skip() {
> >               esac
> >               all="$all $revs"
> >       done
> > -     eval bisect_state 'skip' $all
> > -}
> > -
> > -bisect_state() {
> > -     git bisect--helper --bisect-autostart
> > -     state=$1
> > -     git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> > -     get_terms
> > -     case "$#,$state" in
> > -     0,*)
> > -             die "Please call 'bisect_state' with at least one argument." ;;
> > -     1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
> > -             bisected_head=$(bisect_head)
> > -             rev=$(git rev-parse --verify "$bisected_head") ||
> > -                     die "$(eval_gettext "Bad rev input: \$bisected_head")"
> > -             git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> > -             git bisect--helper --check-expected-revs "$rev" ;;
> > -     2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
> > -             shift
> > -             hash_list=''
> > -             for rev in "$@"
> > -             do
> > -                     sha=$(git rev-parse --verify "$rev^{commit}") ||
> > -                             die "$(eval_gettext "Bad rev input: \$rev")"
> > -                     hash_list="$hash_list $sha"
> > -             done
> > -             for rev in $hash_list
> > -             do
> > -                     git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> > -             done
> > -             git bisect--helper --check-expected-revs $hash_list ;;
> > -     *,"$TERM_BAD")
> > -             die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
> > -     *)
> > -             usage ;;
> > -     esac
> > -     git bisect--helper --bisect-auto-next
> > +     eval git bisect--helper --bisect-state 'skip' $all
> >  }
> >
> >  bisect_visualize() {
> > @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> >                       state="$TERM_GOOD"
> >               fi
> >
> > -             # We have to use a subshell because "bisect_state" can exit.
> > -             ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> > +             git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
> >               res=$?
> >
> >               cat "$GIT_DIR/BISECT_RUN"
> > @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> >               if [ $res -ne 0 ]
> >               then
> >                       eval_gettextln "bisect run failed:
> > -'bisect_state \$state' exited with error code \$res" >&2
> > +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2
>
> This is not your fault, of course, but it does make me shudder to see such
> an obvious implementation detail in a user-facing error message.
>
> Maybe something to fix up in a follow-up?
>
> Ciao,
> Dscho
>
> >                       exit $res
> >               fi
> >
> > @@ -242,7 +195,7 @@ case "$#" in
> >       start)
> >               git bisect--helper --bisect-start "$@" ;;
> >       bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
> > -             bisect_state "$cmd" "$@" ;;
> > +             git bisect--helper --bisect-state "$cmd" "$@" ;;
> >       skip)
> >               bisect_skip "$@" ;;
> >       next)
> > --
> > 2.25.0
> >
> >
Best,
Miriam.

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

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 17614 bytes --]

Hi Miriam,

On Sat, 20 Jun 2020, Miriam R. wrote:

> Hi Johannes,
> I'm finishing the next patch series version and I have an issue about
> one of your suggestions:
>
> El sáb., 23 may. 2020 a las 0:06, Johannes Schindelin
> (<Johannes.Schindelin@gmx.de>) escribió:
> >
> > Hi Miriam,
> >
> > On Thu, 23 Apr 2020, Miriam Rubio wrote:
> >
> > > From: Pranit Bauva <pranit.bauva@gmail.com>
> > >
> > > Reimplement the `bisect_state()` shell functions in C and also add a
> > > subcommand `--bisect-state` to `git-bisect--helper` to call them from
> > > git-bisect.sh .
> > >
> > > Using `--bisect-state` subcommand is a temporary measure to port shell
> > > function to C so as to use the existing test suite. As more functions
> > > are ported, this subcommand will be retired and will be called by some
> > > other methods.
> > >
> > > `bisect_head()` is only called from `bisect_state()`, thus it is not
> > > required to introduce another subcommand.
> > >
> > > Mentored-by: Lars Schneider <larsxschneider@gmail.com>
> > > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
> > > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> > > Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> > > ---
> > >  builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++-
> > >  git-bisect.sh            | 55 +++----------------------------
> > >  2 files changed, 73 insertions(+), 52 deletions(-)
> > >
> > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > > index 2d8660c79f..9db72f5891 100644
> > > --- a/builtin/bisect--helper.c
> > > +++ b/builtin/bisect--helper.c
> > > @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = {
> > >       N_("git bisect--helper --bisect-next"),
> > >       N_("git bisect--helper --bisect-auto-next"),
> > >       N_("git bisect--helper --bisect-autostart"),
> > > +     N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
> > > +     N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
> > >       NULL
> > >  };
> > >
> > > @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms)
> > >       return bisect_start(terms, 0, NULL, 0);
> > >  }
> > >
> > > +static int bisect_head(struct object_id *oid)
> > > +{
> > > +     if (!file_exists(git_path_bisect_head()))
> > > +             return get_oid("HEAD", oid);
> > > +
> > > +     return get_oid("BISECT_HEAD", oid);
> >
> > This can be easily reduced to
> >
> >         return get_oid(file_exists(git_path_bisect_head()) ?
> >                        "BISECT_HEAD" : "HEAD", oid);
> >
> > At the same time, it is wrong, just like the shell script version was
> > wrong: in particular in light of the `hn/reftable` effort, we do _not_
> > want to assume that all refs are backed by files!
> >
> > So really, what this should do instead is this:
> >
> >         enum get_oid_result res = get_oid("BISECT_HEAD", oid);
> >
> >         if (res == MISSING_OBJECT)
> >                 res = get_oid("HEAD", oid);
> >
> > Given that this is still only three lines long, the overhead of having it
> > in its own function for just a _single_ call seems excessive. I'd prefer
> > it to be inlined in `bisect_state()`.
> >
> > > +}
> > > +
> > > +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
> > > +                     int argc)
> > > +{
> >
> > I offered a lengthy discussion about this function in
> > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/
> >
> > It does not look, however, as if v3 benefitted from the entirety of my
> > analysis: All the `check_expected_revs()` function does is to verify that
> > the passed list of revs matches exactly the contents of the
> > `BISECT_EXPECTED_REV` file.
> >
> > That can be done in a much simpler way, though, by first reading the file
> > and parsing the contents into an OID, and then comparing to that parsed
> > OID instead.
> >
> > Besides, `check_expected_revs()` is only used to check one rev at a time.
> >
> > In other words, it could be simplified to something like this:
> >
> > static void check_expected_rev(struct object_id *oid)
> > {
> >         struct object_id expected;
> >         struct strbuf buf = STRBUF_INIT;
> >
> >         if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0)
> >               < the_hash_algo->hexsz ||
> >             get_oid_hex(buf.buf, &expected) < 0)
> >                 return; /* Ignore invalid file contents */
> >
> >         if (!oideq(oid, &expected)) {
> >                 ... unlink ...
> >                 return;
> >         }
> > }
> >
> > But even that would be wasteful, as we would read the file over and over
> > and over again.
> >
> > The good news is that we do not even _need_ `check_expected_rev()`.
> > Because we do not need to have two call sites, we can simplify the code
> > much further. See below:
> >
> > > +     const char *state;
> > > +     const char *hex;
> > > +     int i;
> > > +     struct oid_array revs = OID_ARRAY_INIT;
> > > +     struct object_id oid;
> > > +
> > > +     if (!argc)
> > > +             return error(_("Please call `--bisect-state` with at least one argument"));
> > > +     state = argv[0];
> > > +     if (check_and_set_terms(terms, state) ||
> > > +         !one_of(state, terms->term_good,terms->term_bad, "skip", NULL))
> > > +             return BISECT_FAILED;
> > > +     argv++;
> > > +     argc--;
> > > +     if (!strcmp(state, terms->term_bad) && (argc > 1))
> > > +             return error(_("'git bisect %s' can take only one argument."),terms->term_bad);
> > > +     if (argc == 0) {
> > > +             if (bisect_head(&oid))
> > > +                     return error(_("Bad bisect_head rev input"));
> > > +             hex = oid_to_hex(&oid);
> > > +             if (bisect_write(state, hex, terms, 0))
> > > +                     return BISECT_FAILED;
> > > +             check_expected_revs(&hex, 1);
> > > +             return bisect_auto_next(terms, NULL);
> > > +     }
> > > +
> > > +     /* Here argc > 0 */
> > > +     for (; argc; argc--, argv++) {
> > > +             struct object_id oid;
> > > +             if (get_oid(*argv, &oid))
> > > +                     return error(_("Bad rev input: %s"), *argv);
> > > +             oid_array_append(&revs, &oid);
> > > +     }
> >
> > It really does not make sense to parse the arguments into an OID array,
> > _then_ iterate over the array once, and then immediately releasing it.
> > That OID array is not needed at all.
> >
> > So we'll end up with this loop in case `argc > 0` (where we now call
> > `get_oid()`, too), and note how the loop body looks _eerily_ similar to
> > the conditional `argc == 0` code block above?
> >
> > > +
> > > +     for (i = 0; i < revs.nr; i++) {
> > > +             hex = oid_to_hex(&revs.oid[i]);
> > > +             if (bisect_write(state, hex, terms, 0)) {
> > > +                     oid_array_clear(&revs);
> > > +                     return BISECT_FAILED;
> > > +             }
> > > +             check_expected_revs(&hex, 1);
> > > +     }
> > > +
> > > +     oid_array_clear(&revs);
> > > +     return bisect_auto_next(terms, NULL);
> > > +}
> >
> > So really, this function pretty much _wants_ to look this way (modulo
> > bugs, as I did not even test-compile the code):
> >
> > static enum bisect_error bisect_state(struct bisect_terms *terms,
> >                                       const char **argv, int argc)
> > {
> >         const char *state;
> >         int i, verify_expected = 1;
> >         struct object_id oid, expected;
> >         struct strbuf buf = STRBUF_INIT;
> >
> >         if (!argc)
> >                 return error(_("Please call `--bisect-state` with at least one argument"));
> >
> >         state = argv[0];
> >         if (check_and_set_terms(terms, state) ||
> >             !one_of(state, terms->term_good, terms->term_bad, "skip", NULL))
> >                 return BISECT_FAILED;
> >
> >         argv++;
> >         argc--;
> >         if (argc > 1 && !strcmp(state, terms->term_bad))
> >                 return error(_("'git bisect %s' can take only one argument."), terms->term_bad);
> >
> >         if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz ||
> >             get_oid_hex(buf.buf, &expected) < 0)
> >                 verify_expected = 0; /* Ignore invalid file contents */
> >
> >
> >         for (i = 0; i < argc + !argc; i++) {
> >                 if (argc) {
> >                         if (get_oid(argv[i], &oid)) {
> >                                 error(_("Bad rev input: %s"), *argv);
> >                                 return BISECT_FAILED;
> >                         }
> >                 } else {
> >                         enum get_oid_result res = get_oid("BISECT_HEAD", &oid);
> >
> >                         if (res == MISSING_OBJECT)
> >                                 res = get_oid("HEAD", &oid);
> >                         if (res) {
> >                                 error(_("Bad bisect_head rev input"));
> >                                 return BISECT_FAILED;
> >                         }
> >                 }
> >
> >                 if (bisect_write(state, oid_to_hex(&oid), terms, 0))
> >                         return BISECT_FAILED;
> >
> >                 if (verify_expected && !oideq(&oid, &expected)) {
> >                         unlink_or_warn(git_path_bisect_ancestors_ok());
> >                         unlink_or_warn(git_path_bisect_expected_rev());
> >                         verify_expected = 0;
> >                 }
> >         }
> >
> >         return bisect_auto_next(terms, NULL);
> > }
> >
> > There, not bad, is it?
> >
>  After implementing this solution some tests failed. After debugging
> them, I found that with Pranit's solution, that arguments were parsed
> into an OID array, if bisect received some junk rev the function
> returned and bisect_write() was not executed.

That's a good point, and should be turned into a code comment lest
overzealous developers would attempt the simplification I proposed in the
future.

> With the new solution, if junk rev is received after a valid rev,
> bisect_write() was executed for the valid and the function returned with
> the junk rev.
> So, there is garbage in the file and when for example bisect-porcelain
> test number 5 - 'bisect fails if given any junk instead of revs'
> executes 'test -z' fails.
>
> Should I keep the original patch and add a comment in the code that
> explains why we use an oid array?.

I still think that the `check_expected_revs()` function is not needed, and
unnecessarily complex.

If you _want_ to keep Pranit's original implementation, then I would _at
least_ want you to forward-port the `verify_expected` changes I suggested.

>
> (I also have implemented an alternative solution that when some junk
> rev is found, I delete all refs written, but maybe is too complicated
> or not totally correct:
> https://gitlab.com/mirucam/git/-/commit/93f669877b87d09a30a5d07f0967667b22026511
> )

You seem to share my preference for not writing out bogus revs in the
first place.

Thanks,
Johannes

>
>
> > > +
> > >  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > >  {
> > >       enum {
> > > @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > >               BISECT_START,
> > >               BISECT_NEXT,
> > >               BISECT_AUTO_NEXT,
> > > -             BISECT_AUTOSTART
> > > +             BISECT_AUTOSTART,
> > > +             BISECT_STATE
> > >       } cmdmode = 0;
> > >       int no_checkout = 0, res = 0, nolog = 0;
> > >       struct option options[] = {
> > > @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > >                        N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
> > >               OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
> > >                        N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
> > > +             OPT_CMDMODE(0, "bisect-state", &cmdmode,
> > > +                      N_("mark the state of ref (or refs)"), BISECT_STATE),
> > >               OPT_BOOL(0, "no-checkout", &no_checkout,
> > >                        N_("update BISECT_HEAD instead of checking out the current commit")),
> > >               OPT_BOOL(0, "no-log", &nolog,
> > > @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> > >               set_terms(&terms, "bad", "good");
> > >               res = bisect_autostart(&terms);
> > >               break;
> > > +     case BISECT_STATE:
> > > +             set_terms(&terms, "bad", "good");
> > > +             get_terms(&terms);
> > > +             res = bisect_state(&terms, argv, argc);
> > > +             break;
> > >       default:
> > >               BUG("unknown subcommand %d", (int)cmdmode);
> > >       }
> > > diff --git a/git-bisect.sh b/git-bisect.sh
> > > index 049ffacdff..2da0810b1a 100755
> > > --- a/git-bisect.sh
> > > +++ b/git-bisect.sh
> > > @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
> > >  TERM_BAD=bad
> > >  TERM_GOOD=good
> > >
> > > -bisect_head()
> > > -{
> > > -     if test -f "$GIT_DIR/BISECT_HEAD"
> > > -     then
> > > -             echo BISECT_HEAD
> > > -     else
> > > -             echo HEAD
> > > -     fi
> > > -}
> > > -
> > >  bisect_skip() {
> > >       all=''
> > >       for arg in "$@"
> > > @@ -61,43 +51,7 @@ bisect_skip() {
> > >               esac
> > >               all="$all $revs"
> > >       done
> > > -     eval bisect_state 'skip' $all
> > > -}
> > > -
> > > -bisect_state() {
> > > -     git bisect--helper --bisect-autostart
> > > -     state=$1
> > > -     git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
> > > -     get_terms
> > > -     case "$#,$state" in
> > > -     0,*)
> > > -             die "Please call 'bisect_state' with at least one argument." ;;
> > > -     1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip)
> > > -             bisected_head=$(bisect_head)
> > > -             rev=$(git rev-parse --verify "$bisected_head") ||
> > > -                     die "$(eval_gettext "Bad rev input: \$bisected_head")"
> > > -             git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> > > -             git bisect--helper --check-expected-revs "$rev" ;;
> > > -     2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip)
> > > -             shift
> > > -             hash_list=''
> > > -             for rev in "$@"
> > > -             do
> > > -                     sha=$(git rev-parse --verify "$rev^{commit}") ||
> > > -                             die "$(eval_gettext "Bad rev input: \$rev")"
> > > -                     hash_list="$hash_list $sha"
> > > -             done
> > > -             for rev in $hash_list
> > > -             do
> > > -                     git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit
> > > -             done
> > > -             git bisect--helper --check-expected-revs $hash_list ;;
> > > -     *,"$TERM_BAD")
> > > -             die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;;
> > > -     *)
> > > -             usage ;;
> > > -     esac
> > > -     git bisect--helper --bisect-auto-next
> > > +     eval git bisect--helper --bisect-state 'skip' $all
> > >  }
> > >
> > >  bisect_visualize() {
> > > @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> > >                       state="$TERM_GOOD"
> > >               fi
> > >
> > > -             # We have to use a subshell because "bisect_state" can exit.
> > > -             ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> > > +             git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
> > >               res=$?
> > >
> > >               cat "$GIT_DIR/BISECT_RUN"
> > > @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> > >               if [ $res -ne 0 ]
> > >               then
> > >                       eval_gettextln "bisect run failed:
> > > -'bisect_state \$state' exited with error code \$res" >&2
> > > +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2
> >
> > This is not your fault, of course, but it does make me shudder to see such
> > an obvious implementation detail in a user-facing error message.
> >
> > Maybe something to fix up in a follow-up?
> >
> > Ciao,
> > Dscho
> >
> > >                       exit $res
> > >               fi
> > >
> > > @@ -242,7 +195,7 @@ case "$#" in
> > >       start)
> > >               git bisect--helper --bisect-start "$@" ;;
> > >       bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
> > > -             bisect_state "$cmd" "$@" ;;
> > > +             git bisect--helper --bisect-state "$cmd" "$@" ;;
> > >       skip)
> > >               bisect_skip "$@" ;;
> > >       next)
> > > --
> > > 2.25.0
> > >
> > >
> Best,
> Miriam.
>

[PATCH v3 10/12] 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 9db72f5891..7db58ada6f 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -898,7 +898,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,
@@ -914,8 +913,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,
@@ -956,9 +953,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 v3 11/12] 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 7db58ada6f..e7467dc1c7 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>"),
@@ -897,8 +896,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,
@@ -911,8 +909,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,
@@ -949,10 +945,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 v3 12/12] 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 e7467dc1c7..66f9b8eab6 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
@@ -904,7 +903,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;
@@ -925,8 +923,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,
@@ -988,12 +984,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:
 		set_terms(&terms, "bad", "good");
 		get_terms(&terms);
-- 
2.25.0

Re: [PATCH v3 00/12] Finish converting git bisect to C part 2

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> --- Changes since v2 Finish converting git bisect to C part 2 patch series ---
>
> General changes
> ---------------
>
> * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)

Was there a particular reason why you needed to do this rebase?  It
seems that the patches apply cleanly on the same base as v2 has been
queued?  Does the result cause hard-to-resolve conflicts when merged
to the 'master', 'next' or 'pu' branches unless you do that rebase?

> Specific changes
> ----------------
>
> [1/12] bisect--helper: fix `cmd_*()` function switch default return
>
> * Use `BUG()` instead of `return error()` in default switch.

This rings a bell.  range-diff looks good.

> [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
>
> * New patch: use '-res' instead of 'abs(res)'.

Alright.

> [3/12] bisect--helper: introduce new `write_in_file()` function
>
> * Rename input parameter `filepath` to `path`.
> * Change `error_errno()` to `error()` in mode checking.
> * Change error message when file cannot be opened.
> * Add `fclose()` before error return.

OK.  I am not sure if the error behaviour when fclose() fails is
ideal, but I do not think it is worth further polishing.

> [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C
>
> * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`
> shell functions in C` to use `bisect_autostart()` function in 
> `bisect_append_log_quoted()`.

OK.

Will take a look at individual patches later.

Thanks.

Re: [PATCH v3 00/12] Finish converting git bisect to C part 2

[Miriam R.]

Hi Junio,

El jue., 23 abr. 2020 a las 22:01, Junio C Hamano
(<gitster@pobox.com>) escribió:
>
> Miriam Rubio <mirucam@gmail.com> writes:
>
> > --- Changes since v2 Finish converting git bisect to C part 2 patch series ---
> >
> > General changes
> > ---------------
> >
> > * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)
>
> Was there a particular reason why you needed to do this rebase?  It
> seems that the patches apply cleanly on the same base as v2 has been
> queued?  Does the result cause hard-to-resolve conflicts when merged
> to the 'master', 'next' or 'pu' branches unless you do that rebase?
>
No particular reason. I use to do a rebase on master before applying
reviewers suggestions if I see there are new commits on master.

> > Specific changes
> > ----------------
> >
> > [1/12] bisect--helper: fix `cmd_*()` function switch default return
> >
> > * Use `BUG()` instead of `return error()` in default switch.
>
> This rings a bell.  range-diff looks good.
>
> > [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
> >
> > * New patch: use '-res' instead of 'abs(res)'.
>
> Alright.
>
> > [3/12] bisect--helper: introduce new `write_in_file()` function
> >
> > * Rename input parameter `filepath` to `path`.
> > * Change `error_errno()` to `error()` in mode checking.
> > * Change error message when file cannot be opened.
> > * Add `fclose()` before error return.
>
> OK.  I am not sure if the error behaviour when fclose() fails is
> ideal, but I do not think it is worth further polishing.
>
> > [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C
> >
> > * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`
> > shell functions in C` to use `bisect_autostart()` function in
> > `bisect_append_log_quoted()`.
>
> OK.
>
> Will take a look at individual patches later.
Great, thank you!
>
> Thanks.

Re: [PATCH v3 00/12] Finish converting git bisect to C part 2

[Johannes Schindelin]

Hi Miriam,

On Thu, 23 Apr 2020, Miriam Rubio wrote:

> 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.
>
> These patch series emails were generated from:
> https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v3.
>
> I would like to thank Junio Hamano for reviewing this patch series and
> Christian Couder for his help.
>
> --- Changes since v2 Finish converting git bisect to C part 2 patch series ---

Thank you for this detailed summary!

I reviewed the patches in their entirety, and left a couple of
suggestions, some of them minor.

Hopefully you find them helpful!

Ciao,
Dscho

>
> General changes
> ---------------
>
> * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)
>
> Specific changes
> ----------------
>
> [1/12] bisect--helper: fix `cmd_*()` function switch default return
>
> * Use `BUG()` instead of `return error()` in default switch.
>
> --
>
> [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
>
> * New patch: use '-res' instead of 'abs(res)'.
>
> --
>
> [3/12] bisect--helper: introduce new `write_in_file()` function
>
> * Rename input parameter `filepath` to `path`.
> * Change `error_errno()` to `error()` in mode checking.
> * Change error message when file cannot be opened.
> * Add `fclose()` before error return.
>
> --
>
> [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C
>
> * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`
> shell functions in C` to use `bisect_autostart()` function in
> `bisect_append_log_quoted()`.
>
> --
> `
> [5/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
> functions in C
>
> * Amend commit message.
> * Add `clear_commit_marks()` at the end of bisect_next_all() and remove it from
> `bisect_rev_setup()`.
> * Fix if condition.
> * Add `bisect_autostart()` in `bisect_append_log_quoted()`.
> * Check `git_path_bisect_head()` with `file_exist()` instead of
> `is_empty_or_missing_file()`.
> * Fix return with BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
> * Add conversion in `cmd_*()` to BISECT_OK when return is
> BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
>
> * Note to previous reviewer: suggestion to change `bisect_auto_next()` function
> to return void cannot be done because errors from `bisect_next()` are not
> ignored.
>
> --
>
> [5/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell
> functions in C
>
> * Check `git_path_bisect_head()` with `file_exist()` instead of
> `is_empty_or_missing_file()`.
>
> --
>
> Miriam Rubio (3):
>   bisect--helper: fix `cmd_*()` function switch default return
>   bisect--helper: use '-res' in 'cmd_bisect__helper' return
>   bisect--helper: introduce new `write_in_file()` function
>
> Pranit Bauva (9):
>   bisect--helper: reimplement `bisect_autostart` shell function in C
>   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_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                 |   8 +
>  builtin/bisect--helper.c | 383 +++++++++++++++++++++++++++++++++------
>  git-bisect.sh            | 145 +--------------
>  3 files changed, 347 insertions(+), 189 deletions(-)
>
> --
> 2.25.0
>
>

Re: [PATCH v3 00/12] Finish converting git bisect to C part 2

[Miriam R.]

Hi Johannes,

El sáb., 23 may. 2020 a las 0:09, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Thu, 23 Apr 2020, Miriam Rubio wrote:
>
> > 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.
> >
> > These patch series emails were generated from:
> > https://gitlab.com/mirucam/git/commits/git-bisect-work-part2-v3.
> >
> > I would like to thank Junio Hamano for reviewing this patch series and
> > Christian Couder for his help.
> >
> > --- Changes since v2 Finish converting git bisect to C part 2 patch series ---
>
> Thank you for this detailed summary!
>
> I reviewed the patches in their entirety, and left a couple of
> suggestions, some of them minor.
>
Thank you very much for reviewing.

> Hopefully you find them helpful!
Of course! :)

I hope I can send the next version soon.

Best,
Miriam
>
> Ciao,
> Dscho
>
> >
> > General changes
> > ---------------
> >
> > * Rebase on master branch: efe3874640 (Sync with v2.26.1, 2020-04-13)
> >
> > Specific changes
> > ----------------
> >
> > [1/12] bisect--helper: fix `cmd_*()` function switch default return
> >
> > * Use `BUG()` instead of `return error()` in default switch.
> >
> > --
> >
> > [2/12] bisect--helper: use '-res' in 'cmd_bisect__helper' return
> >
> > * New patch: use '-res' instead of 'abs(res)'.
> >
> > --
> >
> > [3/12] bisect--helper: introduce new `write_in_file()` function
> >
> > * Rename input parameter `filepath` to `path`.
> > * Change `error_errno()` to `error()` in mode checking.
> > * Change error message when file cannot be opened.
> > * Add `fclose()` before error return.
> >
> > --
> >
> > [4/12] bisect--helper: reimplement `bisect_autostart` shell function in C
> >
> > * Reorder patch before `reimplement `bisect_next` and `bisect_auto_next`
> > shell functions in C` to use `bisect_autostart()` function in
> > `bisect_append_log_quoted()`.
> >
> > --
> > `
> > [5/12] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
> > functions in C
> >
> > * Amend commit message.
> > * Add `clear_commit_marks()` at the end of bisect_next_all() and remove it from
> > `bisect_rev_setup()`.
> > * Fix if condition.
> > * Add `bisect_autostart()` in `bisect_append_log_quoted()`.
> > * Check `git_path_bisect_head()` with `file_exist()` instead of
> > `is_empty_or_missing_file()`.
> > * Fix return with BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
> > * Add conversion in `cmd_*()` to BISECT_OK when return is
> > BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND.
> >
> > * Note to previous reviewer: suggestion to change `bisect_auto_next()` function
> > to return void cannot be done because errors from `bisect_next()` are not
> > ignored.
> >
> > --
> >
> > [5/12] bisect--helper: reimplement `bisect_state` & `bisect_head` shell
> > functions in C
> >
> > * Check `git_path_bisect_head()` with `file_exist()` instead of
> > `is_empty_or_missing_file()`.
> >
> > --
> >
> > Miriam Rubio (3):
> >   bisect--helper: fix `cmd_*()` function switch default return
> >   bisect--helper: use '-res' in 'cmd_bisect__helper' return
> >   bisect--helper: introduce new `write_in_file()` function
> >
> > Pranit Bauva (9):
> >   bisect--helper: reimplement `bisect_autostart` shell function in C
> >   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_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                 |   8 +
> >  builtin/bisect--helper.c | 383 +++++++++++++++++++++++++++++++++------
> >  git-bisect.sh            | 145 +--------------
> >  3 files changed, 347 insertions(+), 189 deletions(-)
> >
> > --
> > 2.25.0
> >
> >