From 16538bfd2c429a81a86011f32afa189047afd6e5 Mon Sep 17 00:00:00 2001 From: Miriam Rubio Date: Mon, 17 Feb 2020 09:40:28 +0100 Subject: bisect--helper: convert `vocab_*` char pointers to char arrays 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 Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 1718df7..36c09b7 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 -- cgit v0.10.2-6-g49f6 From 292731c4c24bce5c9e3bdc7a246cfbc565ab93a1 Mon Sep 17 00:00:00 2001 From: Tanushree Tumane Date: Mon, 17 Feb 2020 09:40:29 +0100 Subject: bisect--helper: change `retval` to `res` 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 Signed-off-by: Tanushree Tumane Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 36c09b7..21de5c0 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 @@ finish: 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 " "'."), 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) -- cgit v0.10.2-6-g49f6 From b8e3b2f33994de25c068021e388dbef72a2cdc4a Mon Sep 17 00:00:00 2001 From: Miriam Rubio Date: Mon, 17 Feb 2020 09:40:30 +0100 Subject: bisect: use the standard 'if (!var)' way to check for 0 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 Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/bisect.c b/bisect.c index e81c91d..83cb5b3 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. */ -- cgit v0.10.2-6-g49f6 From bfacfce7d94951fe5b4e70bc2431dae5506b474b Mon Sep 17 00:00:00 2001 From: Tanushree Tumane Date: Mon, 17 Feb 2020 09:40:31 +0100 Subject: bisect--helper: introduce new `decide_next()` function 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. While at it `bad_ref` and `good_glob` are not const any more to void casting them inside `free()`. Mentored-by: Christian Couder Signed-off-by: Tanushree Tumane Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 21de5c0..e21d3d1 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: - free((void *) good_glob); - free((void *) bad_ref); - return res; + 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; + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad); + 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(good_glob); + free(bad_ref); + + return decide_next(terms, current_term, missing_good, missing_bad); } static int get_terms(struct bisect_terms *terms) -- cgit v0.10.2-6-g49f6 From 680e8a01e57cd383048bf4e7d9668ce715d6d649 Mon Sep 17 00:00:00 2001 From: Miriam Rubio Date: Mon, 17 Feb 2020 09:40:32 +0100 Subject: bisect: add enum to represent bisect returning codes 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. Create an enum called `bisect_error` with the bisecting return codes to use in `bisect.c` libification process. Change bisect_next_all() to make it return this enum. Mentored-by: Christian Couder Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/bisect.c b/bisect.c index 83cb5b3..e4573c7 100644 --- a/bisect.c +++ b/bisect.c @@ -945,7 +945,7 @@ void read_bisect_terms(const char **read_bad, const char **read_good) * If no_checkout is non-zero, the bisection process does not * checkout the trial commit but instead simply updates BISECT_HEAD. */ -int bisect_next_all(struct repository *r, const char *prefix, int no_checkout) +enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int no_checkout) { struct rev_info revs; struct commit_list *tried; diff --git a/bisect.h b/bisect.h index 4e69a11..c921ead 100644 --- a/bisect.h +++ b/bisect.h @@ -31,7 +31,19 @@ struct rev_list_info { const char *header_prefix; }; -int bisect_next_all(struct repository *r, +/* + * enum bisect_error represents the following return codes: + * BISECT_OK: success code. Internally, it means that next + * commit has been found (and possibly checked out) and it + * should be tested. + * BISECT_FAILED error code: default error code. + */ +enum bisect_error { + BISECT_OK = 0, + BISECT_FAILED = -1 +}; + +enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int no_checkout); -- cgit v0.10.2-6-g49f6 From 7613ec594a3cb3a5b67bd4f363a557374e744dfb Mon Sep 17 00:00:00 2001 From: Pranit Bauva Date: Mon, 17 Feb 2020 09:40:33 +0100 Subject: bisect--helper: return error codes from `cmd_bisect__helper()` Since we want to get rid of git-bisect.sh, it would be necessary to convert bisect.c exit() calls to return statements so that errors can be reported. Let's prepare for that by making it possible to return different error codes than just 0 or 1. 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. Mentored-by: Christian Couder Mentored-by: Johannes Schindelin Signed-off-by: Tanushree Tumane Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index e21d3d1..e6bd4d6 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -713,5 +713,5 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) return error("BUG: unknown subcommand '%d'", cmdmode); } free_terms(&terms); - return !!res; + return abs(res); } -- cgit v0.10.2-6-g49f6 From ce58b5d8b128568e9b4f37703377cbc22eaf00f5 Mon Sep 17 00:00:00 2001 From: Pranit Bauva Date: Mon, 17 Feb 2020 09:40:34 +0100 Subject: bisect: libify `exit_if_skipped_commits` to `error_if_skipped*` and its dependents 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()` to `return `. Follow POSIX conventions to return to indicate error. Update all callers to handle the error returns. Mentored-by: Christian Couder Mentored-by: Johannes Schindelin Signed-off-by: Pranit Bauva Signed-off-by: Tanushree Tumane Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/bisect.c b/bisect.c index e4573c7..85bda35 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 enum bisect_error error_if_skipped_commits(struct commit_list *tried, const struct object_id *bad) { if (!tried) - return; + return BISECT_OK; 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,8 @@ 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); + + return BISECT_ONLY_SKIPPED_LEFT; } static int is_expected_rev(const struct object_id *oid) @@ -950,6 +951,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int struct rev_info revs; struct commit_list *tried; int reaches = 0, all = 0, nr, steps; + enum bisect_error res = BISECT_OK; struct object_id *bisect_rev; char *steps_msg; @@ -972,8 +974,9 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int * 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 +993,9 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int 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); diff --git a/bisect.h b/bisect.h index c921ead..19d90e4 100644 --- a/bisect.h +++ b/bisect.h @@ -37,10 +37,13 @@ struct rev_list_info { * commit has been found (and possibly checked out) and it * should be tested. * BISECT_FAILED error code: default error code. + * BISECT_ONLY_SKIPPED_LEFT error code: only skipped + * commits left to be tested. */ enum bisect_error { BISECT_OK = 0, - BISECT_FAILED = -1 + BISECT_FAILED = -1, + BISECT_ONLY_SKIPPED_LEFT = -2 }; enum bisect_error bisect_next_all(struct repository *r, -- cgit v0.10.2-6-g49f6 From e8e3ce67185fd493a07d84a5ce21b469701f8f64 Mon Sep 17 00:00:00 2001 From: Pranit Bauva Date: Mon, 17 Feb 2020 09:40:35 +0100 Subject: bisect: libify `bisect_checkout` 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()` to `return `. Follow POSIX conventions to return 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 Mentored-by: Johannes Schindelin Signed-off-by: Pranit Bauva Signed-off-by: Tanushree Tumane Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/bisect.c b/bisect.c index 85bda35..f6582dd 100644 --- a/bisect.c +++ b/bisect.c @@ -704,9 +704,10 @@ static int is_expected_rev(const struct object_id *oid) return res; } -static int bisect_checkout(const struct object_id *bisect_rev, int no_checkout) +static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int no_checkout) { char bisect_rev_hex[GIT_MAX_HEXSZ + 1]; + enum bisect_error res = BISECT_OK; 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); @@ -716,14 +717,24 @@ 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); + /* + * Errors in `run_command()` itself, signaled by res < 0, + * and errors in the child process, signaled by res > 0 + * can both be treated as regular BISECT_FAILURE (-1). + */ + 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); + /* + * Errors in `run_command()` itself, signaled by res < 0, + * and errors in the child process, signaled by res > 0 + * can both be treated as regular BISECT_FAILURE (-1). + */ + return -abs(res); } static struct commit *get_commit_reference(struct repository *r, -- cgit v0.10.2-6-g49f6 From cdd4dc2d6a3e420178100efc780a987495a1a3bd Mon Sep 17 00:00:00 2001 From: Pranit Bauva Date: Mon, 17 Feb 2020 09:40:36 +0100 Subject: bisect: libify `check_merge_bases` and its dependents 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()` to `return `. Follow POSIX conventions to return to indicate error. In `check_merge_bases()` there is an early success special case, so we have introduced special error code BISECT_INTERNAL_SUCCESS_MERGE_BASE (-11) which indicates early success. This BISECT_INTERNAL_SUCCESS_MERGE_BASE is converted back to BISECT_OK (0) in `check_good_are_ancestors_of_bad()`. Update all callers to handle the error returns. Mentored-by: Christian Couder Mentored by: Johannes Schindelin Signed-off-by: Pranit Bauva Signed-off-by: Tanushree Tumane Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/bisect.c b/bisect.c index f6582dd..382e0b4 100644 --- a/bisect.c +++ b/bisect.c @@ -811,13 +811,18 @@ 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 + * BISECT_INTERNAL_SUCCESS_MERGE_BASE (-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 enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) { + enum bisect_error res = BISECT_OK; struct commit_list *result; result = get_merge_bases_many(rev[0], rev_nr - 1, rev + 1); @@ -832,11 +837,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 = BISECT_INTERNAL_SUCCESS_MERGE_BASE; + break; } } free_commit_list(result); + return res; } static int check_ancestors(struct repository *r, int rev_nr, @@ -871,6 +881,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; + enum bisect_error res = BISECT_OK; struct commit **rev; if (!current_bad_oid) @@ -885,10 +896,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 == BISECT_INTERNAL_SUCCESS_MERGE_BASE ? BISECT_OK : -res); /* Create file BISECT_ANCESTORS_OK. */ fd = open(filename, O_CREAT | O_TRUNC | O_WRONLY, 0600); diff --git a/bisect.h b/bisect.h index 19d90e4..f68ae85 100644 --- a/bisect.h +++ b/bisect.h @@ -39,11 +39,16 @@ struct rev_list_info { * BISECT_FAILED error code: default error code. * BISECT_ONLY_SKIPPED_LEFT error code: only skipped * commits left to be tested. + * BISECT_INTERNAL_SUCCESS_MERGE_BASE early success + * code: found merge base that should be tested. + * Early success code BISECT_INTERNAL_SUCCESS_MERGE_BASE + * should be only an internal code. */ enum bisect_error { BISECT_OK = 0, BISECT_FAILED = -1, - BISECT_ONLY_SKIPPED_LEFT = -2 + BISECT_ONLY_SKIPPED_LEFT = -2, + BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11 }; enum bisect_error bisect_next_all(struct repository *r, -- cgit v0.10.2-6-g49f6 From 45b6370812cb9bc50212d50f071c8ae5dd851698 Mon Sep 17 00:00:00 2001 From: Pranit Bauva Date: Mon, 17 Feb 2020 09:40:37 +0100 Subject: bisect: libify `check_good_are_ancestors_of_bad` and its dependents 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()` to `return `. Follow POSIX conventions to return to indicate error. Code that turns BISECT_INTERNAL_SUCCESS_MERGE_BASE (-11) to BISECT_OK (0) from `check_good_are_ancestors_of_bad()` has been moved to `cmd_bisect__helper()`. Update all callers to handle the error returns. Mentored-by: Christian Couder Mentored by: Johannes Schindelin Signed-off-by: Pranit Bauva Signed-off-by: Tanushree Tumane Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/bisect.c b/bisect.c index 382e0b4..f5ce3a4 100644 --- a/bisect.c +++ b/bisect.c @@ -872,20 +872,23 @@ 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 enum bisect_error 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; enum bisect_error res = BISECT_OK; struct commit **rev; 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)) @@ -901,18 +904,26 @@ 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 == BISECT_INTERNAL_SUCCESS_MERGE_BASE ? BISECT_OK : -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) + /* + * BISECT_ANCESTORS_OK file is not absolutely necessary, + * the bisection process will continue at the next + * bisection step. + * So, just signal with a warning that something + * might be wrong. + */ + warning_errno(_("could not create file '%s'"), + filename); + else + close(fd); + } done: free(filename); + return res; } /* @@ -984,7 +995,9 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int 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 e6bd4d6..c1c40b5 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,13 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) return error("BUG: unknown subcommand '%d'", cmdmode); } free_terms(&terms); + + /* + * Handle early success + * From check_merge_bases > check_good_are_ancestors_of_bad > bisect_next_all + */ + if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) + res = BISECT_OK; + return abs(res); } -- cgit v0.10.2-6-g49f6 From 9ec598e0d55ff263e47d95e7d4decef1eb1b6ac6 Mon Sep 17 00:00:00 2001 From: Pranit Bauva Date: Mon, 17 Feb 2020 09:40:38 +0100 Subject: bisect: libify `handle_bad_merge_base` and its dependents 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()` to `return `. Follow POSIX conventions to return to indicate error. Update all callers to handle the error returns. Mentored-by: Christian Couder Mentored-by: Johannes Schindelin Signed-off-by: Pranit Bauva Signed-off-by: Tanushree Tumane Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/bisect.c b/bisect.c index f5ce3a4..837332a 100644 --- a/bisect.c +++ b/bisect.c @@ -761,7 +761,7 @@ static struct commit **get_bad_and_good_commits(struct repository *r, return rev; } -static void handle_bad_merge_base(void) +static enum bisect_error handle_bad_merge_base(void) { if (is_expected_rev(current_bad_oid)) { char *bad_hex = oid_to_hex(current_bad_oid); @@ -782,14 +782,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 BISECT_MERGE_BASE_CHECK; } 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 BISECT_FAILED; } static void handle_skipped_merge_base(const struct object_id *mb) @@ -830,7 +830,8 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int 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)) { diff --git a/bisect.h b/bisect.h index f68ae85..0d97581 100644 --- a/bisect.h +++ b/bisect.h @@ -48,6 +48,7 @@ enum bisect_error { BISECT_OK = 0, BISECT_FAILED = -1, BISECT_ONLY_SKIPPED_LEFT = -2, + BISECT_MERGE_BASE_CHECK = -3, BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11 }; -- cgit v0.10.2-6-g49f6 From 6c69f2223394c200cce1289ac9200f781ef14da7 Mon Sep 17 00:00:00 2001 From: Pranit Bauva Date: Mon, 17 Feb 2020 09:40:39 +0100 Subject: bisect: libify `bisect_next_all` 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()` to `return `. Follow POSIX conventions to return to indicate error. All the functions calling `bisect_next_all()` are already able to handle return values from it. Mentored-by: Christian Couder Mentored-by: Johannes Schindelin Signed-off-by: Pranit Bauva Signed-off-by: Tanushree Tumane Signed-off-by: Miriam Rubio Signed-off-by: Junio C Hamano diff --git a/bisect.c b/bisect.c index 837332a..9154f81 100644 --- a/bisect.c +++ b/bisect.c @@ -976,10 +976,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 + * We use the convention that return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) means * the bisection process finished successfully. - * In this case the calling shell script should exit 0. - * + * 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. * If no_checkout is non-zero, the bisection process does not * checkout the trial commit but instead simply updates BISECT_HEAD. */ @@ -1010,23 +1010,25 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int 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); + + return BISECT_FAILED; } if (!all) { fprintf(stderr, _("No testable commit found.\n" "Maybe you started with bad path parameters?\n")); - exit(4); + + return BISECT_NO_TESTABLE_COMMIT; } bisect_rev = &revs.commits->item->object.oid; @@ -1034,12 +1036,19 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix, int 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 BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10) + * so that the call chain can simply check + * for negative return values for early returns up + * until the cmd_bisect__helper() caller. + */ + return BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND; } nr = all - reaches - 1; diff --git a/bisect.h b/bisect.h index 0d97581..8bad8d8 100644 --- a/bisect.h +++ b/bisect.h @@ -39,16 +39,22 @@ struct rev_list_info { * BISECT_FAILED error code: default error code. * BISECT_ONLY_SKIPPED_LEFT error code: only skipped * commits left to be tested. + * BISECT_MERGE_BASE_CHECK error code: merge base check failed. + * BISECT_NO_TESTABLE_COMMIT error code: no testable commit found. + * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND early success code: + * first term_bad commit found. * BISECT_INTERNAL_SUCCESS_MERGE_BASE early success * code: found merge base that should be tested. - * Early success code BISECT_INTERNAL_SUCCESS_MERGE_BASE - * should be only an internal code. + * Early success codes BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND and + * BISECT_INTERNAL_SUCCESS_MERGE_BASE should be only internal codes. */ 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 }; -- cgit v0.10.2-6-g49f6