git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH v7 00/13] Finish converting git bisect to C part 2
@ 2020-08-31 10:50 Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 01/13] bisect--helper: BUG() in cmd_*() on invalid subcommand Miriam Rubio
                   ` (13 more replies)
  0 siblings, 14 replies; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git; +Cc: 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-v7.

I would like to thank Junio Hamano for reviewing this patch series.

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

* Rebased on e9b77c84a0 (Tenth batch, 2020-08-24), to build on the
  strvec API (instead of argv_array) and adjust to the codebase
  after the "--first-parent" feature was added.


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

[1/13] bisect--helper: BUG() in cmd_*() on invalid subcommand

* Amend commit message.
* Remove casting to int.

---

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

* Amend commit message.

---

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

* Use saved_errno variable.

---

[5/13] bisect--helper: reimplement `bisect_autostart` shell function in C

* Fix bug using empty_strvec instead of NULL in a `bisect_start()` call.

---


[6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
 functions in C
 
* Remove unused `no-checkout` variable.
* Move a comment to more appropriate place.
 
---

.iriam Rubio (4):
  bisect--helper: BUG() in cmd_*() on invalid subcommand
  bisect--helper: use '-res' in 'cmd_bisect__helper' return
  bisect--helper: introduce new `write_in_file()` function
  bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'

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                 |  13 +-
 builtin/bisect--helper.c | 442 ++++++++++++++++++++++++++++++++-------
 git-bisect.sh            | 145 +------------
 3 files changed, 380 insertions(+), 220 deletions(-)

-- 
2.25.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 01/13] bisect--helper: BUG() in cmd_*() on invalid subcommand
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 02/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return Miriam Rubio
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git; +Cc: Miriam Rubio, Christian Couder, Johannes Schindelin

In cmd_bisect__helper() function, if an invalid or no
subcommand is passed there is a BUG.

BUG() out instead of returning an error.

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 cdda279b23..f464e95792 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -720,7 +720,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		res = bisect_start(&terms, argv, argc);
 		break;
 	default:
-		return error("BUG: unknown subcommand '%d'", cmdmode);
+		BUG("unknown subcommand %d", cmdmode);
 	}
 	free_terms(&terms);
 
-- 
2.25.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 02/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 01/13] bisect--helper: BUG() in cmd_*() on invalid subcommand Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 03/13] bisect--helper: introduce new `write_in_file()` function Miriam Rubio
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git; +Cc: Miriam Rubio, Christian Couder, Junio C Hamano

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>
Helped-by: Junio C Hamano <gitster@pobox.com>
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 f464e95792..b7345be3a5 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -731,5 +731,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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 03/13] bisect--helper: introduce new `write_in_file()` function
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 01/13] bisect--helper: BUG() in cmd_*() on invalid subcommand Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 02/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git; +Cc: Miriam Rubio, Christian Couder, Johannes Schindelin

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

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 | 43 +++++++++++++++++++++++++++++++++-------
 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index b7345be3a5..bae09ce65d 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -74,6 +74,40 @@ static int one_of(const char *term, ...)
 	return res;
 }
 
+static int write_in_file(const char *path, const char *mode, const char *format, va_list args)
+{
+	FILE *fp = NULL;
+	int res = 0;
+
+	if (strcmp(mode, "w"))
+		BUG("write-in-file does not support '%s' mode", mode);
+	fp = fopen(path, mode);
+	if (!fp)
+		return error_errno(_("cannot open file '%s' in mode '%s'"), path, mode);
+	res = vfprintf(fp, format, args);
+
+	if (res < 0) {
+		int saved_errno = errno;
+		fclose(fp);
+		errno = saved_errno;
+		return error_errno(_("could not write to file '%s'"), path);
+	}
+
+	return fclose(fp);
+}
+
+static int write_to_file(const char *path, const char *format, ...)
+{
+	int res;
+	va_list args;
+
+	va_start(args, format);
+	res = write_in_file(path, "w", format, args);
+	va_end(args);
+
+	return res;
+}
+
 static int check_term_format(const char *term, const char *orig_term)
 {
 	int res;
@@ -104,7 +138,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,13 +146,9 @@ static int write_terms(const char *bad, const char *good)
 	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
 		return -1;
 
-	fp = fopen(git_path_bisect_terms(), "w");
-	if (!fp)
-		return error_errno(_("could not open the file BISECT_TERMS"));
+	res = write_to_file(git_path_bisect_terms(), "%s\n%s\n", bad, good);
 
-	res = fprintf(fp, "%s\n%s\n", bad, good);
-	res |= fclose(fp);
-	return (res < 0) ? -1 : 0;
+	return res;
 }
 
 static int is_expected_rev(const char *expected_hex)
-- 
2.25.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (2 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 03/13] bisect--helper: introduce new `write_in_file()` function Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-09-03  5:50   ` Johannes Schindelin
  2020-08-31 10:50 ` [PATCH v7 05/13] bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()' Miriam Rubio
                   ` (9 subsequent siblings)
  13 siblings, 1 reply; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git
  Cc: Pranit Bauva, Lars Schneider, Christian Couder,
	Johannes Schindelin, Tanushree Tumane, 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.

Add `--bisect-autostart` subcommand to be called from git-bisect.sh.
Using `--bisect-autostart` subcommand is a temporary measure to port
the 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 | 44 +++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 25 ++---------------------
 2 files changed, 45 insertions(+), 24 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index bae09ce65d..f71e8e54d2 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] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
+	N_("git bisect--helper --bisect-autostart"),
 	NULL
 };
 
@@ -653,6 +654,38 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
 	return res;
 }
 
+static inline int file_is_not_empty(const char *path)
+{
+	return !is_empty_or_missing_file(path);
+}
+
+static int bisect_autostart(struct bisect_terms *terms)
+{
+	int res;
+	const char *yesno;
+
+	if (file_is_not_empty(git_path_bisect_start()))
+		return 0;
+
+	fprintf_ln(stderr, _("You need to start by \"git bisect "
+			  "start\"\n"));
+
+	if (!isatty(STDIN_FILENO))
+		return 0;
+
+	/*
+	 * 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);
+	res = tolower(*yesno) == 'n' ?
+		-1 : bisect_start(terms, empty_strvec, 0);
+
+	return res;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -665,7 +698,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 res = 0, nolog = 0;
 	struct option options[] = {
@@ -689,6 +723,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 it has not yet been started"), BISECT_AUTOSTART),
 		OPT_BOOL(0, "no-log", &nolog,
 			 N_("no log for BISECT_WRITE")),
 		OPT_END()
@@ -748,6 +784,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		set_terms(&terms, "bad", "good");
 		res = bisect_start(&terms, argv, argc);
 		break;
+	case BISECT_AUTOSTART:
+		if (argc)
+			return error(_("--bisect-autostart does not accept arguments"));
+		set_terms(&terms, "bad", "good");
+		res = bisect_autostart(&terms);
+		break;
 	default:
 		BUG("unknown subcommand %d", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index c7580e51a0..9ca583d964 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 05/13] bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (3 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C Miriam Rubio
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git; +Cc: Miriam Rubio, Christian Couder

As there can be other revision walks after bisect_next_all(),
let's add a call to a function to clear all the marks at the
end of bisect_next_all().

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 bisect.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/bisect.c b/bisect.c
index d42a3a3767..c6aba2b9f2 100644
--- a/bisect.c
+++ b/bisect.c
@@ -1082,6 +1082,8 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
 		  "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);
 }
-- 
2.25.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (4 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 05/13] bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()' Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-09-03  9:48   ` Johannes Schindelin
  2020-08-31 10:50 ` [PATCH v7 07/13] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
                   ` (7 subsequent siblings)
  13 siblings, 1 reply; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git
  Cc: Pranit Bauva, Lars Schneider, Christian Couder,
	Johannes Schindelin, Tanushree Tumane, 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                 |  11 ++-
 builtin/bisect--helper.c | 180 ++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            |  47 +---------
 3 files changed, 190 insertions(+), 48 deletions(-)

diff --git a/bisect.c b/bisect.c
index c6aba2b9f2..f5b1368128 100644
--- a/bisect.c
+++ b/bisect.c
@@ -988,8 +988,11 @@ 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.
- * If no_checkout is non-zero, the bisection process does not
- * checkout the trial commit but instead simply updates BISECT_HEAD.
+ *
+ * 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.
  */
 enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
 {
@@ -999,6 +1002,10 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
 	enum bisect_error res = BISECT_OK;
 	struct object_id *bisect_rev;
 	char *steps_msg;
+	/*
+	 * If no_checkout is non-zero, the bisection process does not
+	 * checkout the trial commit but instead simply updates BISECT_HEAD.
+	 */
 	int no_checkout = ref_exists("BISECT_HEAD");
 	unsigned bisect_flags = 0;
 
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index f71e8e54d2..e29e86142a 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,10 +30,17 @@ 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] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
+	N_("git bisect--helper --bisect-next"),
+	N_("git bisect--helper --bisect-auto-next"),
 	N_("git bisect--helper --bisect-autostart"),
 	NULL
 };
 
+struct add_bisect_ref_data {
+	struct rev_info *revs;
+	unsigned int object_flags;
+};
+
 struct bisect_terms {
 	char *term_good;
 	char *term_bad;
@@ -56,6 +64,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.
@@ -80,7 +90,7 @@ static int write_in_file(const char *path, const char *mode, const char *format,
 	FILE *fp = NULL;
 	int res = 0;
 
-	if (strcmp(mode, "w"))
+	if (strcmp(mode, "w") && strcmp(mode, "a"))
 		BUG("write-in-file does not support '%s' mode", mode);
 	fp = fopen(path, mode);
 	if (!fp)
@@ -109,6 +119,18 @@ static int write_to_file(const char *path, const char *format, ...)
 	return res;
 }
 
+static int append_to_file(const char *path, const char *format, ...)
+{
+	int res;
+	va_list args;
+
+	va_start(args, format);
+	res = write_in_file(path, "a", format, args);
+	va_end(args);
+
+	return res;
+}
+
 static int check_term_format(const char *term, const char *orig_term)
 {
 	int res;
@@ -451,6 +473,140 @@ static int bisect_append_log_quoted(const char **argv)
 	return res;
 }
 
+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;
+}
+
+static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
+{
+	int res = 0;
+	struct add_bisect_ref_data cb = { revs };
+	char *good = xstrfmt("%s-*", terms->term_good);
+
+	/*
+	 * We cannot use terms->term_bad directly in
+	 * for_each_glob_ref_in() and we have to append a '*' to it,
+	 * otherwise for_each_glob_ref_in() will append '/' and '*'.
+	 */
+	char *bad = xstrfmt("%s*", terms->term_bad);
+
+	/*
+	 * 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);
+	setup_revisions(0, NULL, revs, NULL);
+	for_each_glob_ref_in(add_bisect_ref, bad, "refs/bisect/", &cb);
+	cb.object_flags = UNINTERESTING;
+	for_each_glob_ref_in(add_bisect_ref, good, "refs/bisect/", &cb);
+	if (prepare_revision_walk(revs))
+		res = error(_("revision walk setup failed\n"));
+
+	free(good);
+	free(bad);
+	return res;
+}
+
+static int bisect_skipped_commits(struct bisect_terms *terms)
+{
+	int res;
+	FILE *fp = NULL;
+	struct rev_info revs;
+	struct commit *commit;
+	struct pretty_print_context pp = {0};
+	struct strbuf commit_name = STRBUF_INIT;
+
+	res = prepare_revs(terms, &revs);
+	if (res)
+		return res;
+
+	fp = fopen(git_path_bisect_log(), "a");
+	if (!fp)
+		return error_errno(_("could not open '%s' for appending"),
+				  git_path_bisect_log());
+
+	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) {
+		strbuf_reset(&commit_name);
+		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);
+	}
+
+	/*
+	 * Reset the flags used by revision walks in case
+	 * there is another revision walk after this one.
+	 */
+	reset_revision_walk();
+
+	strbuf_release(&commit_name);
+	fclose(fp);
+	return 0;
+}
+
+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);
+	commit = lookup_commit_reference_by_name(bad_ref);
+	format_commit_message(commit, "%s", &commit_name, &pp);
+
+	res = append_to_file(git_path_bisect_log(), "# first %s commit: [%s] %s\n",
+			    terms->term_bad, oid_to_hex(&commit->object.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)
+{
+	enum bisect_error res;
+
+	bisect_autostart(terms);
+	if (bisect_next_check(terms, terms->term_good))
+		return BISECT_FAILED;
+
+	/* Perform all bisection computation */
+	res = bisect_next_all(the_repository, prefix);
+
+	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, const char **argv, int argc)
 {
 	int no_checkout = 0;
@@ -699,7 +855,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 res = 0, nolog = 0;
 	struct option options[] = {
@@ -723,6 +881,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 it has not yet been started"), BISECT_AUTOSTART),
 		OPT_BOOL(0, "no-log", &nolog,
@@ -784,6 +946,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		set_terms(&terms, "bad", "good");
 		res = bisect_start(&terms, 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 does not accept arguments"));
@@ -799,7 +973,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 9ca583d964..59424f5b37 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
-	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() {
@@ -213,7 +174,7 @@ bisect_replay () {
 		esac
 	done <"$file"
 	IFS="$oIFS"
-	bisect_auto_next
+	git bisect--helper --bisect-auto-next
 }
 
 bisect_run () {
@@ -310,7 +271,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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 07/13] bisect--helper: finish porting `bisect_start()` to C
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (5 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-09-03  9:56   ` Johannes Schindelin
  2020-08-31 10:50 ` [PATCH v7 08/13] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
                   ` (6 subsequent siblings)
  13 siblings, 1 reply; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git
  Cc: Pranit Bauva, Christian Couder, Johannes Schindelin,
	Tanushree Tumane, 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()`.

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 | 56 ++++++++++++++++++++++++++++------------
 git-bisect.sh            | 26 ++-----------------
 2 files changed, 41 insertions(+), 41 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index e29e86142a..b40369e8aa 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -85,6 +85,19 @@ static int one_of(const char *term, ...)
 	return res;
 }
 
+/*
+ * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE
+ * and BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND are codes
+ * that indicate special success.
+ */
+
+static int is_bisect_success(enum bisect_error res)
+{
+	return !res ||
+		res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND ||
+		res == BISECT_INTERNAL_SUCCESS_MERGE_BASE;
+}
+
 static int write_in_file(const char *path, const char *mode, const char *format, va_list args)
 {
 	FILE *fp = NULL;
@@ -607,12 +620,13 @@ 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, const char **argv, int argc)
+static enum bisect_error bisect_start(struct bisect_terms *terms, const char **argv, int argc)
 {
 	int no_checkout = 0;
 	int first_parent_only = 0;
 	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;
@@ -672,9 +686,12 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
 			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);
@@ -752,14 +769,7 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
 	 * Get rid of any old bisect state.
 	 */
 	if (bisect_clean_state())
-		return -1;
-
-	/*
-	 * 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".
-	 */
+		return BISECT_FAILED;
 
 	/*
 	 * Write new start state
@@ -776,7 +786,7 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
 		}
 		if (update_ref(NULL, "BISECT_HEAD", &oid, NULL, 0,
 			       UPDATE_REFS_MSG_ON_ERR)) {
-			res = -1;
+			res = BISECT_FAILED;
 			goto finish;
 		}
 	}
@@ -788,25 +798,37 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
 	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,
+	 * "bisect_auto_next" above may exit or misbehave.
+	 * We have to handle this to be able to clean up using
+	 * "bisect_clean_state".
+	 */
+	if (!is_bisect_success(res))
+		bisect_clean_state();
 	return res;
 }
 
diff --git a/git-bisect.sh b/git-bisect.sh
index 59424f5b37..356264caf0 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 "$@"
@@ -163,8 +142,7 @@ bisect_replay () {
 		get_terms
 		case "$command" in
 		start)
-			cmd="bisect_start $rev $tail"
-			eval "$cmd" ;;
+			eval "git bisect--helper --bisect-start $rev $tail" ;;
 		"$TERM_GOOD"|"$TERM_BAD"|skip)
 			git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
 		terms)
@@ -264,7 +242,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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 08/13] bisect--helper: retire `--bisect-clean-state` subcommand
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (6 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 07/13] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 09/13] bisect--helper: retire `--next-all` subcommand Miriam Rubio
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git
  Cc: Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane,
	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 b40369e8aa..b8b8a7473c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all"),
 	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>"),
@@ -869,7 +868,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,
@@ -887,8 +885,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,
@@ -930,10 +926,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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 09/13] bisect--helper: retire `--next-all` subcommand
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (7 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 08/13] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 10/13] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C Miriam Rubio
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git
  Cc: Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane,
	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 b8b8a7473c..0796e51672 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 
 static const char * const git_bisect_helper_usage[] = {
-	N_("git bisect--helper --next-all"),
 	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>"),
@@ -866,8 +865,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,
@@ -881,8 +879,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	} cmdmode = 0;
 	int 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,
@@ -919,9 +915,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);
-		break;
 	case WRITE_TERMS:
 		if (argc != 2)
 			return error(_("--write-terms requires two arguments"));
-- 
2.25.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 10/13] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (8 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 09/13] bisect--helper: retire `--next-all` subcommand Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-09-03 12:03   ` Johannes Schindelin
  2020-08-31 10:50 ` [PATCH v7 11/13] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
                   ` (3 subsequent siblings)
  13 siblings, 1 reply; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git
  Cc: Pranit Bauva, Lars Schneider, Christian Couder,
	Johannes Schindelin, Tanushree Tumane, 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 | 78 +++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 55 +++-------------------------
 2 files changed, 81 insertions(+), 52 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 0796e51672..a976f4739c 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
 };
 
@@ -862,6 +864,72 @@ static int bisect_autostart(struct bisect_terms *terms)
 	return res;
 }
 
+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;
+	struct oid_array revs = OID_ARRAY_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 (argc == 0) {
+		enum get_oid_result res_head = get_oid("BISECT_HEAD", &oid);
+		if (res_head == MISSING_OBJECT)
+			res_head = get_oid("HEAD", &oid);
+		if (res_head) {
+			error(_("Bad bisect_head rev input"));
+			return BISECT_FAILED;
+		}
+		oid_array_append(&revs, &oid);
+	}
+
+	/*
+	 * All input revs must be checked before executing bisect_write()
+	 * to discard junk revs.
+	 */
+
+	for (; argc; argc--, argv++) {
+		if (get_oid(*argv, &oid)){
+			error(_("Bad rev input: %s"), *argv);
+			return BISECT_FAILED;
+		}
+		oid_array_append(&revs, &oid);
+	}
+
+	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 */
+	strbuf_release(&buf);
+
+	for (i = 0; i < revs.nr; i++) {
+		if (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0))
+			return BISECT_FAILED;
+
+		if (verify_expected && !oideq(&revs.oid[i], &expected)) {
+			unlink_or_warn(git_path_bisect_ancestors_ok());
+			unlink_or_warn(git_path_bisect_expected_rev());
+			verify_expected = 0;
+		}
+	}
+
+	oid_array_clear(&revs);
+	return bisect_auto_next(terms, NULL);
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -875,7 +943,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 res = 0, nolog = 0;
 	struct option options[] = {
@@ -901,6 +970,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 it has not yet been started"), BISECT_AUTOSTART),
+		OPT_CMDMODE(0, "bisect-state", &cmdmode,
+			 N_("mark the state of ref (or refs)"), BISECT_STATE),
 		OPT_BOOL(0, "no-log", &nolog,
 			 N_("no log for BISECT_WRITE")),
 		OPT_END()
@@ -971,6 +1042,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", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 356264caf0..7a8f796251 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 git rev-parse --verify -q BISECT_HEAD > /dev/null
-	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() {
@@ -187,8 +141,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"
@@ -203,7 +156,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
 
@@ -244,7 +197,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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 11/13] bisect--helper: retire `--check-expected-revs` subcommand
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (9 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 10/13] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 12/13] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git
  Cc: Pranit Bauva, Lars Schneider, Christian Couder,
	Johannes Schindelin, Tanushree Tumane, 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. Functions `check_expected_revs` and
`is_expected_revs` are also deleted.

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

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index a976f4739c..da203c2091 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -187,30 +187,6 @@ static int write_terms(const char *bad, const char *good)
 	return res;
 }
 
-static int is_expected_rev(const char *expected_hex)
-{
-	struct strbuf actual_hex = STRBUF_INIT;
-	int res = 0;
-	if (strbuf_read_file(&actual_hex, git_path_bisect_expected_rev(), 0) >= 40) {
-		strbuf_trim(&actual_hex);
-		res = !strcmp(actual_hex.buf, expected_hex);
-	}
-	strbuf_release(&actual_hex);
-	return res;
-}
-
-static void check_expected_revs(const char **revs, int rev_nr)
-{
-	int i;
-
-	for (i = 0; i < rev_nr; i++) {
-		if (!is_expected_rev(revs[i])) {
-			unlink_or_warn(git_path_bisect_ancestors_ok());
-			unlink_or_warn(git_path_bisect_expected_rev());
-		}
-	}
-}
-
 static int bisect_reset(const char *commit)
 {
 	struct strbuf branch = STRBUF_INIT;
@@ -934,7 +910,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,
@@ -950,8 +925,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,
@@ -990,9 +963,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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 12/13] bisect--helper: retire `--write-terms` subcommand
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (10 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 11/13] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-08-31 10:50 ` [PATCH v7 13/13] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio
  2020-09-03 12:04 ` [PATCH v7 00/13] Finish converting git bisect to C part 2 Johannes Schindelin
  13 siblings, 0 replies; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git
  Cc: Pranit Bauva, Lars Schneider, Christian Couder,
	Johannes Schindelin, Tanushree Tumane, 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 da203c2091..46ddfd5710 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
 
 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>"),
@@ -909,8 +908,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,
@@ -923,8 +921,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	} cmdmode = 0;
 	int 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,
@@ -959,10 +955,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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v7 13/13] bisect--helper: retire `--bisect-autostart` subcommand
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (11 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 12/13] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
@ 2020-08-31 10:50 ` Miriam Rubio
  2020-09-03 12:04 ` [PATCH v7 00/13] Finish converting git bisect to C part 2 Johannes Schindelin
  13 siblings, 0 replies; 27+ messages in thread
From: Miriam Rubio @ 2020-08-31 10:50 UTC (permalink / raw)
  To: git
  Cc: Pranit Bauva, Lars Schneider, Christian Couder,
	Johannes Schindelin, Tanushree Tumane, 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 46ddfd5710..8a48c1ef8e 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] [--first-parent] [<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
@@ -916,7 +915,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 res = 0, nolog = 0;
@@ -937,8 +935,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 it has not yet been started"), BISECT_AUTOSTART),
 		OPT_CMDMODE(0, "bisect-state", &cmdmode,
 			 N_("mark the state of ref (or refs)"), BISECT_STATE),
 		OPT_BOOL(0, "no-log", &nolog,
@@ -998,12 +994,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 does not accept 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


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C
  2020-08-31 10:50 ` [PATCH v7 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
@ 2020-09-03  5:50   ` Johannes Schindelin
  0 siblings, 0 replies; 27+ messages in thread
From: Johannes Schindelin @ 2020-09-03  5:50 UTC (permalink / raw)
  To: Miriam Rubio
  Cc: git, Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane

Hi Miriam,

On Mon, 31 Aug 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.
>
> Add `--bisect-autostart` subcommand to be called from git-bisect.sh.
> Using `--bisect-autostart` subcommand is a temporary measure to port
> the 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 | 44 +++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            | 25 ++---------------------
>  2 files changed, 45 insertions(+), 24 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index bae09ce65d..f71e8e54d2 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] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
> +	N_("git bisect--helper --bisect-autostart"),
>  	NULL
>  };
>
> @@ -653,6 +654,38 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
>  	return res;
>  }
>
> +static inline int file_is_not_empty(const char *path)
> +{
> +	return !is_empty_or_missing_file(path);
> +}
> +
> +static int bisect_autostart(struct bisect_terms *terms)
> +{
> +	int res;
> +	const char *yesno;
> +
> +	if (file_is_not_empty(git_path_bisect_start()))
> +		return 0;
> +
> +	fprintf_ln(stderr, _("You need to start by \"git bisect "
> +			  "start\"\n"));
> +
> +	if (!isatty(STDIN_FILENO))

Not a big deal, but we have only two callers to `isatty()` that use the
`_FILENO` constants, and neither of them is about stdin. But that is not a
big deal.

> +		return 0;

However, when we cannot auto-start, the shell script version calls `exit
1` to cause a failure. I think we need to do the same here, i.e. `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);
> +	res = tolower(*yesno) == 'n' ?
> +		-1 : bisect_start(terms, empty_strvec, 0);

The corresponding shell script code reads like this:

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

That is, if the user does not want to start, the command exits. With exit
code 0, i.e. success.

However, we do not do that here.

Now, you could argue (in the commit message) that this actually fixes a
bug (because if the bisection was aborted, that does not count for
success), and I'd be fine with that.

> +
> +	return res;
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -665,7 +698,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 res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -689,6 +723,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 it has not yet been started"), BISECT_AUTOSTART),
>  		OPT_BOOL(0, "no-log", &nolog,
>  			 N_("no log for BISECT_WRITE")),
>  		OPT_END()
> @@ -748,6 +784,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		set_terms(&terms, "bad", "good");
>  		res = bisect_start(&terms, argv, argc);
>  		break;
> +	case BISECT_AUTOSTART:
> +		if (argc)
> +			return error(_("--bisect-autostart does not accept arguments"));
> +		set_terms(&terms, "bad", "good");
> +		res = bisect_autostart(&terms);
> +		break;
>  	default:
>  		BUG("unknown subcommand %d", cmdmode);
>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index c7580e51a0..9ca583d964 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

This (and the change below as well) is insufficient, as it won't `exit` in
case of an error (or in case the user aborted). I think you need to append
`|| exit` as is done two lines below this line.

Ciao,
Dscho

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

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C
  2020-08-31 10:50 ` [PATCH v7 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C Miriam Rubio
@ 2020-09-03  9:48   ` Johannes Schindelin
  0 siblings, 0 replies; 27+ messages in thread
From: Johannes Schindelin @ 2020-09-03  9:48 UTC (permalink / raw)
  To: Miriam Rubio
  Cc: git, Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane

Hi Miriam,

On Mon, 31 Aug 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                 |  11 ++-
>  builtin/bisect--helper.c | 180 ++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            |  47 +---------
>  3 files changed, 190 insertions(+), 48 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index c6aba2b9f2..f5b1368128 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -988,8 +988,11 @@ 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.
> - * If no_checkout is non-zero, the bisection process does not
> - * checkout the trial commit but instead simply updates BISECT_HEAD.
> + *
> + * 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.
>   */
>  enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
>  {
> @@ -999,6 +1002,10 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
>  	enum bisect_error res = BISECT_OK;
>  	struct object_id *bisect_rev;
>  	char *steps_msg;
> +	/*
> +	 * If no_checkout is non-zero, the bisection process does not
> +	 * checkout the trial commit but instead simply updates BISECT_HEAD.
> +	 */
>  	int no_checkout = ref_exists("BISECT_HEAD");
>  	unsigned bisect_flags = 0;
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index f71e8e54d2..e29e86142a 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,10 +30,17 @@ 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] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
> +	N_("git bisect--helper --bisect-next"),
> +	N_("git bisect--helper --bisect-auto-next"),
>  	N_("git bisect--helper --bisect-autostart"),
>  	NULL
>  };
>
> +struct add_bisect_ref_data {
> +	struct rev_info *revs;
> +	unsigned int object_flags;
> +};
> +
>  struct bisect_terms {
>  	char *term_good;
>  	char *term_bad;
> @@ -56,6 +64,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.
> @@ -80,7 +90,7 @@ static int write_in_file(const char *path, const char *mode, const char *format,
>  	FILE *fp = NULL;
>  	int res = 0;
>
> -	if (strcmp(mode, "w"))
> +	if (strcmp(mode, "w") && strcmp(mode, "a"))
>  		BUG("write-in-file does not support '%s' mode", mode);
>  	fp = fopen(path, mode);
>  	if (!fp)
> @@ -109,6 +119,18 @@ static int write_to_file(const char *path, const char *format, ...)
>  	return res;
>  }
>
> +static int append_to_file(const char *path, const char *format, ...)
> +{
> +	int res;
> +	va_list args;
> +
> +	va_start(args, format);
> +	res = write_in_file(path, "a", format, args);
> +	va_end(args);
> +
> +	return res;
> +}
> +
>  static int check_term_format(const char *term, const char *orig_term)
>  {
>  	int res;
> @@ -451,6 +473,140 @@ static int bisect_append_log_quoted(const char **argv)
>  	return res;
>  }
>
> +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;
> +}
> +
> +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
> +{
> +	int res = 0;
> +	struct add_bisect_ref_data cb = { revs };
> +	char *good = xstrfmt("%s-*", terms->term_good);
> +
> +	/*
> +	 * We cannot use terms->term_bad directly in
> +	 * for_each_glob_ref_in() and we have to append a '*' to it,
> +	 * otherwise for_each_glob_ref_in() will append '/' and '*'.
> +	 */
> +	char *bad = xstrfmt("%s*", terms->term_bad);
> +
> +	/*
> +	 * 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);
> +	setup_revisions(0, NULL, revs, NULL);
> +	for_each_glob_ref_in(add_bisect_ref, bad, "refs/bisect/", &cb);
> +	cb.object_flags = UNINTERESTING;
> +	for_each_glob_ref_in(add_bisect_ref, good, "refs/bisect/", &cb);
> +	if (prepare_revision_walk(revs))
> +		res = error(_("revision walk setup failed\n"));
> +
> +	free(good);
> +	free(bad);
> +	return res;
> +}
> +
> +static int bisect_skipped_commits(struct bisect_terms *terms)
> +{
> +	int res;
> +	FILE *fp = NULL;
> +	struct rev_info revs;
> +	struct commit *commit;
> +	struct pretty_print_context pp = {0};
> +	struct strbuf commit_name = STRBUF_INIT;
> +
> +	res = prepare_revs(terms, &revs);
> +	if (res)
> +		return res;
> +
> +	fp = fopen(git_path_bisect_log(), "a");
> +	if (!fp)
> +		return error_errno(_("could not open '%s' for appending"),
> +				  git_path_bisect_log());
> +
> +	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) {
> +		strbuf_reset(&commit_name);
> +		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);
> +	}
> +
> +	/*
> +	 * Reset the flags used by revision walks in case
> +	 * there is another revision walk after this one.
> +	 */
> +	reset_revision_walk();
> +
> +	strbuf_release(&commit_name);
> +	fclose(fp);
> +	return 0;
> +}
> +
> +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);
> +	commit = lookup_commit_reference_by_name(bad_ref);
> +	format_commit_message(commit, "%s", &commit_name, &pp);
> +
> +	res = append_to_file(git_path_bisect_log(), "# first %s commit: [%s] %s\n",
> +			    terms->term_bad, oid_to_hex(&commit->object.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)
> +{
> +	enum bisect_error res;
> +
> +	bisect_autostart(terms);

We need to handle a negative value returned by `bisect_autostart()` by
aborting, e.g. `return BISECT_FAILED;`.

> +	if (bisect_next_check(terms, terms->term_good))
> +		return BISECT_FAILED;
> +
> +	/* Perform all bisection computation */
> +	res = bisect_next_all(the_repository, prefix);
> +
> +	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);

This returns an error when `bisect_next()` failed, which is different from
what the shell script version did:

	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :

(note the trailing `|| :`, which will always return success).

I think this is a bug fix, though, so the change is good. But it probably
deserves to be called out in the commit message.

> +}
> +
>  static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
>  {
>  	int no_checkout = 0;
> @@ -699,7 +855,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

I am curious: what is the reason not to use a trailing comma, and what is
the reason to force AUTOSTART to stay at the end (instead of appending the
new modes, which strikes me as more natural)?

>  	} cmdmode = 0;
>  	int res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -723,6 +881,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 it has not yet been started"), BISECT_AUTOSTART),
>  		OPT_BOOL(0, "no-log", &nolog,
> @@ -784,6 +946,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		set_terms(&terms, "bad", "good");
>  		res = bisect_start(&terms, 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 does not accept arguments"));
> @@ -799,7 +973,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 9ca583d964..59424f5b37 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
> -	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() {
> @@ -213,7 +174,7 @@ bisect_replay () {
>  		esac
>  	done <"$file"
>  	IFS="$oIFS"
> -	bisect_auto_next
> +	git bisect--helper --bisect-auto-next

We probably need to append `|| exit` here.

Thank you,
Johannes

>  }
>
>  bisect_run () {
> @@ -310,7 +271,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
>
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 07/13] bisect--helper: finish porting `bisect_start()` to C
  2020-08-31 10:50 ` [PATCH v7 07/13] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
@ 2020-09-03  9:56   ` Johannes Schindelin
  0 siblings, 0 replies; 27+ messages in thread
From: Johannes Schindelin @ 2020-09-03  9:56 UTC (permalink / raw)
  To: Miriam Rubio; +Cc: git, Pranit Bauva, Christian Couder, Tanushree Tumane

Hi Miriam,

On Mon, 31 Aug 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()`.
>
> 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 | 56 ++++++++++++++++++++++++++++------------
>  git-bisect.sh            | 26 ++-----------------
>  2 files changed, 41 insertions(+), 41 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index e29e86142a..b40369e8aa 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -85,6 +85,19 @@ static int one_of(const char *term, ...)
>  	return res;
>  }
>
> +/*
> + * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE
> + * and BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND are codes
> + * that indicate special success.
> + */
> +
> +static int is_bisect_success(enum bisect_error res)
> +{
> +	return !res ||
> +		res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND ||
> +		res == BISECT_INTERNAL_SUCCESS_MERGE_BASE;
> +}
> +
>  static int write_in_file(const char *path, const char *mode, const char *format, va_list args)
>  {
>  	FILE *fp = NULL;
> @@ -607,12 +620,13 @@ 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, const char **argv, int argc)
> +static enum bisect_error bisect_start(struct bisect_terms *terms, const char **argv, int argc)
>  {
>  	int no_checkout = 0;
>  	int first_parent_only = 0;
>  	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;
> @@ -672,9 +686,12 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
>  			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);
> @@ -752,14 +769,7 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
>  	 * Get rid of any old bisect state.
>  	 */
>  	if (bisect_clean_state())
> -		return -1;
> -
> -	/*
> -	 * 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".
> -	 */
> +		return BISECT_FAILED;
>
>  	/*
>  	 * Write new start state
> @@ -776,7 +786,7 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
>  		}
>  		if (update_ref(NULL, "BISECT_HEAD", &oid, NULL, 0,
>  			       UPDATE_REFS_MSG_ON_ERR)) {
> -			res = -1;
> +			res = BISECT_FAILED;
>  			goto finish;
>  		}
>  	}
> @@ -788,25 +798,37 @@ static int bisect_start(struct bisect_terms *terms, const char **argv, int argc)
>  	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,
> +	 * "bisect_auto_next" above may exit or misbehave.
> +	 * We have to handle this to be able to clean up using
> +	 * "bisect_clean_state".
> +	 */

Is this comment still correct in the C version? I thought we had succeeded
in removing that shell scripting wart by turning all the `exit`s into
`return`s...

> +	if (!is_bisect_success(res))
> +		bisect_clean_state();
>  	return res;
>  }
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 59424f5b37..356264caf0 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 "$@"
> @@ -163,8 +142,7 @@ bisect_replay () {
>  		get_terms
>  		case "$command" in
>  		start)
> -			cmd="bisect_start $rev $tail"
> -			eval "$cmd" ;;
> +			eval "git bisect--helper --bisect-start $rev $tail" ;;

I think we need to append something like `test 0 = $? || exit $?` here, as
the `eval`ed command can no longer exit.

In fact, I think that the `eval` here no longer makes sense, as the only
reason we did that before was that `bisect_start` could `exit` and we did
not want that here.

>  		"$TERM_GOOD"|"$TERM_BAD"|skip)
>  			git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
>  		terms)
> @@ -264,7 +242,7 @@ case "$#" in
>  	help)
>  		git bisect -h ;;
>  	start)
> -		bisect_start "$@" ;;
> +		git bisect--helper --bisect-start "$@" ;;

I guess we do not need to append `|| exit` here because this will be the
last command in the script to be executed anyway, therefore its exit
status will become the exit status of the script.

Thank you,
Johannes

>  	bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
>  		bisect_state "$cmd" "$@" ;;
>  	skip)
> --
> 2.25.0
>
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 10/13] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C
  2020-08-31 10:50 ` [PATCH v7 10/13] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C Miriam Rubio
@ 2020-09-03 12:03   ` Johannes Schindelin
  0 siblings, 0 replies; 27+ messages in thread
From: Johannes Schindelin @ 2020-09-03 12:03 UTC (permalink / raw)
  To: Miriam Rubio
  Cc: git, Pranit Bauva, Lars Schneider, Christian Couder, Tanushree Tumane

Hi Miriam,

On Mon, 31 Aug 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 | 78 +++++++++++++++++++++++++++++++++++++++-
>  git-bisect.sh            | 55 +++-------------------------
>  2 files changed, 81 insertions(+), 52 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 0796e51672..a976f4739c 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
>  };
>
> @@ -862,6 +864,72 @@ static int bisect_autostart(struct bisect_terms *terms)
>  	return res;
>  }
>
> +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;
> +	struct oid_array revs = OID_ARRAY_INIT;
> +
> +	if (!argc)
> +		return error(_("Please call `--bisect-state` with at least one argument"));

Good. Technically, this is a deviation from the scripted version, but it
is good not to check and set the terms first, only to then error out
anyway.

However, I think we will need a call to `bisect_autostart()` somewhere
around here, as that is what the scripted version did first thing.

> +
> +	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 (argc == 0) {
> +		enum get_oid_result res_head = get_oid("BISECT_HEAD", &oid);
> +		if (res_head == MISSING_OBJECT)
> +			res_head = get_oid("HEAD", &oid);
> +		if (res_head) {
> +			error(_("Bad bisect_head rev input"));

If I am not mistaken, we should convert this shell command more faithfully
here:

			die "$(eval_gettext "Bad rev input: \$bisected_head")"

i.e. something like this:

		const char *head = "BISECT_HEAD";
		enum get_oid_result res_head = get_oid(head, &oid);

		if (res_head == MISSING_OBJECT) {
			head = "HEAD";
			res_head = get_oid(head, &oid);
		}

		if (res_head)
			error(_("Bad rev input: %s"), head);

> +			return BISECT_FAILED;
> +		}
> +		oid_array_append(&revs, &oid);
> +	}
> +
> +	/*
> +	 * All input revs must be checked before executing bisect_write()
> +	 * to discard junk revs.
> +	 */
> +
> +	for (; argc; argc--, argv++) {
> +		if (get_oid(*argv, &oid)){
> +			error(_("Bad rev input: %s"), *argv);
> +			return BISECT_FAILED;
> +		}
> +		oid_array_append(&revs, &oid);
> +	}
> +
> +	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 */
> +	strbuf_release(&buf);
> +
> +	for (i = 0; i < revs.nr; i++) {
> +		if (bisect_write(state, oid_to_hex(&revs.oid[i]), terms, 0))
> +			return BISECT_FAILED;
> +
> +		if (verify_expected && !oideq(&revs.oid[i], &expected)) {
> +			unlink_or_warn(git_path_bisect_ancestors_ok());
> +			unlink_or_warn(git_path_bisect_expected_rev());
> +			verify_expected = 0;
> +		}
> +	}

Nice! This reads so much easier than the shell script version.

> +
> +	oid_array_clear(&revs);
> +	return bisect_auto_next(terms, NULL);
> +}
> +
>  int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  {
>  	enum {
> @@ -875,7 +943,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 res = 0, nolog = 0;
>  	struct option options[] = {
> @@ -901,6 +970,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 it has not yet been started"), BISECT_AUTOSTART),
> +		OPT_CMDMODE(0, "bisect-state", &cmdmode,
> +			 N_("mark the state of ref (or refs)"), BISECT_STATE),
>  		OPT_BOOL(0, "no-log", &nolog,
>  			 N_("no log for BISECT_WRITE")),
>  		OPT_END()
> @@ -971,6 +1042,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", cmdmode);
>  	}
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 356264caf0..7a8f796251 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 git rev-parse --verify -q BISECT_HEAD > /dev/null
> -	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

Since `bisect--helper` cannot cause the shell script to `exit`, the `eval`
should no longer be necessary.

>  }
>
>  bisect_visualize() {
> @@ -187,8 +141,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"
> @@ -203,7 +156,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

I don't think we should include implementation details such as the use of
`bisect--helper` in the error message. Personally, I would either keep
`bisect_state` or paraphrase what failed in English to begin with.

Thank you,
Johannes

>  			exit $res
>  		fi
>
> @@ -244,7 +197,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
>
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 00/13] Finish converting git bisect to C part 2
  2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
                   ` (12 preceding siblings ...)
  2020-08-31 10:50 ` [PATCH v7 13/13] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio
@ 2020-09-03 12:04 ` Johannes Schindelin
  2020-09-23  7:26   ` Miriam R.
  13 siblings, 1 reply; 27+ messages in thread
From: Johannes Schindelin @ 2020-09-03 12:04 UTC (permalink / raw)
  To: Miriam Rubio; +Cc: git

Hi Miriam,


On Mon, 31 Aug 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-v7.

Excellent progress, and thank you for your patience and diligence working
on this patch series. I think we are _really_ close to being ready for
`next`.

Thanks!
Johannes

>
> I would like to thank Junio Hamano for reviewing this patch series.
>
> General changes
> ---------------
>
> * Rebased on e9b77c84a0 (Tenth batch, 2020-08-24), to build on the
>   strvec API (instead of argv_array) and adjust to the codebase
>   after the "--first-parent" feature was added.
>
>
> Specific changes
> ----------------
>
> [1/13] bisect--helper: BUG() in cmd_*() on invalid subcommand
>
> * Amend commit message.
> * Remove casting to int.
>
> ---
>
> [2/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return
>
> * Amend commit message.
>
> ---
>
> [3/13] bisect--helper: introduce new `write_in_file()` function
>
> * Use saved_errno variable.
>
> ---
>
> [5/13] bisect--helper: reimplement `bisect_autostart` shell function in C
>
> * Fix bug using empty_strvec instead of NULL in a `bisect_start()` call.
>
> ---
>
>
> [6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
>  functions in C
>
> * Remove unused `no-checkout` variable.
> * Move a comment to more appropriate place.
>
> ---
>
> .iriam Rubio (4):
>   bisect--helper: BUG() in cmd_*() on invalid subcommand
>   bisect--helper: use '-res' in 'cmd_bisect__helper' return
>   bisect--helper: introduce new `write_in_file()` function
>   bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'
>
> 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                 |  13 +-
>  builtin/bisect--helper.c | 442 ++++++++++++++++++++++++++++++++-------
>  git-bisect.sh            | 145 +------------
>  3 files changed, 380 insertions(+), 220 deletions(-)
>
> --
> 2.25.0
>
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 00/13] Finish converting git bisect to C part 2
  2020-09-03 12:04 ` [PATCH v7 00/13] Finish converting git bisect to C part 2 Johannes Schindelin
@ 2020-09-23  7:26   ` Miriam R.
  2020-09-23 14:48     ` Johannes Schindelin
  0 siblings, 1 reply; 27+ messages in thread
From: Miriam R. @ 2020-09-23  7:26 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git

Hi Johannes,

El jue., 3 sept. 2020 a las 22:29, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
>
> On Mon, 31 Aug 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-v7.
>
> Excellent progress, and thank you for your patience and diligence working
> on this patch series. I think we are _really_ close to being ready for
> `next`.
>
Thank you for your comments and encouragement.
Applying some of your suggestions related to removing some 'eval' in
git-bisect shell script, a bug has appeared. It seems it is related to
a previous code merged before my internship.
Christian Couder will take over this fix, but in the meantime I will
send a subset of this part2, with the first six patches of this patch
series (and that are not affected by the problem) in order to move
them forward and be accepted, hopefully .
Best,
Miriam




> Thanks!
> Johannes
>
> >
> > I would like to thank Junio Hamano for reviewing this patch series.
> >
> > General changes
> > ---------------
> >
> > * Rebased on e9b77c84a0 (Tenth batch, 2020-08-24), to build on the
> >   strvec API (instead of argv_array) and adjust to the codebase
> >   after the "--first-parent" feature was added.
> >
> >
> > Specific changes
> > ----------------
> >
> > [1/13] bisect--helper: BUG() in cmd_*() on invalid subcommand
> >
> > * Amend commit message.
> > * Remove casting to int.
> >
> > ---
> >
> > [2/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return
> >
> > * Amend commit message.
> >
> > ---
> >
> > [3/13] bisect--helper: introduce new `write_in_file()` function
> >
> > * Use saved_errno variable.
> >
> > ---
> >
> > [5/13] bisect--helper: reimplement `bisect_autostart` shell function in C
> >
> > * Fix bug using empty_strvec instead of NULL in a `bisect_start()` call.
> >
> > ---
> >
> >
> > [6/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
> >  functions in C
> >
> > * Remove unused `no-checkout` variable.
> > * Move a comment to more appropriate place.
> >
> > ---
> >
> > .iriam Rubio (4):
> >   bisect--helper: BUG() in cmd_*() on invalid subcommand
> >   bisect--helper: use '-res' in 'cmd_bisect__helper' return
> >   bisect--helper: introduce new `write_in_file()` function
> >   bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()'
> >
> > 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                 |  13 +-
> >  builtin/bisect--helper.c | 442 ++++++++++++++++++++++++++++++++-------
> >  git-bisect.sh            | 145 +------------
> >  3 files changed, 380 insertions(+), 220 deletions(-)
> >
> > --
> > 2.25.0
> >
> >

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 00/13] Finish converting git bisect to C part 2
  2020-09-23  7:26   ` Miriam R.
@ 2020-09-23 14:48     ` Johannes Schindelin
  2020-09-23 21:23       ` Johannes Schindelin
  0 siblings, 1 reply; 27+ messages in thread
From: Johannes Schindelin @ 2020-09-23 14:48 UTC (permalink / raw)
  To: Miriam R.; +Cc: git

Hi Miriam,

On Wed, 23 Sep 2020, Miriam R. wrote:

> Applying some of your suggestions related to removing some 'eval' in
> git-bisect shell script, a bug has appeared. It seems it is related to
> a previous code merged before my internship.

Now you got me curious: what bug did you see?

> Christian Couder will take over this fix, but in the meantime I will
> send a subset of this part2, with the first six patches of this patch
> series (and that are not affected by the problem) in order to move
> them forward and be accepted, hopefully .

Okay, good. Let me know if I can help!

Thanks,
Dscho

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 00/13] Finish converting git bisect to C part 2
  2020-09-23 14:48     ` Johannes Schindelin
@ 2020-09-23 21:23       ` Johannes Schindelin
  2020-09-24  5:33         ` Christian Couder
  2020-09-24 12:56         ` Miriam R.
  0 siblings, 2 replies; 27+ messages in thread
From: Johannes Schindelin @ 2020-09-23 21:23 UTC (permalink / raw)
  To: Miriam R.; +Cc: git

Hi Miriam,

On Wed, 23 Sep 2020, Johannes Schindelin wrote:

> On Wed, 23 Sep 2020, Miriam R. wrote:
>
> > Applying some of your suggestions related to removing some 'eval' in
> > git-bisect shell script, a bug has appeared. It seems it is related to
> > a previous code merged before my internship.
>
> Now you got me curious: what bug did you see?

I found your fork and ran the test, and this is the first symptom:

-- snip --
[...]
++ git bisect skip
Bisecting: 1 revision left to test after this (roughly 1 step)
[32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
++ git bisect good
++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt
3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit
++ git bisect log
++ git bisect reset
Previous HEAD position was 32a594a Add <4: Ciao for now> into <hello>.
Switched to branch 'other'
ok 22 - bisect skip: add line and then a new test

expecting success of 6030.23 'bisect skip and bisect replay':
        git bisect replay log_to_replay.txt > my_bisect_log.txt &&
        grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
        git bisect reset

++ git bisect replay log_to_replay.txt
error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00
error: last command exited with $?=1
not ok 23 - bisect skip and bisect replay
#
#               git bisect replay log_to_replay.txt > my_bisect_log.txt &&
#               grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
#               git bisect reset
-- snap --

So I dug a little bit further (and applied Christian's patch in the
meantime), and it turns out that the `eval` has nothing to do with what I
originally thought it would be required for: I thought that it wanted to
prevent `exit` calls from actually exiting the script.

Instead, those `eval` calls are required because the arguments are
provided in quoted form. For example, during the execution of t6030.68,
the `eval` would expand the call

	eval "git bisect--helper --bisect-start $rev $tail"

to

	git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'

Therefore, the `eval` really needs to stay in place (also the other `eval`
I had originally suggested to remove, for the same reason).

I would still recommend appending `|| exit`, even if it just so happens
that we will eventually abort when the `bisect--helper` command failed
anyway, because the next command will then fail, and abort. But it's
cleaner to abort already when this invocation failed rather than relying
on that side effect.

Ciao,
Dscho


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 00/13] Finish converting git bisect to C part 2
  2020-09-23 21:23       ` Johannes Schindelin
@ 2020-09-24  5:33         ` Christian Couder
  2020-09-24  7:56           ` Johannes Schindelin
  2020-09-24 12:56         ` Miriam R.
  1 sibling, 1 reply; 27+ messages in thread
From: Christian Couder @ 2020-09-24  5:33 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Miriam R., git

HI Dscho,

On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

> On Wed, 23 Sep 2020, Johannes Schindelin wrote:
>
> > On Wed, 23 Sep 2020, Miriam R. wrote:
> >
> > > Applying some of your suggestions related to removing some 'eval' in
> > > git-bisect shell script, a bug has appeared. It seems it is related to
> > > a previous code merged before my internship.
> >
> > Now you got me curious: what bug did you see?
>
> I found your fork and ran the test, and this is the first symptom:
>
> -- snip --
> [...]
> ++ git bisect skip
> Bisecting: 1 revision left to test after this (roughly 1 step)
> [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
> ++ git bisect good
> ++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt
> 3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit
> ++ git bisect log
> ++ git bisect reset
> Previous HEAD position was 32a594a Add <4: Ciao for now> into <hello>.
> Switched to branch 'other'
> ok 22 - bisect skip: add line and then a new test
>
> expecting success of 6030.23 'bisect skip and bisect replay':
>         git bisect replay log_to_replay.txt > my_bisect_log.txt &&
>         grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
>         git bisect reset
>
> ++ git bisect replay log_to_replay.txt
> error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00
> error: last command exited with $?=1
> not ok 23 - bisect skip and bisect replay
> #
> #               git bisect replay log_to_replay.txt > my_bisect_log.txt &&
> #               grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
> #               git bisect reset
> -- snap --
>
> So I dug a little bit further (and applied Christian's patch in the
> meantime), and it turns out that the `eval` has nothing to do with what I
> originally thought it would be required for: I thought that it wanted to
> prevent `exit` calls from actually exiting the script.
>
> Instead, those `eval` calls are required because the arguments are
> provided in quoted form. For example, during the execution of t6030.68,
> the `eval` would expand the call
>
>         eval "git bisect--helper --bisect-start $rev $tail"
>
> to
>
>         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'

Yeah, that was also what I found (along with the bug I sent a patch for).

> Therefore, the `eval` really needs to stay in place (also the other `eval`
> I had originally suggested to remove, for the same reason).
>
> I would still recommend appending `|| exit`, even if it just so happens
> that we will eventually abort when the `bisect--helper` command failed
> anyway, because the next command will then fail, and abort. But it's
> cleaner to abort already when this invocation failed rather than relying
> on that side effect.

Yeah, I think it's a good solution.

Thanks for taking a look,
Christian.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 00/13] Finish converting git bisect to C part 2
  2020-09-24  5:33         ` Christian Couder
@ 2020-09-24  7:56           ` Johannes Schindelin
  2020-09-24 10:46             ` Christian Couder
  0 siblings, 1 reply; 27+ messages in thread
From: Johannes Schindelin @ 2020-09-24  7:56 UTC (permalink / raw)
  To: Christian Couder; +Cc: Miriam R., git

Hi Christian,

On Thu, 24 Sep 2020, Christian Couder wrote:

> On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Wed, 23 Sep 2020, Johannes Schindelin wrote:
> >
> > > On Wed, 23 Sep 2020, Miriam R. wrote:
> > >
> > > > Applying some of your suggestions related to removing some 'eval' in
> > > > git-bisect shell script, a bug has appeared. It seems it is related to
> > > > a previous code merged before my internship.
> > >
> > > Now you got me curious: what bug did you see?
> >
> > I found your fork and ran the test, and this is the first symptom:
> >
> > -- snip --
> > [...]
> > ++ git bisect skip
> > Bisecting: 1 revision left to test after this (roughly 1 step)
> > [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
> > ++ git bisect good
> > ++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt
> > 3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit
> > ++ git bisect log
> > ++ git bisect reset
> > Previous HEAD position was 32a594a Add <4: Ciao for now> into <hello>.
> > Switched to branch 'other'
> > ok 22 - bisect skip: add line and then a new test
> >
> > expecting success of 6030.23 'bisect skip and bisect replay':
> >         git bisect replay log_to_replay.txt > my_bisect_log.txt &&
> >         grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
> >         git bisect reset
> >
> > ++ git bisect replay log_to_replay.txt
> > error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00
> > error: last command exited with $?=1
> > not ok 23 - bisect skip and bisect replay
> > #
> > #               git bisect replay log_to_replay.txt > my_bisect_log.txt &&
> > #               grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
> > #               git bisect reset
> > -- snap --
> >
> > So I dug a little bit further (and applied Christian's patch in the
> > meantime), and it turns out that the `eval` has nothing to do with what I
> > originally thought it would be required for: I thought that it wanted to
> > prevent `exit` calls from actually exiting the script.
> >
> > Instead, those `eval` calls are required because the arguments are
> > provided in quoted form. For example, during the execution of t6030.68,
> > the `eval` would expand the call
> >
> >         eval "git bisect--helper --bisect-start $rev $tail"
> >
> > to
> >
> >         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'
>
> Yeah, that was also what I found (along with the bug I sent a patch for).

I suspected that you had found that out, but I really wanted a record on
the Git mailing list about our findings.

It might be a good idea to add a paragraph to the respective patches,
along these lines:

	Note that the `eval` in the changed line of `git-bisect.sh` cannot be
	dropped: it is necessary because the `rev` and the `tail`
	variables may contain multiple, quoted arguments that need to be
	passed to `bisect--helper` (without the quotes, naturally).

> > Therefore, the `eval` really needs to stay in place (also the other `eval`
> > I had originally suggested to remove, for the same reason).
> >
> > I would still recommend appending `|| exit`, even if it just so happens
> > that we will eventually abort when the `bisect--helper` command failed
> > anyway, because the next command will then fail, and abort. But it's
> > cleaner to abort already when this invocation failed rather than relying
> > on that side effect.
>
> Yeah, I think it's a good solution.

Excellent. I think we can actually move forward with the entire patch
series now, not just the first subset, right?

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 00/13] Finish converting git bisect to C part 2
  2020-09-24  7:56           ` Johannes Schindelin
@ 2020-09-24 10:46             ` Christian Couder
  2020-09-24 12:53               ` Miriam R.
  0 siblings, 1 reply; 27+ messages in thread
From: Christian Couder @ 2020-09-24 10:46 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: Miriam R., git

Hi Dscho,

On Thu, Sep 24, 2020 at 12:06 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

> On Thu, 24 Sep 2020, Christian Couder wrote:
>
> > On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:

> > > Instead, those `eval` calls are required because the arguments are
> > > provided in quoted form. For example, during the execution of t6030.68,
> > > the `eval` would expand the call
> > >
> > >         eval "git bisect--helper --bisect-start $rev $tail"
> > >
> > > to
> > >
> > >         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'
> >
> > Yeah, that was also what I found (along with the bug I sent a patch for).
>
> I suspected that you had found that out, but I really wanted a record on
> the Git mailing list about our findings.
>
> It might be a good idea to add a paragraph to the respective patches,
> along these lines:
>
>         Note that the `eval` in the changed line of `git-bisect.sh` cannot be
>         dropped: it is necessary because the `rev` and the `tail`
>         variables may contain multiple, quoted arguments that need to be
>         passed to `bisect--helper` (without the quotes, naturally).

Yeah, sure. Hopefully Miriam will send this in the commit message of
the right patch which is in the subset of the patch series she hasn't
sent.

> > > Therefore, the `eval` really needs to stay in place (also the other `eval`
> > > I had originally suggested to remove, for the same reason).
> > >
> > > I would still recommend appending `|| exit`, even if it just so happens
> > > that we will eventually abort when the `bisect--helper` command failed
> > > anyway, because the next command will then fail, and abort. But it's
> > > cleaner to abort already when this invocation failed rather than relying
> > > on that side effect.
> >
> > Yeah, I think it's a good solution.
>
> Excellent. I think we can actually move forward with the entire patch
> series now, not just the first subset, right?

Yeah, I think so too.

Thanks,
Christian.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 00/13] Finish converting git bisect to C part 2
  2020-09-24 10:46             ` Christian Couder
@ 2020-09-24 12:53               ` Miriam R.
  0 siblings, 0 replies; 27+ messages in thread
From: Miriam R. @ 2020-09-24 12:53 UTC (permalink / raw)
  To: Christian Couder; +Cc: Johannes Schindelin, git

Hi,

El jue., 24 sept. 2020 a las 12:46, Christian Couder
(<christian.couder@gmail.com>) escribió:
>
> Hi Dscho,
>
> On Thu, Sep 24, 2020 at 12:06 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
>
> > On Thu, 24 Sep 2020, Christian Couder wrote:
> >
> > > On Wed, Sep 23, 2020 at 11:26 PM Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
>
> > > > Instead, those `eval` calls are required because the arguments are
> > > > provided in quoted form. For example, during the execution of t6030.68,
> > > > the `eval` would expand the call
> > > >
> > > >         eval "git bisect--helper --bisect-start $rev $tail"
> > > >
> > > > to
> > > >
> > > >         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'
> > >
> > > Yeah, that was also what I found (along with the bug I sent a patch for).
> >
> > I suspected that you had found that out, but I really wanted a record on
> > the Git mailing list about our findings.
> >
> > It might be a good idea to add a paragraph to the respective patches,
> > along these lines:
> >
> >         Note that the `eval` in the changed line of `git-bisect.sh` cannot be
> >         dropped: it is necessary because the `rev` and the `tail`
> >         variables may contain multiple, quoted arguments that need to be
> >         passed to `bisect--helper` (without the quotes, naturally).
>
> Yeah, sure. Hopefully Miriam will send this in the commit message of
> the right patch which is in the subset of the patch series she hasn't
> sent.
Ok. Noted!!
>
> > > > Therefore, the `eval` really needs to stay in place (also the other `eval`
> > > > I had originally suggested to remove, for the same reason).
> > > >
> > > > I would still recommend appending `|| exit`, even if it just so happens
> > > > that we will eventually abort when the `bisect--helper` command failed
> > > > anyway, because the next command will then fail, and abort. But it's
> > > > cleaner to abort already when this invocation failed rather than relying
> > > > on that side effect.
> > >
> > > Yeah, I think it's a good solution.
> >
> > Excellent. I think we can actually move forward with the entire patch
> > series now, not just the first subset, right?
>
> Yeah, I think so too.
Oops, I have already sent the first subset.

Best,
Miriam
>
> Thanks,
> Christian.

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v7 00/13] Finish converting git bisect to C part 2
  2020-09-23 21:23       ` Johannes Schindelin
  2020-09-24  5:33         ` Christian Couder
@ 2020-09-24 12:56         ` Miriam R.
  1 sibling, 0 replies; 27+ messages in thread
From: Miriam R. @ 2020-09-24 12:56 UTC (permalink / raw)
  To: Johannes Schindelin; +Cc: git

Hi Johannes,

El mié., 23 sept. 2020 a las 23:23, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Wed, 23 Sep 2020, Johannes Schindelin wrote:
>
> > On Wed, 23 Sep 2020, Miriam R. wrote:
> >
> > > Applying some of your suggestions related to removing some 'eval' in
> > > git-bisect shell script, a bug has appeared. It seems it is related to
> > > a previous code merged before my internship.
> >
> > Now you got me curious: what bug did you see?
>
> I found your fork and ran the test, and this is the first symptom:
>
> -- snip --
> [...]
> ++ git bisect skip
> Bisecting: 1 revision left to test after this (roughly 1 step)
> [32a594a3fdac2d57cf6d02987e30eec68511498c] Add <4: Ciao for now> into <hello>.
> ++ git bisect good
> ++ grep '3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit' my_bisect_log.txt
> 3082e11d3a0f2edca194c8ce1eb802256e38e75e is the first bad commit
> ++ git bisect log
> ++ git bisect reset
> Previous HEAD position was 32a594a Add <4: Ciao for now> into <hello>.
> Switched to branch 'other'
> ok 22 - bisect skip: add line and then a new test
>
> expecting success of 6030.23 'bisect skip and bisect replay':
>         git bisect replay log_to_replay.txt > my_bisect_log.txt &&
>         grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
>         git bisect reset
>
> ++ git bisect replay log_to_replay.txt
> error: update_ref failed for ref 'refs/bisect/bad': cannot update ref 'refs/bisect/bad': trying to write ref 'refs/bisect/bad' with nonexistent object 10006d020000000068986d020000000054f65f00
> error: last command exited with $?=1
> not ok 23 - bisect skip and bisect replay
> #
> #               git bisect replay log_to_replay.txt > my_bisect_log.txt &&
> #               grep "$HASH5 is the first bad commit" my_bisect_log.txt &&
> #               git bisect reset
> -- snap --
>
> So I dug a little bit further (and applied Christian's patch in the
> meantime), and it turns out that the `eval` has nothing to do with what I
> originally thought it would be required for: I thought that it wanted to
> prevent `exit` calls from actually exiting the script.
>
> Instead, those `eval` calls are required because the arguments are
> provided in quoted form. For example, during the execution of t6030.68,
> the `eval` would expand the call
>
>         eval "git bisect--helper --bisect-start $rev $tail"
>
> to
>
>         git bisect--helper --bisect-start '--term-old' 'term2' '--term-new' 'term1'
>
> Therefore, the `eval` really needs to stay in place (also the other `eval`
> I had originally suggested to remove, for the same reason).
Ok, I will recover your other suggestions too.
>
> I would still recommend appending `|| exit`, even if it just so happens
> that we will eventually abort when the `bisect--helper` command failed
> anyway, because the next command will then fail, and abort. But it's
> cleaner to abort already when this invocation failed rather than relying
> on that side effect.

Aha.Ok
Best,
Miriam
>
> Ciao,
> Dscho
>

^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2020-09-24 12:56 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-31 10:50 [PATCH v7 00/13] Finish converting git bisect to C part 2 Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 01/13] bisect--helper: BUG() in cmd_*() on invalid subcommand Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 02/13] bisect--helper: use '-res' in 'cmd_bisect__helper' return Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 03/13] bisect--helper: introduce new `write_in_file()` function Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 04/13] bisect--helper: reimplement `bisect_autostart` shell function in C Miriam Rubio
2020-09-03  5:50   ` Johannes Schindelin
2020-08-31 10:50 ` [PATCH v7 05/13] bisect: call 'clear_commit_marks_all()' in 'bisect_next_all()' Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 06/13] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C Miriam Rubio
2020-09-03  9:48   ` Johannes Schindelin
2020-08-31 10:50 ` [PATCH v7 07/13] bisect--helper: finish porting `bisect_start()` to C Miriam Rubio
2020-09-03  9:56   ` Johannes Schindelin
2020-08-31 10:50 ` [PATCH v7 08/13] bisect--helper: retire `--bisect-clean-state` subcommand Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 09/13] bisect--helper: retire `--next-all` subcommand Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 10/13] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C Miriam Rubio
2020-09-03 12:03   ` Johannes Schindelin
2020-08-31 10:50 ` [PATCH v7 11/13] bisect--helper: retire `--check-expected-revs` subcommand Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 12/13] bisect--helper: retire `--write-terms` subcommand Miriam Rubio
2020-08-31 10:50 ` [PATCH v7 13/13] bisect--helper: retire `--bisect-autostart` subcommand Miriam Rubio
2020-09-03 12:04 ` [PATCH v7 00/13] Finish converting git bisect to C part 2 Johannes Schindelin
2020-09-23  7:26   ` Miriam R.
2020-09-23 14:48     ` Johannes Schindelin
2020-09-23 21:23       ` Johannes Schindelin
2020-09-24  5:33         ` Christian Couder
2020-09-24  7:56           ` Johannes Schindelin
2020-09-24 10:46             ` Christian Couder
2020-09-24 12:53               ` Miriam R.
2020-09-24 12:56         ` Miriam R.

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git