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

[PATCH v2 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-v2.

General changes
---------------
* Rebase on master branch: 2e36527f23 (The sixth batch, 2021-04-02).
* Change argv_array structs to strvec.

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

[2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C
* Add `|| exit` to bisect_visualize call in shell script.
---

[3/4] bisect--helper: reimplement `bisect_run` shell function in C
* Add `|| exit` to bisect_run call in shell script.
---


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 | 124 ++++++++++++++++++++++++++++++++++++---
 git-bisect.sh            |  87 +--------------------------
 run-command.c            |   2 +-
 run-command.h            |  10 ++++
 4 files changed, 129 insertions(+), 94 deletions(-)

-- 
2.29.2

[PATCH v2 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.

`exists_in_PATH()` and `locate_in_PATH()` functions don't
depend on file-local variables.

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 | 10 ++++++++++
 2 files changed, 11 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..a520ad1342 100644
--- a/run-command.h
+++ b/run-command.h
@@ -179,6 +179,16 @@ void child_process_clear(struct child_process *);
 
 int is_executable(const char *name);
 
+/**
+ * Returns if a $PATH given by parameter is found or not (it is NULL). This
+ * function uses locate_in_PATH() function that emulates the path search that
+ * execvp would perform. Memory used to store the resultant path is freed by
+ * the function.
+ *
+ * The caller should ensure that $PATH contains no directory separators.
+ */
+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 v2 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..b77c4f6b29 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);
+	strvec_split(&args, sb.buf);
+	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 v2 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 | 71 +++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            | 62 +----------------------------------
 2 files changed, 71 insertions(+), 62 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index b77c4f6b29..31c5f99660 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -31,6 +31,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 +1074,65 @@ 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;
+
+	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);
+
+		printf(_("running %s"), command.buf);
+		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
+
+		if (res < 0 && res >= 128) {
+			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);
+
+		res = bisect_state(terms, args.v, args.nr);
+
+		if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {
+			printf(_("bisect run success"));
+			res = BISECT_OK;
+		} else if (res == BISECT_ONLY_SKIPPED_LEFT)
+			error(_("bisect run cannot continue any more"));
+		else if (res)
+			error(_("bisect run failed:'git bisect--helper --bisect-state"
+				" %s' exited with error code %d"), args.v[0], res);
+		else
+			continue;
+
+		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 {
@@ -1086,7 +1146,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_LOG,
 		BISECT_REPLAY,
 		BISECT_SKIP,
-		BISECT_VISUALIZE
+		BISECT_VISUALIZE,
+		BISECT_RUN,
 	} cmdmode = 0;
 	int res = 0, nolog = 0;
 	struct option options[] = {
@@ -1110,6 +1171,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 +1237,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 v2 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 31c5f99660..c20f43c581 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -21,7 +21,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 --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>...]"),
@@ -1192,12 +1191,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 v2 2/4] bisect--helper: reimplement `bisect_visualize()`shell function in C

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

This bit

> -		case "$1" in
> -		git*|tig) ;;
> -		-*)	set git log "$@" ;;
> -		*)	set git "$@" ;;
> -		esac

in the original corresponds to the following.

> +	} else {
> +		if (argv[0][0] == '-') {
> +			strvec_pushl(&args, "log", NULL);
> +			flags |= RUN_GIT_CMD;

If it begins with "-", we assume that is an option to "git log".  OK.

> +		} else if (strcmp(argv[0], "tig") && !starts_with(argv[0], "git"))
> +			flags |= RUN_GIT_CMD;

The condition (if it is not "tig" and does not start with "git") is
the opposite for the first case arm where we just use the rest of
the command line as is.  We take it as a git subcommand name and its
arguments.  OK.

> +		strvec_pushv(&args, argv);

And the remainder of the command line is pushed into the arguments.

OK.

> +	}

And this part from the original

> -	eval '"$@"' --bisect -- $(cat "$GIT_DIR/BISECT_NAMES")

corresponds to the rest.  What the original calls "$@" is already
captured in args.

> +	strvec_pushl(&args, "--bisect",  NULL);

We lost the "--" ("end of options and revs, now the pathspec
follows") here.  Not good.

> +	strbuf_read_file(&sb, git_path_bisect_names(), 0);
> +	strvec_split(&args, sb.buf);

This is probably quite wrong.  What we will place in the
BISECT_NAMES file, as it is a list of pathspec elements (i.e. a
pathspec), can contain spaces.  For that reason, when we write to
the file, we use sq_quote_argv() on each pathspec elements.

See builtin/bisect--helper.c::bisect_start()

If I understand correctly, strvec_split() is to split a buffer at
runs of whitespaces, which is a helper for a very different purpose.

Perhaps sq_dequote_to_strvec() is what we want to use here?

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

Perhaps it is a good time to add trailing comma after this new
token.  cf. Documentation/CodingGuidelines (look for "since early
2012").  The next patch to add yet another enum won't have to touch
the line added here that way.

Everything below looked trivially correct.

Thanks.

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

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

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

If I am reading the patch correctly, this removes the need for the
$GIT_DIR/BISECT_RUN file that used to be used to keep track of the
state?  If that is true, it is worth noting in the proposed log
message.

As far as I can see, nobody creates $GIT_DIR/BISECT_RUN anymore.

    $ git grep -e path_bisect_run -e BISECT_RUN
    bisect.c:static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
    bisect.c:	unlink_or_warn(git_path_bisect_run());
    builtin/bisect--helper.c:		BISECT_RUN,
    builtin/bisect--helper.c:			 N_("use <cmd>... to automatical...
    builtin/bisect--helper.c:	case BISECT_RUN:
    t/t6030-bisect-porcelain.sh:	test_path_is_missing ".git/BISECT_RUN" &&

What if a run script tried to read from (or checked the presence of)
the file for its correct operation (e.g. I would imagine that "do
this operation when run interactively, but do the same operation
silently when run from the git-bisect machinery" may be a reasonable
thing to do)?

This change just unintendedly broke such a script, didn't it?  The
change makes me a bit worried.

> +	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);
> +
> +		printf(_("running %s"), command.buf);
> +		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);

Nicely used sq_quote_argv() with RUN_USING_SHELL here.  Goodl.

> +		if (res < 0 && res >= 128) {
> +			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);
> +

bisect_state() does so much that it was a bit hard to follow for me
(who hasn't been following the bisect-in-C topic very closely), but
the code around here roughly corresponds to the following snippet in
the original scripted version.

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

I see that the contents of the file BISECT_RUN is shown to the user
in the original but is that part of what bisect_state() does, or did
we lose it during this round of conversion?

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

In any case, being able to check the return value from bisect_state()
and switch is so much nicer than having to sane_grep in BISECT_RUN.

> +		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 {
> @@ -1086,7 +1146,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		BISECT_LOG,
>  		BISECT_REPLAY,
>  		BISECT_SKIP,
> -		BISECT_VISUALIZE
> +		BISECT_VISUALIZE,
> +		BISECT_RUN,

Now this new one has the trailing comma.  I'd suggest doing so in
the previous step.

Thanks.

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

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

On 2021-04-07 19:33:30+0200, Miriam Rubio <mirucam@gmail.com> wrote:
> 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.
> 
> `exists_in_PATH()` and `locate_in_PATH()` functions don't
> depend on file-local variables.

Isn't this implementation detail? I think we shouldn't include them in
the commit message.

> 
> 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 | 10 ++++++++++
>  2 files changed, 11 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..a520ad1342 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -179,6 +179,16 @@ void child_process_clear(struct child_process *);
>  
>  int is_executable(const char *name);
>  
> +/**
> + * Returns if a $PATH given by parameter is found or not (it is NULL). This
> + * function uses locate_in_PATH() function that emulates the path search that
> + * execvp would perform. Memory used to store the resultant path is freed by
> + * the function.

I think this documentation focused too much in implementation detail,
locate_in_PATH is still an internal linkage symbol at this stage.
I think its mention here doesn't improve anything.

Further more, "a $PATH given by parameter" is not what this function
does, the function check if a given "file" is found in "$PATH" or not.

I would copy 2 first paragraphs of locate_in_PATH's documentation, and
append the documentation for return values instead:

-- 
Danh

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

[Junio C Hamano]

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> On 2021-04-07 19:33:30+0200, Miriam Rubio <mirucam@gmail.com> wrote:
>> 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.
>> 
>> `exists_in_PATH()` and `locate_in_PATH()` functions don't
>> depend on file-local variables.
>
> Isn't this implementation detail? I think we shouldn't include them in
> the commit message.

I also was scratching my head about the statement.  What the
sentence says is not incorrect per-se, but it was not clear what the
relevance is to mention it.  I suspect that it may have wanted to
say "because they do not depend on any file scope statics to keep
state or base their computation on, it is safe to expose them as a
generally reusable public helper functions", and if so, "that's an
irrelevant implementation detail" would not be a valid objection
against mentioning it, but as written in the original, the sentence
as a mere statement of the fact does not seem to help readers.

>> +/**
>> + * Returns if a $PATH given by parameter is found or not (it is NULL). This
>> + * function uses locate_in_PATH() function that emulates the path search that
>> + * execvp would perform. Memory used to store the resultant path is freed by
>> + * the function.
>
> I think this documentation focused too much in implementation detail,
> locate_in_PATH is still an internal linkage symbol at this stage.
> I think its mention here doesn't improve anything.

I totally agree with this.  What it does is more important.

If you have to describe how it does it, it is often because you need
to warn callers due to a curious implementation glitch (e.g. "this
uses 4-slot rotating internal buffer, so do not expect its return
value to stay stable after many calls").  In such a case, of course
describing how it does it is important to help callers avoid pitfalls,
but for this function, I do not see a need for that.

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

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> +static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
> +{
> ...
> +	while (1) {
> +		strvec_clear(&args);
> +
> +		printf(_("running %s"), command.buf);
> +		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
> +
> +		if (res < 0 && res >= 128) {

Sorry for not noticing this total nonsense during my earlier review,
but this condition will never trigger.

I would have noticed it if the style consistently used "order
quantities from left to right, as if on a number line" convention,
i.e.

		if (res < 0 && 128 <= res) {

Because there are only two valid range notation around a single
variable, which are:

    (res < 0 || 128 <= res) - meaning 'res' is outside a span, or
    (0 <= res && res < 128) - meaning 'res' is inside a span.

and the above rewriten form is neither, it would have stood out
as an error immediately.

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

[Miriam R.]

El jue, 8 abr 2021 a las 0:09, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Miriam Rubio <mirucam@gmail.com> writes:
>
> > 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>
>
> If I am reading the patch correctly, this removes the need for the
> $GIT_DIR/BISECT_RUN file that used to be used to keep track of the
> state?  If that is true, it is worth noting in the proposed log
> message.
>
> As far as I can see, nobody creates $GIT_DIR/BISECT_RUN anymore.
>
>     $ git grep -e path_bisect_run -e BISECT_RUN
>     bisect.c:static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
>     bisect.c:   unlink_or_warn(git_path_bisect_run());
>     builtin/bisect--helper.c:           BISECT_RUN,
>     builtin/bisect--helper.c:                    N_("use <cmd>... to automatical...
>     builtin/bisect--helper.c:   case BISECT_RUN:
>     t/t6030-bisect-porcelain.sh:        test_path_is_missing ".git/BISECT_RUN" &&
>
> What if a run script tried to read from (or checked the presence of)
> the file for its correct operation (e.g. I would imagine that "do
> this operation when run interactively, but do the same operation
> silently when run from the git-bisect machinery" may be a reasonable
> thing to do)?
>
> This change just unintendedly broke such a script, didn't it?  The
> change makes me a bit worried.
Hi,
thank you for reviewing!.
I don't know why the need for the $GIT_DIR/BISECT_RUN file was
removed, so in the last patch series version I have just sent,
I have added the creation of the file and it contains bisect_state()
output as in original shell script version.
Regards,
Miriam
>
> > +     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);
> > +
> > +             printf(_("running %s"), command.buf);
> > +             res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
>
> Nicely used sq_quote_argv() with RUN_USING_SHELL here.  Goodl.
>
> > +             if (res < 0 && res >= 128) {
> > +                     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);
> > +
>
> bisect_state() does so much that it was a bit hard to follow for me
> (who hasn't been following the bisect-in-C topic very closely), but
> the code around here roughly corresponds to the following snippet in
> the original scripted version.
>
> > -             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
>
> I see that the contents of the file BISECT_RUN is shown to the user
> in the original but is that part of what bisect_state() does, or did
> we lose it during this round of conversion?
>
> > +             res = bisect_state(terms, args.v, args.nr);
> > +             if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) {
> > +                     printf(_("bisect run success"));
> > +                     res = BISECT_OK;
> > +             } else if (res == BISECT_ONLY_SKIPPED_LEFT)
> > +                     error(_("bisect run cannot continue any more"));
> > +             else if (res)
> > +                     error(_("bisect run failed:'git bisect--helper --bisect-state"
> > +                             " %s' exited with error code %d"), args.v[0], res);
> > +             else
> > +                     continue;
>
> In any case, being able to check the return value from bisect_state()
> and switch is so much nicer than having to sane_grep in BISECT_RUN.
>
> > +             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 {
> > @@ -1086,7 +1146,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> >               BISECT_LOG,
> >               BISECT_REPLAY,
> >               BISECT_SKIP,
> > -             BISECT_VISUALIZE
> > +             BISECT_VISUALIZE,
> > +             BISECT_RUN,
>
> Now this new one has the trailing comma.  I'd suggest doing so in
> the previous step.
>
> Thanks.