[PATCH v2 00/11] Finish converting git bisect to C part 2

[PATCH v2 00/11] Finish converting git bisect to C part 2

[Miriam Rubio]

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

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

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

I would like to thank Junio Hamano and Johannes Schindelin for their
reviews and suggestions.

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

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

* Rebase on master branch: 98cedd0233 (Merge https://github.com/prati0100/git-gui, 2020-03-19)

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

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

* New patch that fixes a return `error()` in a `cmd_*()` function.

[2/11] bisect--helper: introduce new `write_in_file()` function

* Improve commit message.
* Change `write_in_file()` function to avoid extra allocation.
* Adapt new `write_in_file()` call in `write_terms()`.

--

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

* Adapt new `write_in_file()` call in `bisect_successful()`.
* Rewrite `register_good_ref()`
* Adapt `prepare_rev_argv()` to new `register_good_ref()`.
* Use `reset_revision_walk()` to reset flags in `process_skipped_commits()`.
* Add code comment.

--
[7/11] bisect--helper: reimplement `bisect_autostart` shell function in C

* Handle exit first in `bisect_autostart()`.
* Add spaces around a curly brackets.
* Pass NULL instead of argv to `bisect_start()`.

Note to previous reviewers:`bisect_autostart` cannot move to forward-declaration 
location, it would imply many functions relocations.

--

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

* Change `bisect_state()` logic to accept `struct object_id *` as input.
* Rewrite `bisect_state()` function following reviewer suggestions.
* Remove subshell use in `git-bisect.sh`.

Note to previous reviewers: Refactor of check_expected_revs() function 
will be in a separate patch in the next patch series (part 3).


Miriam Rubio (2):
  bisect--helper: fix `cmd_*()` function switch default return
  bisect--helper: introduce new `write_in_file()` function

Pranit Bauva (9):
  bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell
    functions in C
  bisect--helper: finish porting `bisect_start()` to C
  bisect--helper: retire `--bisect-clean-state` subcommand
  bisect--helper: retire `--next-all` subcommand
  bisect--helper: reimplement `bisect_autostart` shell function in C
  bisect--helper: reimplement `bisect_state` & `bisect_head` shell
    functions in C
  bisect--helper: retire `--check-expected-revs` subcommand
  bisect--helper: retire `--write-terms` subcommand
  bisect--helper: retire `--bisect-autostart` subcommand

 bisect.c                 |  11 ++
 builtin/bisect--helper.c | 377 +++++++++++++++++++++++++++++++++------
 git-bisect.sh            | 145 +--------------
 3 files changed, 346 insertions(+), 187 deletions(-)

-- 
2.25.0

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

[Miriam Rubio]

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

Let's fix switch default return.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

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

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> In a `cmd_*()` function, return `error()` cannot be used
> because that translates to `-1` and `cmd_*()` functions need
> to return exit codes.
>
> Let's fix switch default return.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>  builtin/bisect--helper.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index c1c40b516d..1f81cff1d8 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		res = bisect_start(&terms, no_checkout, argv, argc);
>  		break;
>  	default:
> -		return error("BUG: unknown subcommand '%d'", cmdmode);
> +		res = error(_("BUG: unknown subcommand."));

The return value from error() is *NOT* taken from "enum
bisect_error"; its value (-1) happens to be the same as
BISECT_FAILED, but that is by accident, and not by design.

So the above code is accident waiting to happen, while

	default:
		error(_("BUG: ..."));
		res = BISECT_FAILED;

would be a lot more correct (by design).

>  	}
>  	free_terms(&terms);

After this part, there is this code:

       if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
                       res = BISECT_OK;

        return abs(res);

This is not a problem with this patch, but the use of abs() is very
misleading, as res is always non-positive, as it is (after fixing
the patch I am responding to) taken from "enum bisect_error"
vocabulary.  "return -res;" would make the intent of the code
clearer, I think.

By the way, under what condition can the "BUG:" be reached?  Would
it only be reachable by a programming error?  If so, it would be
correct to use BUG("...") and force it die there.  If it can be
reached in some other way (e.g. an incorrect input by the user,
corruption in state files "git bisect" uses on the filesystem), then
it is *not* a "BUG".

I think "bisect--helper" is *not* called by end-user, so an unknown
command would be a BUG in the calling program, which is still part
of git, so it probably is more prudent to do something like the
following instead.

Thanks.

 builtin/bisect--helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

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

[Christian Couder]

On Fri, Apr 3, 2020 at 6:58 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Miriam Rubio <mirucam@gmail.com> writes:
>
> > In a `cmd_*()` function, return `error()` cannot be used
> > because that translates to `-1` and `cmd_*()` functions need
> > to return exit codes.
> >
> > Let's fix switch default return.
> >
> > Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> > Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> > ---
> >  builtin/bisect--helper.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > index c1c40b516d..1f81cff1d8 100644
> > --- a/builtin/bisect--helper.c
> > +++ b/builtin/bisect--helper.c
> > @@ -711,7 +711,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> >               res = bisect_start(&terms, no_checkout, argv, argc);
> >               break;
> >       default:
> > -             return error("BUG: unknown subcommand '%d'", cmdmode);
> > +             res = error(_("BUG: unknown subcommand."));
>
> The return value from error() is *NOT* taken from "enum
> bisect_error"; its value (-1) happens to be the same as
> BISECT_FAILED, but that is by accident, and not by design.

In bisect.h we have made sure that BISECT_FAILED would be -1, so it is
not by accident:

enum bisect_error {
        BISECT_OK = 0,
        BISECT_FAILED = -1,
        BISECT_ONLY_SKIPPED_LEFT = -2,
        BISECT_MERGE_BASE_CHECK = -3,
        BISECT_NO_TESTABLE_COMMIT = -4,
        BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND = -10,
        BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11
};

> So the above code is accident waiting to happen, while
>
>         default:
>                 error(_("BUG: ..."));
>                 res = BISECT_FAILED;
>
> would be a lot more correct (by design).

I think it is very unlikely that we will ever change the value
returned by error(), so I don't think there is an accident waiting to
happen.

Maybe we should make it clearer though in bisect.h in the comment
before the enum, that we chose -1 for BISECT_FAILED so that it is the
same as what error() returns. Maybe something like "BISECT_FAILED
error code: default error code, should be the same value as what
error() returns."

> After this part, there is this code:
>
>        if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE)
>                        res = BISECT_OK;
>
>         return abs(res);
>
> This is not a problem with this patch, but the use of abs() is very
> misleading, as res is always non-positive, as it is (after fixing
> the patch I am responding to) taken from "enum bisect_error"
> vocabulary.  "return -res;" would make the intent of the code
> clearer, I think.

I am ok with using "-res" here. There are other places where
"abs(res)" is needed though, so code could look a bit more consistent
if "abs(res)" was used here too.

> By the way, under what condition can the "BUG:" be reached?  Would
> it only be reachable by a programming error?

It could happen if a user would try to directly use `git
bisect--helper <cmd> ...` with an unsupported <cmd>. Users are not
supposed to directly use bisect--helper though.

It could also happen if a developer uses `git bisect--helper <cmd>
...` in a script, program or alias if <cmd> is not properly spelled or
is unavailable for some reason.

> If so, it would be
> correct to use BUG("...") and force it die there.  If it can be
> reached in some other way (e.g. an incorrect input by the user,
> corruption in state files "git bisect" uses on the filesystem), then
> it is *not* a "BUG".

In this case I think it's difficult to tell if it will be a bug or not.

> I think "bisect--helper" is *not* called by end-user, so an unknown
> command would be a BUG in the calling program, which is still part
> of git, so it probably is more prudent to do something like the
> following instead.

I am ok with both ways.

Thanks,
Christian.

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

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

>> The return value from error() is *NOT* taken from "enum
>> bisect_error"; its value (-1) happens to be the same as
>> BISECT_FAILED, but that is by accident, and not by design.
>
> In bisect.h we have made sure that BISECT_FAILED would be -1, so it is
> not by accident:

It *is* accident waiting to happen, unless you have a comment to
tell future developers that they are forbidden from changing the
assignment of values; "We've made sure" alone is not a good excuse.

> enum bisect_error {
>         BISECT_OK = 0,
>         BISECT_FAILED = -1,
>         BISECT_ONLY_SKIPPED_LEFT = -2,
>         BISECT_MERGE_BASE_CHECK = -3,
>         BISECT_NO_TESTABLE_COMMIT = -4,
>         BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND = -10,
>         BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11
> };
>
>> So the above code is accident waiting to happen, while
>>
>>         default:
>>                 error(_("BUG: ..."));
>>                 res = BISECT_FAILED;
>>
>> would be a lot more correct (by design).
>
> I think it is very unlikely that we will ever change the value
> returned by error(), so I don't think there is an accident waiting to
> happen.
>
> Maybe we should make it clearer though in bisect.h in the comment
> before the enum, that we chose -1 for BISECT_FAILED so that it is the
> same as what error() returns....

In this particular case, you do not even need to rely on such a
comment to tie hands of future developers' needs (e.g. they may need
to add a new enum value that must come between OK and FAILED because
they will find "if (err < FAILED)" is an easy way to do something
they need to do; an ordering requirement similar to how "enum
todo_command" in sequencer.h wants to enforce certain ordering of
values is not uncommon, and they will find it awkward if they are
told that they cannot move FAILED to some value other than -1).  You
were even shown a better way to separate "res" from the value
error() returns (which will always be -1) and BISECT_FAILED (which
may be -1 right now, but future developers may want to change it,
and you have the power to allow it).

I do not see why you are still giving a lame excuse after that.

I even do not like the fact that you are doing so in the context of
being a mentor---please do not spoil the opportunity to educate good
developers of our future; instead please lead them by showing a good
example.

> I am ok with using "-res" here. There are other places where
> "abs(res)" is needed though, so code could look a bit more consistent
> if "abs(res)" was used here too.

If there are two kinds of codepaths, some *need* to deal with both
positive and negative for good reasons, and others only need to deal
with non-positive values, it would make it easier to understand the
code by consistently using -res for the latter while using abs() for
the former.

This is a tangent, but a codepath that needs abs(res) may need to be
reexaimined for correctness, as it is likely that it is a sign that
a sloppy developer swept a deeper underlying problem under the rug.

Imagine that a function A, in one if() statement in it, returns
error() whose value is -1, and in some other if() statement returns
BAD_XYZZY whose value is 1.  The function A also returns BAD_FROTZ
whose value is 2.  The only guarantee the caller gets from the
function A is that an error is signaled by non-zero value, and zero
means success.

And if you use abs() to squash an error and BAD_XYZZY into 1 in your
function B that calls A, what good are you doing to the callers of
your B?  They cannot tell between error and BAD_XYZZY, but they can
tell them from BAD_FROTZ, but does such an arrangement make any
sense?  It would be far more rational to make your B either (1)
return -1 for any error, if B thinks callers do not have to care
(which could be a valid stance to take, depending on the nature of
B), or (2) add an error code to BAD_{XYZZY,FROTZ} family and map -1
that comes from an error to that value, so that the callers can tell
them apart, or (3) do the equivalent of (2) but do so inside A (not
in B), and update call the callers of A.

Any of the above is more sensible and future-proof, compared to
blindly using abs(res) and claim that you are safe because you are
not returning a negative value.

>> By the way, under what condition can the "BUG:" be reached?  Would
>> it only be reachable by a programming error?
>
> It could happen if a user would try to directly use `git
> bisect--helper <cmd> ...` with an unsupported <cmd>. Users are not
> supposed to directly use bisect--helper though.
>
> It could also happen if a developer uses `git bisect--helper <cmd>
> ...` in a script, program or alias if <cmd> is not properly spelled or
> is unavailable for some reason.

If the user can legitimately trigger it, it is not a "BUG:".  Let's
make sure we use "BUG:" (whether it comes from BUG("...") or handcrafted
message like this one here) only when there is a bug in our program.
In other words, when a user sees "BUG:" emitted from our program and
reports it to us, there shouldn't be a room for us to say, "eh,
thanks for reporting, but it is an intended behaviour---you are just
holding it wrong".

If I did not know bisect--helper is its way out (which would be the
endgame of making "git bisect" fully converted to C), I would say
that we should just mark it as an error, but in the endgame state,
there won't be any end-user visible bisect--helper, so I am OK to
label it as a "BUG:" in this case.  It will be in the endgame state.

Thanks.

[PATCH v2 02/11] bisect--helper: introduce new `write_in_file()` function

[Miriam Rubio]

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

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

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1f81cff1d8..e949ea74e2 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -74,6 +74,26 @@ static int one_of(const char *term, ...)
 	return res;
 }
 
+static int write_in_file(const char *filepath, const char *mode, const char *format,...)
+{
+	FILE *fp = NULL;
+	va_list args;
+	int res = 0;
+
+	if (!strcmp(mode, "a") && !strcmp(mode, "w"))
+		return error_errno(_("wrong writing mode '%s'"), mode);
+	fp = fopen(filepath, mode);
+	if (!fp)
+		return error_errno(_("could not open file '%s'"), filepath);
+	va_start(args, format);
+	res = vfprintf(fp, format, args);
+	va_end(args);
+	if (!res)
+		return error_errno(_("could not write to file '%s'"), filepath);
+
+	return fclose(fp);
+}
+
 static int check_term_format(const char *term, const char *orig_term)
 {
 	int res;
@@ -104,7 +124,6 @@ static int check_term_format(const char *term, const char *orig_term)
 
 static int write_terms(const char *bad, const char *good)
 {
-	FILE *fp = NULL;
 	int res;
 
 	if (!strcmp(bad, good))
@@ -113,12 +132,8 @@ static int write_terms(const char *bad, const char *good)
 	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
 		return -1;
 
-	fp = fopen(git_path_bisect_terms(), "w");
-	if (!fp)
-		return error_errno(_("could not open the file BISECT_TERMS"));
+	res = write_in_file(git_path_bisect_terms(), "w", "%s\n%s\n", bad, good);
 
-	res = fprintf(fp, "%s\n%s\n", bad, good);
-	res |= fclose(fp);
 	return (res < 0) ? -1 : 0;
 }
 
-- 
2.25.0

Re: [PATCH v2 02/11] bisect--helper: introduce new `write_in_file()` function

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> Let's refactor code adding a new `write_in_file()` function
> that opens a file for writing a message and closes it.
>
> This helper will be used in later steps and makes the code
> simpler and easier to understand.
>
> Mentored-by: Christian Couder <chriscool@tuxfamily.org>
> Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Signed-off-by: Miriam Rubio <mirucam@gmail.com>
> ---
>  builtin/bisect--helper.c | 27 +++++++++++++++++++++------
>  1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 1f81cff1d8..e949ea74e2 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -74,6 +74,26 @@ static int one_of(const char *term, ...)
>  	return res;
>  }
>  
> +static int write_in_file(const char *filepath, const char *mode, const char *format,...)
> +{
> +	FILE *fp = NULL;

It is crystal clear in this concise helper function that fp will
never be used without getting assigned the returned value from
fopen(), so I do not think there is any need to initialize it to
NULL.

I'd use "path", not "filepath" (which I do not think we use anywhere
in our codebase), if I were writing this function, by the way.

> +	va_list args;
> +	int res = 0;
> +
> +	if (!strcmp(mode, "a") && !strcmp(mode, "w"))
> +		return error_errno(_("wrong writing mode '%s'"), mode);

I do not see where you saw a failure from a call to system library
function, which would make 'errno' variable valid at this point, so
I am puzzled.  By using error_errno(), whose error status are you
trying to show?

Puzzled.  Shouldn't it be just error(_("..."), mode)?

> +	fp = fopen(filepath, mode);
> +	if (!fp)
> +		return error_errno(_("could not open file '%s'"), filepath);

This one would show why fopen() failed, so error_errno() would be good.
Does it help us help the users if they can tell us which mode we failed
to write to the file?   Something like

	cannot open file '%s' in mode '%s'

perhaps?

> +	va_start(args, format);
> +	res = vfprintf(fp, format, args);
> +	va_end(args);
> +	if (!res)
> +		return error_errno(_("could not write to file '%s'"), filepath);

This would show errors from vfprintf(), which is good.  However, you
fail to fclose the FILE when this return is hit, which is not good.

> +	return fclose(fp);
> +}
> +
>  static int check_term_format(const char *term, const char *orig_term)
>  {
>  	int res;
> @@ -104,7 +124,6 @@ static int check_term_format(const char *term, const char *orig_term)
>  
>  static int write_terms(const char *bad, const char *good)
>  {
> -	FILE *fp = NULL;
>  	int res;
>  
>  	if (!strcmp(bad, good))
> @@ -113,12 +132,8 @@ static int write_terms(const char *bad, const char *good)
>  	if (check_term_format(bad, "bad") || check_term_format(good, "good"))
>  		return -1;
>  
> -	fp = fopen(git_path_bisect_terms(), "w");
> -	if (!fp)
> -		return error_errno(_("could not open the file BISECT_TERMS"));
> +	res = write_in_file(git_path_bisect_terms(), "w", "%s\n%s\n", bad, good);
>  
> -	res = fprintf(fp, "%s\n%s\n", bad, good);
> -	res |= fclose(fp);
>  	return (res < 0) ? -1 : 0;
>  }

[PATCH v2 03/11] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Miriam Rubio]

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

Reimplement the `bisect_next()` and the `bisect_auto_next()` shell functions
in C and add the subcommands to `git bisect--helper` to call them from
git-bisect.sh .

Using `--bisect-next` and `--bisect-auto-start` subcommands is a
temporary measure to port shell function to C so as to use the existing
test suite. As more functions are ported, `--bisect-auto-start`
subcommand will be retired and will be called by some other methods.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 bisect.c                 |  11 +++
 builtin/bisect--helper.c | 166 ++++++++++++++++++++++++++++++++++++++-
 git-bisect.sh            |  47 +----------
 3 files changed, 180 insertions(+), 44 deletions(-)

diff --git a/bisect.c b/bisect.c
index 9154f810f7..a50278ea7e 100644
--- a/bisect.c
+++ b/bisect.c
@@ -635,6 +635,11 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
 	struct argv_array rev_argv = ARGV_ARRAY_INIT;
 	int i;
 
+	/*
+	 * `revs` could has been used before.
+	 * Thus we first need to reset it.
+	 */
+	reset_revision_walk();
 	repo_init_revisions(r, revs, prefix);
 	revs->abbrev = 0;
 	revs->commit_format = CMIT_FMT_UNSPECIFIED;
@@ -980,6 +985,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
  * the bisection process finished successfully.
  * In this case the calling function or command should not turn a
  * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code.
+ *
+ * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND
+ * in bisect_helper::bisect_next() and only transforming it to 0 at
+ * the end of bisect_helper::cmd_bisect__helper() helps bypassing
+ * all the code related to finding a commit to test.
+ *
  * If no_checkout is non-zero, the bisection process does not
  * checkout the trial commit but instead simply updates BISECT_HEAD.
  */
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index e949ea74e2..447e9e75db 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -8,6 +8,7 @@
 #include "run-command.h"
 #include "prompt.h"
 #include "quote.h"
+#include "revision.h"
 
 static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS")
 static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV")
@@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
 	N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]"
 					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
+	N_("git bisect--helper --bisect-next"),
+	N_("git bisect--helper --bisect-auto-next"),
 	NULL
 };
 
@@ -436,6 +439,149 @@ static int bisect_append_log_quoted(const char **argv)
 	return res;
 }
 
+static int register_good_ref(const char *refname,
+			     const struct object_id *oid, int flags,
+			     void *cb_data)
+{
+	struct argv_array *rev_argv = cb_data;
+
+	argv_array_push(rev_argv, oid_to_hex(oid));
+	return 0;
+}
+
+static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
+{
+	char *term_good = xstrfmt("%s-*", terms->term_good);
+
+	argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
+	for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv);
+
+	free(term_good);
+}
+
+static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
+{
+	int res = 0;
+	struct argv_array rev_argv = ARGV_ARRAY_INIT;
+
+	prepare_rev_argv(terms, &rev_argv);
+
+	/*
+	 * It is important to reset the flags used by revision walks
+	 * as the previous call to bisect_next_all() in turn
+	 * sets up a revision walk.
+	 */
+	reset_revision_walk();
+	init_revisions(revs, NULL);
+	rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);
+	if (prepare_revision_walk(revs))
+		res = error(_("revision walk setup failed\n"));
+
+	argv_array_clear(&rev_argv);
+	return res;
+}
+
+static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)
+{
+	struct commit *commit;
+	struct pretty_print_context pp = {0};
+
+	if (fprintf(fp, "# only skipped commits left to test\n") < 1)
+		return -1;
+
+	while ((commit = get_revision(revs)) != NULL) {
+		struct strbuf commit_name = STRBUF_INIT;
+		format_commit_message(commit, "%s",
+				      &commit_name, &pp);
+		fprintf(fp, "# possible first %s commit: [%s] %s\n",
+			terms->term_bad, oid_to_hex(&commit->object.oid),
+			commit_name.buf);
+		strbuf_release(&commit_name);
+	}
+
+	/*
+	 * Reset the flags used by revision walks in case
+	 * there is another revision walk after this one.
+	 */
+	reset_revision_walk();
+
+	return 0;
+}
+
+static int bisect_skipped_commits(struct bisect_terms *terms)
+{
+	int res = 0;
+	FILE *fp = NULL;
+	struct rev_info revs;
+
+	fp = fopen(git_path_bisect_log(), "a");
+	if (!fp)
+		return error_errno(_("could not open '%s' for appending"),
+				  git_path_bisect_log());
+
+	res = prepare_revs(terms, &revs);
+
+	if (!res)
+		res = process_skipped_commits(fp, terms, &revs);
+
+	fclose(fp);
+	return res;
+}
+
+static int bisect_successful(struct bisect_terms *terms)
+{
+	struct object_id oid;
+	struct commit *commit;
+	struct pretty_print_context pp = {0};
+	struct strbuf commit_name = STRBUF_INIT;
+	char *bad_ref = xstrfmt("refs/bisect/%s",terms->term_bad);
+	int res;
+
+	read_ref(bad_ref, &oid);
+	printf("%s\n", bad_ref);
+	commit = lookup_commit_reference(the_repository, &oid);
+	format_commit_message(commit, "%s", &commit_name, &pp);
+
+	res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n",
+			    terms->term_bad, oid_to_hex(&oid),
+			    commit_name.buf);
+
+	strbuf_release(&commit_name);
+	free(bad_ref);
+	return res;
+}
+
+static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
+{
+	int no_checkout;
+	enum bisect_error res;
+
+	if (bisect_next_check(terms, terms->term_good))
+		return BISECT_FAILED;
+
+	no_checkout = !is_empty_or_missing_file(git_path_bisect_head());
+
+	/* Perform all bisection computation, display and checkout */
+	res = bisect_next_all(the_repository, prefix, no_checkout);
+
+	if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
+		res = bisect_successful(terms);
+		return res ? res : BISECT_INTERNAL_SUCCESS_MERGE_BASE;
+	} else if (res == BISECT_ONLY_SKIPPED_LEFT) {
+		res = bisect_skipped_commits(terms);
+		return res ? res : BISECT_ONLY_SKIPPED_LEFT;
+	}
+	return res;
+}
+
+static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
+{
+	if (bisect_next_check(terms, NULL))
+		return BISECT_OK;
+
+	return bisect_next(terms, prefix);
+}
+
 static int bisect_start(struct bisect_terms *terms, int no_checkout,
 			const char **argv, int argc)
 {
@@ -640,7 +786,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		CHECK_AND_SET_TERMS,
 		BISECT_NEXT_CHECK,
 		BISECT_TERMS,
-		BISECT_START
+		BISECT_START,
+		BISECT_NEXT,
+		BISECT_AUTO_NEXT,
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
@@ -664,6 +812,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("print out the bisect terms"), BISECT_TERMS),
 		OPT_CMDMODE(0, "bisect-start", &cmdmode,
 			 N_("start the bisect session"), BISECT_START),
+		OPT_CMDMODE(0, "bisect-next", &cmdmode,
+			 N_("find the next bisection commit"), BISECT_NEXT),
+		OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
+			 N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_BOOL(0, "no-log", &nolog,
@@ -725,6 +877,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		set_terms(&terms, "bad", "good");
 		res = bisect_start(&terms, no_checkout, argv, argc);
 		break;
+	case BISECT_NEXT:
+		if (argc)
+			return error(_("--bisect-next requires 0 arguments"));
+		get_terms(&terms);
+		res = bisect_next(&terms, prefix);
+		break;
+	case BISECT_AUTO_NEXT:
+		if (argc)
+			return error(_("--bisect-auto-next requires 0 arguments"));
+		get_terms(&terms);
+		res = bisect_auto_next(&terms, prefix);
+		break;
 	default:
 		res = error(_("BUG: unknown subcommand."));
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index efee12b8b1..e03f210d55 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -86,8 +86,7 @@ bisect_start() {
 	#
 	# Check if we can proceed to the next bisect state.
 	#
-	get_terms
-	bisect_auto_next
+	git bisect--helper --bisect-auto-next || exit
 
 	trap '-' 0
 }
@@ -140,45 +139,7 @@ bisect_state() {
 	*)
 		usage ;;
 	esac
-	bisect_auto_next
-}
-
-bisect_auto_next() {
-	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :
-}
-
-bisect_next() {
-	case "$#" in 0) ;; *) usage ;; esac
-	bisect_autostart
-	git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit
-
-	# Perform all bisection computation, display and checkout
-	git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo --no-checkout)
-	res=$?
-
-	# Check if we should exit because bisection is finished
-	if test $res -eq 10
-	then
-		bad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD)
-		bad_commit=$(git show-branch $bad_rev)
-		echo "# first $TERM_BAD commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG"
-		exit 0
-	elif test $res -eq 2
-	then
-		echo "# only skipped commits left to test" >>"$GIT_DIR/BISECT_LOG"
-		good_revs=$(git for-each-ref --format="%(objectname)" "refs/bisect/$TERM_GOOD-*")
-		for skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs)
-		do
-			skipped_commit=$(git show-branch $skipped)
-			echo "# possible first $TERM_BAD commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG"
-		done
-		exit $res
-	fi
-
-	# Check for an error in the bisection process
-	test $res -ne 0 && exit $res
-
-	return 0
+	git bisect--helper --bisect-auto-next
 }
 
 bisect_visualize() {
@@ -232,7 +193,7 @@ bisect_replay () {
 			die "$(gettext "?? what are you talking about?")" ;;
 		esac
 	done <"$file"
-	bisect_auto_next
+	git bisect--helper --bisect-auto-next
 }
 
 bisect_run () {
@@ -329,7 +290,7 @@ case "$#" in
 		bisect_skip "$@" ;;
 	next)
 		# Not sure we want "next" at the UI level anymore.
-		bisect_next "$@" ;;
+		git bisect--helper --bisect-next "$@" || exit ;;
 	visualize|view)
 		bisect_visualize "$@" ;;
 	reset)
-- 
2.25.0

Re: [PATCH v2 03/11] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> diff --git a/bisect.c b/bisect.c
> index 9154f810f7..a50278ea7e 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -635,6 +635,11 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
>  	struct argv_array rev_argv = ARGV_ARRAY_INIT;
>  	int i;
>  
> +	/*
> +	 * `revs` could has been used before.
> +	 * Thus we first need to reset it.
> +	 */
> +	reset_revision_walk();
>  	repo_init_revisions(r, revs, prefix);

I don't have enough time and concentration to follow all the
codepaths in "bisect" that walk the commit graph starting here, but
seeing one codepath, e.g. check_ancestors(), after calling this and
walking with bisect_common(), uses clear_commit_marks_many() to
clear ALL_REV_FLAGS, not just the ones that reset_revision_walk()
clears, I am not sure what value this addition has.

To put it differently, what codepath are you protecting the revision
walk that is about to happen against with this "reset"?  Who are the
callers that could have used `revs` before calling this function and
touched only SEEN, ADDED, SHOWN, etc. without touching other bits
like COUNTED, TREESAME, UNINTERESTING that matter to the correct
operation of "bisect"?

The rest of the patch looks quite reasonably done.  Let me comment
on the patch out of order (in other words, I'll rearrange the
functions in the order I read them).  I am realizing that I feel it
easier to understand to read the code in a bottom-up fashion.

> +static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
> +{
> +	if (bisect_next_check(terms, NULL))
> +		return BISECT_OK;

The bisect_next_check() function returns what decide_next() returns,
which is either 0 or error() which is -1.  So this says "if
nect-check says there was an error, we return OK".  For the purpose
of not proceeding to bisect_next(), returning is perfectly correct,
but is the value returned correct?  The scripted original does

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

meaning "try next-check and go on to bisect_next if check succeeds;
either way, ignore all errors from them".  So it probably is more
faithful conversion to make the returned value from auto_next()
void.

> +
> +	return bisect_next(terms, prefix);
> +}

In any case, this conversion of auto_next looks like a good one,
with or without fixing its type.  The caller in cmd_bisect__helper()
seems to store the returned value in 'res' and uses it for the exit
status, but for this to be a faithful conversion, it should ignore
the returned value from here and always exit with success (and if we
do so, it is one more reason why we would want to update the type of
this function to return void).

> +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
> +{
> +	int no_checkout;
> +	enum bisect_error res;
>
> +	if (bisect_next_check(terms, terms->term_good))
> +		return BISECT_FAILED;

The original makes sure it does not get any argument, but that is
done in cmd_bisect__helper(), so it is OK.

The next thing the original does is to call bisect_autostart, before
doing the next-check.  Was it a dead code that wasn't necessary, or
is its loss a regression during the conversion?

> +
> +	no_checkout = !is_empty_or_missing_file(git_path_bisect_head());
> +
> +	/* Perform all bisection computation, display and checkout */
> +	res = bisect_next_all(the_repository, prefix, no_checkout);

Looking good.  We've already converted next_all() in the previous
series, and we call it the same way as the original by checking if
$GIT_DIR/BISECT_HEAD is there.  The original said "if BISECT_HEAD
exists as a file, use '--no-checkout'" and did not care if its empty
or not, but the updated code seems to care.  Does the difference
matter (i.e. is it more correct to ignore an empty BISECT_HEAD and
pretend as if it did not exist)?

> +	if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
> +		res = bisect_successful(terms);
> +		return res ? res : BISECT_INTERNAL_SUCCESS_MERGE_BASE;

It is unclear why "1st bad commit found" is turned into "success
merge base" here.  bisect_successfull() returns an error when it
cannot append to the log, and otherwise we would want to relay "we
are done successfully" back to the caller, no?  Puzzled....

> +	} else if (res == BISECT_ONLY_SKIPPED_LEFT) {
> +		res = bisect_skipped_commits(terms);
> +		return res ? res : BISECT_ONLY_SKIPPED_LEFT;
> +	}
> +	return res;
> +}
> +

This side, I can understand what it wants to do to the "we only have
skipped ones so we cannot continue" status.

What is done in bisect_skipped_commits() is dubious, though (we'll
see it later).


> +static int bisect_skipped_commits(struct bisect_terms *terms)
> +{
> +	int res = 0;
> +	FILE *fp = NULL;
> +	struct rev_info revs;
> +
> +	fp = fopen(git_path_bisect_log(), "a");
> +	if (!fp)
> +		return error_errno(_("could not open '%s' for appending"),
> +				  git_path_bisect_log());
> +
> +	res = prepare_revs(terms, &revs);
> +
> +	if (!res)
> +		res = process_skipped_commits(fp, terms, &revs);
> +
> +	fclose(fp);
> +	return res;
> +}
> +

Opens the log to append to, or returns if we cannot.  Calls
prepare_revs() and process() if successfully prepared, and then
close.  No resource leak, and the returned value is the result code
from the last function that matters.  Looks good.

> +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
> +{
> +	int res = 0;
> +	struct argv_array rev_argv = ARGV_ARRAY_INIT;
> +
> +	prepare_rev_argv(terms, &rev_argv);
> +
> +	/*
> +	 * It is important to reset the flags used by revision walks
> +	 * as the previous call to bisect_next_all() in turn
> +	 * sets up a revision walk.
> +	 */
> +	reset_revision_walk();
> +	init_revisions(revs, NULL);
> +	rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);
> +	if (prepare_revision_walk(revs))
> +		res = error(_("revision walk setup failed\n"));
> +
> +	argv_array_clear(&rev_argv);
> +	return res;
> +}
> +

Unlike the one in rev_setup() above, the only codepath this thing is
used is quite limited (i.e. after doing all the bisection
computation including walking the graph and counting the weights
with various bits in bisect_next) and we know all that is left to do
is to run a single rev-list call, so it is clear to see that
reset_revision_walk() is sufficient here.

> +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
> +{
> +	char *term_good = xstrfmt("%s-*", terms->term_good);
> +
> +	argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
> +	for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv);
> +
> +	free(term_good);
> +}
> +

This sets up to find commits that can be reached by BAD that cannot
be reached by any GOOD revs, which is a quite faithful translation
from the original one.

> +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)
> +{
> +	struct commit *commit;
> +	struct pretty_print_context pp = {0};
> +
> +	if (fprintf(fp, "# only skipped commits left to test\n") < 1)
> +		return -1;

What's that comparison with "< 1" doing?  That's a 36-byte message,
and you are saying that it is OK if we showed only 10-byte from it,
but it is not OK, even if the function did not report an output error
by returning a negative value, if it returned that it wrote 0 bytes?

I can understand it if it were

	if (fprintf(fp, "...") < 0)
		return error_errno(_("failed to write to ..."));

though.

> +	while ((commit = get_revision(revs)) != NULL) {
> +		struct strbuf commit_name = STRBUF_INIT;
> +		format_commit_message(commit, "%s",
> +				      &commit_name, &pp);
> +		fprintf(fp, "# possible first %s commit: [%s] %s\n",
> +			terms->term_bad, oid_to_hex(&commit->object.oid),
> +			commit_name.buf);
> +		strbuf_release(&commit_name);
> +	}

Again, this is a faithful translation of the rev-list that appears
in the original, provided that &revs was set up correctly, and
relevant object flags cleared correctly before we start the
traversal, both of which seem to be the case.

> +	/*
> +	 * Reset the flags used by revision walks in case
> +	 * there is another revision walk after this one.
> +	 */
> +	reset_revision_walk();
> +
> +	return 0;
> +}
> +

So, overall, this step was a quite pleasant read, except for the
very first bit above.

Thanks.

> +static int register_good_ref(const char *refname,
> +			     const struct object_id *oid, int flags,
> +			     void *cb_data)
> +{
> +	struct argv_array *rev_argv = cb_data;
> +
> +	argv_array_push(rev_argv, oid_to_hex(oid));
> +	return 0;
> +}
> +
> +static int bisect_successful(struct bisect_terms *terms)
> +{
> +	struct object_id oid;
> +	struct commit *commit;
> +	struct pretty_print_context pp = {0};
> +	struct strbuf commit_name = STRBUF_INIT;
> +	char *bad_ref = xstrfmt("refs/bisect/%s",terms->term_bad);
> +	int res;
> +
> +	read_ref(bad_ref, &oid);
> +	printf("%s\n", bad_ref);
> +	commit = lookup_commit_reference(the_repository, &oid);
> +	format_commit_message(commit, "%s", &commit_name, &pp);
> +
> +	res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n",
> +			    terms->term_bad, oid_to_hex(&oid),
> +			    commit_name.buf);
> +
> +	strbuf_release(&commit_name);
> +	free(bad_ref);
> +	return res;
> +}
> +

Re: [PATCH v2 03/11] bisect--helper: reimplement `bisect_next` and `bisect_auto_next` shell functions in C

[Miriam R.]

Hi Junio,
thank you very much for reviewing, I have just sent the new version of
this patch series with your suggestions.

El vie., 3 abr. 2020 a las 23:19, Junio C Hamano (<gitster@pobox.com>) escribió:
>
> Miriam Rubio <mirucam@gmail.com> writes:
>
> > diff --git a/bisect.c b/bisect.c
> > index 9154f810f7..a50278ea7e 100644
> > --- a/bisect.c
> > +++ b/bisect.c
> > @@ -635,6 +635,11 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs,
> >       struct argv_array rev_argv = ARGV_ARRAY_INIT;
> >       int i;
> >
> > +     /*
> > +      * `revs` could has been used before.
> > +      * Thus we first need to reset it.
> > +      */
> > +     reset_revision_walk();
> >       repo_init_revisions(r, revs, prefix);
>
> I don't have enough time and concentration to follow all the
> codepaths in "bisect" that walk the commit graph starting here, but
> seeing one codepath, e.g. check_ancestors(), after calling this and
> walking with bisect_common(), uses clear_commit_marks_many() to
> clear ALL_REV_FLAGS, not just the ones that reset_revision_walk()
> clears, I am not sure what value this addition has.
>
> To put it differently, what codepath are you protecting the revision
> walk that is about to happen against with this "reset"?  Who are the
> callers that could have used `revs` before calling this function and
> touched only SEEN, ADDED, SHOWN, etc. without touching other bits
> like COUNTED, TREESAME, UNINTERESTING that matter to the correct
> operation of "bisect"?
>
> The rest of the patch looks quite reasonably done.  Let me comment
> on the patch out of order (in other words, I'll rearrange the
> functions in the order I read them).  I am realizing that I feel it
> easier to understand to read the code in a bottom-up fashion.
>
> > +static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
> > +{
> > +     if (bisect_next_check(terms, NULL))
> > +             return BISECT_OK;
>
> The bisect_next_check() function returns what decide_next() returns,
> which is either 0 or error() which is -1.  So this says "if
> nect-check says there was an error, we return OK".  For the purpose
> of not proceeding to bisect_next(), returning is perfectly correct,
> but is the value returned correct?  The scripted original does
>
>   git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || :
>
> meaning "try next-check and go on to bisect_next if check succeeds;
> either way, ignore all errors from them".  So it probably is more
> faithful conversion to make the returned value from auto_next()
> void.
>
> > +
> > +     return bisect_next(terms, prefix);
> > +}
>
> In any case, this conversion of auto_next looks like a good one,
> with or without fixing its type.  The caller in cmd_bisect__helper()
> seems to store the returned value in 'res' and uses it for the exit
> status, but for this to be a faithful conversion, it should ignore
> the returned value from here and always exit with success (and if we
> do so, it is one more reason why we would want to update the type of
> this function to return void).
>

I cannot change bisect_auto_next()  to return void because
in shell the bisect_next() function used "exit $res" , so the following code:

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

can result that whole `git bisect` exits with an error code when
bisect_next() does an "exit $res" with $res != 0.

So errors from bisect_next() are not ignored and I cannot make
bisect_auto_next() return void.

Best,
Miriam.


> > +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
> > +{
> > +     int no_checkout;
> > +     enum bisect_error res;
> >
> > +     if (bisect_next_check(terms, terms->term_good))
> > +             return BISECT_FAILED;
>
> The original makes sure it does not get any argument, but that is
> done in cmd_bisect__helper(), so it is OK.
>
> The next thing the original does is to call bisect_autostart, before
> doing the next-check.  Was it a dead code that wasn't necessary, or
> is its loss a regression during the conversion?
>
> > +
> > +     no_checkout = !is_empty_or_missing_file(git_path_bisect_head());
> > +
> > +     /* Perform all bisection computation, display and checkout */
> > +     res = bisect_next_all(the_repository, prefix, no_checkout);
>
> Looking good.  We've already converted next_all() in the previous
> series, and we call it the same way as the original by checking if
> $GIT_DIR/BISECT_HEAD is there.  The original said "if BISECT_HEAD
> exists as a file, use '--no-checkout'" and did not care if its empty
> or not, but the updated code seems to care.  Does the difference
> matter (i.e. is it more correct to ignore an empty BISECT_HEAD and
> pretend as if it did not exist)?
>
> > +     if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
> > +             res = bisect_successful(terms);
> > +             return res ? res : BISECT_INTERNAL_SUCCESS_MERGE_BASE;
>
> It is unclear why "1st bad commit found" is turned into "success
> merge base" here.  bisect_successfull() returns an error when it
> cannot append to the log, and otherwise we would want to relay "we
> are done successfully" back to the caller, no?  Puzzled....
>
> > +     } else if (res == BISECT_ONLY_SKIPPED_LEFT) {
> > +             res = bisect_skipped_commits(terms);
> > +             return res ? res : BISECT_ONLY_SKIPPED_LEFT;
> > +     }
> > +     return res;
> > +}
> > +
>
> This side, I can understand what it wants to do to the "we only have
> skipped ones so we cannot continue" status.
>
> What is done in bisect_skipped_commits() is dubious, though (we'll
> see it later).
>
>
> > +static int bisect_skipped_commits(struct bisect_terms *terms)
> > +{
> > +     int res = 0;
> > +     FILE *fp = NULL;
> > +     struct rev_info revs;
> > +
> > +     fp = fopen(git_path_bisect_log(), "a");
> > +     if (!fp)
> > +             return error_errno(_("could not open '%s' for appending"),
> > +                               git_path_bisect_log());
> > +
> > +     res = prepare_revs(terms, &revs);
> > +
> > +     if (!res)
> > +             res = process_skipped_commits(fp, terms, &revs);
> > +
> > +     fclose(fp);
> > +     return res;
> > +}
> > +
>
> Opens the log to append to, or returns if we cannot.  Calls
> prepare_revs() and process() if successfully prepared, and then
> close.  No resource leak, and the returned value is the result code
> from the last function that matters.  Looks good.
>
> > +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs)
> > +{
> > +     int res = 0;
> > +     struct argv_array rev_argv = ARGV_ARRAY_INIT;
> > +
> > +     prepare_rev_argv(terms, &rev_argv);
> > +
> > +     /*
> > +      * It is important to reset the flags used by revision walks
> > +      * as the previous call to bisect_next_all() in turn
> > +      * sets up a revision walk.
> > +      */
> > +     reset_revision_walk();
> > +     init_revisions(revs, NULL);
> > +     rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL);
> > +     if (prepare_revision_walk(revs))
> > +             res = error(_("revision walk setup failed\n"));
> > +
> > +     argv_array_clear(&rev_argv);
> > +     return res;
> > +}
> > +
>
> Unlike the one in rev_setup() above, the only codepath this thing is
> used is quite limited (i.e. after doing all the bisection
> computation including walking the graph and counting the weights
> with various bits in bisect_next) and we know all that is left to do
> is to run a single rev-list call, so it is clear to see that
> reset_revision_walk() is sufficient here.
>
> > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv)
> > +{
> > +     char *term_good = xstrfmt("%s-*", terms->term_good);
> > +
> > +     argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL);
> > +     for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv);
> > +
> > +     free(term_good);
> > +}
> > +
>
> This sets up to find commits that can be reached by BAD that cannot
> be reached by any GOOD revs, which is a quite faithful translation
> from the original one.
>
> > +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs)
> > +{
> > +     struct commit *commit;
> > +     struct pretty_print_context pp = {0};
> > +
> > +     if (fprintf(fp, "# only skipped commits left to test\n") < 1)
> > +             return -1;
>
> What's that comparison with "< 1" doing?  That's a 36-byte message,
> and you are saying that it is OK if we showed only 10-byte from it,
> but it is not OK, even if the function did not report an output error
> by returning a negative value, if it returned that it wrote 0 bytes?
>
> I can understand it if it were
>
>         if (fprintf(fp, "...") < 0)
>                 return error_errno(_("failed to write to ..."));
>
> though.
>
> > +     while ((commit = get_revision(revs)) != NULL) {
> > +             struct strbuf commit_name = STRBUF_INIT;
> > +             format_commit_message(commit, "%s",
> > +                                   &commit_name, &pp);
> > +             fprintf(fp, "# possible first %s commit: [%s] %s\n",
> > +                     terms->term_bad, oid_to_hex(&commit->object.oid),
> > +                     commit_name.buf);
> > +             strbuf_release(&commit_name);
> > +     }
>
> Again, this is a faithful translation of the rev-list that appears
> in the original, provided that &revs was set up correctly, and
> relevant object flags cleared correctly before we start the
> traversal, both of which seem to be the case.
>
> > +     /*
> > +      * Reset the flags used by revision walks in case
> > +      * there is another revision walk after this one.
> > +      */
> > +     reset_revision_walk();
> > +
> > +     return 0;
> > +}
> > +
>
> So, overall, this step was a quite pleasant read, except for the
> very first bit above.
>
> Thanks.
>
> > +static int register_good_ref(const char *refname,
> > +                          const struct object_id *oid, int flags,
> > +                          void *cb_data)
> > +{
> > +     struct argv_array *rev_argv = cb_data;
> > +
> > +     argv_array_push(rev_argv, oid_to_hex(oid));
> > +     return 0;
> > +}
> > +
> > +static int bisect_successful(struct bisect_terms *terms)
> > +{
> > +     struct object_id oid;
> > +     struct commit *commit;
> > +     struct pretty_print_context pp = {0};
> > +     struct strbuf commit_name = STRBUF_INIT;
> > +     char *bad_ref = xstrfmt("refs/bisect/%s",terms->term_bad);
> > +     int res;
> > +
> > +     read_ref(bad_ref, &oid);
> > +     printf("%s\n", bad_ref);
> > +     commit = lookup_commit_reference(the_repository, &oid);
> > +     format_commit_message(commit, "%s", &commit_name, &pp);
> > +
> > +     res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n",
> > +                         terms->term_bad, oid_to_hex(&oid),
> > +                         commit_name.buf);
> > +
> > +     strbuf_release(&commit_name);
> > +     free(bad_ref);
> > +     return res;
> > +}
> > +

[PATCH v2 04/11] bisect--helper: finish porting `bisect_start()` to C

[Miriam Rubio]

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

Add the subcommand to `git bisect--helper` and call it from
git-bisect.sh.

With the conversion of `bisect_auto_next()` from shell to C in a
previous commit, `bisect_start()` can now be fully ported to C.

So let's complete the `--bisect-start` subcommand of
`git bisect--helper` so that it fully implements `bisect_start()`,
and let's use this subcommand in `git-bisect.sh` instead of
`bisect_start()`.

This removes the signal handling we had in `bisect_start()` as we
don't really need it. While at it, "trap" is changed to "handle".
As "trap" is a reference to the shell "trap" builtin, which isn't
used anymore.

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

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 447e9e75db..84aac08ced 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -582,11 +582,12 @@ static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char
 	return bisect_next(terms, prefix);
 }
 
-static int bisect_start(struct bisect_terms *terms, int no_checkout,
+static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkout,
 			const char **argv, int argc)
 {
 	int i, has_double_dash = 0, must_write_terms = 0, bad_seen = 0;
-	int flags, pathspec_pos, res = 0;
+	int flags, pathspec_pos;
+	enum bisect_error res = BISECT_OK;
 	struct string_list revs = STRING_LIST_INIT_DUP;
 	struct string_list states = STRING_LIST_INIT_DUP;
 	struct strbuf start_head = STRBUF_INIT;
@@ -639,9 +640,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 			return error(_("unrecognized option: '%s'"), arg);
 		} else {
 			char *commit_id = xstrfmt("%s^{commit}", arg);
-			if (get_oid(commit_id, &oid) && has_double_dash)
-				die(_("'%s' does not appear to be a valid "
-				      "revision"), arg);
+			if (get_oid(commit_id, &oid) && has_double_dash) {
+				error(_("'%s' does not appear to be a valid "
+					"revision"), arg);
+				free(commit_id);
+				return BISECT_FAILED;
+			}
 
 			string_list_append(&revs, oid_to_hex(&oid));
 			free(commit_id);
@@ -719,12 +723,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 	 * Get rid of any old bisect state.
 	 */
 	if (bisect_clean_state())
-		return -1;
+		return BISECT_FAILED;
 
 	/*
-	 * In case of mistaken revs or checkout error, or signals received,
+	 * In case of mistaken revs or checkout error,
 	 * "bisect_auto_next" below may exit or misbehave.
-	 * We have to trap this to be able to clean up using
+	 * We have to handle this to be able to clean up using
 	 * "bisect_clean_state".
 	 */
 
@@ -740,7 +744,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 		}
 		if (update_ref(NULL, "BISECT_HEAD", &oid, NULL, 0,
 			       UPDATE_REFS_MSG_ON_ERR)) {
-			res = -1;
+			res = BISECT_FAILED;
 			goto finish;
 		}
 	}
@@ -752,25 +756,51 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 	for (i = 0; i < states.nr; i++)
 		if (bisect_write(states.items[i].string,
 				 revs.items[i].string, terms, 1)) {
-			res = -1;
+			res = BISECT_FAILED;
 			goto finish;
 		}
 
 	if (must_write_terms && write_terms(terms->term_bad,
 					    terms->term_good)) {
-		res = -1;
+		res = BISECT_FAILED;
 		goto finish;
 	}
 
 	res = bisect_append_log_quoted(argv);
 	if (res)
-		res = -1;
+		res = BISECT_FAILED;
 
 finish:
 	string_list_clear(&revs, 0);
 	string_list_clear(&states, 0);
 	strbuf_release(&start_head);
 	strbuf_release(&bisect_names);
+	if (res)
+		return res;
+
+	res = bisect_auto_next(terms, NULL);
+	/*
+	 * In case of mistaken revs or checkout error, or signals received,
+	 * "bisect_auto_next" below may exit or misbehave.
+	 * We have to handle this to be able to clean up using
+	 * "bisect_clean_state".
+	 * return code BISECT_INTERNAL_SUCCESS_MERGE_BASE is special code
+	 * that indicates special success.
+	 *	-> bisect_start()
+	 *	   . res = bisect_auto_next()
+	 *	    -> bisect_auto_next()
+	 *	       . return bisect_next()
+	 *	       -> bisect_next()
+	 *		  . res = bisect_next_all()
+	 *		  -> bisect_next_all()
+	 *		     . res = check_good_are_ancestors_of_bad()
+	 *		     -> check_good_are_ancestors_of_bad()
+	 *			. res = check_merge_bases()
+	 *			-> check_merge_bases()
+	 *			   . res = BISECT_INTERNAL_SUCCESS_MERGE_BASE
+	 */
+	if (res && res != BISECT_INTERNAL_SUCCESS_MERGE_BASE)
+		bisect_clean_state();
 	return res;
 }
 
diff --git a/git-bisect.sh b/git-bisect.sh
index e03f210d55..166f6a64dd 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -63,34 +63,13 @@ bisect_autostart() {
 			[Nn]*)
 				exit ;;
 			esac
-			bisect_start
+			git bisect--helper --bisect-start
 		else
 			exit 1
 		fi
 	}
 }
 
-bisect_start() {
-	git bisect--helper --bisect-start $@ || exit
-
-	#
-	# Change state.
-	# In case of mistaken revs or checkout error, or signals received,
-	# "bisect_auto_next" below may exit or misbehave.
-	# We have to trap this to be able to clean up using
-	# "bisect_clean_state".
-	#
-	trap 'git bisect--helper --bisect-clean-state' 0
-	trap 'exit 255' 1 2 3 15
-
-	#
-	# Check if we can proceed to the next bisect state.
-	#
-	git bisect--helper --bisect-auto-next || exit
-
-	trap '-' 0
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -183,8 +162,7 @@ bisect_replay () {
 		get_terms
 		case "$command" in
 		start)
-			cmd="bisect_start $rev"
-			eval "$cmd" ;;
+			eval "git bisect--helper --bisect-start $rev" ;;
 		"$TERM_GOOD"|"$TERM_BAD"|skip)
 			git bisect--helper --bisect-write "$command" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit;;
 		terms)
@@ -283,7 +261,7 @@ case "$#" in
 	help)
 		git bisect -h ;;
 	start)
-		bisect_start "$@" ;;
+		git bisect--helper --bisect-start "$@" ;;
 	bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD")
 		bisect_state "$cmd" "$@" ;;
 	skip)
-- 
2.25.0

[PATCH v2 05/11] bisect--helper: retire `--bisect-clean-state` subcommand

[Miriam Rubio]

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

The `--bisect-clean-state` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function
`bisect_clean_state()` is directly called from the C
implementation.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 84aac08ced..0534adf216 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -22,7 +22,6 @@ static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --next-all [--no-checkout]"),
 	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
-	N_("git bisect--helper --bisect-clean-state"),
 	N_("git bisect--helper --bisect-reset [<commit>]"),
 	N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
 	N_("git bisect--helper --bisect-check-and-set-terms <command> <good_term> <bad_term>"),
@@ -809,7 +808,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	enum {
 		NEXT_ALL = 1,
 		WRITE_TERMS,
-		BISECT_CLEAN_STATE,
 		CHECK_EXPECTED_REVS,
 		BISECT_RESET,
 		BISECT_WRITE,
@@ -826,8 +824,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_CMDMODE(0, "write-terms", &cmdmode,
 			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
-		OPT_CMDMODE(0, "bisect-clean-state", &cmdmode,
-			 N_("cleanup the bisection state"), BISECT_CLEAN_STATE),
 		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
 			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
 		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
@@ -869,10 +865,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 2)
 			return error(_("--write-terms requires two arguments"));
 		return write_terms(argv[0], argv[1]);
-	case BISECT_CLEAN_STATE:
-		if (argc != 0)
-			return error(_("--bisect-clean-state requires no arguments"));
-		return bisect_clean_state();
 	case CHECK_EXPECTED_REVS:
 		check_expected_revs(argv, argc);
 		return 0;
-- 
2.25.0

[PATCH v2 06/11] bisect--helper: retire `--next-all` subcommand

[Miriam Rubio]

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

The `--next-all` subcommand is no longer used from the git-bisect.sh
shell script. Instead the function `bisect_next_all()` is called from
the C implementation of `bisect_next()`.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 0534adf216..086ab8e46b 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
-	N_("git bisect--helper --next-all [--no-checkout]"),
 	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	N_("git bisect--helper --bisect-reset [<commit>]"),
 	N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
@@ -806,8 +805,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkou
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
-		NEXT_ALL = 1,
-		WRITE_TERMS,
+		WRITE_TERMS = 1,
 		CHECK_EXPECTED_REVS,
 		BISECT_RESET,
 		BISECT_WRITE,
@@ -820,8 +818,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
-		OPT_CMDMODE(0, "next-all", &cmdmode,
-			 N_("perform 'git bisect next'"), NEXT_ALL),
 		OPT_CMDMODE(0, "write-terms", &cmdmode,
 			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
@@ -858,9 +854,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_bisect_helper_usage, options);
 
 	switch (cmdmode) {
-	case NEXT_ALL:
-		res = bisect_next_all(the_repository, prefix, no_checkout);
-		break;
 	case WRITE_TERMS:
 		if (argc != 2)
 			return error(_("--write-terms requires two arguments"));
-- 
2.25.0

[PATCH v2 07/11] bisect--helper: reimplement `bisect_autostart` shell function in C

[Miriam Rubio]

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

Reimplement the `bisect_autostart()` shell function in C and add the
C implementation from `bisect_next()` which was previously left
uncovered. Also add a subcommand `--bisect-autostart` to
`git bisect--helper` be called from `bisect_state()` from
git-bisect.sh .

Using `--bisect-autostart` subcommand is a temporary measure to port
shell function to C so as to use the existing test suite. As more
functions are ported, this subcommand will be retired and
bisect_autostart() will be called directly by `bisect_state()`.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 39 +++++++++++++++++++++++++++++++++++++++
 git-bisect.sh            | 23 +----------------------
 2 files changed, 40 insertions(+), 22 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 086ab8e46b..4496fd2228 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -30,6 +30,7 @@ static const char * const git_bisect_helper_usage[] = {
 					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
 	N_("git bisect--helper --bisect-next"),
 	N_("git bisect--helper --bisect-auto-next"),
+	N_("git bisect--helper --bisect-autostart"),
 	NULL
 };
 
@@ -56,6 +57,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,
 static const char vocab_bad[] = "bad|new";
 static const char vocab_good[] = "good|old";
 
+static int bisect_autostart(struct bisect_terms *terms);
+
 /*
  * Check whether the string `term` belongs to the set of strings
  * included in the variable arguments.
@@ -554,6 +557,7 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
 	int no_checkout;
 	enum bisect_error res;
 
+	bisect_autostart(terms);
 	if (bisect_next_check(terms, terms->term_good))
 		return BISECT_FAILED;
 
@@ -802,6 +806,32 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, int no_checkou
 	return res;
 }
 
+static int bisect_autostart(struct bisect_terms *terms)
+{
+	const char *yesno;
+
+	if (!is_empty_or_missing_file(git_path_bisect_start()))
+		return 0;
+
+	fprintf(stderr, _("You need to start by \"git bisect "
+			  "start\"\n"));
+
+	if (!isatty(STDIN_FILENO))
+		return 1;
+
+	/*
+	 * TRANSLATORS: Make sure to include [Y] and [n] in your
+	 * translation. The program will only accept English input
+	 * at this point.
+	 */
+	yesno = git_prompt(_("Do you want me to do it for you "
+			     "[Y/n]? "), PROMPT_ECHO);
+	if (starts_with(yesno, _("n")) || starts_with(yesno, _("N")))
+		return 1;
+
+	return bisect_start(terms, 0, NULL, 0);
+}
+
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
@@ -815,6 +845,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_START,
 		BISECT_NEXT,
 		BISECT_AUTO_NEXT,
+		BISECT_AUTOSTART,
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
@@ -838,6 +869,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("find the next bisection commit"), BISECT_NEXT),
 		OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
 			 N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
+		OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
+			 N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
 			 N_("update BISECT_HEAD instead of checking out the current commit")),
 		OPT_BOOL(0, "no-log", &nolog,
@@ -904,6 +937,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		get_terms(&terms);
 		res = bisect_auto_next(&terms, prefix);
 		break;
+	case BISECT_AUTOSTART:
+		if (argc)
+			return error(_("--bisect-autostart requires 0 arguments"));
+		set_terms(&terms, "bad", "good");
+		res = bisect_autostart(&terms);
+		break;
 	default:
 		res = error(_("BUG: unknown subcommand."));
 	}
diff --git a/git-bisect.sh b/git-bisect.sh
index 166f6a64dd..049ffacdff 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -49,27 +49,6 @@ bisect_head()
 	fi
 }
 
-bisect_autostart() {
-	test -s "$GIT_DIR/BISECT_START" || {
-		gettextln "You need to start by \"git bisect start\"" >&2
-		if test -t 0
-		then
-			# TRANSLATORS: Make sure to include [Y] and [n] in your
-			# translation. The program will only accept English input
-			# at this point.
-			gettext "Do you want me to do it for you [Y/n]? " >&2
-			read yesno
-			case "$yesno" in
-			[Nn]*)
-				exit ;;
-			esac
-			git bisect--helper --bisect-start
-		else
-			exit 1
-		fi
-	}
-}
-
 bisect_skip() {
 	all=''
 	for arg in "$@"
@@ -86,7 +65,7 @@ bisect_skip() {
 }
 
 bisect_state() {
-	bisect_autostart
+	git bisect--helper --bisect-autostart
 	state=$1
 	git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit
 	get_terms
-- 
2.25.0

[PATCH v2 08/11] bisect--helper: reimplement `bisect_state` & `bisect_head` shell functions in C

[Miriam Rubio]

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

Reimplement the `bisect_state()` shell functions in C and also add a
subcommand `--bisect-state` to `git-bisect--helper` to call them from
git-bisect.sh .

Using `--bisect-state` subcommand is a temporary measure to port shell
function to C so as to use the existing test suite. As more functions
are ported, this subcommand will be retired and will be called by some
other methods.

`bisect_head()` is only called from `bisect_state()`, thus it is not
required to introduce another subcommand.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 68 ++++++++++++++++++++++++++++++++++++++++
 git-bisect.sh            | 55 +++-----------------------------
 2 files changed, 72 insertions(+), 51 deletions(-)

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

[PATCH v2 09/11] bisect--helper: retire `--check-expected-revs` subcommand

[Miriam Rubio]

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

The `--check-expected-revs` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function
`check_expected_revs()` is called from the C implementation of
`bisect-next()`.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 6364bd82c0..2b1d918497 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -896,7 +896,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
 		WRITE_TERMS = 1,
-		CHECK_EXPECTED_REVS,
 		BISECT_RESET,
 		BISECT_WRITE,
 		CHECK_AND_SET_TERMS,
@@ -912,8 +911,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	struct option options[] = {
 		OPT_CMDMODE(0, "write-terms", &cmdmode,
 			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
-		OPT_CMDMODE(0, "check-expected-revs", &cmdmode,
-			 N_("check for expected revs"), CHECK_EXPECTED_REVS),
 		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
 			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_CMDMODE(0, "bisect-write", &cmdmode,
@@ -954,9 +951,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		if (argc != 2)
 			return error(_("--write-terms requires two arguments"));
 		return write_terms(argv[0], argv[1]);
-	case CHECK_EXPECTED_REVS:
-		check_expected_revs(argv, argc);
-		return 0;
 	case BISECT_RESET:
 		if (argc > 1)
 			return error(_("--bisect-reset requires either no argument or a commit"));
-- 
2.25.0

[PATCH v2 10/11] bisect--helper: retire `--write-terms` subcommand

[Miriam Rubio]

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

The `--write-terms` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function `write_terms()`
is called from the C implementation of `set_terms()` and
`bisect_start()`.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 2b1d918497..8fdefb611b 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -20,7 +20,6 @@ static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 
 static const char * const git_bisect_helper_usage[] = {
-	N_("git bisect--helper --write-terms <bad_term> <good_term>"),
 	N_("git bisect--helper --bisect-reset [<commit>]"),
 	N_("git bisect--helper --bisect-write [--no-log] <state> <revision> <good_term> <bad_term>"),
 	N_("git bisect--helper --bisect-check-and-set-terms <command> <good_term> <bad_term>"),
@@ -895,8 +894,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 {
 	enum {
-		WRITE_TERMS = 1,
-		BISECT_RESET,
+		BISECT_RESET = 1,
 		BISECT_WRITE,
 		CHECK_AND_SET_TERMS,
 		BISECT_NEXT_CHECK,
@@ -909,8 +907,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
 	struct option options[] = {
-		OPT_CMDMODE(0, "write-terms", &cmdmode,
-			 N_("write the terms to .git/BISECT_TERMS"), WRITE_TERMS),
 		OPT_CMDMODE(0, "bisect-reset", &cmdmode,
 			 N_("reset the bisection state"), BISECT_RESET),
 		OPT_CMDMODE(0, "bisect-write", &cmdmode,
@@ -947,10 +943,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_bisect_helper_usage, options);
 
 	switch (cmdmode) {
-	case WRITE_TERMS:
-		if (argc != 2)
-			return error(_("--write-terms requires two arguments"));
-		return write_terms(argv[0], argv[1]);
 	case BISECT_RESET:
 		if (argc > 1)
 			return error(_("--bisect-reset requires either no argument or a commit"));
-- 
2.25.0

[PATCH v2 11/11] bisect--helper: retire `--bisect-autostart` subcommand

[Miriam Rubio]

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

The `--bisect-autostart` subcommand is no longer used from the
git-bisect.sh shell script. Instead the function
`bisect_autostart()` is directly called from the C implementation.

Mentored-by: Lars Schneider <larsxschneider@gmail.com>
Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 builtin/bisect--helper.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 8fdefb611b..d3ec926ae4 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -29,7 +29,6 @@ static const char * const git_bisect_helper_usage[] = {
 					     "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"),
 	N_("git bisect--helper --bisect-next"),
 	N_("git bisect--helper --bisect-auto-next"),
-	N_("git bisect--helper --bisect-autostart"),
 	N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
 	N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
 	NULL
@@ -902,7 +901,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		BISECT_START,
 		BISECT_NEXT,
 		BISECT_AUTO_NEXT,
-		BISECT_AUTOSTART,
 		BISECT_STATE
 	} cmdmode = 0;
 	int no_checkout = 0, res = 0, nolog = 0;
@@ -923,8 +921,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 			 N_("find the next bisection commit"), BISECT_NEXT),
 		OPT_CMDMODE(0, "bisect-auto-next", &cmdmode,
 			 N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT),
-		OPT_CMDMODE(0, "bisect-autostart", &cmdmode,
-			 N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART),
 		OPT_CMDMODE(0, "bisect-state", &cmdmode,
 			 N_("mark the state of ref (or refs)"), BISECT_STATE),
 		OPT_BOOL(0, "no-checkout", &no_checkout,
@@ -986,12 +982,6 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		get_terms(&terms);
 		res = bisect_auto_next(&terms, prefix);
 		break;
-	case BISECT_AUTOSTART:
-		if (argc)
-			return error(_("--bisect-autostart requires 0 arguments"));
-		set_terms(&terms, "bad", "good");
-		res = bisect_autostart(&terms);
-		break;
 	case BISECT_STATE:
 		set_terms(&terms, "bad", "good");
 		get_terms(&terms);
-- 
2.25.0