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

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

[Miriam Rubio]

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

This first part are formed of preparatory/clean-up patches and all 
`bisect.c` libification work. 

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

It has to be noted that in this version 2 nothing has been done about a 
reviewer suggestion of using enums for error codes, because there was no
consensus about using them by the reviewers.

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

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

* Previous patch series version has been split in smaller groups 
in order to facilitate revision and integration.
* Rebase on master branch: c7a6207591 (Sync with maint, 2020-01-27).
* Improve commit messages.

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

[6/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` 
and its dependents
    
* Remove redundant sentences in commit message.
* Use `if (res < 0)` instead of `if (res)`.

[8/11] bisect: libify `check_merge_bases` and its dependents

* Remove redundant sentence in commit message.

--

[9/11] bisect: libify `check_good_are_ancestors_of_bad` and its 
dependents

* Remove redundant sentences in commit message.
* Return in `if (!current_bad_oid)` condition.

--

[10/11] bisect: libify `handle_bad_merge_base` and its dependents

* Remove redundant sentence in commit message.

--

[11/11] bisect: libify `bisect_next_all`

* Remove redundant sentence in commit message.
* Add return codes explanations in `bisect.h`.

--

Miriam Rubio (2):
  bisect--helper: convert `vocab_*` char pointers to char arrays
  bisect: use the standard 'if (!var)' way to check for 0

Pranit Bauva (7):
  run-command: make `exists_in_PATH()` non-static
  bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and
    its dependents
  bisect: libify `bisect_checkout`
  bisect: libify `check_merge_bases` and its dependents
  bisect: libify `check_good_are_ancestors_of_bad` and its dependents
  bisect: libify `handle_bad_merge_base` and its dependents
  bisect: libify `bisect_next_all`

Tanushree Tumane (2):
  bisect--helper: change `retval` to `res`
  bisect--helper: introduce new `decide_next()` function

 bisect.c                 | 136 +++++++++++++++++++++++++++------------
 bisect.h                 |  23 +++++++
 builtin/bisect--helper.c | 118 +++++++++++++++++----------------
 run-command.c            |   2 +-
 run-command.h            |  11 ++++
 5 files changed, 193 insertions(+), 97 deletions(-)

-- 
2.21.1 (Apple Git-122.3)

[PATCH v2 01/11] bisect--helper: convert `vocab_*` char pointers to char arrays

[Miriam Rubio]

Instead of using a pointer that points at a constant string,
just give name directly to the constant string; this way, we
do not have to allocate a pointer variable in addition to
the string we want to use.

Let's convert `vocab_bad` and `vocab_good` char pointers to char arrays.

Mentored-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Miriam Rubio <mirucam@gmail.com>
---
 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 1718df7f09..36c09b7238 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -52,8 +52,8 @@ static void set_terms(struct bisect_terms *terms, const char *bad,
 	terms->term_bad = xstrdup(bad);
 }
 
-static const char *vocab_bad = "bad|new";
-static const char *vocab_good = "good|old";
+static const char vocab_bad[] = "bad|new";
+static const char vocab_good[] = "good|old";
 
 /*
  * Check whether the string `term` belongs to the set of strings
-- 
2.21.1 (Apple Git-122.3)

[PATCH v2 02/11] bisect--helper: change `retval` to `res`

[Miriam Rubio]

From: Tanushree Tumane <tanushreetumane@gmail.com>

Let's rename variable retval to res, so that variable names
in bisect--helper.c are more consistent.

After this change, there are 110 occurrences of res in the file
and zero of retval, while there were 26 instances of retval before.

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

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 36c09b7238..21de5c096c 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -206,31 +206,31 @@ static int bisect_write(const char *state, const char *rev,
 	struct object_id oid;
 	struct commit *commit;
 	FILE *fp = NULL;
-	int retval = 0;
+	int res = 0;
 
 	if (!strcmp(state, terms->term_bad)) {
 		strbuf_addf(&tag, "refs/bisect/%s", state);
 	} else if (one_of(state, terms->term_good, "skip", NULL)) {
 		strbuf_addf(&tag, "refs/bisect/%s-%s", state, rev);
 	} else {
-		retval = error(_("Bad bisect_write argument: %s"), state);
+		res = error(_("Bad bisect_write argument: %s"), state);
 		goto finish;
 	}
 
 	if (get_oid(rev, &oid)) {
-		retval = error(_("couldn't get the oid of the rev '%s'"), rev);
+		res = error(_("couldn't get the oid of the rev '%s'"), rev);
 		goto finish;
 	}
 
 	if (update_ref(NULL, tag.buf, &oid, NULL, 0,
 		       UPDATE_REFS_MSG_ON_ERR)) {
-		retval = -1;
+		res = -1;
 		goto finish;
 	}
 
 	fp = fopen(git_path_bisect_log(), "a");
 	if (!fp) {
-		retval = error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
+		res = error_errno(_("couldn't open the file '%s'"), git_path_bisect_log());
 		goto finish;
 	}
 
@@ -244,7 +244,7 @@ static int bisect_write(const char *state, const char *rev,
 	if (fp)
 		fclose(fp);
 	strbuf_release(&tag);
-	return retval;
+	return res;
 }
 
 static int check_and_set_terms(struct bisect_terms *terms, const char *cmd)
@@ -294,7 +294,7 @@ static const char need_bisect_start_warning[] =
 static int bisect_next_check(const struct bisect_terms *terms,
 			     const char *current_term)
 {
-	int missing_good = 1, missing_bad = 1, retval = 0;
+	int missing_good = 1, missing_bad = 1, res = 0;
 	const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
 	const char *good_glob = xstrfmt("%s-*", terms->term_good);
 
@@ -308,7 +308,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
 		goto finish;
 
 	if (!current_term) {
-		retval = -1;
+		res = -1;
 		goto finish;
 	}
 
@@ -329,21 +329,21 @@ static int bisect_next_check(const struct bisect_terms *terms,
 		 */
 		yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
 		if (starts_with(yesno, "N") || starts_with(yesno, "n"))
-			retval = -1;
+			res = -1;
 		goto finish;
 	}
 	if (!is_empty_or_missing_file(git_path_bisect_start())) {
-		retval = error(_(need_bad_and_good_revision_warning),
+		res = error(_(need_bad_and_good_revision_warning),
 			       vocab_bad, vocab_good, vocab_bad, vocab_good);
 	} else {
-		retval = error(_(need_bisect_start_warning),
+		res = error(_(need_bisect_start_warning),
 			       vocab_good, vocab_bad, vocab_good, vocab_bad);
 	}
 
 finish:
 	free((void *) good_glob);
 	free((void *) bad_ref);
-	return retval;
+	return res;
 }
 
 static int get_terms(struct bisect_terms *terms)
@@ -397,7 +397,7 @@ static int bisect_terms(struct bisect_terms *terms, const char *option)
 
 static int bisect_append_log_quoted(const char **argv)
 {
-	int retval = 0;
+	int res = 0;
 	FILE *fp = fopen(git_path_bisect_log(), "a");
 	struct strbuf orig_args = STRBUF_INIT;
 
@@ -405,25 +405,25 @@ static int bisect_append_log_quoted(const char **argv)
 		return -1;
 
 	if (fprintf(fp, "git bisect start") < 1) {
-		retval = -1;
+		res = -1;
 		goto finish;
 	}
 
 	sq_quote_argv(&orig_args, argv);
 	if (fprintf(fp, "%s\n", orig_args.buf) < 1)
-		retval = -1;
+		res = -1;
 
 finish:
 	fclose(fp);
 	strbuf_release(&orig_args);
-	return retval;
+	return res;
 }
 
 static int 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, retval = 0;
+	int flags, pathspec_pos, res = 0;
 	struct string_list revs = STRING_LIST_INIT_DUP;
 	struct string_list states = STRING_LIST_INIT_DUP;
 	struct strbuf start_head = STRBUF_INIT;
@@ -524,7 +524,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 			argv_array_pushl(&argv, "checkout", start_head.buf,
 					 "--", NULL);
 			if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
-				retval = error(_("checking out '%s' failed."
+				res = error(_("checking out '%s' failed."
 						 " Try 'git bisect start "
 						 "<valid-branch>'."),
 					       start_head.buf);
@@ -572,12 +572,12 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 
 	if (no_checkout) {
 		if (get_oid(start_head.buf, &oid) < 0) {
-			retval = error(_("invalid ref: '%s'"), start_head.buf);
+			res = error(_("invalid ref: '%s'"), start_head.buf);
 			goto finish;
 		}
 		if (update_ref(NULL, "BISECT_HEAD", &oid, NULL, 0,
 			       UPDATE_REFS_MSG_ON_ERR)) {
-			retval = -1;
+			res = -1;
 			goto finish;
 		}
 	}
@@ -589,26 +589,26 @@ 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)) {
-			retval = -1;
+			res = -1;
 			goto finish;
 		}
 
 	if (must_write_terms && write_terms(terms->term_bad,
 					    terms->term_good)) {
-		retval = -1;
+		res = -1;
 		goto finish;
 	}
 
-	retval = bisect_append_log_quoted(argv);
-	if (retval)
-		retval = -1;
+	res = bisect_append_log_quoted(argv);
+	if (res)
+		res = -1;
 
 finish:
 	string_list_clear(&revs, 0);
 	string_list_clear(&states, 0);
 	strbuf_release(&start_head);
 	strbuf_release(&bisect_names);
-	return retval;
+	return res;
 }
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
-- 
2.21.1 (Apple Git-122.3)

[PATCH v2 03/11] bisect: use the standard 'if (!var)' way to check for 0

[Miriam Rubio]

Instead of using 'var == 0' in an if condition, let's use '!var' and
make 'bisect.c' more consistent with the rest of the code.

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

diff --git a/bisect.c b/bisect.c
index e81c91d02c..83cb5b3a98 100644
--- a/bisect.c
+++ b/bisect.c
@@ -572,7 +572,7 @@ static int sqrti(int val)
 {
 	float d, x = val;
 
-	if (val == 0)
+	if (!val)
 		return 0;
 
 	do {
@@ -869,7 +869,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
 		goto done;
 
 	/* Bisecting with no good rev is ok. */
-	if (good_revs.nr == 0)
+	if (!good_revs.nr)
 		goto done;
 
 	/* Check if all good revs are ancestor of the bad rev. */
-- 
2.21.1 (Apple Git-122.3)

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

[Miriam Rubio]

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

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

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

diff --git a/run-command.c b/run-command.c
index f5e1149f9b..4df975178d 100644
--- a/run-command.c
+++ b/run-command.c
@@ -210,7 +210,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 592d9dc035..7c8e206d97 100644
--- a/run-command.h
+++ b/run-command.h
@@ -172,6 +172,17 @@ 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 *);
+
 /**
  * Start a sub-process. Takes a pointer to a `struct child_process`
  * that specifies the details and returns pipe FDs (if requested).
-- 
2.21.1 (Apple Git-122.3)

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

[Johannes Schindelin]

Hi Miriam,

On Tue, 28 Jan 2020, Miriam Rubio 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.

I inspected the code in `exists_in_PATH()` and in `locate_in_PATH()` and
it looks as if neither of them depended on file-local variables (which
would otherwise need to be addressed when exporting the function).

If you contribute another iteration of this patch series, it might make
sense to mention this in the commit message explicitly.

Thanks,
Dscho

>
> 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 | 11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/run-command.c b/run-command.c
> index f5e1149f9b..4df975178d 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -210,7 +210,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 592d9dc035..7c8e206d97 100644
> --- a/run-command.h
> +++ b/run-command.h
> @@ -172,6 +172,17 @@ 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 *);
> +
>  /**
>   * Start a sub-process. Takes a pointer to a `struct child_process`
>   * that specifies the details and returns pipe FDs (if requested).
> --
> 2.21.1 (Apple Git-122.3)
>
>

Fwd: [PATCH v2 04/11] run-command: make `exists_in_PATH()` non-static

[Miriam R.]

Hi,

El jue., 30 ene. 2020 a las 13:36, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Tue, 28 Jan 2020, Miriam Rubio 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.
>
> I inspected the code in `exists_in_PATH()` and in `locate_in_PATH()` and
> it looks as if neither of them depended on file-local variables (which
> would otherwise need to be addressed when exporting the function).
>
> If you contribute another iteration of this patch series, it might make
> sense to mention this in the commit message explicitly.

Ok, I will add the comment in the commit message.
Thank you.

Best,
Miriam

>
> Thanks,
> Dscho
>
> >
> > 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 | 11 +++++++++++
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/run-command.c b/run-command.c
> > index f5e1149f9b..4df975178d 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -210,7 +210,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 592d9dc035..7c8e206d97 100644
> > --- a/run-command.h
> > +++ b/run-command.h
> > @@ -172,6 +172,17 @@ 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 *);
> > +
> >  /**
> >   * Start a sub-process. Takes a pointer to a `struct child_process`
> >   * that specifies the details and returns pipe FDs (if requested).
> > --
> > 2.21.1 (Apple Git-122.3)
> >
> >

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

[Miriam Rubio]

From: Tanushree Tumane <tanushreetumane@gmail.com>

Let's refactor code from bisect_next_check() into a new
decide_next() helper function.

This removes some goto statements and makes the code simpler,
clearer and easier to understand.

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

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 21de5c096c..826fcba2ed 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -291,26 +291,14 @@ static const char need_bisect_start_warning[] =
 	   "You then need to give me at least one %s and %s revision.\n"
 	   "You can use \"git bisect %s\" and \"git bisect %s\" for that.");
 
-static int bisect_next_check(const struct bisect_terms *terms,
-			     const char *current_term)
+static int decide_next(const struct bisect_terms *terms,
+		       const char *current_term, int missing_good,
+		       int missing_bad)
 {
-	int missing_good = 1, missing_bad = 1, res = 0;
-	const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
-	const char *good_glob = xstrfmt("%s-*", terms->term_good);
-
-	if (ref_exists(bad_ref))
-		missing_bad = 0;
-
-	for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
-			     (void *) &missing_good);
-
 	if (!missing_good && !missing_bad)
-		goto finish;
-
-	if (!current_term) {
-		res = -1;
-		goto finish;
-	}
+		return 0;
+	if (!current_term)
+		return -1;
 
 	if (missing_good && !missing_bad &&
 	    !strcmp(current_term, terms->term_good)) {
@@ -321,7 +309,7 @@ static int bisect_next_check(const struct bisect_terms *terms,
 		 */
 		warning(_("bisecting only with a %s commit"), terms->term_bad);
 		if (!isatty(0))
-			goto finish;
+			return 0;
 		/*
 		 * TRANSLATORS: Make sure to include [Y] and [n] in your
 		 * translation. The program will only accept English input
@@ -329,21 +317,35 @@ static int bisect_next_check(const struct bisect_terms *terms,
 		 */
 		yesno = git_prompt(_("Are you sure [Y/n]? "), PROMPT_ECHO);
 		if (starts_with(yesno, "N") || starts_with(yesno, "n"))
-			res = -1;
-		goto finish;
-	}
-	if (!is_empty_or_missing_file(git_path_bisect_start())) {
-		res = error(_(need_bad_and_good_revision_warning),
-			       vocab_bad, vocab_good, vocab_bad, vocab_good);
-	} else {
-		res = error(_(need_bisect_start_warning),
-			       vocab_good, vocab_bad, vocab_good, vocab_bad);
+			return -1;
+		return 0;
 	}
 
-finish:
+	if (!is_empty_or_missing_file(git_path_bisect_start()))
+		return error(_(need_bad_and_good_revision_warning),
+			     vocab_bad, vocab_good, vocab_bad, vocab_good);
+	else
+		return error(_(need_bisect_start_warning),
+			     vocab_good, vocab_bad, vocab_good, vocab_bad);
+}
+
+static int bisect_next_check(const struct bisect_terms *terms,
+			     const char *current_term)
+{
+	int missing_good = 1, missing_bad = 1;
+	const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
+	const char *good_glob = xstrfmt("%s-*", terms->term_good);
+
+	if (ref_exists(bad_ref))
+		missing_bad = 0;
+
+	for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
+			     (void *) &missing_good);
+
 	free((void *) good_glob);
 	free((void *) bad_ref);
-	return res;
+
+	return decide_next(terms, current_term, missing_good, missing_bad);
 }
 
 static int get_terms(struct bisect_terms *terms)
-- 
2.21.1 (Apple Git-122.3)

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

[Johannes Schindelin]

Hi Miriam,

On Tue, 28 Jan 2020, Miriam Rubio wrote:

> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 21de5c096c..826fcba2ed 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -291,26 +291,14 @@ static const char need_bisect_start_warning[] =
>  	   "You then need to give me at least one %s and %s revision.\n"
>  	   "You can use \"git bisect %s\" and \"git bisect %s\" for that.");
>
> -static int bisect_next_check(const struct bisect_terms *terms,
> -			     const char *current_term)
> +static int decide_next(const struct bisect_terms *terms,
> +		       const char *current_term, int missing_good,
> +		       int missing_bad)
>  {
> -	int missing_good = 1, missing_bad = 1, res = 0;
> -	const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> -	const char *good_glob = xstrfmt("%s-*", terms->term_good);
> -
> -	if (ref_exists(bad_ref))
> -		missing_bad = 0;
> -
> -	for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> -			     (void *) &missing_good);
> -
>  	if (!missing_good && !missing_bad)
> -		goto finish;
> -
> -	if (!current_term) {
> -		res = -1;
> -		goto finish;
> -	}
> +		return 0;
> +	if (!current_term)
> +		return -1;
>  [...]
> +
> +static int bisect_next_check(const struct bisect_terms *terms,
> +			     const char *current_term)
> +{
> +	int missing_good = 1, missing_bad = 1;
> +	const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> +	const char *good_glob = xstrfmt("%s-*", terms->term_good);
> +
> +	if (ref_exists(bad_ref))
> +		missing_bad = 0;
> +
> +	for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> +			     (void *) &missing_good);
> +
>  	free((void *) good_glob);
>  	free((void *) bad_ref);

I know this is not something you introduced, but while you are already in
the neighborhood, why not fix the types of `bad_ref` and `good_glob`? The
`xstrfmt()` function returns `char *` for a reason: so that you do not
have to cast it when `free()`ing the memory.

Thanks,
Dscho

> -	return res;
> +
> +	return decide_next(terms, current_term, missing_good, missing_bad);
>  }
>
>  static int get_terms(struct bisect_terms *terms)
> --
> 2.21.1 (Apple Git-122.3)
>
>

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

[Miriam R.]

Hi,

El jue., 30 ene. 2020 a las 13:31, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Tue, 28 Jan 2020, Miriam Rubio wrote:
>
> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > index 21de5c096c..826fcba2ed 100644
> > --- a/builtin/bisect--helper.c
> > +++ b/builtin/bisect--helper.c
> > @@ -291,26 +291,14 @@ static const char need_bisect_start_warning[] =
> >          "You then need to give me at least one %s and %s revision.\n"
> >          "You can use \"git bisect %s\" and \"git bisect %s\" for that.");
> >
> > -static int bisect_next_check(const struct bisect_terms *terms,
> > -                          const char *current_term)
> > +static int decide_next(const struct bisect_terms *terms,
> > +                    const char *current_term, int missing_good,
> > +                    int missing_bad)
> >  {
> > -     int missing_good = 1, missing_bad = 1, res = 0;
> > -     const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> > -     const char *good_glob = xstrfmt("%s-*", terms->term_good);
> > -
> > -     if (ref_exists(bad_ref))
> > -             missing_bad = 0;
> > -
> > -     for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> > -                          (void *) &missing_good);
> > -
> >       if (!missing_good && !missing_bad)
> > -             goto finish;
> > -
> > -     if (!current_term) {
> > -             res = -1;
> > -             goto finish;
> > -     }
> > +             return 0;
> > +     if (!current_term)
> > +             return -1;
> >  [...]
> > +
> > +static int bisect_next_check(const struct bisect_terms *terms,
> > +                          const char *current_term)
> > +{
> > +     int missing_good = 1, missing_bad = 1;
> > +     const char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> > +     const char *good_glob = xstrfmt("%s-*", terms->term_good);
> > +
> > +     if (ref_exists(bad_ref))
> > +             missing_bad = 0;
> > +
> > +     for_each_glob_ref_in(mark_good, good_glob, "refs/bisect/",
> > +                          (void *) &missing_good);
> > +
> >       free((void *) good_glob);
> >       free((void *) bad_ref);
>
> I know this is not something you introduced, but while you are already in
> the neighborhood, why not fix the types of `bad_ref` and `good_glob`? The
> `xstrfmt()` function returns `char *` for a reason: so that you do not
> have to cast it when `free()`ing the memory.

Sure! I will fix this.
Thank you for reviewing.
Best,
Miriam

>
> Thanks,
> Dscho
>
> > -     return res;
> > +
> > +     return decide_next(terms, current_term, missing_good, missing_bad);
> >  }
> >
> >  static int get_terms(struct bisect_terms *terms)
> > --
> > 2.21.1 (Apple Git-122.3)
> >
> >

[PATCH v2 06/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents

[Miriam Rubio]

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

Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.

Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.

Handle this return in dependent function `bisect_next_all()`.

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

diff --git a/bisect.c b/bisect.c
index 83cb5b3a98..a7a5d158e6 100644
--- a/bisect.c
+++ b/bisect.c
@@ -661,11 +661,11 @@ static void bisect_common(struct rev_info *revs)
 		mark_edges_uninteresting(revs, NULL, 0);
 }
 
-static void exit_if_skipped_commits(struct commit_list *tried,
+static int error_if_skipped_commits(struct commit_list *tried,
 				    const struct object_id *bad)
 {
 	if (!tried)
-		return;
+		return 0;
 
 	printf("There are only 'skip'ped commits left to test.\n"
 	       "The first %s commit could be any of:\n", term_bad);
@@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,
 	if (bad)
 		printf("%s\n", oid_to_hex(bad));
 	printf(_("We cannot bisect more!\n"));
-	exit(2);
+
+	/*
+	 * We don't want to clean the bisection state
+	 * as we need to get back to where we started
+	 * by using `git bisect reset`.
+	 */
+	return -2;
 }
 
 static int is_expected_rev(const struct object_id *oid)
@@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
 {
 	struct rev_info revs;
 	struct commit_list *tried;
-	int reaches = 0, all = 0, nr, steps;
+	int reaches = 0, all = 0, nr, steps, res;
 	struct object_id *bisect_rev;
 	char *steps_msg;
 
@@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
 		 * We should exit here only if the "bad"
 		 * commit is also a "skip" commit.
 		 */
-		exit_if_skipped_commits(tried, NULL);
-
+		res = error_if_skipped_commits(tried, NULL);
+		if (res < 0)
+			exit(-res);
 		printf(_("%s was both %s and %s\n"),
 		       oid_to_hex(current_bad_oid),
 		       term_good,
@@ -990,7 +997,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
 	bisect_rev = &revs.commits->item->object.oid;
 
 	if (oideq(bisect_rev, current_bad_oid)) {
-		exit_if_skipped_commits(tried, current_bad_oid);
+		res = error_if_skipped_commits(tried, current_bad_oid);
+		if (res)
+			exit(-res);
 		printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
 			term_bad);
 		show_diff_tree(r, prefix, revs.commits->item);
-- 
2.21.1 (Apple Git-122.3)

Re: [PATCH v2 06/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Since we want to get rid of git-bisect.sh it would be necessary to

Please add back the missing comma after ".sh" for readability.

> convert those exit() calls to return statements so that errors can be
> reported.
>
> Emulate try catch in C by converting `exit(<positive-value>)` to
> `return <negative-value>`. Follow POSIX conventions to return
> <negative-value> to indicate error.
>
> Handle this return in dependent function `bisect_next_all()`.

It makes it sound as if there were multiple callers and this change
converted only one of them---"Update all callers to handle the error
returns" would avoid such a misunderstanding.

> diff --git a/bisect.c b/bisect.c
> index 83cb5b3a98..a7a5d158e6 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -661,11 +661,11 @@ static void bisect_common(struct rev_info *revs)
>  		mark_edges_uninteresting(revs, NULL, 0);
>  }
>  
> -static void exit_if_skipped_commits(struct commit_list *tried,
> +static int error_if_skipped_commits(struct commit_list *tried,
>  				    const struct object_id *bad)
>  {
>  	if (!tried)
> -		return;
> +		return 0;
>  
>  	printf("There are only 'skip'ped commits left to test.\n"
>  	       "The first %s commit could be any of:\n", term_bad);
> @@ -676,7 +676,13 @@ static void exit_if_skipped_commits(struct commit_list *tried,
>  	if (bad)
>  		printf("%s\n", oid_to_hex(bad));
>  	printf(_("We cannot bisect more!\n"));
> -	exit(2);
> +
> +	/*
> +	 * We don't want to clean the bisection state
> +	 * as we need to get back to where we started
> +	 * by using `git bisect reset`.
> +	 */

What this comment says may be true, but does it belong here?  After
returning, the caller will exit(2) without cleaning anyway with or
without this patch, so I am a bit puzzled about the comment.

> +	return -2;
>  }
>  
>  static int is_expected_rev(const struct object_id *oid)
> @@ -949,7 +955,7 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
>  {
>  	struct rev_info revs;
>  	struct commit_list *tried;
> -	int reaches = 0, all = 0, nr, steps;
> +	int reaches = 0, all = 0, nr, steps, res;
>  	struct object_id *bisect_rev;
>  	char *steps_msg;
>  
> @@ -972,8 +978,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
>  		 * We should exit here only if the "bad"
>  		 * commit is also a "skip" commit.
>  		 */
> -		exit_if_skipped_commits(tried, NULL);
> -
> +		res = error_if_skipped_commits(tried, NULL);
> +		if (res < 0)
> +			exit(-res);
>  		printf(_("%s was both %s and %s\n"),
>  		       oid_to_hex(current_bad_oid),
>  		       term_good,
> @@ -990,7 +997,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
>  	bisect_rev = &revs.commits->item->object.oid;
>  
>  	if (oideq(bisect_rev, current_bad_oid)) {
> -		exit_if_skipped_commits(tried, current_bad_oid);
> +		res = error_if_skipped_commits(tried, current_bad_oid);
> +		if (res)
> +			exit(-res);
>  		printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
>  			term_bad);
>  		show_diff_tree(r, prefix, revs.commits->item);

[PATCH v2 07/11] bisect: libify `bisect_checkout`

[Miriam Rubio]

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

Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.

Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.

Turn `exit()` to `return` calls in `bisect_checkout()`.
Changes related to return values have no bad side effects on the
code that calls `bisect_checkout()`.

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

diff --git a/bisect.c b/bisect.c
index a7a5d158e6..dee8318d9b 100644
--- a/bisect.c
+++ b/bisect.c
@@ -713,6 +713,7 @@ static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)
 {
 	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
 
+	int res = 0;
 	memcpy(bisect_rev_hex, oid_to_hex(bisect_rev), the_hash_algo->hexsz + 1);
 	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
 
@@ -721,14 +722,14 @@ static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)
 		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
 			   UPDATE_REFS_DIE_ON_ERR);
 	} else {
-		int res;
 		res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
 		if (res)
-			exit(res);
+			return res > 0 ? -res : res;
 	}
 
 	argv_show_branch[1] = bisect_rev_hex;
-	return run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
+	res = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
+	return res > 0 ? -res : res;
 }
 
 static struct commit *get_commit_reference(struct repository *r,
-- 
2.21.1 (Apple Git-122.3)

Re: [PATCH v2 07/11] bisect: libify `bisect_checkout`

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> From: Pranit Bauva <pranit.bauva@gmail.com>
>
> Since we want to get rid of git-bisect.sh it would be necessary to
> convert those exit() calls to return statements so that errors can be
> reported.
>
> Emulate try catch in C by converting `exit(<positive-value>)` to
> `return <negative-value>`. Follow POSIX conventions to return
> <negative-value> to indicate error.
>
> Turn `exit()` to `return` calls in `bisect_checkout()`.
> Changes related to return values have no bad side effects on the
> code that calls `bisect_checkout()`.
>
> 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 | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/bisect.c b/bisect.c
> index a7a5d158e6..dee8318d9b 100644
> --- a/bisect.c
> +++ b/bisect.c
> @@ -713,6 +713,7 @@ static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)
>  {
>  	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
>  
> +	int res = 0;
>  	memcpy(bisect_rev_hex, oid_to_hex(bisect_rev), the_hash_algo->hexsz + 1);

Wrong placement of a new decl.  Have a block of decls and then have
a blank line before the first statement, i.e.

		char bisect_rev_hex[...];
	+	int res = 0;

		memcpy(...);

This comment probably applies to other hunks in the entire series.

> @@ -721,14 +722,14 @@ static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout)
>  		update_ref(NULL, "BISECT_HEAD", bisect_rev, NULL, 0,
>  			   UPDATE_REFS_DIE_ON_ERR);
>  	} else {
> -		int res;
>  		res = run_command_v_opt(argv_checkout, RUN_GIT_CMD);
>  		if (res)
> -			exit(res);
> +			return res > 0 ? -res : res;

Hmph.  

This means that res == -1 and res == 1 from run_command_v_opt()
cannot be distinguished by our callers.  Is that what we want here?

If that is really what we want, it probably is easier to read if
this were written like so:

		return -abs(res);

>  	argv_show_branch[1] = bisect_rev_hex;
> -	return run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
> +	res = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
> +	return res > 0 ? -res : res;

Likewise.

[PATCH v2 08/11] bisect: libify `check_merge_bases` and its dependents

[Miriam Rubio]

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

Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.

Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.

In `check_merge_bases()` there is an early success special case,
so we have introduced special error code `-11` which indicates early
success. This `-11` is converted back to `0` in `check_good_are_ancestors_of_bad()`.

Handle the return value in dependent function `check_good_are_ancestors_of_bad()`.

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 | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/bisect.c b/bisect.c
index dee8318d9b..2a6566d066 100644
--- a/bisect.c
+++ b/bisect.c
@@ -806,13 +806,16 @@ static void handle_skipped_merge_base(const struct object_id *mb)
  * "check_merge_bases" checks that merge bases are not "bad" (or "new").
  *
  * - If one is "bad" (or "new"), it means the user assumed something wrong
- * and we must exit with a non 0 error code.
+ * and we must return error with a non 0 error code.
  * - If one is "good" (or "old"), that's good, we have nothing to do.
  * - If one is "skipped", we can't know but we should warn.
  * - If we don't know, we should check it out and ask the user to test.
+ * - If a merge base must be tested, on success return -11 a special condition
+ * for early success, this will be converted back to 0 in check_good_are_ancestors_of_bad().
  */
-static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
+static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
 {
+	int res = 0;
 	struct commit_list *result;
 
 	result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1);
@@ -827,11 +830,16 @@ static void check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
 			handle_skipped_merge_base(mb);
 		} else {
 			printf(_("Bisecting: a merge base must be tested\n"));
-			exit(bisect_checkout(mb, no_checkout));
+			res = bisect_checkout(mb, no_checkout);
+			if (!res)
+				/* indicate early success */
+				res = -11;
+			break;
 		}
 	}
 
 	free_commit_list(result);
+	return res;
 }
 
 static int check_ancestors(struct repository *r, int rev_nr,
@@ -865,7 +873,7 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
 {
 	char *filename = git_pathdup("BISECT_ANCESTORS_OK");
 	struct stat st;
-	int fd, rev_nr;
+	int fd, rev_nr, res = 0;
 	struct commit **rev;
 
 	if (!current_bad_oid)
@@ -880,10 +888,13 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
 		goto done;
 
 	/* Check if all good revs are ancestor of the bad rev. */
+
 	rev = get_bad_and_good_commits(r, &rev_nr);
 	if (check_ancestors(r, rev_nr, rev, prefix))
-		check_merge_bases(rev_nr, rev, no_checkout);
+		res = check_merge_bases(rev_nr, rev, no_checkout);
 	free(rev);
+	if (res)
+		exit(res == -11 ? 0 : -res);
 
 	/* Create file BISECT_ANCESTORS_OK. */
 	fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
-- 
2.21.1 (Apple Git-122.3)

Re: [PATCH v2 08/11] bisect: libify `check_merge_bases` and its dependents

[Junio C Hamano]

Miriam Rubio <mirucam@gmail.com> writes:

> + * - If a merge base must be tested, on success return -11 a special condition

Is this eleven something the original (scripted version) used, or
what this series invented?  If the latter, what was the reason the
value was chosen, and what were the constraints when choosing the
value (e.g. it cannot be used an exit code by something)?

In any case, these values (if there other special codes defined)
should be given symbolic names (either #define or enum).

[PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents

[Miriam Rubio]

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

Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.

Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.

Code that turns -11(early success code) to 0 from
`check_good_are_ancestors_of_bad()` has been moved to
`cmd_bisect__helper()`.

Handle the return value in dependent function `bisect_next_all()`.

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                 | 40 ++++++++++++++++++++++++++--------------
 builtin/bisect--helper.c | 12 ++++++++++--
 2 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/bisect.c b/bisect.c
index 2a6566d066..d519e10827 100644
--- a/bisect.c
+++ b/bisect.c
@@ -865,19 +865,27 @@ static int check_ancestors(struct repository *r, int rev_nr,
  *
  * If that's not the case, we need to check the merge bases.
  * If a merge base must be tested by the user, its source code will be
- * checked out to be tested by the user and we will exit.
+ * checked out to be tested by the user and we will return.
  */
-static void check_good_are_ancestors_of_bad(struct repository *r,
+
+static int check_good_are_ancestors_of_bad(struct repository *r,
 					    const char *prefix,
 					    int no_checkout)
 {
-	char *filename = git_pathdup("BISECT_ANCESTORS_OK");
+	char *filename;
 	struct stat st;
 	int fd, rev_nr, res = 0;
 	struct commit **rev;
 
+	/*
+	 * We don't want to clean the bisection state
+	 * as we need to get back to where we started
+	 * by using `git bisect reset`.
+	 */
 	if (!current_bad_oid)
-		die(_("a %s revision is needed"), term_bad);
+		return error(_("a %s revision is needed"), term_bad);
+
+	filename = git_pathdup("BISECT_ANCESTORS_OK");
 
 	/* Check if file BISECT_ANCESTORS_OK exists. */
 	if (!stat(filename, &st) && S_ISREG(st.st_mode))
@@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
 	if (check_ancestors(r, rev_nr, rev, prefix))
 		res = check_merge_bases(rev_nr, rev, no_checkout);
 	free(rev);
-	if (res)
-		exit(res == -11 ? 0 : -res);
 
-	/* Create file BISECT_ANCESTORS_OK. */
-	fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
-	if (fd < 0)
-		warning_errno(_("could not create file '%s'"),
-			      filename);
-	else
-		close(fd);
+	if (!res)
+	{
+		/* Create file BISECT_ANCESTORS_OK. */
+		fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+		if (fd < 0)
+			warning_errno(_("could not create file '%s'"),
+				      filename);
+		else
+			close(fd);
+	}
  done:
 	free(filename);
+	return res;
 }
 
 /*
@@ -975,7 +985,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
 	if (read_bisect_refs())
 		die(_("reading bisect refs failed"));
 
-	check_good_are_ancestors_of_bad(r, prefix, no_checkout);
+	res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);
+	if (res)
+		return res;
 
 	bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
 	revs.limited = 1;
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 826fcba2ed..3442bfe2cb 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -666,7 +666,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 
 	switch (cmdmode) {
 	case NEXT_ALL:
-		return bisect_next_all(the_repository, prefix, no_checkout);
+		res = bisect_next_all(the_repository, prefix, no_checkout);
+		break;
 	case WRITE_TERMS:
 		if (argc != 2)
 			return error(_("--write-terms requires two arguments"));
@@ -713,5 +714,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
 		return error("BUG: unknown subcommand '%d'", cmdmode);
 	}
 	free_terms(&terms);
-	return !!res;
+	/*
+	 * Handle early success
+	 * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
+	 */
+	if (res == -11)
+		res = 0;
+
+	return res < 0 ? -res : res;
 }
-- 
2.21.1 (Apple Git-122.3)

Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents

[Johannes Schindelin]

Hi Miriam,

On Tue, 28 Jan 2020, Miriam Rubio wrote:

> @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
>  	if (check_ancestors(r, rev_nr, rev, prefix))
>  		res = check_merge_bases(rev_nr, rev, no_checkout);
>  	free(rev);
> -	if (res)
> -		exit(res == -11 ? 0 : -res);
>
> -	/* Create file BISECT_ANCESTORS_OK. */
> -	fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> -	if (fd < 0)
> -		warning_errno(_("could not create file '%s'"),
> -			      filename);
> -	else
> -		close(fd);
> +	if (!res)
> +	{

Please move the opening `{` to the same line as the `if (!res)`.

> +		/* Create file BISECT_ANCESTORS_OK. */
> +		fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> +		if (fd < 0)
> +			warning_errno(_("could not create file '%s'"),
> +				      filename);
> +		else
> +			close(fd);
> +	}

I wonder whether this would be easier to read:

	if (res == -11)
		res = 0;
	else if (res)
		; /* error out */
	else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)
		res = warning_errno(_("could not create file '%s'"), filename);
	else
		close(fd);

Note: my code explicitly assigns `res = -1` if the file could not be
created, which is technically a change in behavior, but I think it is
actually a bug fix.

>   done:
>  	free(filename);
> +	return res;
>  }
>
>  /*
> @@ -975,7 +985,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
>  	if (read_bisect_refs())
>  		die(_("reading bisect refs failed"));

I see that there is still a `die()` here, and you left it alone in this
patch because at this point, only the callers of
`check_good_are_ancestors_of_bad()` need to be addressed. Good.

At a later point, this will have to be dealt with, of course.

Another thing will need to be handled, too: while I was looking at the
code whether any resources need to be released (returning a negative
integer does not release memory or close file handles, unlike `die()`), I
stumbled across the fact that `term_bad` and `term_good` are file-local
variables. They will need to be made attributes of a `struct` and will
need to be released properly, i.e. the ownership will have to be clarified
(is a failed `bisect_next_all()` responsible for releasing the memory it
allocated via `read_bisect_terms()`, or its caller?).

Just something to keep in mind. Or better: to jot down in a TODO list.

>
> -	check_good_are_ancestors_of_bad(r, prefix, no_checkout);
> +	res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);
> +	if (res)
> +		return res;
>
>  	bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
>  	revs.limited = 1;
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index 826fcba2ed..3442bfe2cb 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -666,7 +666,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>
>  	switch (cmdmode) {
>  	case NEXT_ALL:
> -		return bisect_next_all(the_repository, prefix, no_checkout);
> +		res = bisect_next_all(the_repository, prefix, no_checkout);
> +		break;
>  	case WRITE_TERMS:
>  		if (argc != 2)
>  			return error(_("--write-terms requires two arguments"));
> @@ -713,5 +714,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
>  		return error("BUG: unknown subcommand '%d'", cmdmode);
>  	}
>  	free_terms(&terms);
> -	return !!res;
> +	/*
> +	 * Handle early success
> +	 * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
> +	 */
> +	if (res == -11)
> +		res = 0;

Hmm. Is this the correct place, though? Should `bisect_next_all()` not be
the function that already turns `-11` into `0`? In other words, _which_
code are we supposed to skip over when `check_good_are_ancestors_of_bad()`
returns `-11`? In other words, where would the `catch` of the
`try`/`catch` be, if we had portable exceptions in C?

> +
> +	return res < 0 ? -res : res;

This is a change in behavior, though: previously we guaranteed that the
exit code is either 0 on success or 1 upon failure. I am not quite sure
that we want to change that behavior.

Ciao,
Dscho

>  }
> --
> 2.21.1 (Apple Git-122.3)
>
>

Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents

[Miriam R.]

Hi,

El jue., 30 ene. 2020 a las 14:46, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Tue, 28 Jan 2020, Miriam Rubio wrote:
>
> > @@ -893,18 +901,20 @@ static void check_good_are_ancestors_of_bad(struct repository *r,
> >       if (check_ancestors(r, rev_nr, rev, prefix))
> >               res = check_merge_bases(rev_nr, rev, no_checkout);
> >       free(rev);
> > -     if (res)
> > -             exit(res == -11 ? 0 : -res);
> >
> > -     /* Create file BISECT_ANCESTORS_OK. */
> > -     fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> > -     if (fd < 0)
> > -             warning_errno(_("could not create file '%s'"),
> > -                           filename);
> > -     else
> > -             close(fd);
> > +     if (!res)
> > +     {
>
> Please move the opening `{` to the same line as the `if (!res)`.
Noted.
>
> > +             /* Create file BISECT_ANCESTORS_OK. */
> > +             fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> > +             if (fd < 0)
> > +                     warning_errno(_("could not create file '%s'"),
> > +                                   filename);
> > +             else
> > +                     close(fd);
> > +     }
>
> I wonder whether this would be easier to read:
>
>         if (res == -11)
>                 res = 0;
>         else if (res)
>                 ; /* error out */
>         else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)
>                 res = warning_errno(_("could not create file '%s'"), filename);
>         else
>                 close(fd);
>
Yes, I think it is a good improvement.

> Note: my code explicitly assigns `res = -1` if the file could not be
> created, which is technically a change in behavior, but I think it is
> actually a bug fix.

Aha.

If my mentor is ok with this change, I will apply the improvement you
suggested :).

>
> >   done:
> >       free(filename);
> > +     return res;
> >  }
> >
> >  /*
> > @@ -975,7 +985,9 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
> >       if (read_bisect_refs())
> >               die(_("reading bisect refs failed"));
>
> I see that there is still a `die()` here, and you left it alone in this
> patch because at this point, only the callers of
> `check_good_are_ancestors_of_bad()` need to be addressed. Good.
>
> At a later point, this will have to be dealt with, of course.
>
> Another thing will need to be handled, too: while I was looking at the
> code whether any resources need to be released (returning a negative
> integer does not release memory or close file handles, unlike `die()`), I
> stumbled across the fact that `term_bad` and `term_good` are file-local
> variables. They will need to be made attributes of a `struct` and will
> need to be released properly, i.e. the ownership will have to be clarified
> (is a failed `bisect_next_all()` responsible for releasing the memory it
> allocated via `read_bisect_terms()`, or its caller?).
>
> Just something to keep in mind. Or better: to jot down in a TODO list.

Ok. I will write this down for future improvements. Thank you!
>
> >
> > -     check_good_are_ancestors_of_bad(r, prefix, no_checkout);
> > +     res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);
> > +     if (res)
> > +             return res;
> >
> >       bisect_rev_setup(r, &revs, prefix, "%s", "^%s", 1);
> >       revs.limited = 1;
> > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> > index 826fcba2ed..3442bfe2cb 100644
> > --- a/builtin/bisect--helper.c
> > +++ b/builtin/bisect--helper.c
> > @@ -666,7 +666,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> >
> >       switch (cmdmode) {
> >       case NEXT_ALL:
> > -             return bisect_next_all(the_repository, prefix, no_checkout);
> > +             res = bisect_next_all(the_repository, prefix, no_checkout);
> > +             break;
> >       case WRITE_TERMS:
> >               if (argc != 2)
> >                       return error(_("--write-terms requires two arguments"));
> > @@ -713,5 +714,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
> >               return error("BUG: unknown subcommand '%d'", cmdmode);
> >       }
> >       free_terms(&terms);
> > -     return !!res;
> > +     /*
> > +      * Handle early success
> > +      * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all
> > +      */
> > +     if (res == -11)
> > +             res = 0;
>
> Hmm. Is this the correct place, though? Should `bisect_next_all()` not be
> the function that already turns `-11` into `0`? In other words, _which_
> code are we supposed to skip over when `check_good_are_ancestors_of_bad()`
> returns `-11`? In other words, where would the `catch` of the
> `try`/`catch` be, if we had portable exceptions in C?

I think there must be a reason to do it there (but I don't know
exactly), because there are some comments in code that say explicitly
that this -11 to 0 is done in cmd_bisect_helper(), when the bisecting
command exits.

>
> > +
> > +     return res < 0 ? -res : res;
>
> This is a change in behavior, though: previously we guaranteed that the
> exit code is either 0 on success or 1 upon failure. I am not quite sure
> that we want to change that behavior.

I think this is because different error codes might enable a bisecting
script calling the bisect command that uses this function to do
different things depending on the exit status of the bisect command.

Thank you for reviewing.
Best,
Miriam.
>
> Ciao,
> Dscho
>
> >  }
> > --
> > 2.21.1 (Apple Git-122.3)
> >
> >

Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

Hi Miriam,

On Thu, 30 Jan 2020, Miriam R. wrote:

> El jue., 30 ene. 2020 a las 14:46, Johannes Schindelin
> (<Johannes.Schindelin@gmx.de>) escribió:
> >
> > On Tue, 28 Jan 2020, Miriam Rubio wrote:
> >
> > > +
> > > +     return res < 0 ? -res : res;
> >
> > This is a change in behavior, though: previously we guaranteed that the
> > exit code is either 0 on success or 1 upon failure. I am not quite sure
> > that we want to change that behavior.
>
> I think this is because different error codes might enable a bisecting
> script calling the bisect command that uses this function to do
> different things depending on the exit status of the bisect command.

Oops. I am _totally_ wrong on this.

As you are changing a lot of `exit(<n>)` to `return -<n>` with the
intention to turn the value into an exit code only at the
`cmd_bisect__helper()` level, this is actually required a change.

I am a bit uneasy about this, but I could not see any return values in
`bisect.c` other than 0 and -1, prior to this patch series.

However, I would love to see this refactored into its own commit, more
toward the beginning of the patch series, with a very clean commit message
that describes that intention of being _the_ exit point from `bisect.c`.

Without this change, you simply cannot change the `exit(<n>);` calls in
`bisect.c` for any `<n>` other than 0 or 1.

Sorry that it took me so long to wrap my head around this rather trivial
idea.

Ciao,
Dscho

Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents

[Miriam R.]

Hi,

El jue., 30 ene. 2020 a las 16:01, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Thu, 30 Jan 2020, Miriam R. wrote:
>
> > El jue., 30 ene. 2020 a las 14:46, Johannes Schindelin
> > (<Johannes.Schindelin@gmx.de>) escribió:
> > >
> > > On Tue, 28 Jan 2020, Miriam Rubio wrote:
> > >
> > > > +
> > > > +     return res < 0 ? -res : res;
> > >
> > > This is a change in behavior, though: previously we guaranteed that the
> > > exit code is either 0 on success or 1 upon failure. I am not quite sure
> > > that we want to change that behavior.
> >
> > I think this is because different error codes might enable a bisecting
> > script calling the bisect command that uses this function to do
> > different things depending on the exit status of the bisect command.
>
> Oops. I am _totally_ wrong on this.
>
> As you are changing a lot of `exit(<n>)` to `return -<n>` with the
> intention to turn the value into an exit code only at the
> `cmd_bisect__helper()` level, this is actually required a change.
>
> I am a bit uneasy about this, but I could not see any return values in
> `bisect.c` other than 0 and -1, prior to this patch series.
>
> However, I would love to see this refactored into its own commit, more
> toward the beginning of the patch series, with a very clean commit message
> that describes that intention of being _the_ exit point from `bisect.c`.

Ok. Noted
>
> Without this change, you simply cannot change the `exit(<n>);` calls in
> `bisect.c` for any `<n>` other than 0 or 1.
>
> Sorry that it took me so long to wrap my head around this rather trivial
> idea.

Please, don't worry :)
Thank you again!

Best,
Miriam.

>
> Ciao,
> Dscho

Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents

[Christian Couder]

Hi Dscho,

On Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Tue, 28 Jan 2020, Miriam Rubio wrote:

> > +             /* Create file BISECT_ANCESTORS_OK. */
> > +             fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> > +             if (fd < 0)
> > +                     warning_errno(_("could not create file '%s'"),
> > +                                   filename);
> > +             else
> > +                     close(fd);
> > +     }
>
> I wonder whether this would be easier to read:
>
>         if (res == -11)
>                 res = 0;
>         else if (res)
>                 ; /* error out */
>         else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)
>                 res = warning_errno(_("could not create file '%s'"), filename);
>         else
>                 close(fd);
>
> Note: my code explicitly assigns `res = -1` if the file could not be
> created, which is technically a change in behavior, but I think it is
> actually a bug fix.

I don't think so. I think creating the BISECT_ANCESTORS_OK file is not
absolutely necessary. If it doesn't exist we will just check if
ancestors are ok again at the next bisection step, which will take a
bit of time, but which will not make us give any false result, or
prevent us from continuing the bisection process.

I think that it's also the reason why warning_errno(...) is used in
case we could not create the file instead of error_errno(...). We just
want to signal with a warning that something might be wrong because we
could not create the file, but not stop everything because of that.

Best,
Christian.

Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents

[Johannes Schindelin]

Hi Chris,

On Thu, 30 Jan 2020, Christian Couder wrote:

> Hi Dscho,
>
> On Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Tue, 28 Jan 2020, Miriam Rubio wrote:
>
> > > +             /* Create file BISECT_ANCESTORS_OK. */
> > > +             fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> > > +             if (fd < 0)
> > > +                     warning_errno(_("could not create file '%s'"),
> > > +                                   filename);
> > > +             else
> > > +                     close(fd);
> > > +     }
> >
> > I wonder whether this would be easier to read:
> >
> >         if (res == -11)
> >                 res = 0;
> >         else if (res)
> >                 ; /* error out */
> >         else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)
> >                 res = warning_errno(_("could not create file '%s'"), filename);
> >         else
> >                 close(fd);
> >
> > Note: my code explicitly assigns `res = -1` if the file could not be
> > created, which is technically a change in behavior, but I think it is
> > actually a bug fix.
>
> I don't think so. I think creating the BISECT_ANCESTORS_OK file is not
> absolutely necessary. If it doesn't exist we will just check if
> ancestors are ok again at the next bisection step, which will take a
> bit of time, but which will not make us give any false result, or
> prevent us from continuing the bisection process.
>
> I think that it's also the reason why warning_errno(...) is used in
> case we could not create the file instead of error_errno(...). We just
> want to signal with a warning that something might be wrong because we
> could not create the file, but not stop everything because of that.

Thank you for this explanation, it makes sense to me.

Maybe a code comment would be in order?

Ciao,
Dscho

Re: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents

[Christian Couder]

Hi Dscho,

On Fri, Jan 31, 2020 at 10:07 AM Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
>
> On Thu, 30 Jan 2020, Christian Couder wrote:
>
> > On Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin
> > <Johannes.Schindelin@gmx.de> wrote:
> > >
> > > On Tue, 28 Jan 2020, Miriam Rubio wrote:
> >
> > > > +             /* Create file BISECT_ANCESTORS_OK. */
> > > > +             fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> > > > +             if (fd < 0)
> > > > +                     warning_errno(_("could not create file '%s'"),
> > > > +                                   filename);
> > > > +             else
> > > > +                     close(fd);
> > > > +     }
> > >
> > > I wonder whether this would be easier to read:
> > >
> > >         if (res == -11)
> > >                 res = 0;
> > >         else if (res)
> > >                 ; /* error out */
> > >         else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)
> > >                 res = warning_errno(_("could not create file '%s'"), filename);
> > >         else
> > >                 close(fd);
> > >
> > > Note: my code explicitly assigns `res = -1` if the file could not be
> > > created, which is technically a change in behavior, but I think it is
> > > actually a bug fix.
> >
> > I don't think so. I think creating the BISECT_ANCESTORS_OK file is not
> > absolutely necessary. If it doesn't exist we will just check if
> > ancestors are ok again at the next bisection step, which will take a
> > bit of time, but which will not make us give any false result, or
> > prevent us from continuing the bisection process.
> >
> > I think that it's also the reason why warning_errno(...) is used in
> > case we could not create the file instead of error_errno(...). We just
> > want to signal with a warning that something might be wrong because we
> > could not create the file, but not stop everything because of that.
>
> Thank you for this explanation, it makes sense to me.
>
> Maybe a code comment would be in order?

Yeah, I agree it would help.

Thanks for your review,
Christian.

Fwd: [PATCH v2 09/11] bisect: libify `check_good_are_ancestors_of_bad` and its dependents

[Miriam R.]

Hi,

El vie., 31 ene. 2020 a las 10:15, Christian Couder
(<christian.couder@gmail.com>) escribió:
>
> Hi Dscho,
>
> On Fri, Jan 31, 2020 at 10:07 AM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Thu, 30 Jan 2020, Christian Couder wrote:
> >
> > > On Thu, Jan 30, 2020 at 2:46 PM Johannes Schindelin
> > > <Johannes.Schindelin@gmx.de> wrote:
> > > >
> > > > On Tue, 28 Jan 2020, Miriam Rubio wrote:
> > >
> > > > > +             /* Create file BISECT_ANCESTORS_OK. */
> > > > > +             fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600);
> > > > > +             if (fd < 0)
> > > > > +                     warning_errno(_("could not create file '%s'"),
> > > > > +                                   filename);
> > > > > +             else
> > > > > +                     close(fd);
> > > > > +     }
> > > >
> > > > I wonder whether this would be easier to read:
> > > >
> > > >         if (res == -11)
> > > >                 res = 0;
> > > >         else if (res)
> > > >                 ; /* error out */
> > > >         else if ((fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600)) < 0)
> > > >                 res = warning_errno(_("could not create file '%s'"), filename);
> > > >         else
> > > >                 close(fd);
> > > >
> > > > Note: my code explicitly assigns `res = -1` if the file could not be
> > > > created, which is technically a change in behavior, but I think it is
> > > > actually a bug fix.
> > >
> > > I don't think so. I think creating the BISECT_ANCESTORS_OK file is not
> > > absolutely necessary. If it doesn't exist we will just check if
> > > ancestors are ok again at the next bisection step, which will take a
> > > bit of time, but which will not make us give any false result, or
> > > prevent us from continuing the bisection process.
> > >
> > > I think that it's also the reason why warning_errno(...) is used in
> > > case we could not create the file instead of error_errno(...). We just
> > > want to signal with a warning that something might be wrong because we
> > > could not create the file, but not stop everything because of that.
> >
> > Thank you for this explanation, it makes sense to me.
> >
> > Maybe a code comment would be in order?
>
> Yeah, I agree it would help.
>
Noted.
Thank you both for the review!

Best,
Miriam
> Thanks for your review,
> Christian.

[PATCH v2 10/11] bisect: libify `handle_bad_merge_base` and its dependents

[Miriam Rubio]

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

Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.

Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.

Handle the return value in dependent function check_merge_bases().

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 | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/bisect.c b/bisect.c
index d519e10827..43baa3df28 100644
--- a/bisect.c
+++ b/bisect.c
@@ -756,7 +756,7 @@ static struct commit **get_bad_and_good_commits(struct repository *r,
 	return rev;
 }
 
-static void handle_bad_merge_base(void)
+static int handle_bad_merge_base(void)
 {
 	if (is_expected_rev(current_bad_oid)) {
 		char *bad_hex = oid_to_hex(current_bad_oid);
@@ -777,14 +777,14 @@ static void handle_bad_merge_base(void)
 				"between %s and [%s].\n"),
 				bad_hex, term_bad, term_good, bad_hex, good_hex);
 		}
-		exit(3);
+		return -3;
 	}
 
 	fprintf(stderr, _("Some %s revs are not ancestors of the %s rev.\n"
 		"git bisect cannot work properly in this case.\n"
 		"Maybe you mistook %s and %s revs?\n"),
 		term_good, term_bad, term_good, term_bad);
-	exit(1);
+	return -1;
 }
 
 static void handle_skipped_merge_base(const struct object_id *mb)
@@ -823,7 +823,8 @@ static int check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
 	for (; result; result = result->next) {
 		const struct object_id *mb = &result->item->object.oid;
 		if (oideq(mb, current_bad_oid)) {
-			handle_bad_merge_base();
+			res = handle_bad_merge_base();
+			break;
 		} else if (0 <= oid_array_lookup(&good_revs, mb)) {
 			continue;
 		} else if (0 <= oid_array_lookup(&skipped_revs, mb)) {
-- 
2.21.1 (Apple Git-122.3)

[PATCH v2 11/11] bisect: libify `bisect_next_all`

[Miriam Rubio]

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

Since we want to get rid of git-bisect.sh it would be necessary to
convert those exit() calls to return statements so that errors can be
reported.

Emulate try catch in C by converting `exit(<positive-value>)` to
`return <negative-value>`. Follow POSIX conventions to return
<negative-value> to indicate error.

All the functions calling `bisect_next_all()` are already able to
handle return values from it.

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 | 40 +++++++++++++++++++++++++++++-----------
 bisect.h | 23 +++++++++++++++++++++++
 2 files changed, 52 insertions(+), 11 deletions(-)

diff --git a/bisect.c b/bisect.c
index 43baa3df28..d4b883f67d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -967,10 +967,10 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
 }
 
 /*
- * We use the convention that exiting with an exit code 10 means that
- * the bisection process finished successfully.
- * In this case the calling shell script should exit 0.
- *
+ * We use the convention that return -10 means the bisection process
+ * finished successfully.
+ * In this case the calling function or command should not turn a -10
+ * return code into an error or a non zero exit code.
  * If no_checkout is non-zero, the bisection process does not
  * checkout the trial commit but instead simply updates BISECT_HEAD.
  */
@@ -1000,23 +1000,35 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
 
 	if (!revs.commits) {
 		/*
-		 * We should exit here only if the "bad"
+		 * We should return error here only if the "bad"
 		 * commit is also a "skip" commit.
 		 */
 		res = error_if_skipped_commits(tried, NULL);
 		if (res < 0)
-			exit(-res);
+			return res;
 		printf(_("%s was both %s and %s\n"),
 		       oid_to_hex(current_bad_oid),
 		       term_good,
 		       term_bad);
-		exit(1);
+
+		/*
+		 * We don't want to clean the bisection state
+		 * as we need to get back to where we started
+		 * by using `git bisect reset`.
+		 */
+		return -1;
 	}
 
 	if (!all) {
 		fprintf(stderr, _("No testable commit found.\n"
 			"Maybe you started with bad path parameters?\n"));
-		exit(4);
+
+		/*
+		 * We don't want to clean the bisection state
+		 * as we need to get back to where we started
+		 * by using `git bisect reset`.
+		 */
+		return -4;
 	}
 
 	bisect_rev = &revs.commits->item->object.oid;
@@ -1024,12 +1036,18 @@ int bisect_next_all(struct repository *r, const char *prefix, int no_checkout)
 	if (oideq(bisect_rev, current_bad_oid)) {
 		res = error_if_skipped_commits(tried, current_bad_oid);
 		if (res)
-			exit(-res);
+			return res;
 		printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
 			term_bad);
+
 		show_diff_tree(r, prefix, revs.commits->item);
-		/* This means the bisection process succeeded. */
-		exit(10);
+		/*
+		 * This means the bisection process succeeded.
+		 * Using -10 so that the call chain can simply check
+		 * for negative return values for early returns up
+		 * until the cmd_bisect__helper() caller.
+		 */
+		return -10;
 	}
 
 	nr = all - reaches - 1;
diff --git a/bisect.h b/bisect.h
index 4e69a11ea8..f640c4f963 100644
--- a/bisect.h
+++ b/bisect.h
@@ -31,6 +31,29 @@ struct rev_list_info {
 	const char *header_prefix;
 };
 
+/*
+ * bisect_next_all() could return the following codes:
+ * 0 success code (from bisect_checkout()): a new commit to test
+ * has been found (and possibly checked out too).
+ * -1 error code (from bisect_next_all()): default error code.
+ * -2 error code (from error_if_skipped_commits()): only skipped
+ * commits left to be tested.
+ * -3 error code (from handle_bad_merge_base() through
+ * check_merge_bases() and check_good_are_ancestors_of_bad()): merge
+ * base check failed.
+ * -4 error code (from bisect_next_all()): no testable commit found.
+ * -10 early success code (from bisect_next_all()): first term_bad
+ * commit found.
+ * This value is checked in builtin/bisect_helper::bisect_next().
+ * -11 early success code (from check_merge_bases() through
+ * check_good_are_ancestors_of_bad()): successfully tested
+ * merge base.
+ * Early success codes -10 and -11 should be only internal codes and
+ * converted to a 0 status code when the bisecting command exits.
+ * Different error codes might enable a bisecting script calling the
+ * bisect command that uses this function to do different things
+ * depending on the exit status of the bisect command.
+ */
 int bisect_next_all(struct repository *r,
 		    const char *prefix,
 		    int no_checkout);
-- 
2.21.1 (Apple Git-122.3)

Re: [Outreachy][PATCH v2 00/11] Finish converting git bisect to C part 1

[Johannes Schindelin]

Hi Miriam,

On Tue, 28 Jan 2020, Miriam Rubio wrote:

> These patches correspond to a first part of patch series
> of Outreachy project "Finish converting `git bisect` from shell to C"
> started by the interns Pranit Bauva and Tanushree Tumane
> (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
> continued by Miriam Rubio.
>
> This first part are formed of preparatory/clean-up patches and all
> `bisect.c` libification work.
>
> These patch series emails were generated from:
> https://gitlab.com/mirucam/git/commits/git-bisect-work-part1
>
> It has to be noted that in this version 2 nothing has been done about a
> reviewer suggestion of using enums for error codes, because there was no
> consensus about using them by the reviewers.

It is a pity, as "magic" constants tend to remain "magic" (and eventually
"tragic" when they have not been addressed properly).

However, this does not need to hold up your patch series.

I noted a couple of rather minor things; It looks pretty good to me
already, though, and I thank you very much for splitting this patch series
off, so that only the libification remains. It made for a pleasant read.

Thanks,
Dscho

>
> --- Changes since v1 Finish converting git bisect to C part 1 patch series ---
>
> General changes
> ---------------
>
> * Previous patch series version has been split in smaller groups
> in order to facilitate revision and integration.
> * Rebase on master branch: c7a6207591 (Sync with maint, 2020-01-27).
> * Improve commit messages.
>
> Specific changes
> ----------------
>
> [6/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*`
> and its dependents
>
> * Remove redundant sentences in commit message.
> * Use `if (res < 0)` instead of `if (res)`.
>
> [8/11] bisect: libify `check_merge_bases` and its dependents
>
> * Remove redundant sentence in commit message.
>
> --
>
> [9/11] bisect: libify `check_good_are_ancestors_of_bad` and its
> dependents
>
> * Remove redundant sentences in commit message.
> * Return in `if (!current_bad_oid)` condition.
>
> --
>
> [10/11] bisect: libify `handle_bad_merge_base` and its dependents
>
> * Remove redundant sentence in commit message.
>
> --
>
> [11/11] bisect: libify `bisect_next_all`
>
> * Remove redundant sentence in commit message.
> * Add return codes explanations in `bisect.h`.
>
> --
>
> Miriam Rubio (2):
>   bisect--helper: convert `vocab_*` char pointers to char arrays
>   bisect: use the standard 'if (!var)' way to check for 0
>
> Pranit Bauva (7):
>   run-command: make `exists_in_PATH()` non-static
>   bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and
>     its dependents
>   bisect: libify `bisect_checkout`
>   bisect: libify `check_merge_bases` and its dependents
>   bisect: libify `check_good_are_ancestors_of_bad` and its dependents
>   bisect: libify `handle_bad_merge_base` and its dependents
>   bisect: libify `bisect_next_all`
>
> Tanushree Tumane (2):
>   bisect--helper: change `retval` to `res`
>   bisect--helper: introduce new `decide_next()` function
>
>  bisect.c                 | 136 +++++++++++++++++++++++++++------------
>  bisect.h                 |  23 +++++++
>  builtin/bisect--helper.c | 118 +++++++++++++++++----------------
>  run-command.c            |   2 +-
>  run-command.h            |  11 ++++
>  5 files changed, 193 insertions(+), 97 deletions(-)
>
> --
> 2.21.1 (Apple Git-122.3)
>
>

Re: [Outreachy][PATCH v2 00/11] Finish converting git bisect to C part 1

[Miriam R.]

El jue., 30 ene. 2020 a las 16:04, Johannes Schindelin
(<Johannes.Schindelin@gmx.de>) escribió:
>
> Hi Miriam,
>
> On Tue, 28 Jan 2020, Miriam Rubio wrote:
>
> > These patches correspond to a first part of patch series
> > of Outreachy project "Finish converting `git bisect` from shell to C"
> > started by the interns Pranit Bauva and Tanushree Tumane
> > (https://public-inbox.org/git/pull.117.git.gitgitgadget@gmail.com) and
> > continued by Miriam Rubio.
> >
> > This first part are formed of preparatory/clean-up patches and all
> > `bisect.c` libification work.
> >
> > These patch series emails were generated from:
> > https://gitlab.com/mirucam/git/commits/git-bisect-work-part1
> >
> > It has to be noted that in this version 2 nothing has been done about a
> > reviewer suggestion of using enums for error codes, because there was no
> > consensus about using them by the reviewers.
>
> It is a pity, as "magic" constants tend to remain "magic" (and eventually
> "tragic" when they have not been addressed properly).
>
> However, this does not need to hold up your patch series.
>
> I noted a couple of rather minor things; It looks pretty good to me
> already, though, and I thank you very much for splitting this patch series
> off, so that only the libification remains. It made for a pleasant read.
>
Thank you very much for the review.
I hope to send another patch series version soon with the new changes
and suggestions :)

Best,
Miriam

> Thanks,
> Dscho
>
> >
> > --- Changes since v1 Finish converting git bisect to C part 1 patch series ---
> >
> > General changes
> > ---------------
> >
> > * Previous patch series version has been split in smaller groups
> > in order to facilitate revision and integration.
> > * Rebase on master branch: c7a6207591 (Sync with maint, 2020-01-27).
> > * Improve commit messages.
> >
> > Specific changes
> > ----------------
> >
> > [6/11] bisect: libify `exit_if_skipped_commits` to `error_if_skipped*`
> > and its dependents
> >
> > * Remove redundant sentences in commit message.
> > * Use `if (res < 0)` instead of `if (res)`.
> >
> > [8/11] bisect: libify `check_merge_bases` and its dependents
> >
> > * Remove redundant sentence in commit message.
> >
> > --
> >
> > [9/11] bisect: libify `check_good_are_ancestors_of_bad` and its
> > dependents
> >
> > * Remove redundant sentences in commit message.
> > * Return in `if (!current_bad_oid)` condition.
> >
> > --
> >
> > [10/11] bisect: libify `handle_bad_merge_base` and its dependents
> >
> > * Remove redundant sentence in commit message.
> >
> > --
> >
> > [11/11] bisect: libify `bisect_next_all`
> >
> > * Remove redundant sentence in commit message.
> > * Add return codes explanations in `bisect.h`.
> >
> > --
> >
> > Miriam Rubio (2):
> >   bisect--helper: convert `vocab_*` char pointers to char arrays
> >   bisect: use the standard 'if (!var)' way to check for 0
> >
> > Pranit Bauva (7):
> >   run-command: make `exists_in_PATH()` non-static
> >   bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and
> >     its dependents
> >   bisect: libify `bisect_checkout`
> >   bisect: libify `check_merge_bases` and its dependents
> >   bisect: libify `check_good_are_ancestors_of_bad` and its dependents
> >   bisect: libify `handle_bad_merge_base` and its dependents
> >   bisect: libify `bisect_next_all`
> >
> > Tanushree Tumane (2):
> >   bisect--helper: change `retval` to `res`
> >   bisect--helper: introduce new `decide_next()` function
> >
> >  bisect.c                 | 136 +++++++++++++++++++++++++++------------
> >  bisect.h                 |  23 +++++++
> >  builtin/bisect--helper.c | 118 +++++++++++++++++----------------
> >  run-command.c            |   2 +-
> >  run-command.h            |  11 ++++
> >  5 files changed, 193 insertions(+), 97 deletions(-)
> >
> > --
> > 2.21.1 (Apple Git-122.3)
> >
> >