| Message ID | 20200423070704.39872-10-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Finish converting git bisect to C part 2 | expand |
Hi Miriam, On Thu, 23 Apr 2020, Miriam Rubio wrote: > From: Pranit Bauva <pranit.bauva@gmail.com> > > Reimplement the `bisect_state()` shell functions in C and also add a > subcommand `--bisect-state` to `git-bisect--helper` to call them from > git-bisect.sh . > > Using `--bisect-state` subcommand is a temporary measure to port shell > function to C so as to use the existing test suite. As more functions > are ported, this subcommand will be retired and will be called by some > other methods. > > `bisect_head()` is only called from `bisect_state()`, thus it is not > required to introduce another subcommand. > > Mentored-by: Lars Schneider <larsxschneider@gmail.com> > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> > Signed-off-by: Miriam Rubio <mirucam@gmail.com> > --- > builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++- > git-bisect.sh | 55 +++---------------------------- > 2 files changed, 73 insertions(+), 52 deletions(-) > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 2d8660c79f..9db72f5891 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = { > N_("git bisect--helper --bisect-next"), > N_("git bisect--helper --bisect-auto-next"), > N_("git bisect--helper --bisect-autostart"), > + N_("git bisect--helper --bisect-state (bad|new) [<rev>]"), > + N_("git bisect--helper --bisect-state (good|old) [<rev>...]"), > NULL > }; > > @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms) > return bisect_start(terms, 0, NULL, 0); > } > > +static int bisect_head(struct object_id *oid) > +{ > + if (!file_exists(git_path_bisect_head())) > + return get_oid("HEAD", oid); > + > + return get_oid("BISECT_HEAD", oid); This can be easily reduced to return get_oid(file_exists(git_path_bisect_head()) ? "BISECT_HEAD" : "HEAD", oid); At the same time, it is wrong, just like the shell script version was wrong: in particular in light of the `hn/reftable` effort, we do _not_ want to assume that all refs are backed by files! So really, what this should do instead is this: enum get_oid_result res = get_oid("BISECT_HEAD", oid); if (res == MISSING_OBJECT) res = get_oid("HEAD", oid); Given that this is still only three lines long, the overhead of having it in its own function for just a _single_ call seems excessive. I'd prefer it to be inlined in `bisect_state()`. > +} > + > +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv, > + int argc) > +{ I offered a lengthy discussion about this function in https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/ It does not look, however, as if v3 benefitted from the entirety of my analysis: All the `check_expected_revs()` function does is to verify that the passed list of revs matches exactly the contents of the `BISECT_EXPECTED_REV` file. That can be done in a much simpler way, though, by first reading the file and parsing the contents into an OID, and then comparing to that parsed OID instead. Besides, `check_expected_revs()` is only used to check one rev at a time. In other words, it could be simplified to something like this: static void check_expected_rev(struct object_id *oid) { struct object_id expected; struct strbuf buf = STRBUF_INIT; if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz || get_oid_hex(buf.buf, &expected) < 0) return; /* Ignore invalid file contents */ if (!oideq(oid, &expected)) { ... unlink ... return; } } But even that would be wasteful, as we would read the file over and over and over again. The good news is that we do not even _need_ `check_expected_rev()`. Because we do not need to have two call sites, we can simplify the code much further. See below: > + const char *state; > + const char *hex; > + int i; > + struct oid_array revs = OID_ARRAY_INIT; > + struct object_id oid; > + > + if (!argc) > + return error(_("Please call `--bisect-state` with at least one argument")); > + state = argv[0]; > + if (check_and_set_terms(terms, state) || > + !one_of(state, terms->term_good,terms->term_bad, "skip", NULL)) > + return BISECT_FAILED; > + argv++; > + argc--; > + if (!strcmp(state, terms->term_bad) && (argc > 1)) > + return error(_("'git bisect %s' can take only one argument."),terms->term_bad); > + if (argc == 0) { > + if (bisect_head(&oid)) > + return error(_("Bad bisect_head rev input")); > + hex = oid_to_hex(&oid); > + if (bisect_write(state, hex, terms, 0)) > + return BISECT_FAILED; > + check_expected_revs(&hex, 1); > + return bisect_auto_next(terms, NULL); > + } > + > + /* Here argc > 0 */ > + for (; argc; argc--, argv++) { > + struct object_id oid; > + if (get_oid(*argv, &oid)) > + return error(_("Bad rev input: %s"), *argv); > + oid_array_append(&revs, &oid); > + } It really does not make sense to parse the arguments into an OID array, _then_ iterate over the array once, and then immediately releasing it. That OID array is not needed at all. So we'll end up with this loop in case `argc > 0` (where we now call `get_oid()`, too), and note how the loop body looks _eerily_ similar to the conditional `argc == 0` code block above? > + > + for (i = 0; i < revs.nr; i++) { > + hex = oid_to_hex(&revs.oid[i]); > + if (bisect_write(state, hex, terms, 0)) { > + oid_array_clear(&revs); > + return BISECT_FAILED; > + } > + check_expected_revs(&hex, 1); > + } > + > + oid_array_clear(&revs); > + return bisect_auto_next(terms, NULL); > +} So really, this function pretty much _wants_ to look this way (modulo bugs, as I did not even test-compile the code): static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv, int argc) { const char *state; int i, verify_expected = 1; struct object_id oid, expected; struct strbuf buf = STRBUF_INIT; if (!argc) return error(_("Please call `--bisect-state` with at least one argument")); state = argv[0]; if (check_and_set_terms(terms, state) || !one_of(state, terms->term_good, terms->term_bad, "skip", NULL)) return BISECT_FAILED; argv++; argc--; if (argc > 1 && !strcmp(state, terms->term_bad)) return error(_("'git bisect %s' can take only one argument."), terms->term_bad); if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz || get_oid_hex(buf.buf, &expected) < 0) verify_expected = 0; /* Ignore invalid file contents */ for (i = 0; i < argc + !argc; i++) { if (argc) { if (get_oid(argv[i], &oid)) { error(_("Bad rev input: %s"), *argv); return BISECT_FAILED; } } else { enum get_oid_result res = get_oid("BISECT_HEAD", &oid); if (res == MISSING_OBJECT) res = get_oid("HEAD", &oid); if (res) { error(_("Bad bisect_head rev input")); return BISECT_FAILED; } } if (bisect_write(state, oid_to_hex(&oid), terms, 0)) return BISECT_FAILED; if (verify_expected && !oideq(&oid, &expected)) { unlink_or_warn(git_path_bisect_ancestors_ok()); unlink_or_warn(git_path_bisect_expected_rev()); verify_expected = 0; } } return bisect_auto_next(terms, NULL); } There, not bad, is it? > + > int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > { > enum { > @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > BISECT_START, > BISECT_NEXT, > BISECT_AUTO_NEXT, > - BISECT_AUTOSTART > + BISECT_AUTOSTART, > + BISECT_STATE > } cmdmode = 0; > int no_checkout = 0, res = 0, nolog = 0; > struct option options[] = { > @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT), > OPT_CMDMODE(0, "bisect-autostart", &cmdmode, > N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART), > + OPT_CMDMODE(0, "bisect-state", &cmdmode, > + N_("mark the state of ref (or refs)"), BISECT_STATE), > OPT_BOOL(0, "no-checkout", &no_checkout, > N_("update BISECT_HEAD instead of checking out the current commit")), > OPT_BOOL(0, "no-log", &nolog, > @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > set_terms(&terms, "bad", "good"); > res = bisect_autostart(&terms); > break; > + case BISECT_STATE: > + set_terms(&terms, "bad", "good"); > + get_terms(&terms); > + res = bisect_state(&terms, argv, argc); > + break; > default: > BUG("unknown subcommand %d", (int)cmdmode); > } > diff --git a/git-bisect.sh b/git-bisect.sh > index 049ffacdff..2da0810b1a 100755 > --- a/git-bisect.sh > +++ b/git-bisect.sh > @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" > TERM_BAD=bad > TERM_GOOD=good > > -bisect_head() > -{ > - if test -f "$GIT_DIR/BISECT_HEAD" > - then > - echo BISECT_HEAD > - else > - echo HEAD > - fi > -} > - > bisect_skip() { > all='' > for arg in "$@" > @@ -61,43 +51,7 @@ bisect_skip() { > esac > all="$all $revs" > done > - eval bisect_state 'skip' $all > -} > - > -bisect_state() { > - git bisect--helper --bisect-autostart > - state=$1 > - git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit > - get_terms > - case "$#,$state" in > - 0,*) > - die "Please call 'bisect_state' with at least one argument." ;; > - 1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip) > - bisected_head=$(bisect_head) > - rev=$(git rev-parse --verify "$bisected_head") || > - die "$(eval_gettext "Bad rev input: \$bisected_head")" > - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit > - git bisect--helper --check-expected-revs "$rev" ;; > - 2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip) > - shift > - hash_list='' > - for rev in "$@" > - do > - sha=$(git rev-parse --verify "$rev^{commit}") || > - die "$(eval_gettext "Bad rev input: \$rev")" > - hash_list="$hash_list $sha" > - done > - for rev in $hash_list > - do > - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit > - done > - git bisect--helper --check-expected-revs $hash_list ;; > - *,"$TERM_BAD") > - die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;; > - *) > - usage ;; > - esac > - git bisect--helper --bisect-auto-next > + eval git bisect--helper --bisect-state 'skip' $all > } > > bisect_visualize() { > @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2 > state="$TERM_GOOD" > fi > > - # We have to use a subshell because "bisect_state" can exit. > - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" ) > + git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" > res=$? > > cat "$GIT_DIR/BISECT_RUN" > @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2 > if [ $res -ne 0 ] > then > eval_gettextln "bisect run failed: > -'bisect_state \$state' exited with error code \$res" >&2 > +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2 This is not your fault, of course, but it does make me shudder to see such an obvious implementation detail in a user-facing error message. Maybe something to fix up in a follow-up? Ciao, Dscho > exit $res > fi > > @@ -242,7 +195,7 @@ case "$#" in > start) > git bisect--helper --bisect-start "$@" ;; > bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") > - bisect_state "$cmd" "$@" ;; > + git bisect--helper --bisect-state "$cmd" "$@" ;; > skip) > bisect_skip "$@" ;; > next) > -- > 2.25.0 > >
Hi Miriam, On Sat, 20 Jun 2020, Miriam R. wrote: > Hi Johannes, > I'm finishing the next patch series version and I have an issue about > one of your suggestions: > > El sáb., 23 may. 2020 a las 0:06, Johannes Schindelin > (<Johannes.Schindelin@gmx.de>) escribió: > > > > Hi Miriam, > > > > On Thu, 23 Apr 2020, Miriam Rubio wrote: > > > > > From: Pranit Bauva <pranit.bauva@gmail.com> > > > > > > Reimplement the `bisect_state()` shell functions in C and also add a > > > subcommand `--bisect-state` to `git-bisect--helper` to call them from > > > git-bisect.sh . > > > > > > Using `--bisect-state` subcommand is a temporary measure to port shell > > > function to C so as to use the existing test suite. As more functions > > > are ported, this subcommand will be retired and will be called by some > > > other methods. > > > > > > `bisect_head()` is only called from `bisect_state()`, thus it is not > > > required to introduce another subcommand. > > > > > > Mentored-by: Lars Schneider <larsxschneider@gmail.com> > > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > > > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > > > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> > > > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> > > > Signed-off-by: Miriam Rubio <mirucam@gmail.com> > > > --- > > > builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++- > > > git-bisect.sh | 55 +++---------------------------- > > > 2 files changed, 73 insertions(+), 52 deletions(-) > > > > > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > > > index 2d8660c79f..9db72f5891 100644 > > > --- a/builtin/bisect--helper.c > > > +++ b/builtin/bisect--helper.c > > > @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = { > > > N_("git bisect--helper --bisect-next"), > > > N_("git bisect--helper --bisect-auto-next"), > > > N_("git bisect--helper --bisect-autostart"), > > > + N_("git bisect--helper --bisect-state (bad|new) [<rev>]"), > > > + N_("git bisect--helper --bisect-state (good|old) [<rev>...]"), > > > NULL > > > }; > > > > > > @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms) > > > return bisect_start(terms, 0, NULL, 0); > > > } > > > > > > +static int bisect_head(struct object_id *oid) > > > +{ > > > + if (!file_exists(git_path_bisect_head())) > > > + return get_oid("HEAD", oid); > > > + > > > + return get_oid("BISECT_HEAD", oid); > > > > This can be easily reduced to > > > > return get_oid(file_exists(git_path_bisect_head()) ? > > "BISECT_HEAD" : "HEAD", oid); > > > > At the same time, it is wrong, just like the shell script version was > > wrong: in particular in light of the `hn/reftable` effort, we do _not_ > > want to assume that all refs are backed by files! > > > > So really, what this should do instead is this: > > > > enum get_oid_result res = get_oid("BISECT_HEAD", oid); > > > > if (res == MISSING_OBJECT) > > res = get_oid("HEAD", oid); > > > > Given that this is still only three lines long, the overhead of having it > > in its own function for just a _single_ call seems excessive. I'd prefer > > it to be inlined in `bisect_state()`. > > > > > +} > > > + > > > +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv, > > > + int argc) > > > +{ > > > > I offered a lengthy discussion about this function in > > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/ > > > > It does not look, however, as if v3 benefitted from the entirety of my > > analysis: All the `check_expected_revs()` function does is to verify that > > the passed list of revs matches exactly the contents of the > > `BISECT_EXPECTED_REV` file. > > > > That can be done in a much simpler way, though, by first reading the file > > and parsing the contents into an OID, and then comparing to that parsed > > OID instead. > > > > Besides, `check_expected_revs()` is only used to check one rev at a time. > > > > In other words, it could be simplified to something like this: > > > > static void check_expected_rev(struct object_id *oid) > > { > > struct object_id expected; > > struct strbuf buf = STRBUF_INIT; > > > > if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) > > < the_hash_algo->hexsz || > > get_oid_hex(buf.buf, &expected) < 0) > > return; /* Ignore invalid file contents */ > > > > if (!oideq(oid, &expected)) { > > ... unlink ... > > return; > > } > > } > > > > But even that would be wasteful, as we would read the file over and over > > and over again. > > > > The good news is that we do not even _need_ `check_expected_rev()`. > > Because we do not need to have two call sites, we can simplify the code > > much further. See below: > > > > > + const char *state; > > > + const char *hex; > > > + int i; > > > + struct oid_array revs = OID_ARRAY_INIT; > > > + struct object_id oid; > > > + > > > + if (!argc) > > > + return error(_("Please call `--bisect-state` with at least one argument")); > > > + state = argv[0]; > > > + if (check_and_set_terms(terms, state) || > > > + !one_of(state, terms->term_good,terms->term_bad, "skip", NULL)) > > > + return BISECT_FAILED; > > > + argv++; > > > + argc--; > > > + if (!strcmp(state, terms->term_bad) && (argc > 1)) > > > + return error(_("'git bisect %s' can take only one argument."),terms->term_bad); > > > + if (argc == 0) { > > > + if (bisect_head(&oid)) > > > + return error(_("Bad bisect_head rev input")); > > > + hex = oid_to_hex(&oid); > > > + if (bisect_write(state, hex, terms, 0)) > > > + return BISECT_FAILED; > > > + check_expected_revs(&hex, 1); > > > + return bisect_auto_next(terms, NULL); > > > + } > > > + > > > + /* Here argc > 0 */ > > > + for (; argc; argc--, argv++) { > > > + struct object_id oid; > > > + if (get_oid(*argv, &oid)) > > > + return error(_("Bad rev input: %s"), *argv); > > > + oid_array_append(&revs, &oid); > > > + } > > > > It really does not make sense to parse the arguments into an OID array, > > _then_ iterate over the array once, and then immediately releasing it. > > That OID array is not needed at all. > > > > So we'll end up with this loop in case `argc > 0` (where we now call > > `get_oid()`, too), and note how the loop body looks _eerily_ similar to > > the conditional `argc == 0` code block above? > > > > > + > > > + for (i = 0; i < revs.nr; i++) { > > > + hex = oid_to_hex(&revs.oid[i]); > > > + if (bisect_write(state, hex, terms, 0)) { > > > + oid_array_clear(&revs); > > > + return BISECT_FAILED; > > > + } > > > + check_expected_revs(&hex, 1); > > > + } > > > + > > > + oid_array_clear(&revs); > > > + return bisect_auto_next(terms, NULL); > > > +} > > > > So really, this function pretty much _wants_ to look this way (modulo > > bugs, as I did not even test-compile the code): > > > > static enum bisect_error bisect_state(struct bisect_terms *terms, > > const char **argv, int argc) > > { > > const char *state; > > int i, verify_expected = 1; > > struct object_id oid, expected; > > struct strbuf buf = STRBUF_INIT; > > > > if (!argc) > > return error(_("Please call `--bisect-state` with at least one argument")); > > > > state = argv[0]; > > if (check_and_set_terms(terms, state) || > > !one_of(state, terms->term_good, terms->term_bad, "skip", NULL)) > > return BISECT_FAILED; > > > > argv++; > > argc--; > > if (argc > 1 && !strcmp(state, terms->term_bad)) > > return error(_("'git bisect %s' can take only one argument."), terms->term_bad); > > > > if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz || > > get_oid_hex(buf.buf, &expected) < 0) > > verify_expected = 0; /* Ignore invalid file contents */ > > > > > > for (i = 0; i < argc + !argc; i++) { > > if (argc) { > > if (get_oid(argv[i], &oid)) { > > error(_("Bad rev input: %s"), *argv); > > return BISECT_FAILED; > > } > > } else { > > enum get_oid_result res = get_oid("BISECT_HEAD", &oid); > > > > if (res == MISSING_OBJECT) > > res = get_oid("HEAD", &oid); > > if (res) { > > error(_("Bad bisect_head rev input")); > > return BISECT_FAILED; > > } > > } > > > > if (bisect_write(state, oid_to_hex(&oid), terms, 0)) > > return BISECT_FAILED; > > > > if (verify_expected && !oideq(&oid, &expected)) { > > unlink_or_warn(git_path_bisect_ancestors_ok()); > > unlink_or_warn(git_path_bisect_expected_rev()); > > verify_expected = 0; > > } > > } > > > > return bisect_auto_next(terms, NULL); > > } > > > > There, not bad, is it? > > > After implementing this solution some tests failed. After debugging > them, I found that with Pranit's solution, that arguments were parsed > into an OID array, if bisect received some junk rev the function > returned and bisect_write() was not executed. That's a good point, and should be turned into a code comment lest overzealous developers would attempt the simplification I proposed in the future. > With the new solution, if junk rev is received after a valid rev, > bisect_write() was executed for the valid and the function returned with > the junk rev. > So, there is garbage in the file and when for example bisect-porcelain > test number 5 - 'bisect fails if given any junk instead of revs' > executes 'test -z' fails. > > Should I keep the original patch and add a comment in the code that > explains why we use an oid array?. I still think that the `check_expected_revs()` function is not needed, and unnecessarily complex. If you _want_ to keep Pranit's original implementation, then I would _at least_ want you to forward-port the `verify_expected` changes I suggested. > > (I also have implemented an alternative solution that when some junk > rev is found, I delete all refs written, but maybe is too complicated > or not totally correct: > https://gitlab.com/mirucam/git/-/commit/93f669877b87d09a30a5d07f0967667b22026511 > ) You seem to share my preference for not writing out bogus revs in the first place. Thanks, Johannes > > > > > + > > > int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > > { > > > enum { > > > @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > > BISECT_START, > > > BISECT_NEXT, > > > BISECT_AUTO_NEXT, > > > - BISECT_AUTOSTART > > > + BISECT_AUTOSTART, > > > + BISECT_STATE > > > } cmdmode = 0; > > > int no_checkout = 0, res = 0, nolog = 0; > > > struct option options[] = { > > > @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > > N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT), > > > OPT_CMDMODE(0, "bisect-autostart", &cmdmode, > > > N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART), > > > + OPT_CMDMODE(0, "bisect-state", &cmdmode, > > > + N_("mark the state of ref (or refs)"), BISECT_STATE), > > > OPT_BOOL(0, "no-checkout", &no_checkout, > > > N_("update BISECT_HEAD instead of checking out the current commit")), > > > OPT_BOOL(0, "no-log", &nolog, > > > @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > > set_terms(&terms, "bad", "good"); > > > res = bisect_autostart(&terms); > > > break; > > > + case BISECT_STATE: > > > + set_terms(&terms, "bad", "good"); > > > + get_terms(&terms); > > > + res = bisect_state(&terms, argv, argc); > > > + break; > > > default: > > > BUG("unknown subcommand %d", (int)cmdmode); > > > } > > > diff --git a/git-bisect.sh b/git-bisect.sh > > > index 049ffacdff..2da0810b1a 100755 > > > --- a/git-bisect.sh > > > +++ b/git-bisect.sh > > > @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" > > > TERM_BAD=bad > > > TERM_GOOD=good > > > > > > -bisect_head() > > > -{ > > > - if test -f "$GIT_DIR/BISECT_HEAD" > > > - then > > > - echo BISECT_HEAD > > > - else > > > - echo HEAD > > > - fi > > > -} > > > - > > > bisect_skip() { > > > all='' > > > for arg in "$@" > > > @@ -61,43 +51,7 @@ bisect_skip() { > > > esac > > > all="$all $revs" > > > done > > > - eval bisect_state 'skip' $all > > > -} > > > - > > > -bisect_state() { > > > - git bisect--helper --bisect-autostart > > > - state=$1 > > > - git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit > > > - get_terms > > > - case "$#,$state" in > > > - 0,*) > > > - die "Please call 'bisect_state' with at least one argument." ;; > > > - 1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip) > > > - bisected_head=$(bisect_head) > > > - rev=$(git rev-parse --verify "$bisected_head") || > > > - die "$(eval_gettext "Bad rev input: \$bisected_head")" > > > - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit > > > - git bisect--helper --check-expected-revs "$rev" ;; > > > - 2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip) > > > - shift > > > - hash_list='' > > > - for rev in "$@" > > > - do > > > - sha=$(git rev-parse --verify "$rev^{commit}") || > > > - die "$(eval_gettext "Bad rev input: \$rev")" > > > - hash_list="$hash_list $sha" > > > - done > > > - for rev in $hash_list > > > - do > > > - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit > > > - done > > > - git bisect--helper --check-expected-revs $hash_list ;; > > > - *,"$TERM_BAD") > > > - die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;; > > > - *) > > > - usage ;; > > > - esac > > > - git bisect--helper --bisect-auto-next > > > + eval git bisect--helper --bisect-state 'skip' $all > > > } > > > > > > bisect_visualize() { > > > @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2 > > > state="$TERM_GOOD" > > > fi > > > > > > - # We have to use a subshell because "bisect_state" can exit. > > > - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" ) > > > + git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" > > > res=$? > > > > > > cat "$GIT_DIR/BISECT_RUN" > > > @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2 > > > if [ $res -ne 0 ] > > > then > > > eval_gettextln "bisect run failed: > > > -'bisect_state \$state' exited with error code \$res" >&2 > > > +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2 > > > > This is not your fault, of course, but it does make me shudder to see such > > an obvious implementation detail in a user-facing error message. > > > > Maybe something to fix up in a follow-up? > > > > Ciao, > > Dscho > > > > > exit $res > > > fi > > > > > > @@ -242,7 +195,7 @@ case "$#" in > > > start) > > > git bisect--helper --bisect-start "$@" ;; > > > bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") > > > - bisect_state "$cmd" "$@" ;; > > > + git bisect--helper --bisect-state "$cmd" "$@" ;; > > > skip) > > > bisect_skip "$@" ;; > > > next) > > > -- > > > 2.25.0 > > > > > > > Best, > Miriam. >
Hi Johannes, I'm finishing the next patch series version and I have an issue about one of your suggestions: El sáb., 23 may. 2020 a las 0:06, Johannes Schindelin (<Johannes.Schindelin@gmx.de>) escribió: > > Hi Miriam, > > On Thu, 23 Apr 2020, Miriam Rubio wrote: > > > From: Pranit Bauva <pranit.bauva@gmail.com> > > > > Reimplement the `bisect_state()` shell functions in C and also add a > > subcommand `--bisect-state` to `git-bisect--helper` to call them from > > git-bisect.sh . > > > > Using `--bisect-state` subcommand is a temporary measure to port shell > > function to C so as to use the existing test suite. As more functions > > are ported, this subcommand will be retired and will be called by some > > other methods. > > > > `bisect_head()` is only called from `bisect_state()`, thus it is not > > required to introduce another subcommand. > > > > Mentored-by: Lars Schneider <larsxschneider@gmail.com> > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > > Mentored-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > > Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com> > > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> > > Signed-off-by: Miriam Rubio <mirucam@gmail.com> > > --- > > builtin/bisect--helper.c | 70 +++++++++++++++++++++++++++++++++++++++- > > git-bisect.sh | 55 +++---------------------------- > > 2 files changed, 73 insertions(+), 52 deletions(-) > > > > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > > index 2d8660c79f..9db72f5891 100644 > > --- a/builtin/bisect--helper.c > > +++ b/builtin/bisect--helper.c > > @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = { > > N_("git bisect--helper --bisect-next"), > > N_("git bisect--helper --bisect-auto-next"), > > N_("git bisect--helper --bisect-autostart"), > > + N_("git bisect--helper --bisect-state (bad|new) [<rev>]"), > > + N_("git bisect--helper --bisect-state (good|old) [<rev>...]"), > > NULL > > }; > > > > @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms) > > return bisect_start(terms, 0, NULL, 0); > > } > > > > +static int bisect_head(struct object_id *oid) > > +{ > > + if (!file_exists(git_path_bisect_head())) > > + return get_oid("HEAD", oid); > > + > > + return get_oid("BISECT_HEAD", oid); > > This can be easily reduced to > > return get_oid(file_exists(git_path_bisect_head()) ? > "BISECT_HEAD" : "HEAD", oid); > > At the same time, it is wrong, just like the shell script version was > wrong: in particular in light of the `hn/reftable` effort, we do _not_ > want to assume that all refs are backed by files! > > So really, what this should do instead is this: > > enum get_oid_result res = get_oid("BISECT_HEAD", oid); > > if (res == MISSING_OBJECT) > res = get_oid("HEAD", oid); > > Given that this is still only three lines long, the overhead of having it > in its own function for just a _single_ call seems excessive. I'd prefer > it to be inlined in `bisect_state()`. > > > +} > > + > > +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv, > > + int argc) > > +{ > > I offered a lengthy discussion about this function in > https://lore.kernel.org/git/nycvar.QRO.7.76.6.2002272244150.9783@tvgsbejvaqbjf.bet/ > > It does not look, however, as if v3 benefitted from the entirety of my > analysis: All the `check_expected_revs()` function does is to verify that > the passed list of revs matches exactly the contents of the > `BISECT_EXPECTED_REV` file. > > That can be done in a much simpler way, though, by first reading the file > and parsing the contents into an OID, and then comparing to that parsed > OID instead. > > Besides, `check_expected_revs()` is only used to check one rev at a time. > > In other words, it could be simplified to something like this: > > static void check_expected_rev(struct object_id *oid) > { > struct object_id expected; > struct strbuf buf = STRBUF_INIT; > > if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) > < the_hash_algo->hexsz || > get_oid_hex(buf.buf, &expected) < 0) > return; /* Ignore invalid file contents */ > > if (!oideq(oid, &expected)) { > ... unlink ... > return; > } > } > > But even that would be wasteful, as we would read the file over and over > and over again. > > The good news is that we do not even _need_ `check_expected_rev()`. > Because we do not need to have two call sites, we can simplify the code > much further. See below: > > > + const char *state; > > + const char *hex; > > + int i; > > + struct oid_array revs = OID_ARRAY_INIT; > > + struct object_id oid; > > + > > + if (!argc) > > + return error(_("Please call `--bisect-state` with at least one argument")); > > + state = argv[0]; > > + if (check_and_set_terms(terms, state) || > > + !one_of(state, terms->term_good,terms->term_bad, "skip", NULL)) > > + return BISECT_FAILED; > > + argv++; > > + argc--; > > + if (!strcmp(state, terms->term_bad) && (argc > 1)) > > + return error(_("'git bisect %s' can take only one argument."),terms->term_bad); > > + if (argc == 0) { > > + if (bisect_head(&oid)) > > + return error(_("Bad bisect_head rev input")); > > + hex = oid_to_hex(&oid); > > + if (bisect_write(state, hex, terms, 0)) > > + return BISECT_FAILED; > > + check_expected_revs(&hex, 1); > > + return bisect_auto_next(terms, NULL); > > + } > > + > > + /* Here argc > 0 */ > > + for (; argc; argc--, argv++) { > > + struct object_id oid; > > + if (get_oid(*argv, &oid)) > > + return error(_("Bad rev input: %s"), *argv); > > + oid_array_append(&revs, &oid); > > + } > > It really does not make sense to parse the arguments into an OID array, > _then_ iterate over the array once, and then immediately releasing it. > That OID array is not needed at all. > > So we'll end up with this loop in case `argc > 0` (where we now call > `get_oid()`, too), and note how the loop body looks _eerily_ similar to > the conditional `argc == 0` code block above? > > > + > > + for (i = 0; i < revs.nr; i++) { > > + hex = oid_to_hex(&revs.oid[i]); > > + if (bisect_write(state, hex, terms, 0)) { > > + oid_array_clear(&revs); > > + return BISECT_FAILED; > > + } > > + check_expected_revs(&hex, 1); > > + } > > + > > + oid_array_clear(&revs); > > + return bisect_auto_next(terms, NULL); > > +} > > So really, this function pretty much _wants_ to look this way (modulo > bugs, as I did not even test-compile the code): > > static enum bisect_error bisect_state(struct bisect_terms *terms, > const char **argv, int argc) > { > const char *state; > int i, verify_expected = 1; > struct object_id oid, expected; > struct strbuf buf = STRBUF_INIT; > > if (!argc) > return error(_("Please call `--bisect-state` with at least one argument")); > > state = argv[0]; > if (check_and_set_terms(terms, state) || > !one_of(state, terms->term_good, terms->term_bad, "skip", NULL)) > return BISECT_FAILED; > > argv++; > argc--; > if (argc > 1 && !strcmp(state, terms->term_bad)) > return error(_("'git bisect %s' can take only one argument."), terms->term_bad); > > if (strbuf_read_file(&buf, git_path_bisect_expected_rev(), 0) < the_hash_algo->hexsz || > get_oid_hex(buf.buf, &expected) < 0) > verify_expected = 0; /* Ignore invalid file contents */ > > > for (i = 0; i < argc + !argc; i++) { > if (argc) { > if (get_oid(argv[i], &oid)) { > error(_("Bad rev input: %s"), *argv); > return BISECT_FAILED; > } > } else { > enum get_oid_result res = get_oid("BISECT_HEAD", &oid); > > if (res == MISSING_OBJECT) > res = get_oid("HEAD", &oid); > if (res) { > error(_("Bad bisect_head rev input")); > return BISECT_FAILED; > } > } > > if (bisect_write(state, oid_to_hex(&oid), terms, 0)) > return BISECT_FAILED; > > if (verify_expected && !oideq(&oid, &expected)) { > unlink_or_warn(git_path_bisect_ancestors_ok()); > unlink_or_warn(git_path_bisect_expected_rev()); > verify_expected = 0; > } > } > > return bisect_auto_next(terms, NULL); > } > > There, not bad, is it? > After implementing this solution some tests failed. After debugging them, I found that with Pranit's solution, that arguments were parsed into an OID array, if bisect received some junk rev the function returned and bisect_write() was not executed. With the new solution, if junk rev is received after a valid rev, bisect_write() was executed for the valid and the function returned with the junk rev. So, there is garbage in the file and when for example bisect-porcelain test number 5 - 'bisect fails if given any junk instead of revs' executes 'test -z' fails. Should I keep the original patch and add a comment in the code that explains why we use an oid array?. (I also have implemented an alternative solution that when some junk rev is found, I delete all refs written, but maybe is too complicated or not totally correct: https://gitlab.com/mirucam/git/-/commit/93f669877b87d09a30a5d07f0967667b22026511 ) > > + > > int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > { > > enum { > > @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > BISECT_START, > > BISECT_NEXT, > > BISECT_AUTO_NEXT, > > - BISECT_AUTOSTART > > + BISECT_AUTOSTART, > > + BISECT_STATE > > } cmdmode = 0; > > int no_checkout = 0, res = 0, nolog = 0; > > struct option options[] = { > > @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT), > > OPT_CMDMODE(0, "bisect-autostart", &cmdmode, > > N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART), > > + OPT_CMDMODE(0, "bisect-state", &cmdmode, > > + N_("mark the state of ref (or refs)"), BISECT_STATE), > > OPT_BOOL(0, "no-checkout", &no_checkout, > > N_("update BISECT_HEAD instead of checking out the current commit")), > > OPT_BOOL(0, "no-log", &nolog, > > @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > set_terms(&terms, "bad", "good"); > > res = bisect_autostart(&terms); > > break; > > + case BISECT_STATE: > > + set_terms(&terms, "bad", "good"); > > + get_terms(&terms); > > + res = bisect_state(&terms, argv, argc); > > + break; > > default: > > BUG("unknown subcommand %d", (int)cmdmode); > > } > > diff --git a/git-bisect.sh b/git-bisect.sh > > index 049ffacdff..2da0810b1a 100755 > > --- a/git-bisect.sh > > +++ b/git-bisect.sh > > @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" > > TERM_BAD=bad > > TERM_GOOD=good > > > > -bisect_head() > > -{ > > - if test -f "$GIT_DIR/BISECT_HEAD" > > - then > > - echo BISECT_HEAD > > - else > > - echo HEAD > > - fi > > -} > > - > > bisect_skip() { > > all='' > > for arg in "$@" > > @@ -61,43 +51,7 @@ bisect_skip() { > > esac > > all="$all $revs" > > done > > - eval bisect_state 'skip' $all > > -} > > - > > -bisect_state() { > > - git bisect--helper --bisect-autostart > > - state=$1 > > - git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit > > - get_terms > > - case "$#,$state" in > > - 0,*) > > - die "Please call 'bisect_state' with at least one argument." ;; > > - 1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip) > > - bisected_head=$(bisect_head) > > - rev=$(git rev-parse --verify "$bisected_head") || > > - die "$(eval_gettext "Bad rev input: \$bisected_head")" > > - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit > > - git bisect--helper --check-expected-revs "$rev" ;; > > - 2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip) > > - shift > > - hash_list='' > > - for rev in "$@" > > - do > > - sha=$(git rev-parse --verify "$rev^{commit}") || > > - die "$(eval_gettext "Bad rev input: \$rev")" > > - hash_list="$hash_list $sha" > > - done > > - for rev in $hash_list > > - do > > - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit > > - done > > - git bisect--helper --check-expected-revs $hash_list ;; > > - *,"$TERM_BAD") > > - die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;; > > - *) > > - usage ;; > > - esac > > - git bisect--helper --bisect-auto-next > > + eval git bisect--helper --bisect-state 'skip' $all > > } > > > > bisect_visualize() { > > @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2 > > state="$TERM_GOOD" > > fi > > > > - # We have to use a subshell because "bisect_state" can exit. > > - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" ) > > + git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" > > res=$? > > > > cat "$GIT_DIR/BISECT_RUN" > > @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2 > > if [ $res -ne 0 ] > > then > > eval_gettextln "bisect run failed: > > -'bisect_state \$state' exited with error code \$res" >&2 > > +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2 > > This is not your fault, of course, but it does make me shudder to see such > an obvious implementation detail in a user-facing error message. > > Maybe something to fix up in a follow-up? > > Ciao, > Dscho > > > exit $res > > fi > > > > @@ -242,7 +195,7 @@ case "$#" in > > start) > > git bisect--helper --bisect-start "$@" ;; > > bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") > > - bisect_state "$cmd" "$@" ;; > > + git bisect--helper --bisect-state "$cmd" "$@" ;; > > skip) > > bisect_skip "$@" ;; > > next) > > -- > > 2.25.0 > > > > Best, Miriam.
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 2d8660c79f..9db72f5891 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -31,6 +31,8 @@ static const char * const git_bisect_helper_usage[] = { N_("git bisect--helper --bisect-next"), N_("git bisect--helper --bisect-auto-next"), N_("git bisect--helper --bisect-autostart"), + N_("git bisect--helper --bisect-state (bad|new) [<rev>]"), + N_("git bisect--helper --bisect-state (good|old) [<rev>...]"), NULL }; @@ -834,6 +836,64 @@ static int bisect_autostart(struct bisect_terms *terms) return bisect_start(terms, 0, NULL, 0); } +static int bisect_head(struct object_id *oid) +{ + if (!file_exists(git_path_bisect_head())) + return get_oid("HEAD", oid); + + return get_oid("BISECT_HEAD", oid); +} + +static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv, + int argc) +{ + const char *state; + const char *hex; + int i; + struct oid_array revs = OID_ARRAY_INIT; + struct object_id oid; + + if (!argc) + return error(_("Please call `--bisect-state` with at least one argument")); + state = argv[0]; + if (check_and_set_terms(terms, state) || + !one_of(state, terms->term_good,terms->term_bad, "skip", NULL)) + return BISECT_FAILED; + argv++; + argc--; + if (!strcmp(state, terms->term_bad) && (argc > 1)) + return error(_("'git bisect %s' can take only one argument."),terms->term_bad); + if (argc == 0) { + if (bisect_head(&oid)) + return error(_("Bad bisect_head rev input")); + hex = oid_to_hex(&oid); + if (bisect_write(state, hex, terms, 0)) + return BISECT_FAILED; + check_expected_revs(&hex, 1); + return bisect_auto_next(terms, NULL); + } + + /* Here argc > 0 */ + for (; argc; argc--, argv++) { + struct object_id oid; + if (get_oid(*argv, &oid)) + return error(_("Bad rev input: %s"), *argv); + oid_array_append(&revs, &oid); + } + + for (i = 0; i < revs.nr; i++) { + hex = oid_to_hex(&revs.oid[i]); + if (bisect_write(state, hex, terms, 0)) { + oid_array_clear(&revs); + return BISECT_FAILED; + } + check_expected_revs(&hex, 1); + } + + oid_array_clear(&revs); + return bisect_auto_next(terms, NULL); +} + int cmd_bisect__helper(int argc, const char **argv, const char *prefix) { enum { @@ -847,7 +907,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) BISECT_START, BISECT_NEXT, BISECT_AUTO_NEXT, - BISECT_AUTOSTART + BISECT_AUTOSTART, + BISECT_STATE } cmdmode = 0; int no_checkout = 0, res = 0, nolog = 0; struct option options[] = { @@ -873,6 +934,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("verify the next bisection state then checkout the next bisection commit"), BISECT_AUTO_NEXT), OPT_CMDMODE(0, "bisect-autostart", &cmdmode, N_("start the bisection if BISECT_START is empty or missing"), BISECT_AUTOSTART), + OPT_CMDMODE(0, "bisect-state", &cmdmode, + N_("mark the state of ref (or refs)"), BISECT_STATE), OPT_BOOL(0, "no-checkout", &no_checkout, N_("update BISECT_HEAD instead of checking out the current commit")), OPT_BOOL(0, "no-log", &nolog, @@ -945,6 +1008,11 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) set_terms(&terms, "bad", "good"); res = bisect_autostart(&terms); break; + case BISECT_STATE: + set_terms(&terms, "bad", "good"); + get_terms(&terms); + res = bisect_state(&terms, argv, argc); + break; default: BUG("unknown subcommand %d", (int)cmdmode); } diff --git a/git-bisect.sh b/git-bisect.sh index 049ffacdff..2da0810b1a 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -39,16 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" TERM_BAD=bad TERM_GOOD=good -bisect_head() -{ - if test -f "$GIT_DIR/BISECT_HEAD" - then - echo BISECT_HEAD - else - echo HEAD - fi -} - bisect_skip() { all='' for arg in "$@" @@ -61,43 +51,7 @@ bisect_skip() { esac all="$all $revs" done - eval bisect_state 'skip' $all -} - -bisect_state() { - git bisect--helper --bisect-autostart - state=$1 - git bisect--helper --check-and-set-terms $state $TERM_GOOD $TERM_BAD || exit - get_terms - case "$#,$state" in - 0,*) - die "Please call 'bisect_state' with at least one argument." ;; - 1,"$TERM_BAD"|1,"$TERM_GOOD"|1,skip) - bisected_head=$(bisect_head) - rev=$(git rev-parse --verify "$bisected_head") || - die "$(eval_gettext "Bad rev input: \$bisected_head")" - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit - git bisect--helper --check-expected-revs "$rev" ;; - 2,"$TERM_BAD"|*,"$TERM_GOOD"|*,skip) - shift - hash_list='' - for rev in "$@" - do - sha=$(git rev-parse --verify "$rev^{commit}") || - die "$(eval_gettext "Bad rev input: \$rev")" - hash_list="$hash_list $sha" - done - for rev in $hash_list - do - git bisect--helper --bisect-write "$state" "$rev" "$TERM_GOOD" "$TERM_BAD" || exit - done - git bisect--helper --check-expected-revs $hash_list ;; - *,"$TERM_BAD") - die "$(eval_gettext "'git bisect \$TERM_BAD' can take only one argument.")" ;; - *) - usage ;; - esac - git bisect--helper --bisect-auto-next + eval git bisect--helper --bisect-state 'skip' $all } bisect_visualize() { @@ -185,8 +139,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2 state="$TERM_GOOD" fi - # We have to use a subshell because "bisect_state" can exit. - ( bisect_state $state >"$GIT_DIR/BISECT_RUN" ) + git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" res=$? cat "$GIT_DIR/BISECT_RUN" @@ -201,7 +154,7 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2 if [ $res -ne 0 ] then eval_gettextln "bisect run failed: -'bisect_state \$state' exited with error code \$res" >&2 +'git bisect--helper --bisect-state \$state' exited with error code \$res" >&2 exit $res fi @@ -242,7 +195,7 @@ case "$#" in start) git bisect--helper --bisect-start "$@" ;; bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") - bisect_state "$cmd" "$@" ;; + git bisect--helper --bisect-state "$cmd" "$@" ;; skip) bisect_skip "$@" ;; next)