Merge branch 'rs/bisect-executable-not-found'

A not-so-common mistake is to write a script to feed "git bisect
run" without making it executable, in which case all tests will
exit with 126 or 127 error codes, even on revisions that are marked
as good.  Try to recognize this situation and stop iteration early.

* rs/bisect-executable-not-found:
  bisect--helper: double-check run command on exit code 126 and 127
  bisect: document run behavior with exit codes 126 and 127
  bisect--helper: release strbuf and strvec on run error
  bisect--helper: report actual bisect_state() argument on error

4 files changed, 125 insertions, 14 deletions

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..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
@@ -69,4 +70,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 f962dbd430..626de92098 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -1089,14 +1089,52 @@ 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", &current_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(&current_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;
 	struct strbuf command = STRBUF_INIT;
-	struct strvec args = STRVEC_INIT;
 	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;
@@ -1111,16 +1149,37 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 	strvec_push(&run_args, command.buf);
 
 	while (1) {
-		strvec_clear(&args);
-
 		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);
-			strbuf_release(&command);
-			return res;
+			break;
 		}
 
 		if (res == 125)
@@ -1132,8 +1191,10 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 
 		temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);
 
-		if (temporary_stdout_fd < 0)
-			return error_errno(_("cannot open file '%s' for writing"), git_path_bisect_run());
+		if (temporary_stdout_fd < 0) {
+			res = error_errno(_("cannot open file '%s' for writing"), git_path_bisect_run());
+			break;
+		}
 
 		fflush(stdout);
 		saved_stdout = dup(1);
@@ -1158,16 +1219,16 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
 			res = BISECT_OK;
 		} else if (res) {
 			error(_("bisect run failed: 'git bisect--helper --bisect-state"
-			" %s' exited with error code %d"), args.v[0], res);
+			" %s' exited with error code %d"), new_state, res);
 		} else {
 			continue;
 		}
-
-		strbuf_release(&command);
-		strvec_clear(&args);
-		strvec_clear(&run_args);
-		return res;
+		break;
 	}
+
+	strbuf_release(&command);
+	strvec_clear(&run_args);
+	return res;
 }
 
 int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 1be85d064e..5382e5d216 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -278,6 +278,51 @@ test_expect_success '"git bisect run" with more complex "git bisect start"' '
 	git bisect reset
 '
 
+test_expect_success 'bisect run accepts exit code 126 as bad' '
+	test_when_finished "git bisect reset" &&
+	write_script test_script.sh <<-\EOF &&
+	! grep Another hello || exit 126 >/dev/null
+	EOF
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "$HASH3 is the first bad commit" my_bisect_log.txt
+'
+
+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 &&
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	test_must_fail git bisect run ./not-executable.sh >my_bisect_log.txt &&
+	! grep "is the first bad commit" my_bisect_log.txt
+'
+
+test_expect_success 'bisect run accepts exit code 127 as bad' '
+	test_when_finished "git bisect reset" &&
+	write_script test_script.sh <<-\EOF &&
+	! grep Another hello || exit 127 >/dev/null
+	EOF
+	git bisect start &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	git bisect run ./test_script.sh >my_bisect_log.txt &&
+	grep "$HASH3 is the first bad commit" my_bisect_log.txt
+'
+
+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 &&
+	git bisect good $HASH1 &&
+	git bisect bad $HASH4 &&
+	test_must_fail git bisect run ./does-not-exist.sh >my_bisect_log.txt &&
+	! grep "is the first bad commit" my_bisect_log.txt
+'
+
 # $HASH1 is good, $HASH5 is bad, we skip $HASH3
 # but $HASH4 is good,
 # so we should find $HASH5 as the first bad commit