Am 04.02.22 um 01:42 schrieb Junio C Hamano:

René Scharfe [off-list ref] writes:

quoted

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?

quoted

+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.

quoted

+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", &current_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, ...

quoted

+	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", &current_rev))
 		return -1;
--

2.35.0