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

Commit Message

Miriam R. Jan. 28, 2020, 2:40 p.m. UTC

From: Pranit Bauva <[e-mail]>

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 <[e-mail]>
Mentored by: Johannes Schindelin <[e-mail]>
Signed-off-by: Pranit Bauva <[e-mail]>
Signed-off-by: Tanushree Tumane <[e-mail]>
Signed-off-by: Miriam Rubio <[e-mail]>
---
 bisect.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

Comments

Junio C Hamano Jan. 31, 2020, 6:40 p.m. UTC | #1

Miriam Rubio <[e-mail]> 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

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