threads / patch / 55448

v2, 4 partsFinish converting git bisect to C part 4

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

## tl;dr

11 messages between Apr 7, 2021 and Apr 11, 2021. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Miriam Rubio· Apr 7, 2021, 17:33 UTC · lore

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
Miriam Rubio· Apr 7, 2021, 17:33 UTC · re: Miriam Rubio · lore

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

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(-)
Show changes to 2 files +11 −1

run-command.c, run-command.h

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
Đoàn Trần Công Danh· Apr 8, 2021, 11:22 UTC · re: Miriam Rubio · lore

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

On 2021-04-07 19:33:30+0200, Miriam Rubio <mirucam@gmail.com> wrote:
Show 9 quoted lines
> 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.

Show 36 quoted lines
> 
> 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
Junio C Hamano· Apr 8, 2021, 17:38 UTC · re: Đoàn Trần Công Danh · lore

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

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:
Show 13 quoted lines
> 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.

Show 9 quoted lines
>> +/**
>> + * 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.

Miriam Rubio· Apr 7, 2021, 17:33 UTC · re: Miriam Rubio · lore

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

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(-)
Show changes to 2 files +71 −62

builtin/bisect--helper.c, git-bisect.sh

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
Junio C Hamano· Apr 7, 2021, 22:09 UTC · re: Miriam Rubio · lore

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

Miriam Rubio <mirucam@gmail.com> writes:
Show 9 quoted lines
> 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.

Show 16 quoted lines
> +	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.
Show 14 quoted lines
> +		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.

Show 11 quoted lines
> -		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?

Show 11 quoted lines
> +		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.

Show 18 quoted lines
> +		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.
Miriam R.· Apr 11, 2021, 10:04 UTC · re: Junio C Hamano · lore

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

El jue, 8 abr 2021 a las 0:09, Junio C Hamano (<gitster@pobox.com>) escribió:
Show 36 quoted lines
>
> 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

Show 94 quoted lines
>
> > +     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.
Junio C Hamano· Apr 9, 2021, 06:15 UTC · re: Miriam Rubio · lore

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

Miriam Rubio <mirucam@gmail.com> writes:
Show 10 quoted lines
> +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.

Miriam Rubio· Apr 7, 2021, 17:33 UTC · re: Miriam Rubio · lore

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

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(-)
Show changes to 2 files +48 −25

builtin/bisect--helper.c, git-bisect.sh

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
Junio C Hamano· Apr 7, 2021, 21:37 UTC · re: Miriam Rubio · lore

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

Miriam Rubio <mirucam@gmail.com> writes:
This bit
Show 5 quoted lines
> -		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?
Show 16 quoted lines
> +	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.
Miriam Rubio· Apr 7, 2021, 17:33 UTC · re: Miriam Rubio · lore

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

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(-)
Show changes to builtin/bisect--helper.c +0 −7
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

← back to recent threads