| Message ID | 20987dc6-e0c7-6ca2-19fd-2b323b3f6d9f@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bisect: report actual bisect_state() argument on error | expand |
> diff --git a/bisect.h b/bisect.h > index ec24ac2d7e..748adf0cff 100644 > --- a/bisect.h > +++ b/bisect.h > @@ -69,4 +69,7 @@ void read_bisect_terms(const char **bad, const char **good); > > int bisect_clean_state(void); > > +enum bisect_error bisect_checkout(const struct object_id *bisect_rev, > + int no_checkout); > + > #endif https://github.com/git/git/runs/4861805265?check_suite_focus=true#step:4:65 In file included from bisect.hcc:2:0: bisect.h:72:48: error: ‘struct object_id’ declared inside parameter list will not be visible outside of this definition or declaration [-Werror] enum bisect_error bisect_checkout(const struct object_id *bisect_rev, ^~~~~~~~~ cc1: all warnings being treated as errors
Am 19.01.22 um 03:36 schrieb Junio C Hamano: >> diff --git a/bisect.h b/bisect.h >> index ec24ac2d7e..748adf0cff 100644 >> --- a/bisect.h >> +++ b/bisect.h >> @@ -69,4 +69,7 @@ void read_bisect_terms(const char **bad, const char **good); >> >> int bisect_clean_state(void); >> >> +enum bisect_error bisect_checkout(const struct object_id *bisect_rev, >> + int no_checkout); >> + >> #endif > > https://github.com/git/git/runs/4861805265?check_suite_focus=true#step:4:65 > > In file included from bisect.hcc:2:0: > bisect.h:72:48: error: ‘struct object_id’ declared inside parameter > list will not be visible outside of this definition or declaration > [-Werror] > enum bisect_error bisect_checkout(const struct object_id *bisect_rev, > ^~~~~~~~~ > cc1: all warnings being treated as errors Oops, and I didn't even know the make target hdr-check exists. :-/ --- >8 --- Subject: [PATCH] fixup! bisect--helper: double-check run command on exit code 126 and 127 --- bisect.h | 1 + 1 file changed, 1 insertion(+) diff --git a/bisect.h b/bisect.h index 748adf0cff..1015aeb8ea 100644 --- a/bisect.h +++ b/bisect.h @@ -3,6 +3,7 @@ struct commit_list; struct repository; +struct object_id; /* * Find bisection. If something is found, `reaches` will be the number of -- 2.34.1
René Scharfe <l.s.r@web.de> writes: > When a run command cannot be executed or found, shells return exit code > 126 or 127, respectively. Valid run commands are allowed to return > these codes as well to indicate bad revisions, though, for historical > reasons. This means typos can cause bogus bisect runs that go over the > full distance and end up reporting invalid results. > > The best solution would be to reserve exit codes 126 and 127, like > 71b0251cdd (Bisect run: "skip" current commit if script exit code is > 125., 2007-10-26) did for 125, and abort bisect run when we get them. > That might be inconvenient for those who relied on the documentation > stating that 126 and 127 can be used for bad revisions, though. I think the basic idea is sound and useful. How happy are we who was involved in the discussion with this result? > +static int get_first_good(const char *refname, const struct object_id *oid, > + int flag, void *cb_data) > +{ > + oidcpy(cb_data, oid); > + return 1; > +} OK, this iterates and stops at the first one. > +static int verify_good(const struct bisect_terms *terms, > + const char **quoted_argv) > +{ > + int rc; > + enum bisect_error res; > + struct object_id good_rev; > + struct object_id current_rev; > + char *good_glob = xstrfmt("%s-*", terms->term_good); > + int no_checkout = ref_exists("BISECT_HEAD"); > + > + for_each_glob_ref_in(get_first_good, good_glob, "refs/bisect/", > + &good_rev); > + free(good_glob); > + > + if (read_ref(no_checkout ? "BISECT_HEAD" : "HEAD", ¤t_rev)) > + return -1; * Could the current_rev already be marked as "good", in which case we can avoid cost of rewriting working tree files to a potentially distant revision? I often do manual tests to mark "bisect good" or "bisect bad" before using "bisect run". * Can we have *no* rev that is marked as "good"? I think we made it possible to say "my time is more valuable than machine cycles, so I'll only tell you that this revision is broken and give you no limit on the bottom side of the history. still assume that there was only one good-to-bad transition in the history and find it" by supplying only one "bad" and no "good" when starting to bisect. And in such a case, ... > + res = bisect_checkout(&good_rev, no_checkout); ... this would feed an uninitialized object_id to bisect_checkout. Thanks.
Am 04.02.22 um 01:42 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> When a run command cannot be executed or found, shells return exit code >> 126 or 127, respectively. Valid run commands are allowed to return >> these codes as well to indicate bad revisions, though, for historical >> reasons. This means typos can cause bogus bisect runs that go over the >> full distance and end up reporting invalid results. >> >> The best solution would be to reserve exit codes 126 and 127, like >> 71b0251cdd (Bisect run: "skip" current commit if script exit code is >> 125., 2007-10-26) did for 125, and abort bisect run when we get them. >> That might be inconvenient for those who relied on the documentation >> stating that 126 and 127 can be used for bad revisions, though. > > I think the basic idea is sound and useful. How happy are we who > was involved in the discussion with this result? > >> +static int get_first_good(const char *refname, const struct object_id *oid, >> + int flag, void *cb_data) >> +{ >> + oidcpy(cb_data, oid); >> + return 1; >> +} > > OK, this iterates and stops at the first one. > >> +static int verify_good(const struct bisect_terms *terms, >> + const char **quoted_argv) >> +{ >> + int rc; >> + enum bisect_error res; >> + struct object_id good_rev; >> + struct object_id current_rev; >> + char *good_glob = xstrfmt("%s-*", terms->term_good); >> + int no_checkout = ref_exists("BISECT_HEAD"); >> + >> + for_each_glob_ref_in(get_first_good, good_glob, "refs/bisect/", >> + &good_rev); >> + free(good_glob); >> + >> + if (read_ref(no_checkout ? "BISECT_HEAD" : "HEAD", ¤t_rev)) >> + return -1; > > * Could the current_rev already be marked as "good", in which case > we can avoid cost of rewriting working tree files to a > potentially distant revision? I often do manual tests to mark > "bisect good" or "bisect bad" before using "bisect run". > > * Can we have *no* rev that is marked as "good"? I think we made > it possible to say "my time is more valuable than machine cycles, > so I'll only tell you that this revision is broken and give you > no limit on the bottom side of the history. still assume that > there was only one good-to-bad transition in the history and find > it" by supplying only one "bad" and no "good" when starting to > bisect. And in such a case, ... > >> + res = bisect_checkout(&good_rev, no_checkout); > > ... this would feed an uninitialized object_id to bisect_checkout. bisect_run() starts by calling bisect_next_check() with a current_term parameter value of NULL. It checks if the good rev is missing and calls decide_next(), which returns -1 if current_term is NULL unless both good and bad revs are present. bisect_next_check() passes this value along. bisect_run() exits if it's non-zero. So AFAICS the uninitialized access would only happen if the good rev ref was deleted between the bisect_next_check() call and the verify_good() call. I considered this scenario to be practically impossible with the current code. We can handle it more gracefully by doing something like in the patch below. Supporting a bad-only git bisect run would take more work -- perhaps by making verify_good() pick a root commit to check as an assumed good rev (plus fix whatever else caused the current code to pass NULL as current_term). René --- builtin/bisect--helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index 50783a586c..e1e58de3b2 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -1106,9 +1106,12 @@ static int verify_good(const struct bisect_terms *terms, char *good_glob = xstrfmt("%s-*", terms->term_good); int no_checkout = ref_exists("BISECT_HEAD"); + oidcpy(&good_rev, null_oid()); for_each_glob_ref_in(get_first_good, good_glob, "refs/bisect/", &good_rev); free(good_glob); + if (is_null_oid(&good_rev)) + return -1; if (read_ref(no_checkout ? "BISECT_HEAD" : "HEAD", ¤t_rev)) return -1; -- 2.35.0
René Scharfe wrote: > The workaround used by this patch is to run the command on a known-good > revision and abort if we still get the same error code. This adds one > step to runs with scripts that use exit codes 126 and 127, but keeps > them supported, with one exception: It won't work with commands that > cannot recognize the (manually marked) known-good revision as such. > > Run commands that use low exit codes are unaffected. Typos are reported > after executing the missing command twice and three checkouts (the first > step, the known good revision and back to the revision of the first > step). I'm happy with the description of this patch. It doesn't add an extra step for an overwhelming majority of users, while not breaking backward compatibility. > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -1089,6 +1089,44 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a > +static int get_first_good(const char *refname, const struct object_id *oid, > + int flag, void *cb_data) > +{ > + oidcpy(cb_data, oid); > + return 1; I assume you return 1 here to stop the for_each_glob_ref() iteration, after copying the oid. > @@ -1113,6 +1152,30 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) > + int rc = verify_good(terms, run_args.v); > + is_first_run = 0; > + if (rc < 0) { > + error(_("unable to verify '%s' on good" > + " revision"), command.buf); Perhaps in a subsequent patch, we can consider sha1_abbrev() to print nicer messages everywhere in bisect. > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh > index fc18796517..5382e5d216 100755 > --- a/t/t6030-bisect-porcelain.sh > +++ b/t/t6030-bisect-porcelain.sh > @@ -290,7 +290,7 @@ test_expect_success 'bisect run accepts exit code 126 as bad' ' > -test_expect_failure POSIXPERM 'bisect run fails with non-executable test script' ' > +test_expect_success POSIXPERM 'bisect run fails with non-executable test script' ' > -test_expect_failure 'bisect run fails with missing test script' ' > +test_expect_success 'bisect run fails with missing test script' ' Yes, these are precisely the two problems I had. Thanks. Warm regards, Ram
Junio C Hamano wrote: > * Can we have *no* rev that is marked as "good"? I think we made > it possible to say "my time is more valuable than machine cycles, > so I'll only tell you that this revision is broken and give you > no limit on the bottom side of the history. still assume that > there was only one good-to-bad transition in the history and find > it" by supplying only one "bad" and no "good" when starting to > bisect. And in such a case, ... Hm, this addition might be an unpleasant special-case syntax, breaking both `git bisect start [bad [good]]` and `git bisect bad ...; git bisect start`. R.
"Ramkumar Ramachandra" <r@artagnon.com> writes: > Junio C Hamano wrote: >> * Can we have *no* rev that is marked as "good"? I think we made >> it possible to say "my time is more valuable than machine cycles, >> so I'll only tell you that this revision is broken and give you >> no limit on the bottom side of the history. still assume that >> there was only one good-to-bad transition in the history and find >> it" by supplying only one "bad" and no "good" when starting to >> bisect. And in such a case, ... > > Hm, this addition might be an unpleasant special-case syntax, breaking both `git bisect start [bad [good]]` and `git bisect bad ...; git bisect start`. Interesting. Our "start" does allow you to give one "bad" and then zero "good" commits. And it will sit and wait until you give at least one "good". So we'd need an "--assume-roots-are-good" option or something to force bisect the whole history below the "bad" one.
diff --git a/bisect.c b/bisect.c index 888949fba6..9e6a2b7f20 100644 --- a/bisect.c +++ b/bisect.c @@ -724,7 +724,8 @@ static int is_expected_rev(const struct object_id *oid) return res; } -static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int no_checkout) +enum bisect_error bisect_checkout(const struct object_id *bisect_rev, + int no_checkout) { char bisect_rev_hex[GIT_MAX_HEXSZ + 1]; struct commit *commit; diff --git a/bisect.h b/bisect.h index ec24ac2d7e..748adf0cff 100644 --- a/bisect.h +++ b/bisect.h @@ -69,4 +69,7 @@ void read_bisect_terms(const char **bad, const char **good); int bisect_clean_state(void); +enum bisect_error bisect_checkout(const struct object_id *bisect_rev, + int no_checkout); + #endif diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index e529665d9f..50783a586c 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -1089,6 +1089,44 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a return res; } +static int get_first_good(const char *refname, const struct object_id *oid, + int flag, void *cb_data) +{ + oidcpy(cb_data, oid); + return 1; +} + +static int verify_good(const struct bisect_terms *terms, + const char **quoted_argv) +{ + int rc; + enum bisect_error res; + struct object_id good_rev; + struct object_id current_rev; + char *good_glob = xstrfmt("%s-*", terms->term_good); + int no_checkout = ref_exists("BISECT_HEAD"); + + for_each_glob_ref_in(get_first_good, good_glob, "refs/bisect/", + &good_rev); + free(good_glob); + + if (read_ref(no_checkout ? "BISECT_HEAD" : "HEAD", ¤t_rev)) + return -1; + + res = bisect_checkout(&good_rev, no_checkout); + if (res != BISECT_OK) + return -1; + + printf(_("running %s\n"), quoted_argv[0]); + rc = run_command_v_opt(quoted_argv, RUN_USING_SHELL); + + res = bisect_checkout(¤t_rev, no_checkout); + if (res != BISECT_OK) + return -1; + + return rc; +} + static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) { int res = BISECT_OK; @@ -1096,6 +1134,7 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) struct strvec run_args = STRVEC_INIT; const char *new_state; int temporary_stdout_fd, saved_stdout; + int is_first_run = 1; if (bisect_next_check(terms, NULL)) return BISECT_FAILED; @@ -1113,6 +1152,30 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) printf(_("running %s\n"), command.buf); res = run_command_v_opt(run_args.v, RUN_USING_SHELL); + /* + * Exit code 126 and 127 can either come from the shell + * if it was unable to execute or even find the script, + * or from the script itself. Check with a known-good + * revision to avoid trashing the bisect run due to a + * missing or non-executable script. + */ + if (is_first_run && (res == 126 || res == 127)) { + int rc = verify_good(terms, run_args.v); + is_first_run = 0; + if (rc < 0) { + error(_("unable to verify '%s' on good" + " revision"), command.buf); + res = BISECT_FAILED; + break; + } + if (rc == res) { + error(_("bogus exit code %d for good revision"), + rc); + res = BISECT_FAILED; + break; + } + } + if (res < 0 || 128 <= res) { error(_("bisect run failed: exit code %d from" " '%s' is < 0 or >= 128"), res, command.buf); diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh index fc18796517..5382e5d216 100755 --- a/t/t6030-bisect-porcelain.sh +++ b/t/t6030-bisect-porcelain.sh @@ -290,7 +290,7 @@ test_expect_success 'bisect run accepts exit code 126 as bad' ' grep "$HASH3 is the first bad commit" my_bisect_log.txt ' -test_expect_failure POSIXPERM 'bisect run fails with non-executable test script' ' +test_expect_success POSIXPERM 'bisect run fails with non-executable test script' ' test_when_finished "git bisect reset" && >not-executable.sh && chmod -x not-executable.sh && @@ -313,7 +313,7 @@ test_expect_success 'bisect run accepts exit code 127 as bad' ' grep "$HASH3 is the first bad commit" my_bisect_log.txt ' -test_expect_failure 'bisect run fails with missing test script' ' +test_expect_success 'bisect run fails with missing test script' ' test_when_finished "git bisect reset" && rm -f does-not-exist.sh && git bisect start &&
When a run command cannot be executed or found, shells return exit code 126 or 127, respectively. Valid run commands are allowed to return these codes as well to indicate bad revisions, though, for historical reasons. This means typos can cause bogus bisect runs that go over the full distance and end up reporting invalid results. The best solution would be to reserve exit codes 126 and 127, like 71b0251cdd (Bisect run: "skip" current commit if script exit code is 125., 2007-10-26) did for 125, and abort bisect run when we get them. That might be inconvenient for those who relied on the documentation stating that 126 and 127 can be used for bad revisions, though. The workaround used by this patch is to run the command on a known-good revision and abort if we still get the same error code. This adds one step to runs with scripts that use exit codes 126 and 127, but keeps them supported, with one exception: It won't work with commands that cannot recognize the (manually marked) known-good revision as such. Run commands that use low exit codes are unaffected. Typos are reported after executing the missing command twice and three checkouts (the first step, the known good revision and back to the revision of the first step). Signed-off-by: René Scharfe <l.s.r@web.de> --- bisect.c | 3 +- bisect.h | 3 ++ builtin/bisect--helper.c | 63 +++++++++++++++++++++++++++++++++++++ t/t6030-bisect-porcelain.sh | 4 +-- 4 files changed, 70 insertions(+), 3 deletions(-) -- 2.34.1