[PATCH v3 0/4] Finish converting git bisect to C part 4

[PATCH v3 0/4] Finish converting git bisect to C part 4

[Miriam Rubio]

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

This fourth part is formed by reimplementations of some `git bisect` 
subcommands and removal of some temporary subcommands.

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

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

[1/4] run-command: make `exists_in_PATH()` non-static
* Amend comments.
---

[2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C
* Replace strvec_split() to sq_dequote_to_strcvec().
* Add missing "--" argument.
* Add missing comma in cmdmode enum.

---

[3/4] bisect--helper: reimplement `bisect_run` shell function in C
* Create BISECT_RUN file.
* Fix if condition.
* Remove continue statement in loop.
---


Miriam Rubio (1):
  bisect--helper: retire `--bisect-next-check` subcommand

Pranit Bauva (2):
  run-command: make `exists_in_PATH()` non-static
  bisect--helper: reimplement `bisect_visualize()`shell function in C

Tanushree Tumane (1):
  bisect--helper: reimplement `bisect_run` shell function in C

 builtin/bisect--helper.c | 138 ++++++++++++++++++++++++++++++++++++---
 git-bisect.sh            |  87 +-----------------------
 run-command.c            |   2 +-
 run-command.h            |  12 ++++
 4 files changed, 145 insertions(+), 94 deletions(-)

-- 
2.29.2

[PATCH v3 1/4] run-command: make `exists_in_PATH()` non-static

[Miriam Rubio]

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

Removes the `static` keyword from `exists_in_PATH()` function
and declares the function in `run-command.h` file.
The function will be used in bisect_visualize() in a later
commit.

Mentored by: Christian Couder <chriscool@tuxfamily.org>
Mentored by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 run-command.c |  2 +-
 run-command.h | 12 ++++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/run-command.c b/run-command.c
index be6bc128cd..210b8858f7 100644
--- a/run-command.c
+++ b/run-command.c
@@ -211,7 +211,7 @@ static char *locate_in_PATH(const char *file)
 	return NULL;
 }
 
-static int exists_in_PATH(const char *file)
+int exists_in_PATH(const char *file)
 {
 	char *r = locate_in_PATH(file);
 	int found = r != NULL;
diff --git a/run-command.h b/run-command.h
index d08414a92e..cc6f1bad20 100644
--- a/run-command.h
+++ b/run-command.h
@@ -179,6 +179,18 @@ void child_process_clear(struct child_process *);
 
 int is_executable(const char *name);
 
+/**
+ * Search if a $PATH for a command exists.  This emulates the path search that
+ * execvp would perform, without actually executing the command so it
+ * can be used before fork() to prepare to run a command using
+ * execve() or after execvp() to diagnose why it failed.
+ *
+ * The caller should ensure that file contains no directory separators.
+ *
+ * Returns 1 if it is found in $PATH or 0 if the command could not be found.
+ */
+int exists_in_PATH(const char *file);
+
 /**
  * Start a sub-process. Takes a pointer to a `struct child_process`
  * that specifies the details and returns pipe FDs (if requested).
-- 
2.29.2

[PATCH v3 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C

[Miriam Rubio]

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

Reimplement the `bisect_visualize()` shell function
in C and also add `--bisect-visualize` subcommand to
`git bisect--helper` to call it from git-bisect.sh.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 48 +++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 25 +--------------------
 2 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1fdb7d9d10..71963979b1 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
 	N_("git bisect--helper --bisect-replay <filename>"),
 	N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
+	N_("git bisect--helper --bisect-visualize"),
 	NULL
 };
 
@@ -1034,6 +1035,44 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar
 	return res;
 }
 
+static int bisect_visualize(struct bisect_terms *terms, const char **argv, int argc)
+{
+	struct strvec args = STRVEC_INIT;
+	int flags = RUN_COMMAND_NO_STDIN, res = 0;
+	struct strbuf sb = STRBUF_INIT;
+
+	if (bisect_next_check(terms, NULL) != 0)
+		return BISECT_FAILED;
+
+	if (!argc) {
+		if ((getenv("DISPLAY") || getenv("SESSIONNAME") || getenv("MSYSTEM") ||
+		     getenv("SECURITYSESSIONID")) && exists_in_PATH("gitk"))
+			strvec_push(&args, "gitk");
+		else {
+			strvec_pushl(&args, "log", NULL);
+			flags |= RUN_GIT_CMD;
+		}
+	} else {
+		if (argv[0][0] == '-') {
+			strvec_pushl(&args, "log", NULL);
+			flags |= RUN_GIT_CMD;
+		} else if (strcmp(argv[0], "tig") && !starts_with(argv[0], "git"))
+			flags |= RUN_GIT_CMD;
+
+		strvec_pushv(&args, argv);
+	}
+
+	strvec_pushl(&args, "--bisect", "--", NULL);
+
+	strbuf_read_file(&sb, git_path_bisect_names(), 0);
+	sq_dequote_to_strvec(sb.buf, &args);
+	strbuf_release(&sb);
+
+	res = run_command_v_opt(args.v, flags);
+	strvec_clear(&args);
+	return res;
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -1046,7 +1085,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_STATE,
 		BISECT_LOG,
 		BISECT_REPLAY,
-		BISECT_SKIP
+		BISECT_SKIP,
+		BISECT_VISUALIZE,
 	} cmdmode = 0;
 	int res = 0, nolog = 0;
 	struct option options[] = {
@@ -1068,6 +1108,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("replay the bisection process from the given file"), BISECT_REPLAY),
 		OPT_CMDMODE(0, "bisect-skip", &cmdmode,
 			 N_("skip some commits for checkout"), BISECT_SKIP),
+		OPT_CMDMODE(0, "bisect-visualize", &cmdmode,
+			 N_("visualize the bisection"), BISECT_VISUALIZE),
 		OPT_BOOL(0, "no-log", &nolog,
 			 N_("no log for BISECT_WRITE")),
 		OPT_END()
@@ -1128,6 +1170,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		set_terms(&terms, "bad", "good");
 		res = bisect_skip(&terms, argv, argc);
 		break;
+	case BISECT_VISUALIZE:
+		get_terms(&terms);
+		res = bisect_visualize(&terms, argv, argc);
+		break;
 	default:
 		BUG("unknown subcommand %d", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 6a7afaea8d..95f7f3fb8c 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -39,29 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
 TERM_BAD=bad
 TERM_GOOD=good
 
-bisect_visualize() {
-	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
-
-	if test $# = 0
-	then
-		if test -n "${DISPLAY+set}${SESSIONNAME+set}${MSYSTEM+set}${SECURITYSESSIONID+set}" &&
-			type gitk >/dev/null 2>&1
-		then
-			set gitk
-		else
-			set git log
-		fi
-	else
-		case "$1" in
-		git*|tig) ;;
-		-*)	set git log "$@" ;;
-		*)	set git "$@" ;;
-		esac
-	fi
-
-	eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")
-}
-
 bisect_run () {
 	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
 
@@ -152,7 +129,7 @@ case "$#" in
 		# Not sure we want "next" at the UI level anymore.
 		git bisect--helper --bisect-next "$@" || exit ;;
 	visualize|view)
-		bisect_visualize "$@" ;;
+		git bisect--helper --bisect-visualize "$@" || exit;;
 	reset)
 		git bisect--helper --bisect-reset "$@" ;;
 	replay)
-- 
2.29.2

[PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C

[Miriam Rubio]

From: Tanushree Tumane <tanushreetumane@gmail.com>

Reimplement the `bisect_run()` shell function
in C and also add `--bisect-run` subcommand to
`git bisect--helper` to call it from git-bisect.sh.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 83 ++++++++++++++++++++++++++++++++++++++++
 git-bisect.sh            | 62 +-----------------------------
 2 files changed, 84 insertions(+), 61 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 71963979b1..dc87fc7dd0 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -18,6 +18,7 @@ static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
+static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-reset [<commit>]"),
@@ -31,6 +32,7 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-replay <filename>"),
 	N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
 	N_("git bisect--helper --bisect-visualize"),
+	N_("git bisect--helper --bisect-run <cmd>..."),
 	NULL
 };
 
@@ -1073,6 +1075,78 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a
 	return res;
 }
 
+static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
+{
+	int res = BISECT_OK;
+	struct strbuf command = STRBUF_INIT;
+	struct strvec args = STRVEC_INIT;
+	struct strvec run_args = STRVEC_INIT;
+	int exit = 0;
+	int temporary_stdout_fd, saved_stdout;
+
+	if (bisect_next_check(terms, NULL))
+		return BISECT_FAILED;
+
+	if (argc)
+		sq_quote_argv(&command, argv);
+	else
+		return BISECT_FAILED;
+
+	run_args.v[0] = xstrdup(command.buf);
+	run_args.nr = 1;
+
+	while (1) {
+		strvec_clear(&args);
+		exit = 1;
+
+		printf(_("running %s"), command.buf);
+		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
+
+		if (res < 0 || 128 <= res) {
+			error(_("bisect run failed: exit code %d from"
+				" '%s' is < 0 or >= 128"), res, command.buf);
+			strbuf_release(&command);
+			return res;
+		}
+
+		if (res == 125)
+			strvec_push(&args, "skip");
+		else if (res > 0)
+			strvec_push(&args, terms->term_bad);
+		else
+			strvec_push(&args, terms->term_good);
+
+		temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);
+		saved_stdout = dup(1);
+		dup2(temporary_stdout_fd, 1);
+
+		res = bisect_state(terms, args.v, args.nr);
+
+		dup2(saved_stdout, 1);
+		close(saved_stdout);
+		close(temporary_stdout_fd);
+
+		if (res == BISECT_ONLY_SKIPPED_LEFT)
+			error(_("bisect run cannot continue any more"));
+		else if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {
+			printf(_("bisect run success"));
+			res = BISECT_OK;
+		} else if (res) {
+			error(_("bisect run failed:'git bisect--helper --bisect-state"
+			" %s' exited with error code %d"), args.v[0], res);
+		} else {
+			exit = 0;
+		}
+
+		if (exit) {
+			strbuf_release(&command);
+			strvec_clear(&args);
+			strvec_clear(&run_args);
+			return res;
+		}
+	}
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -1087,6 +1161,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_REPLAY,
 		BISECT_SKIP,
 		BISECT_VISUALIZE,
+		BISECT_RUN,
 	} cmdmode = 0;
 	int res = 0, nolog = 0;
 	struct option options[] = {
@@ -1110,6 +1185,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("skip some commits for checkout"), BISECT_SKIP),
 		OPT_CMDMODE(0, "bisect-visualize", &cmdmode,
 			 N_("visualize the bisection"), BISECT_VISUALIZE),
+		OPT_CMDMODE(0, "bisect-run", &cmdmode,
+			 N_("use <cmd>... to automatically bisect."), BISECT_RUN),
 		OPT_BOOL(0, "no-log", &nolog,
 			 N_("no log for BISECT_WRITE")),
 		OPT_END()
@@ -1174,6 +1251,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		get_terms(&terms);
 		res = bisect_visualize(&terms, argv, argc);
 		break;
+	case BISECT_RUN:
+		if (!argc)
+			return error(_("bisect run failed: no command provided."));
+		get_terms(&terms);
+		res = bisect_run(&terms, argv, argc);
+		break;
 	default:
 		BUG("unknown subcommand %d", cmdmode);
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 95f7f3fb8c..e83d011e17 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -39,66 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
 TERM_BAD=bad
 TERM_GOOD=good
 
-bisect_run () {
-	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
-
-	test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
-
-	while true
-	do
-		command="$@"
-		eval_gettextln "running \$command"
-		"$@"
-		res=$?
-
-		# Check for really bad run error.
-		if [ $res -lt 0 -o $res -ge 128 ]
-		then
-			eval_gettextln "bisect run failed:
-exit code \$res from '\$command' is < 0 or >= 128" >&2
-			exit $res
-		fi
-
-		# Find current state depending on run success or failure.
-		# A special exit code of 125 means cannot test.
-		if [ $res -eq 125 ]
-		then
-			state='skip'
-		elif [ $res -gt 0 ]
-		then
-			state="$TERM_BAD"
-		else
-			state="$TERM_GOOD"
-		fi
-
-		git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN"
-		res=$?
-
-		cat "$GIT_DIR/BISECT_RUN"
-
-		if sane_grep "first $TERM_BAD commit could be any of" "$GIT_DIR/BISECT_RUN" \
-			>/dev/null
-		then
-			gettextln "bisect run cannot continue any more" >&2
-			exit $res
-		fi
-
-		if [ $res -ne 0 ]
-		then
-			eval_gettextln "bisect run failed:
-'bisect-state \$state' exited with error code \$res" >&2
-			exit $res
-		fi
-
-		if sane_grep "is the first $TERM_BAD commit" "$GIT_DIR/BISECT_RUN" >/dev/null
-		then
-			gettextln "bisect run success"
-			exit 0;
-		fi
-
-	done
-}
-
 get_terms () {
 	if test -s "$GIT_DIR/BISECT_TERMS"
 	then
@@ -137,7 +77,7 @@ case "$#" in
 	log)
 		git bisect--helper --bisect-log || exit ;;
 	run)
-		bisect_run "$@" ;;
+		git bisect--helper --bisect-run "$@" || exit;;
 	terms)
 		git bisect--helper --bisect-terms "$@" || exit;;
 	*)
-- 
2.29.2

[PATCH v3 4/4] bisect--helper: retire `--bisect-next-check` subcommand

[Miriam Rubio]

After reimplementation of `git bisect run` in C,
`--bisect-next-check` subcommand is not needed anymore.

Let's remove it from options list and code.

Mentored by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index dc87fc7dd0..0ff34007be 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-reset [<commit>]"),
-	N_("git bisect--helper --bisect-next-check <good_term> <bad_term> [<term>]"),
 	N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
 	N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]"
 					    " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
@@ -1206,12 +1205,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			return error(_("--bisect-reset requires either no argument or a commit"));
 		res = bisect_reset(argc ? argv[0] : NULL);
 		break;
-	case BISECT_NEXT_CHECK:
-		if (argc != 2 && argc != 3)
-			return error(_("--bisect-next-check requires 2 or 3 arguments"));
-		set_terms(&terms, argv[1], argv[0]);
-		res = bisect_next_check(&terms, argc == 3 ? argv[2] : NULL);
-		break;
 	case BISECT_TERMS:
 		if (argc > 1)
 			return error(_("--bisect-terms requires 0 or 1 argument"));
-- 
2.29.2

Re: [PATCH v3 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> +	strvec_pushl(&args, "--bisect", "--", NULL);
> +
> +	strbuf_read_file(&sb, git_path_bisect_names(), 0);
> +	sq_dequote_to_strvec(sb.buf, &args);
> +	strbuf_release(&sb);

Even though it is textually not all that different, this part
changed the behaviour (hopefully in a good way) quite a bit from the
previous round.  It would make a big difference when an element in
the pathspec has shell specials.

I suspect that the reason why you didn't notice the breakage before
submitting the previous round (v2) is because of a gap in the test
coverage.  Can we add a test, or tweak an existing one?  I think a
file with a space in its name would be sufficient to demonstrate the
breakage in v2 and its fix in this round around "split" (in v2) vs
"dequote" (in v3).  A file whose name begins with a dash would
demonstrate the bug that would arise due to lack of "--".

Thanks.

Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> +		if (res < 0 || 128 <= res) {
> +			error(_("bisect run failed: exit code %d from"
> +				" '%s' is < 0 or >= 128"), res, command.buf);

Good now.

> +		temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);
> +		saved_stdout = dup(1);
> +		dup2(temporary_stdout_fd, 1);
> +
> +		res = bisect_state(terms, args.v, args.nr);
> +
> +		dup2(saved_stdout, 1);
> +		close(saved_stdout);
> +		close(temporary_stdout_fd);

In v2, we just let bisect_state() to write to our standard output,
which was the reason why the loss of "cat" in the "write to
BISECT_RUN file and cat it to show to the user" in the scripted
version in v2 was OK.  Now, we are writing out, just like the
scripted version did, to BISECT_RUN, the user does not see its
contents.

Does the distinction matter?  Christian, what's your call on this?

If it does not matter, then the code and tests are good as-is, but
if it does, the fact that you posted this round without noticing any
broken tests means that we have a gap in the test coverage.  Can we
have a new test about this (i.e. at the end of each round in "bisect
run", the output from bisect_state() is shown to the user)?

> +		if (res == BISECT_ONLY_SKIPPED_LEFT)
> +			error(_("bisect run cannot continue any more"));
> +		else if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {
> +			printf(_("bisect run success"));
> +			res = BISECT_OK;
> +		} else if (res) {
> +			error(_("bisect run failed:'git bisect--helper --bisect-state"
> +			" %s' exited with error code %d"), args.v[0], res);
> +		} else {
> +			exit = 0;
> +		}

Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Miriam Rubio <mirucam@gmail.com> writes:
>
>> +		if (res < 0 || 128 <= res) {
>> +			error(_("bisect run failed: exit code %d from"
>> +				" '%s' is < 0 or >= 128"), res, command.buf);
>
> Good now.

Oh, while asking for better test coverage, it is somewhat surprising
that the broken error condition check in v2 was never noticed.  Can
we add a test for this, too?

Thanks.

Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C

[Andrzej Hunt]

On 11/04/2021 11:55, Miriam Rubio wrote:
> From: Tanushree Tumane <tanushreetumane@gmail.com>
> 
> Reimplement the `bisect_run()` shell function
> in C and also add `--bisect-run` subcommand to
> `git bisect--helper` to call it from git-bisect.sh.
> 
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>   builtin/bisect--helper.c | 83 ++++++++++++++++++++++++++++++++++++++++
>   git-bisect.sh            | 62 +-----------------------------
>   2 files changed, 84 insertions(+), 61 deletions(-)
> 
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 71963979b1..dc87fc7dd0 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -18,6 +18,7 @@ static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
>   static GIT_PATH_FUNC(git_path_head_name, "head-name")
>   static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
>   static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
> +static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
>   
>   static const char * const git_bisect_helper_usage[] = {
>   	N_("git bisect--helper --bisect-reset [<commit>]"),
> @@ -31,6 +32,7 @@ static const char * const git_bisect_helper_usage[] = {
>   	N_("git bisect--helper --bisect-replay <filename>"),
>   	N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
>   	N_("git bisect--helper --bisect-visualize"),
> +	N_("git bisect--helper --bisect-run <cmd>..."),
>   	NULL
>   };
>   
> @@ -1073,6 +1075,78 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a
>   	return res;
>   }
>   
> +static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
> +{
> +	int res = BISECT_OK;
> +	struct strbuf command = STRBUF_INIT;
> +	struct strvec args = STRVEC_INIT;
> +	struct strvec run_args = STRVEC_INIT;
> +	int exit = 0;
> +	int temporary_stdout_fd, saved_stdout;
> +
> +	if (bisect_next_check(terms, NULL))
> +		return BISECT_FAILED;
> +
> +	if (argc)
> +		sq_quote_argv(&command, argv);
> +	else
> +		return BISECT_FAILED;
> +
> +	run_args.v[0] = xstrdup(command.buf);
> +	run_args.nr = 1;

AFAIUI manipulating the strvec directly like this means that we will 
violate the promise that strvec.v is always NULL-terminated. It's 
probably safer to call 'strvec_push(run_args, command.buf)' instead of 
manipulating v and nr?

Violating the NULL-termination promise a problem because... (continued 
below)

> +
> +	while (1) {
> +		strvec_clear(&args);
> +		exit = 1;
> +
> +		printf(_("running %s"), command.buf);
> +		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);

run_command_v_opt() implicitly expects a NULL-terminated list of 
strings. It's not documented in run_command_v_opt()'s comments, however 
run_command_v_opt() does explain that it's a wrapper around 
start_command(), which uses child_process, and child_process.argv is 
documented to require a NULL-terminated list.

If argv is not NULL-terminated, we hit a buffer overflow read  in 
prepare_shell_cmd(), which can be reproduced by running something like:

   make CC=clang-11 SANITIZE=address COPTS="-Og -g" GIT_TEST_OPTS=-v 
T=t6030-bisect-porcelain.sh test

which results in ASAN reporting this error:

==2116==ERROR: AddressSanitizer: global-buffer-overflow on address 
0x000001a51e28 at pc 0x0000009c6c1f bp 0x7ffcf0f60670 sp 0x7ffcf0f60668
READ of size 8 at 0x000001a51e28 thread T0
     #0 0x9c6c1e in prepare_shell_cmd run-command.c:284:8
     #1 0x9c6c1e in prepare_cmd run-command.c:419:3
     #2 0x9c6c1e in start_command run-command.c:753:6
     #3 0x9c7d35 in run_command run-command.c:1015:9
     #4 0x9c800c in run_command_v_opt_cd_env_tr2 run-command.c:1051:9
     #5 0x9c800c in run_command_v_opt_cd_env run-command.c:1033:9
     #6 0x9c800c in run_command_v_opt run-command.c:1023:9
     #7 0x4e5b60 in bisect_run builtin/bisect--helper.c:1102:9
     #8 0x4e5b60 in cmd_bisect__helper builtin/bisect--helper.c:1252:9
     #9 0x4ce8fe in run_builtin git.c:461:11
     #10 0x4ccbc8 in handle_builtin git.c:718:3
     #11 0x4cb0cc in run_argv git.c:785:4
     #12 0x4cb0cc in cmd_main git.c:916:19
     #13 0x6beded in main common-main.c:52:11
     #14 0x7f28636f7349 in __libc_start_main (/lib64/libc.so.6+0x24349)
     #15 0x420769 in _start ../sysdeps/x86_64/start.S:120

0x000001a51e28 is located 56 bytes to the left of global variable 
'config_update_recurse_submodules' defined in 'submodule.c:26:12' 
(0x1a51e60) of size 4
0x000001a51e28 is located 0 bytes to the right of global variable 
'empty_strvec' defined in 'strvec.c:5:13' (0x1a51e20) of size 8
SUMMARY: AddressSanitizer: global-buffer-overflow run-command.c:284:8 in 
prepare_shell_cmd


[... snip the rest ...]

Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C

[Junio C Hamano]

Andrzej Hunt <andrzej@ahunt.org> writes:

>> +	struct strbuf command = STRBUF_INIT;
>> +	struct strvec args = STRVEC_INIT;
>> +	struct strvec run_args = STRVEC_INIT;
>> + ...
>> +	run_args.v[0] = xstrdup(command.buf);
>> +	run_args.nr = 1;
>
> AFAIUI manipulating the strvec directly like this means that we will
> violate the promise that strvec.v is always NULL-terminated. It's 
> probably safer to call 'strvec_push(run_args, command.buf)' instead of
> manipulating v and nr?

True.

> Violating the NULL-termination promise a problem because... (continued
> below)
>
>> +
>> +	while (1) {
>> +		strvec_clear(&args);
>> +		exit = 1;
>> +
>> +		printf(_("running %s"), command.buf);
>> +		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
>
> run_command_v_opt() implicitly expects a NULL-terminated list of
> strings. It's not documented in run_command_v_opt()'s comments,
> however run_command_v_opt() does explain that it's a wrapper around 
> start_command(), which uses child_process, and child_process.argv is
> documented to require a NULL-terminated list.
>
> If argv is not NULL-terminated, we hit a buffer overflow read  in
> prepare_shell_cmd(), which can be reproduced by running something
> like:
>
>   make CC=clang-11 SANITIZE=address COPTS="-Og -g" GIT_TEST_OPTS=-v
>   T=t6030-bisect-porcelain.sh test
>
> which results in ASAN reporting this error:
> ...

Thanks for a careful explanation.

Re: [PATCH v3 3/4] bisect--helper: reimplement `bisect_run` shell function in C

[Christian Couder]

On Sun, Apr 11, 2021 at 10:31 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Miriam Rubio <mirucam@gmail.com> writes:

> > +             temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);
> > +             saved_stdout = dup(1);
> > +             dup2(temporary_stdout_fd, 1);
> > +
> > +             res = bisect_state(terms, args.v, args.nr);
> > +
> > +             dup2(saved_stdout, 1);
> > +             close(saved_stdout);
> > +             close(temporary_stdout_fd);
>
> In v2, we just let bisect_state() to write to our standard output,
> which was the reason why the loss of "cat" in the "write to
> BISECT_RUN file and cat it to show to the user" in the scripted
> version in v2 was OK.  Now, we are writing out, just like the
> scripted version did, to BISECT_RUN, the user does not see its
> contents.
>
> Does the distinction matter?  Christian, what's your call on this?

Sorry for the late answer. I think the content of the BISECT_RUN file
should indeed be shown.

bisect_state() calls bisect_auto_next() which calls bisect_next()
which calls bisect_next_all(), and bisect_next_all() uses printf() to
show things like "XXX is the first bad commit" which should be shown
when using `git bisect run`.

Also when adding an "exit 1 &&" before "git bisect reset" at the end
of the `"git bisect run" simple case` test, with 'next' I get:

-----------------
$ ./t6030-bisect-porcelain.sh -i -v
...
expecting success of 6030.21 '"git bisect run" simple case':
       write_script test_script.sh <<-\EOF &&
       ! grep Another hello >/dev/null
       EOF
       git bisect start &&
       git bisect good $HASH1 &&
       git bisect bad $HASH4 &&
       git bisect run ./test_script.sh >my_bisect_log.txt &&
       grep "$HASH3 is the first bad commit" my_bisect_log.txt &&
       exit 1 &&
       git bisect reset

Bisecting: 0 revisions left to test after this (roughly 1 step)
[3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for
git> into <hello>.
3de952f2416b6084f557ec417709eac740c6818c is the first bad commit
FATAL: Unexpected exit with code 1
-----------------

and:

-----------------
$ cat trash\ directory.t6030-bisect-porcelain/.git/BISECT_RUN
3de952f2416b6084f557ec417709eac740c6818c is the first bad commit
commit 3de952f2416b6084f557ec417709eac740c6818c
Author: A U Thor <author@example.com>
Date:   Thu Apr 7 15:15:13 2005 -0700

   Add <3: Another new day for git> into <hello>.

hello | 1 +
1 file changed, 1 insertion(+)
-----------------

while with 'seen' I get:

-----------------
$ ./t6030-bisect-porcelain.sh -i -v
...
expecting success of 6030.21 '"git bisect run" simple case':
       write_script test_script.sh <<-\EOF &&
       ! grep Another hello >/dev/null
       EOF
       git bisect start &&
       git bisect good $HASH1 &&
       git bisect bad $HASH4 &&
       git bisect run ./test_script.sh >my_bisect_log.txt &&
       grep "$HASH3 is the first bad commit" my_bisect_log.txt &&
       exit 1 &&
       git bisect reset

Bisecting: 0 revisions left to test after this (roughly 1 step)
[3de952f2416b6084f557ec417709eac740c6818c] Add <3: Another new day for
git> into <hello>.
error: bisect run failed:'git bisect--helper --bisect-state good'
exited with error code -10
running  './test_script.sh'running
'./test_script.sh'3de952f2416b6084f557ec417709eac740c6818c is the
first bad commit
FATAL: Unexpected exit with code 1
-----------------

and:

-----------------
$ cat trash\ directory.t6030-bisect-porcelain/.git/BISECT_RUN
-----------------

So it looks like there might be another issue with what's in 'seen'.

> If it does not matter, then the code and tests are good as-is, but
> if it does, the fact that you posted this round without noticing any
> broken tests means that we have a gap in the test coverage.  Can we
> have a new test about this (i.e. at the end of each round in "bisect
> run", the output from bisect_state() is shown to the user)?

Definitely it seems that taking a look at the tests is a good idea.
For example, comparing the verbose (with -v) output of t6030 before
and after each patch (and before and after this patch series) might
help find issues.