| Message ID | 20200226101429.81327-3-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: > +static int register_good_ref(const char *refname, > + const struct object_id *oid, int flags, > + void *cb_data) > +{ > + struct string_list *good_refs = cb_data; > + > + string_list_append(good_refs, oid_to_hex(oid)); > + return 0; > +} > + > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv) > +{ > + struct string_list good_revs = STRING_LIST_INIT_DUP; > + char *term_good = xstrfmt("%s-*", terms->term_good); > + > + for_each_glob_ref_in(register_good_ref, term_good, > + "refs/bisect/", &good_revs); > + > + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL); > + for (int i = 0; i < good_revs.nr; i++) > + argv_array_push(rev_argv, good_revs.items[i].string); > + > + string_list_clear(&good_revs, 0); > + free(term_good); Why do you need good_revs string_list in the first place? Wouldn't it be simpler and easier to understand to pass in the argv as part of the callback data and push the good refs in the callback function? > +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 > + * setups a revision walk. "setup" is not a verb X-<. "... in turn sets up a ..." would be OK. > +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); > + clear_commit_marks(commit, ALL_REV_FLAGS); clear_commit_marks() is to clear the given flag bits from the named commit *AND* all of its ancestors (recursively) as long as they have these bits on, and it typically is used in order to clean up the state bits left on objects _after_ a revision walk is _done_. Calling it, especially to clear ALL_REV_FLAGS that contains crucial flag bits necessary to drive get_revision(), inside a loop that is still walking commits one by one by calling get_revision(), is extremely unusual. It would be surprising if this code were correct. It may be that it happens to (appear to) work because parents of the commit hasn't been painted and calling the helper only clears the mark from the commit (so we won't see repeated "painting down to the root commit") in which case this might be an extremely expensive looking variant of commit->object.flags &= ~ALL_REV_FLAGS; that only confuses the readers. Even then, I think by clearing bits like SEEN from commit, it breaks the revision traversal machinery---for example, doesn't this mean that the commit we just processed can be re-visited by get_revision() without deduping in a history with forks and merges? Has this been shown to any of your mentors before sending it to the list? > +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); > + char *content; > + 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); > + > + content = xstrfmt("# first %s commit: [%s] %s", > + terms->term_bad, oid_to_hex(&oid), > + commit_name.buf); Strange indentation. > + res = write_in_file(git_path_bisect_log(), content, 1); So this is a new use of the helper introduced in [01/10]. It is true that use of it lets this caller not to worry about opening, writing and closing, but it needs an extra allocation to prepare "content". If the calling convention were more like this: res = write_to_file(git_path_bisect_log(), "a", "# first %s commit: [%s] %s", terms->term_bad, oid_to_hex(&oid), commit_name.buf, NULL); this callsite and the callsite in [01/10] wouldn't have had to make an unnecessary allocation, perhaps?
Hi Junio, El mié., 26 feb. 2020 a las 20:34, Junio C Hamano (<gitster@pobox.com>) escribió: > > Miriam Rubio <mirucam@gmail.com> writes: > > > +static int register_good_ref(const char *refname, > > + const struct object_id *oid, int flags, > > + void *cb_data) > > +{ > > + struct string_list *good_refs = cb_data; > > + > > + string_list_append(good_refs, oid_to_hex(oid)); > > + return 0; > > +} > > + > > +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv) > > +{ > > + struct string_list good_revs = STRING_LIST_INIT_DUP; > > + char *term_good = xstrfmt("%s-*", terms->term_good); > > + > > + for_each_glob_ref_in(register_good_ref, term_good, > > + "refs/bisect/", &good_revs); > > + > > + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL); > > + for (int i = 0; i < good_revs.nr; i++) > > + argv_array_push(rev_argv, good_revs.items[i].string); > > + > > + string_list_clear(&good_revs, 0); > > + free(term_good); > > Why do you need good_revs string_list in the first place? Wouldn't > it be simpler and easier to understand to pass in the argv as part > of the callback data and push the good refs in the callback function? > Ok, I will do it that way. > > +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 > > + * setups a revision walk. > > "setup" is not a verb X-<. "... in turn sets up a ..." would be OK. > Noted. > > +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); > > + clear_commit_marks(commit, ALL_REV_FLAGS); > > clear_commit_marks() is to clear the given flag bits from the named > commit *AND* all of its ancestors (recursively) as long as they have > these bits on, and it typically is used in order to clean up the > state bits left on objects _after_ a revision walk is _done_. > > Calling it, especially to clear ALL_REV_FLAGS that contains crucial > flag bits necessary to drive get_revision(), inside a loop that is > still walking commits one by one by calling get_revision(), is > extremely unusual. > > It would be surprising if this code were correct. It may be that it > happens to (appear to) work because parents of the commit hasn't > been painted and calling the helper only clears the mark from the > commit (so we won't see repeated "painting down to the root commit") > in which case this might be an extremely expensive looking variant > of > > commit->object.flags &= ~ALL_REV_FLAGS; > > that only confuses the readers. > > Even then, I think by clearing bits like SEEN from commit, it breaks > the revision traversal machinery---for example, doesn't this mean > that the commit we just processed can be re-visited by > get_revision() without deduping in a history with forks and merges? > > Has this been shown to any of your mentors before sending it to the > list? Adding clear_commit_marks() was a suggestion of a previous review of this patch: https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/ And of course, my mentor always reviews my patch series before sending. > > > +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); > > + char *content; > > + 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); > > + > > + content = xstrfmt("# first %s commit: [%s] %s", > > + terms->term_bad, oid_to_hex(&oid), > > + commit_name.buf); > > Strange indentation. Noted. > > > + res = write_in_file(git_path_bisect_log(), content, 1); > > So this is a new use of the helper introduced in [01/10]. It is > true that use of it lets this caller not to worry about opening, > writing and closing, but it needs an extra allocation to prepare > "content". > > If the calling convention were more like this: > > res = write_to_file(git_path_bisect_log(), "a", > "# first %s commit: [%s] %s", > terms->term_bad, oid_to_hex(&oid), commit_name.buf, > NULL); > > this callsite and the callsite in [01/10] wouldn't have had to make > an unnecessary allocation, perhaps? > Aha. I will change it. This helper function was also a suggestion of the previous reviewer: https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/ Thank you very much for reviewing!. Best, Miriam
"Miriam R." <mirucam@gmail.com> writes: >> It would be surprising if this code were correct. It may be that it >> happens to (appear to) work because parents of the commit hasn't >> been painted and calling the helper only clears the mark from the >> commit (so we won't see repeated "painting down to the root commit") >> in which case this might be an extremely expensive looking variant >> of >> >> commit->object.flags &= ~ALL_REV_FLAGS; >> >> that only confuses the readers. >> >> Even then, I think by clearing bits like SEEN from commit, it breaks >> the revision traversal machinery---for example, doesn't this mean >> that the commit we just processed can be re-visited by >> get_revision() without deduping in a history with forks and merges? >> >> Has this been shown to any of your mentors before sending it to the >> list? > > Adding clear_commit_marks() was a suggestion of a previous review of this patch: > https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/ > > And of course, my mentor always reviews my patch series before sending. OK, I just didn't know how you and your mentors work. Some leave their door open for mentees and help them when asked but otherwise act as an ordinary reviewer who somehow prioritises reviewing the work by their mentees. So your mentors may be a better source of information why this piece of code, which I still do not know how it could be correct, is supposed to work. Good. After reading the above URL, I think you may have misread the suggestion you were given. Resetting using clear_commit_marks() may be necessary, but you do so when you finished walking so that you can do unrelated revision walk later. For that, you clear the flag bits after the while() loop that asks get_revision() to yield commits are done, using the initial set of commits that you used to start iteration. That is how bisect.c::check_ancestors() work, that is - it initializes a rev_info 'revs' from an array of commit rev[] - it lets bisect_common() use the 'revs', which is allowed to smudge the flag bits of commit objects. - it uses clear_commit_marks_many() to clear the flags of the commits whose flag bits may have been smudged and their ancestors, recursively. In order to use as the starting points, the original array of commit rev[] that started the revision traversal is used.
Hi Junio, El jue., 27 feb. 2020 a las 17:41, Junio C Hamano (<gitster@pobox.com>) escribió: > > "Miriam R." <mirucam@gmail.com> writes: > > >> It would be surprising if this code were correct. It may be that it > >> happens to (appear to) work because parents of the commit hasn't > >> been painted and calling the helper only clears the mark from the > >> commit (so we won't see repeated "painting down to the root commit") > >> in which case this might be an extremely expensive looking variant > >> of > >> > >> commit->object.flags &= ~ALL_REV_FLAGS; > >> > >> that only confuses the readers. > >> > >> Even then, I think by clearing bits like SEEN from commit, it breaks > >> the revision traversal machinery---for example, doesn't this mean > >> that the commit we just processed can be re-visited by > >> get_revision() without deduping in a history with forks and merges? > >> > >> Has this been shown to any of your mentors before sending it to the > >> list? > > > > Adding clear_commit_marks() was a suggestion of a previous review of this patch: > > https://public-inbox.org/git/nycvar.QRO.7.76.6.2001301619340.46@tvgsbejvaqbjf.bet/ > > > > And of course, my mentor always reviews my patch series before sending. > > OK, I just didn't know how you and your mentors work. Some leave > their door open for mentees and help them when asked but otherwise > act as an ordinary reviewer who somehow prioritises reviewing the > work by their mentees. So your mentors may be a better source of > information why this piece of code, which I still do not know how it > could be correct, is supposed to work. Good. > > After reading the above URL, I think you may have misread the > suggestion you were given. Resetting using clear_commit_marks() may > be necessary, but you do so when you finished walking so that you > can do unrelated revision walk later. For that, you clear the flag > bits after the while() loop that asks get_revision() to yield > commits are done, using the initial set of commits that you used to > start iteration. > > That is how bisect.c::check_ancestors() work, that is > > - it initializes a rev_info 'revs' from an array of commit rev[] > > - it lets bisect_common() use the 'revs', which is allowed to > smudge the flag bits of commit objects. > > - it uses clear_commit_marks_many() to clear the flags of the > commits whose flag bits may have been smudged and their > ancestors, recursively. In order to use as the starting points, > the original array of commit rev[] that started the revision > traversal is used. Thank you for your explanation. To my understanding, it looks like calling reset_revision_walk() after the while() loop should be enough. Am I right or am I missing something?
"Miriam R." <mirucam@gmail.com> writes: > To my understanding, it looks like calling reset_revision_walk() after > the while() loop should be enough. Am I right or am I missing > something? I suspect that reset_revision_walk() may be too-big a hammer, as it clears everything, regardless of the reason why the flag bits were set. On the other hand, the clearly strategy that uses clear_commit_marks() is to clear only the flag bits that were set during the previous revision walk from only the commits that were walked during the previous revision walk. I offhand do not know what flag bits on what objects that were not involved in the previous revision walk are still necessary at the point of the call made by the caller (that's a question for your mentors who volunteered their expertise on the program in question), so if there isn't any, reset_revision_walk() may be an easy way out. I just do not know if it clears too much to break the code that comes after the function returns. Thanks.
On Fri, Mar 6, 2020 at 8:06 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Miriam R." <mirucam@gmail.com> writes: > > > To my understanding, it looks like calling reset_revision_walk() after > > the while() loop should be enough. Am I right or am I missing > > something? > > I suspect that reset_revision_walk() may be too-big a hammer, as it > clears everything, regardless of the reason why the flag bits were > set. On the other hand, the clearly strategy that uses > clear_commit_marks() is to clear only the flag bits that were set > during the previous revision walk from only the commits that were > walked during the previous revision walk. > > I offhand do not know what flag bits on what objects that were not > involved in the previous revision walk are still necessary at the > point of the call made by the caller (that's a question for your > mentors who volunteered their expertise on the program in question), > so if there isn't any, reset_revision_walk() may be an easy way out. > I just do not know if it clears too much to break the code that > comes after the function returns. process_skipped_commits(), the function that does this revision walk, is called by bisect_skipped_commits() to print the possible first bad commits when there are only skipped commits left to test and we therefore cannot bisect more. This can be seen in bisect_next() which does basically the following: bisect_next() { ... /* Perform all bisection computation, display and checkout */ res = bisect_next_all(the_repository, prefix, no_checkout); if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) { ... } else if (res == BISECT_ONLY_SKIPPED_LEFT) { res = bisect_skipped_commits(terms); return res ? res : BISECT_ONLY_SKIPPED_LEFT; } return res; } BISECT_ONLY_SKIPPED_LEFT is an error code (-2) so bisect_next() will always return an error in this case. This means that the revision walk in process_skipped_commits() is very likely to be the last revision walk performed by the command. So my opinion is that not clearing anything at the end of that revision walk is fine. If we are worried about what could happen one day, when people might be interested in actually doing another revision walk after this one, then as we don't know what they will want to do and might be interested in, cleaning everything with reset_revision_walk() might be the safest thing to do and is probably cheap enough that it's ok to use it right now. Thanks for your review, Christian.
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 ee1be630da..c82ffe9c1c 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 }; @@ -431,6 +434,155 @@ 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 string_list *good_refs = cb_data; + + string_list_append(good_refs, oid_to_hex(oid)); + return 0; +} + +static void prepare_rev_argv(struct bisect_terms *terms, struct argv_array *rev_argv) +{ + struct string_list good_revs = STRING_LIST_INIT_DUP; + char *term_good = xstrfmt("%s-*", terms->term_good); + + for_each_glob_ref_in(register_good_ref, term_good, + "refs/bisect/", &good_revs); + + argv_array_pushl(rev_argv, "skipped_commits", "refs/bisect/bad", "--not", NULL); + for (int i = 0; i < good_revs.nr; i++) + argv_array_push(rev_argv, good_revs.items[i].string); + + string_list_clear(&good_revs, 0); + 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 + * setups 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); + clear_commit_marks(commit, ALL_REV_FLAGS); + } + + 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); + char *content; + 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); + + content = xstrfmt("# first %s commit: [%s] %s", + terms->term_bad, oid_to_hex(&oid), + commit_name.buf); + + res = write_in_file(git_path_bisect_log(), content, 1); + + strbuf_release(&commit_name); + free(bad_ref); + free(content); + 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) { @@ -635,7 +787,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[] = { @@ -659,6 +813,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, @@ -720,6 +878,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: return error("BUG: unknown subcommand '%d'", cmdmode); } 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)