| Message ID | 20200321161020.22817-4-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Finish converting git bisect to C part 2 | expand |
Miriam Rubio <mirucam@gmail.com> writes: > diff --git a/bisect.c b/bisect.c > index 9154f810f7..a50278ea7e 100644 > --- a/bisect.c > +++ b/bisect.c > @@ -635,6 +635,11 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs, > struct argv_array rev_argv = ARGV_ARRAY_INIT; > int i; > > + /* > + * `revs` could has been used before. > + * Thus we first need to reset it. > + */ > + reset_revision_walk(); > repo_init_revisions(r, revs, prefix); I don't have enough time and concentration to follow all the codepaths in "bisect" that walk the commit graph starting here, but seeing one codepath, e.g. check_ancestors(), after calling this and walking with bisect_common(), uses clear_commit_marks_many() to clear ALL_REV_FLAGS, not just the ones that reset_revision_walk() clears, I am not sure what value this addition has. To put it differently, what codepath are you protecting the revision walk that is about to happen against with this "reset"? Who are the callers that could have used `revs` before calling this function and touched only SEEN, ADDED, SHOWN, etc. without touching other bits like COUNTED, TREESAME, UNINTERESTING that matter to the correct operation of "bisect"? The rest of the patch looks quite reasonably done. Let me comment on the patch out of order (in other words, I'll rearrange the functions in the order I read them). I am realizing that I feel it easier to understand to read the code in a bottom-up fashion. > +static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix) > +{ > + if (bisect_next_check(terms, NULL)) > + return BISECT_OK; The bisect_next_check() function returns what decide_next() returns, which is either 0 or error() which is -1. So this says "if nect-check says there was an error, we return OK". For the purpose of not proceeding to bisect_next(), returning is perfectly correct, but is the value returned correct? The scripted original does git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || : meaning "try next-check and go on to bisect_next if check succeeds; either way, ignore all errors from them". So it probably is more faithful conversion to make the returned value from auto_next() void. > + > + return bisect_next(terms, prefix); > +} In any case, this conversion of auto_next looks like a good one, with or without fixing its type. The caller in cmd_bisect__helper() seems to store the returned value in 'res' and uses it for the exit status, but for this to be a faithful conversion, it should ignore the returned value from here and always exit with success (and if we do so, it is one more reason why we would want to update the type of this function to return void). > +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix) > +{ > + int no_checkout; > + enum bisect_error res; > > + if (bisect_next_check(terms, terms->term_good)) > + return BISECT_FAILED; The original makes sure it does not get any argument, but that is done in cmd_bisect__helper(), so it is OK. The next thing the original does is to call bisect_autostart, before doing the next-check. Was it a dead code that wasn't necessary, or is its loss a regression during the conversion? > + > + no_checkout = !is_empty_or_missing_file(git_path_bisect_head()); > + > + /* Perform all bisection computation, display and checkout */ > + res = bisect_next_all(the_repository, prefix, no_checkout); Looking good. We've already converted next_all() in the previous series, and we call it the same way as the original by checking if $GIT_DIR/BISECT_HEAD is there. The original said "if BISECT_HEAD exists as a file, use '--no-checkout'" and did not care if its empty or not, but the updated code seems to care. Does the difference matter (i.e. is it more correct to ignore an empty BISECT_HEAD and pretend as if it did not exist)? > + if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) { > + res = bisect_successful(terms); > + return res ? res : BISECT_INTERNAL_SUCCESS_MERGE_BASE; It is unclear why "1st bad commit found" is turned into "success merge base" here. bisect_successfull() returns an error when it cannot append to the log, and otherwise we would want to relay "we are done successfully" back to the caller, no? Puzzled.... > + } else if (res == BISECT_ONLY_SKIPPED_LEFT) { > + res = bisect_skipped_commits(terms); > + return res ? res : BISECT_ONLY_SKIPPED_LEFT; > + } > + return res; > +} > + This side, I can understand what it wants to do to the "we only have skipped ones so we cannot continue" status. What is done in bisect_skipped_commits() is dubious, though (we'll see it later). > +static int bisect_skipped_commits(struct bisect_terms *terms) > +{ > + int res = 0; > + FILE *fp = NULL; > + struct rev_info revs; > + > + fp = fopen(git_path_bisect_log(), "a"); > + if (!fp) > + return error_errno(_("could not open '%s' for appending"), > + git_path_bisect_log()); > + > + res = prepare_revs(terms, &revs); > + > + if (!res) > + res = process_skipped_commits(fp, terms, &revs); > + > + fclose(fp); > + return res; > +} > + Opens the log to append to, or returns if we cannot. Calls prepare_revs() and process() if successfully prepared, and then close. No resource leak, and the returned value is the result code from the last function that matters. Looks good. > +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs) > +{ > + int res = 0; > + struct argv_array rev_argv = ARGV_ARRAY_INIT; > + > + prepare_rev_argv(terms, &rev_argv); > + > + /* > + * It is important to reset the flags used by revision walks > + * as the previous call to bisect_next_all() in turn > + * sets up a revision walk. > + */ > + reset_revision_walk(); > + init_revisions(revs, NULL); > + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL); > + if (prepare_revision_walk(revs)) > + res = error(_("revision walk setup failed\n")); > + > + argv_array_clear(&rev_argv); > + return res; > +} > + Unlike the one in rev_setup() above, the only codepath this thing is used is quite limited (i.e. after doing all the bisection computation including walking the graph and counting the weights with various bits in bisect_next) and we know all that is left to do is to run a single rev-list call, so it is clear to see that reset_revision_walk() is sufficient here. > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv) > +{ > + char *term_good = xstrfmt("%s-*", terms->term_good); > + > + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL); > + for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv); > + > + free(term_good); > +} > + This sets up to find commits that can be reached by BAD that cannot be reached by any GOOD revs, which is a quite faithful translation from the original one. > +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs) > +{ > + struct commit *commit; > + struct pretty_print_context pp = {0}; > + > + if (fprintf(fp, "# only skipped commits left to test\n") < 1) > + return -1; What's that comparison with "< 1" doing? That's a 36-byte message, and you are saying that it is OK if we showed only 10-byte from it, but it is not OK, even if the function did not report an output error by returning a negative value, if it returned that it wrote 0 bytes? I can understand it if it were if (fprintf(fp, "...") < 0) return error_errno(_("failed to write to ...")); though. > + while ((commit = get_revision(revs)) != NULL) { > + struct strbuf commit_name = STRBUF_INIT; > + format_commit_message(commit, "%s", > + &commit_name, &pp); > + fprintf(fp, "# possible first %s commit: [%s] %s\n", > + terms->term_bad, oid_to_hex(&commit->object.oid), > + commit_name.buf); > + strbuf_release(&commit_name); > + } Again, this is a faithful translation of the rev-list that appears in the original, provided that &revs was set up correctly, and relevant object flags cleared correctly before we start the traversal, both of which seem to be the case. > + /* > + * Reset the flags used by revision walks in case > + * there is another revision walk after this one. > + */ > + reset_revision_walk(); > + > + return 0; > +} > + So, overall, this step was a quite pleasant read, except for the very first bit above. Thanks. > +static int register_good_ref(const char *refname, > + const struct object_id *oid, int flags, > + void *cb_data) > +{ > + struct argv_array *rev_argv = cb_data; > + > + argv_array_push(rev_argv, oid_to_hex(oid)); > + return 0; > +} > + > +static int bisect_successful(struct bisect_terms *terms) > +{ > + struct object_id oid; > + struct commit *commit; > + struct pretty_print_context pp = {0}; > + struct strbuf commit_name = STRBUF_INIT; > + char *bad_ref = xstrfmt("refs/bisect/%s",terms->term_bad); > + int res; > + > + read_ref(bad_ref, &oid); > + printf("%s\n", bad_ref); > + commit = lookup_commit_reference(the_repository, &oid); > + format_commit_message(commit, "%s", &commit_name, &pp); > + > + res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n", > + terms->term_bad, oid_to_hex(&oid), > + commit_name.buf); > + > + strbuf_release(&commit_name); > + free(bad_ref); > + return res; > +} > +
Hi Junio, thank you very much for reviewing, I have just sent the new version of this patch series with your suggestions. El vie., 3 abr. 2020 a las 23:19, Junio C Hamano (<gitster@pobox.com>) escribió: > > Miriam Rubio <mirucam@gmail.com> writes: > > > diff --git a/bisect.c b/bisect.c > > index 9154f810f7..a50278ea7e 100644 > > --- a/bisect.c > > +++ b/bisect.c > > @@ -635,6 +635,11 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs, > > struct argv_array rev_argv = ARGV_ARRAY_INIT; > > int i; > > > > + /* > > + * `revs` could has been used before. > > + * Thus we first need to reset it. > > + */ > > + reset_revision_walk(); > > repo_init_revisions(r, revs, prefix); > > I don't have enough time and concentration to follow all the > codepaths in "bisect" that walk the commit graph starting here, but > seeing one codepath, e.g. check_ancestors(), after calling this and > walking with bisect_common(), uses clear_commit_marks_many() to > clear ALL_REV_FLAGS, not just the ones that reset_revision_walk() > clears, I am not sure what value this addition has. > > To put it differently, what codepath are you protecting the revision > walk that is about to happen against with this "reset"? Who are the > callers that could have used `revs` before calling this function and > touched only SEEN, ADDED, SHOWN, etc. without touching other bits > like COUNTED, TREESAME, UNINTERESTING that matter to the correct > operation of "bisect"? > > The rest of the patch looks quite reasonably done. Let me comment > on the patch out of order (in other words, I'll rearrange the > functions in the order I read them). I am realizing that I feel it > easier to understand to read the code in a bottom-up fashion. > > > +static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix) > > +{ > > + if (bisect_next_check(terms, NULL)) > > + return BISECT_OK; > > The bisect_next_check() function returns what decide_next() returns, > which is either 0 or error() which is -1. So this says "if > nect-check says there was an error, we return OK". For the purpose > of not proceeding to bisect_next(), returning is perfectly correct, > but is the value returned correct? The scripted original does > > git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || : > > meaning "try next-check and go on to bisect_next if check succeeds; > either way, ignore all errors from them". So it probably is more > faithful conversion to make the returned value from auto_next() > void. > > > + > > + return bisect_next(terms, prefix); > > +} > > In any case, this conversion of auto_next looks like a good one, > with or without fixing its type. The caller in cmd_bisect__helper() > seems to store the returned value in 'res' and uses it for the exit > status, but for this to be a faithful conversion, it should ignore > the returned value from here and always exit with success (and if we > do so, it is one more reason why we would want to update the type of > this function to return void). > I cannot change bisect_auto_next() to return void because in shell the bisect_next() function used "exit $res" , so the following code: git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || : can result that whole `git bisect` exits with an error code when bisect_next() does an "exit $res" with $res != 0. So errors from bisect_next() are not ignored and I cannot make bisect_auto_next() return void. Best, Miriam. > > +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix) > > +{ > > + int no_checkout; > > + enum bisect_error res; > > > > + if (bisect_next_check(terms, terms->term_good)) > > + return BISECT_FAILED; > > The original makes sure it does not get any argument, but that is > done in cmd_bisect__helper(), so it is OK. > > The next thing the original does is to call bisect_autostart, before > doing the next-check. Was it a dead code that wasn't necessary, or > is its loss a regression during the conversion? > > > + > > + no_checkout = !is_empty_or_missing_file(git_path_bisect_head()); > > + > > + /* Perform all bisection computation, display and checkout */ > > + res = bisect_next_all(the_repository, prefix, no_checkout); > > Looking good. We've already converted next_all() in the previous > series, and we call it the same way as the original by checking if > $GIT_DIR/BISECT_HEAD is there. The original said "if BISECT_HEAD > exists as a file, use '--no-checkout'" and did not care if its empty > or not, but the updated code seems to care. Does the difference > matter (i.e. is it more correct to ignore an empty BISECT_HEAD and > pretend as if it did not exist)? > > > + if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) { > > + res = bisect_successful(terms); > > + return res ? res : BISECT_INTERNAL_SUCCESS_MERGE_BASE; > > It is unclear why "1st bad commit found" is turned into "success > merge base" here. bisect_successfull() returns an error when it > cannot append to the log, and otherwise we would want to relay "we > are done successfully" back to the caller, no? Puzzled.... > > > + } else if (res == BISECT_ONLY_SKIPPED_LEFT) { > > + res = bisect_skipped_commits(terms); > > + return res ? res : BISECT_ONLY_SKIPPED_LEFT; > > + } > > + return res; > > +} > > + > > This side, I can understand what it wants to do to the "we only have > skipped ones so we cannot continue" status. > > What is done in bisect_skipped_commits() is dubious, though (we'll > see it later). > > > > +static int bisect_skipped_commits(struct bisect_terms *terms) > > +{ > > + int res = 0; > > + FILE *fp = NULL; > > + struct rev_info revs; > > + > > + fp = fopen(git_path_bisect_log(), "a"); > > + if (!fp) > > + return error_errno(_("could not open '%s' for appending"), > > + git_path_bisect_log()); > > + > > + res = prepare_revs(terms, &revs); > > + > > + if (!res) > > + res = process_skipped_commits(fp, terms, &revs); > > + > > + fclose(fp); > > + return res; > > +} > > + > > Opens the log to append to, or returns if we cannot. Calls > prepare_revs() and process() if successfully prepared, and then > close. No resource leak, and the returned value is the result code > from the last function that matters. Looks good. > > > +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs) > > +{ > > + int res = 0; > > + struct argv_array rev_argv = ARGV_ARRAY_INIT; > > + > > + prepare_rev_argv(terms, &rev_argv); > > + > > + /* > > + * It is important to reset the flags used by revision walks > > + * as the previous call to bisect_next_all() in turn > > + * sets up a revision walk. > > + */ > > + reset_revision_walk(); > > + init_revisions(revs, NULL); > > + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL); > > + if (prepare_revision_walk(revs)) > > + res = error(_("revision walk setup failed\n")); > > + > > + argv_array_clear(&rev_argv); > > + return res; > > +} > > + > > Unlike the one in rev_setup() above, the only codepath this thing is > used is quite limited (i.e. after doing all the bisection > computation including walking the graph and counting the weights > with various bits in bisect_next) and we know all that is left to do > is to run a single rev-list call, so it is clear to see that > reset_revision_walk() is sufficient here. > > > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv) > > +{ > > + char *term_good = xstrfmt("%s-*", terms->term_good); > > + > > + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL); > > + for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv); > > + > > + free(term_good); > > +} > > + > > This sets up to find commits that can be reached by BAD that cannot > be reached by any GOOD revs, which is a quite faithful translation > from the original one. > > > +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs) > > +{ > > + struct commit *commit; > > + struct pretty_print_context pp = {0}; > > + > > + if (fprintf(fp, "# only skipped commits left to test\n") < 1) > > + return -1; > > What's that comparison with "< 1" doing? That's a 36-byte message, > and you are saying that it is OK if we showed only 10-byte from it, > but it is not OK, even if the function did not report an output error > by returning a negative value, if it returned that it wrote 0 bytes? > > I can understand it if it were > > if (fprintf(fp, "...") < 0) > return error_errno(_("failed to write to ...")); > > though. > > > + while ((commit = get_revision(revs)) != NULL) { > > + struct strbuf commit_name = STRBUF_INIT; > > + format_commit_message(commit, "%s", > > + &commit_name, &pp); > > + fprintf(fp, "# possible first %s commit: [%s] %s\n", > > + terms->term_bad, oid_to_hex(&commit->object.oid), > > + commit_name.buf); > > + strbuf_release(&commit_name); > > + } > > Again, this is a faithful translation of the rev-list that appears > in the original, provided that &revs was set up correctly, and > relevant object flags cleared correctly before we start the > traversal, both of which seem to be the case. > > > + /* > > + * Reset the flags used by revision walks in case > > + * there is another revision walk after this one. > > + */ > > + reset_revision_walk(); > > + > > + return 0; > > +} > > + > > So, overall, this step was a quite pleasant read, except for the > very first bit above. > > Thanks. > > > +static int register_good_ref(const char *refname, > > + const struct object_id *oid, int flags, > > + void *cb_data) > > +{ > > + struct argv_array *rev_argv = cb_data; > > + > > + argv_array_push(rev_argv, oid_to_hex(oid)); > > + return 0; > > +} > > + > > +static int bisect_successful(struct bisect_terms *terms) > > +{ > > + struct object_id oid; > > + struct commit *commit; > > + struct pretty_print_context pp = {0}; > > + struct strbuf commit_name = STRBUF_INIT; > > + char *bad_ref = xstrfmt("refs/bisect/%s",terms->term_bad); > > + int res; > > + > > + read_ref(bad_ref, &oid); > > + printf("%s\n", bad_ref); > > + commit = lookup_commit_reference(the_repository, &oid); > > + format_commit_message(commit, "%s", &commit_name, &pp); > > + > > + res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n", > > + terms->term_bad, oid_to_hex(&oid), > > + commit_name.buf); > > + > > + strbuf_release(&commit_name); > > + free(bad_ref); > > + return res; > > +} > > +
diff --git a/bisect.c b/bisect.c index 9154f810f7..a50278ea7e 100644 --- a/bisect.c +++ b/bisect.c @@ -635,6 +635,11 @@ static void bisect_rev_setup(struct repository *r, struct rev_info *revs, struct argv_array rev_argv = ARGV_ARRAY_INIT; int i; + /* + * `revs` could has been used before. + * Thus we first need to reset it. + */ + reset_revision_walk(); repo_init_revisions(r, revs, prefix); revs->abbrev = 0; revs->commit_format = CMIT_FMT_UNSPECIFIED; @@ -980,6 +985,12 @@ void read_bisect_terms(const char **read_bad, const char **read_good) * the bisection process finished successfully. * In this case the calling function or command should not turn a * BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND return code into an error or a non zero exit code. + * + * Checking BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND + * in bisect_helper::bisect_next() and only transforming it to 0 at + * the end of bisect_helper::cmd_bisect__helper() helps bypassing + * all the code related to finding a commit to test. + * * If no_checkout is non-zero, the bisection process does not * checkout the trial commit but instead simply updates BISECT_HEAD. */ diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index e949ea74e2..447e9e75db 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -8,6 +8,7 @@ #include "run-command.h" #include "prompt.h" #include "quote.h" +#include "revision.h" static GIT_PATH_FUNC(git_path_bisect_terms, "BISECT_TERMS") static GIT_PATH_FUNC(git_path_bisect_expected_rev, "BISECT_EXPECTED_REV") @@ -29,6 +30,8 @@ static const char * const git_bisect_helper_usage[] = { N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"), N_("git bisect--helper --bisect-start [--term-{old,good}=<term> --term-{new,bad}=<term>]" "[--no-checkout] [<bad> [<good>...]] [--] [<paths>...]"), + N_("git bisect--helper --bisect-next"), + N_("git bisect--helper --bisect-auto-next"), NULL }; @@ -436,6 +439,149 @@ static int bisect_append_log_quoted(const char **argv) return res; } +static int register_good_ref(const char *refname, + const struct object_id *oid, int flags, + void *cb_data) +{ + struct argv_array *rev_argv = cb_data; + + argv_array_push(rev_argv, oid_to_hex(oid)); + return 0; +} + +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv) +{ + char *term_good = xstrfmt("%s-*", terms->term_good); + + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL); + for_each_glob_ref_in(register_good_ref, term_good, "refs/bisect/", rev_argv); + + free(term_good); +} + +static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs) +{ + int res = 0; + struct argv_array rev_argv = ARGV_ARRAY_INIT; + + prepare_rev_argv(terms, &rev_argv); + + /* + * It is important to reset the flags used by revision walks + * as the previous call to bisect_next_all() in turn + * sets up a revision walk. + */ + reset_revision_walk(); + init_revisions(revs, NULL); + rev_argv.argc = setup_revisions(rev_argv.argc, rev_argv.argv, revs, NULL); + if (prepare_revision_walk(revs)) + res = error(_("revision walk setup failed\n")); + + argv_array_clear(&rev_argv); + return res; +} + +static int process_skipped_commits(FILE *fp, struct bisect_terms *terms, struct rev_info *revs) +{ + struct commit *commit; + struct pretty_print_context pp = {0}; + + if (fprintf(fp, "# only skipped commits left to test\n") < 1) + return -1; + + while ((commit = get_revision(revs)) != NULL) { + struct strbuf commit_name = STRBUF_INIT; + format_commit_message(commit, "%s", + &commit_name, &pp); + fprintf(fp, "# possible first %s commit: [%s] %s\n", + terms->term_bad, oid_to_hex(&commit->object.oid), + commit_name.buf); + strbuf_release(&commit_name); + } + + /* + * Reset the flags used by revision walks in case + * there is another revision walk after this one. + */ + reset_revision_walk(); + + return 0; +} + +static int bisect_skipped_commits(struct bisect_terms *terms) +{ + int res = 0; + FILE *fp = NULL; + struct rev_info revs; + + fp = fopen(git_path_bisect_log(), "a"); + if (!fp) + return error_errno(_("could not open '%s' for appending"), + git_path_bisect_log()); + + res = prepare_revs(terms, &revs); + + if (!res) + res = process_skipped_commits(fp, terms, &revs); + + fclose(fp); + return res; +} + +static int bisect_successful(struct bisect_terms *terms) +{ + struct object_id oid; + struct commit *commit; + struct pretty_print_context pp = {0}; + struct strbuf commit_name = STRBUF_INIT; + char *bad_ref = xstrfmt("refs/bisect/%s",terms->term_bad); + int res; + + read_ref(bad_ref, &oid); + printf("%s\n", bad_ref); + commit = lookup_commit_reference(the_repository, &oid); + format_commit_message(commit, "%s", &commit_name, &pp); + + res = write_in_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s\n", + terms->term_bad, oid_to_hex(&oid), + commit_name.buf); + + strbuf_release(&commit_name); + free(bad_ref); + return res; +} + +static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix) +{ + int no_checkout; + enum bisect_error res; + + if (bisect_next_check(terms, terms->term_good)) + return BISECT_FAILED; + + no_checkout = !is_empty_or_missing_file(git_path_bisect_head()); + + /* Perform all bisection computation, display and checkout */ + res = bisect_next_all(the_repository, prefix, no_checkout); + + if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) { + res = bisect_successful(terms); + return res ? res : BISECT_INTERNAL_SUCCESS_MERGE_BASE; + } else if (res == BISECT_ONLY_SKIPPED_LEFT) { + res = bisect_skipped_commits(terms); + return res ? res : BISECT_ONLY_SKIPPED_LEFT; + } + return res; +} + +static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix) +{ + if (bisect_next_check(terms, NULL)) + return BISECT_OK; + + return bisect_next(terms, prefix); +} + static int bisect_start(struct bisect_terms *terms, int no_checkout, const char **argv, int argc) { @@ -640,7 +786,9 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) CHECK_AND_SET_TERMS, BISECT_NEXT_CHECK, BISECT_TERMS, - BISECT_START + BISECT_START, + BISECT_NEXT, + BISECT_AUTO_NEXT, } cmdmode = 0; int no_checkout = 0, res = 0, nolog = 0; struct option options[] = { @@ -664,6 +812,10 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("print out the bisect terms"), BISECT_TERMS), OPT_CMDMODE(0, "bisect-start", &cmdmode, N_("start the bisect session"), BISECT_START), + OPT_CMDMODE(0, "bisect-next", &cmdmode, + N_("find the next bisection commit"), BISECT_NEXT), + OPT_CMDMODE(0, "bisect-auto-next", &cmdmode, + N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT), OPT_BOOL(0, "no-checkout", &no_checkout, N_("update BISECT_HEAD instead of checking out the current commit")), OPT_BOOL(0, "no-log", &nolog, @@ -725,6 +877,18 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) set_terms(&terms, "bad", "good"); res = bisect_start(&terms, no_checkout, argv, argc); break; + case BISECT_NEXT: + if (argc) + return error(_("--bisect-next requires 0 arguments")); + get_terms(&terms); + res = bisect_next(&terms, prefix); + break; + case BISECT_AUTO_NEXT: + if (argc) + return error(_("--bisect-auto-next requires 0 arguments")); + get_terms(&terms); + res = bisect_auto_next(&terms, prefix); + break; default: res = error(_("BUG: unknown subcommand.")); } diff --git a/git-bisect.sh b/git-bisect.sh index efee12b8b1..e03f210d55 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -86,8 +86,7 @@ bisect_start() { # # Check if we can proceed to the next bisect state. # - get_terms - bisect_auto_next + git bisect--helper --bisect-auto-next || exit trap '-' 0 } @@ -140,45 +139,7 @@ bisect_state() { *) usage ;; esac - bisect_auto_next -} - -bisect_auto_next() { - git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD && bisect_next || : -} - -bisect_next() { - case "$#" in 0) ;; *) usage ;; esac - bisect_autostart - git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD $TERM_GOOD|| exit - - # Perform all bisection computation, display and checkout - git bisect--helper --next-all $(test -f "$GIT_DIR/BISECT_HEAD" && echo --no-checkout) - res=$? - - # Check if we should exit because bisection is finished - if test $res -eq 10 - then - bad_rev=$(git show-ref --hash --verify refs/bisect/$TERM_BAD) - bad_commit=$(git show-branch $bad_rev) - echo "# first $TERM_BAD commit: $bad_commit" >>"$GIT_DIR/BISECT_LOG" - exit 0 - elif test $res -eq 2 - then - echo "# only skipped commits left to test" >>"$GIT_DIR/BISECT_LOG" - good_revs=$(git for-each-ref --format="%(objectname)" "refs/bisect/$TERM_GOOD-*") - for skipped in $(git rev-list refs/bisect/$TERM_BAD --not $good_revs) - do - skipped_commit=$(git show-branch $skipped) - echo "# possible first $TERM_BAD commit: $skipped_commit" >>"$GIT_DIR/BISECT_LOG" - done - exit $res - fi - - # Check for an error in the bisection process - test $res -ne 0 && exit $res - - return 0 + git bisect--helper --bisect-auto-next } bisect_visualize() { @@ -232,7 +193,7 @@ bisect_replay () { die "$(gettext "?? what are you talking about?")" ;; esac done <"$file" - bisect_auto_next + git bisect--helper --bisect-auto-next } bisect_run () { @@ -329,7 +290,7 @@ case "$#" in bisect_skip "$@" ;; next) # Not sure we want "next" at the UI level anymore. - bisect_next "$@" ;; + git bisect--helper --bisect-next "$@" || exit ;; visualize|view) bisect_visualize "$@" ;; reset)