Re: [PATCH v6 5/6] bisect--helper: reimplement `bisect_run` shell

From: Tanushree Tumane <redacted>

Reimplement the `bisect_run()` shell function
in C and also add `--bisect-run` subcommand to
`git bisect--helper` to call it from git-bisect.sh.

Mentored-by: Christian Couder [off-list ref]
Signed-off-by: Tanushree Tumane <redacted>
Signed-off-by: Miriam Rubio <redacted>
---
 builtin/bisect--helper.c | 97 ++++++++++++++++++++++++++++++++++++++++
 git-bisect.sh            | 62 +------------------------
 2 files changed, 98 insertions(+), 61 deletions(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1e118a966a..8e9ed9c318 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c

@@ -18,6 +18,7 @@ static GIT_PATH_FUNC(git_path_bisect_log, "BISECT_LOG")
 static GIT_PATH_FUNC(git_path_head_name, "head-name")
 static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NAMES")
 static GIT_PATH_FUNC(git_path_bisect_first_parent, "BISECT_FIRST_PARENT")
+static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN")
 
 static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-reset [<commit>]"),

@@ -31,6 +32,7 @@ static const char * const git_bisect_helper_usage[] = {
 	N_("git bisect--helper --bisect-replay <filename>"),
 	N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
 	N_("git bisect--helper --bisect-visualize"),
+	N_("git bisect--helper --bisect-run <cmd>..."),
 	NULL
 };
 

@@ -144,6 +146,19 @@ static int append_to_file(const char *path, const char *format, ...)
 	return res;
 }
 
+static int print_file_to_stdout(const char *path)
+{
+	int fd = open(path, O_RDONLY);
+	int ret = 0;
+
+	if (fd < 0)
+		return error_errno(_("cannot open file '%s' for reading"), path);
+	if (copy_fd(fd, 1) < 0)
+		ret = error_errno(_("failed to read '%s'"), path);
+	close(fd);
+	return ret;
+}
+
 static int check_term_format(const char *term, const char *orig_term)
 {
 	int res;

@@ -1075,6 +1090,79 @@ static int bisect_visualize(struct bisect_terms *terms, const char **argv, int a
 	return res;
 }
 
+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;
+
+	if (bisect_next_check(terms, NULL))
+		return BISECT_FAILED;
+
+	if (argc)
+		sq_quote_argv(&command, argv);
+	else {
+		error(_("bisect run failed: no command provided."));
+		return BISECT_FAILED;
+	}
+	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);
+
+		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;
+		}
+
+		if (res == 125)
+			new_state = "skip";
+		else
+			new_state = res > 0 ? terms->term_bad : terms->term_good;

-- snipsnap --

diff --git a/bisect.c b/bisect.c
index af2863d044b..405bf60b4b6 100644
--- a/bisect.c
+++ b/bisect.c

@@ -683,20 +683,21 @@ static void bisect_common(struct rev_info *revs)
 }

 static enum bisect_error error_if_skipped_commits(struct commit_list *tried,
-                                   const struct object_id *bad)
+                                                 const struct object_id *bad,
+                                                 FILE *out)
 {
        if (!tried)
                return BISECT_OK;

-       printf("There are only 'skip'ped commits left to test.\n"
-              "The first %s commit could be any of:\n", term_bad);
+       fprintf(out, "There are only 'skip'ped commits left to test.\n"
+               "The first %s commit could be any of:\n", term_bad);

        for ( ; tried; tried = tried->next)
-               printf("%s\n", oid_to_hex(&tried->item->object.oid));
+               fprintf(out, "%s\n", oid_to_hex(&tried->item->object.oid));

        if (bad)
-               printf("%s\n", oid_to_hex(bad));
-       printf(_("We cannot bisect more!\n"));
+               fprintf(out, "%s\n", oid_to_hex(bad));
+       fprintf(out, _("We cannot bisect more!\n"));

        return BISECT_ONLY_SKIPPED_LEFT;
 }

@@ -725,10 +726,12 @@ 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)
+static enum bisect_error bisect_checkout(const struct object_id *bisect_rev,
+                                        int no_checkout, FILE *out)
 {
        char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
        enum bisect_error res = BISECT_OK;
+       struct child_process cp = CHILD_PROCESS_INIT;

        oid_to_hex_r(bisect_rev_hex, bisect_rev);
        update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);

@@ -749,7 +752,10 @@ static enum bisect_error bisect_checkout(const struct object_id *bisect_rev, int
        }

        argv_show_branch[1] = bisect_rev_hex;
-       res = run_command_v_opt(argv_show_branch, RUN_GIT_CMD);
+       cp.argv = argv_show_branch;
+       cp.git_cmd = 1;
+       cp.out = dup(fileno(out));
+       res = run_command(&cp);
        /*
         * Errors in `run_command()` itself, signaled by res < 0,
         * and errors in the child process, signaled by res > 0

@@ -841,7 +847,8 @@ static void handle_skipped_merge_base(const struct object_id *mb)
  * for early success, this will be converted back to 0 in
  * check_good_are_ancestors_of_bad().
  */
-static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
+static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev,
+                                          int no_checkout, FILE *out)
 {
        enum bisect_error res = BISECT_OK;
        struct commit_list *result;

@@ -858,8 +865,8 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int
                } else if (0 <= oid_array_lookup(&skipped_revs, mb)) {
                        handle_skipped_merge_base(mb);
                } else {
-                       printf(_("Bisecting: a merge base must be tested\n"));
-                       res = bisect_checkout(mb, no_checkout);
+                       fprintf(out, _("Bisecting: a merge base must be tested\n"));
+                       res = bisect_checkout(mb, no_checkout, out);
                        if (!res)
                                /* indicate early success */
                                res = BISECT_INTERNAL_SUCCESS_MERGE_BASE;

@@ -898,8 +905,9 @@ static int check_ancestors(struct repository *r, int rev_nr,
  */

 static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,
-                                           const char *prefix,
-                                           int no_checkout)
+                                                        const char *prefix,
+                                                        int no_checkout,
+                                                        FILE *out)
 {
        char *filename;
        struct stat st;

@@ -924,7 +932,7 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,

        rev = get_bad_and_good_commits(r, &rev_nr);
        if (check_ancestors(r, rev_nr, rev, prefix))
-               res = check_merge_bases(rev_nr, rev, no_checkout);
+               res = check_merge_bases(rev_nr, rev, no_checkout, out);
        free(rev);

        if (!res) {

@@ -953,7 +961,7 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,
  */
 static void show_diff_tree(struct repository *r,
                           const char *prefix,
-                          struct commit *commit)
+                          struct commit *commit, FILE *out)
 {
        const char *argv[] = {
                "diff-tree", "--pretty", "--stat", "--summary", "--cc", NULL

@@ -964,6 +972,7 @@ static void show_diff_tree(struct repository *r,
        repo_init_revisions(r, &opt, prefix);

        setup_revisions(ARRAY_SIZE(argv) - 1, argv, &opt, NULL);
+       opt.diffopt.file = out;
        log_tree_commit(&opt, commit);
 }

@@ -1007,7 +1016,8 @@ void read_bisect_terms(const char **read_bad, const char **read_good)
  * the end of bisect_helper::cmd_bisect__helper() helps bypassing
  * all the code related to finding a commit to test.
  */
-enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
+enum bisect_error bisect_next_all(struct repository *r, const char *prefix,
+                                 FILE *out)
 {
        struct rev_info revs;
        struct commit_list *tried;

@@ -1032,7 +1042,7 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
        if (skipped_revs.nr)
                bisect_flags |= FIND_BISECTION_ALL;

-       res = check_good_are_ancestors_of_bad(r, prefix, no_checkout);
+       res = check_good_are_ancestors_of_bad(r, prefix, no_checkout, out);
        if (res)
                return res;

@@ -1051,10 +1061,10 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
                 * We should return error here only if the "bad"
                 * commit is also a "skip" commit.
                 */
-               res = error_if_skipped_commits(tried, NULL);
+               res = error_if_skipped_commits(tried, NULL, out);
                if (res < 0)
                        return res;
-               printf(_("%s was both %s and %s\n"),
+               fprintf(out, _("%s was both %s and %s\n"),
                       oid_to_hex(current_bad_oid),
                       term_good,
                       term_bad);

@@ -1072,13 +1082,13 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
        bisect_rev = &revs.commits->item->object.oid;

        if (oideq(bisect_rev, current_bad_oid)) {
-               res = error_if_skipped_commits(tried, current_bad_oid);
+               res = error_if_skipped_commits(tried, current_bad_oid, out);
                if (res)
                        return res;
-               printf("%s is the first %s commit\n", oid_to_hex(bisect_rev),
-                       term_bad);
+               fprintf(out, "%s is the first %s commit\n",
+                       oid_to_hex(bisect_rev), term_bad);

-               show_diff_tree(r, prefix, revs.commits->item);
+               show_diff_tree(r, prefix, revs.commits->item, out);
                /*
                 * This means the bisection process succeeded.
                 * Using BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND (-10)

@@ -1098,14 +1108,14 @@ enum bisect_error bisect_next_all(struct repository *r, const char *prefix)
         * TRANSLATORS: the last %s will be replaced with "(roughly %d
         * steps)" translation.
         */
-       printf(Q_("Bisecting: %d revision left to test after this %s\n",
-                 "Bisecting: %d revisions left to test after this %s\n",
-                 nr), nr, steps_msg);
+       fprintf(out, Q_("Bisecting: %d revision left to test after this %s\n",
+                       "Bisecting: %d revisions left to test after this %s\n",
+                       nr), nr, steps_msg);
        free(steps_msg);
        /* Clean up objects used, as they will be reused. */
        repo_clear_commit_marks(r, ALL_REV_FLAGS);

-       return bisect_checkout(bisect_rev, no_checkout);
+       return bisect_checkout(bisect_rev, no_checkout, out);
 }

 static inline int log2i(int n)

diff --git a/bisect.h b/bisect.h
index ec24ac2d7ee..72bfd7b0053 100644
--- a/bisect.h
+++ b/bisect.h

@@ -61,7 +61,8 @@ enum bisect_error {
        BISECT_INTERNAL_SUCCESS_MERGE_BASE = -11
 };

-enum bisect_error bisect_next_all(struct repository *r, const char *prefix);
+enum bisect_error bisect_next_all(struct repository *r, const char *prefix,
+                                 FILE *out);

 int estimate_bisect_steps(int all);

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index 1c96580bd49..29969763d35 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c

@@ -581,7 +581,8 @@ static int bisect_successful(struct bisect_terms *terms)
        return res;
 }

-static enum bisect_error bisect_next(struct bisect_terms *terms, const char *prefix)
+static enum bisect_error bisect_next(struct bisect_terms *terms,
+                                    const char *prefix, FILE *out)
 {
        enum bisect_error res;

@@ -592,7 +593,7 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
                return BISECT_FAILED;

        /* Perform all bisection computation */
-       res = bisect_next_all(the_repository, prefix);
+       res = bisect_next_all(the_repository, prefix, out);

        if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) {
                res = bisect_successful(terms);

@@ -604,12 +605,13 @@ static enum bisect_error bisect_next(struct bisect_terms *terms, const char *pre
        return res;
 }

-static enum bisect_error bisect_auto_next(struct bisect_terms *terms, const char *prefix)
+static enum bisect_error bisect_auto_next(struct bisect_terms *terms,
+                                         const char *prefix, FILE *out)
 {
        if (bisect_next_check(terms, NULL))
                return BISECT_OK;

-       return bisect_next(terms, prefix);
+       return bisect_next(terms, prefix, out);
 }

 static enum bisect_error bisect_start(struct bisect_terms *terms, const char **argv, int argc)

@@ -808,7 +810,7 @@ static enum bisect_error bisect_start(struct bisect_terms *terms, const char **a
        if (res)
                return res;

-       res = bisect_auto_next(terms, NULL);
+       res = bisect_auto_next(terms, NULL, stdout);
        if (!is_bisect_success(res))
                bisect_clean_state();
        return res;

@@ -847,7 +849,7 @@ static int bisect_autostart(struct bisect_terms *terms)
 }

 static enum bisect_error bisect_state(struct bisect_terms *terms, const char **argv,
-                                     int argc)
+                                     int argc, FILE *out)
 {
        const char *state;
        int i, verify_expected = 1;

@@ -924,7 +926,7 @@ static enum bisect_error bisect_state(struct bisect_terms *terms, const char **a
        }

        oid_array_clear(&revs);
-       return bisect_auto_next(terms, NULL);
+       return bisect_auto_next(terms, NULL, out);
 }

 static enum bisect_error bisect_log(void)

@@ -1013,7 +1015,7 @@ static enum bisect_error bisect_replay(struct bisect_terms *terms, const char *f
        if (res)
                return BISECT_FAILED;

-       return bisect_auto_next(terms, NULL);
+       return bisect_auto_next(terms, NULL, stdout);
 }

 static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **argv, int argc)

@@ -1045,7 +1047,7 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, const char **ar
                        strvec_push(&argv_state, argv[i]);
                }
        }
-       res = bisect_state(terms, argv_state.v, argv_state.nr);
+       res = bisect_state(terms, argv_state.v, argv_state.nr, stdout);

        strvec_clear(&argv_state);
        return res;

@@ -1096,7 +1098,6 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
        struct strvec args = STRVEC_INIT;
        struct strvec run_args = STRVEC_INIT;
        const char *new_state;
-       int temporary_stdout_fd, saved_stdout;

        if (bisect_next_check(terms, NULL))
                return BISECT_FAILED;

@@ -1111,6 +1112,8 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
        strvec_push(&run_args, command.buf);

        while (1) {
+               FILE *f;
+
                strvec_clear(&args);

                printf(_("running %s\n"), command.buf);

@@ -1130,19 +1133,13 @@ static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
                else
                        new_state = terms->term_bad;

-               temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666);
+               f = fopen_for_writing(git_path_bisect_run());

-               if (temporary_stdout_fd < 0)
+               if (!f)
                        return error_errno(_("cannot open file '%s' for writing"), git_path_bisect_run());

-               saved_stdout = dup(1);
-               dup2(temporary_stdout_fd, 1);
-
-               res = bisect_state(terms, &new_state, 1);
-
-               dup2(saved_stdout, 1);
-               close(saved_stdout);
-               close(temporary_stdout_fd);
+               res = bisect_state(terms, &new_state, 1, f);
+               fclose(f);

                print_file_to_stdout(git_path_bisect_run());

@@ -1240,12 +1237,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
                if (argc)
                        return error(_("--bisect-next requires 0 arguments"));
                get_terms(&terms);
-               res = bisect_next(&terms, prefix);
+               res = bisect_next(&terms, prefix, stdout);
                break;
        case BISECT_STATE:
                set_terms(&terms, "bad", "good");
                get_terms(&terms);
-               res = bisect_state(&terms, argv, argc);
+               res = bisect_state(&terms, argv, argc, stdout);
                break;
        case BISECT_LOG:
                if (argc)