| Message ID | 20210902090421.93113-6-mirucam@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Finish converting git bisect to C part 4 | expand |
Miriam Rubio <mirucam@gmail.com> writes: > From: Tanushree Tumane <tanushreetumane@gmail.com> > > 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 <chriscool@tuxfamily.org> > Signed-off-by: Tanushree Tumane <tanushreetumane@gmail.com> > Signed-off-by: Miriam Rubio <mirucam@gmail.com> > --- > 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; It is easier to follow the code if you spelled out this part as else if (!res) new_state = terms->term_good; else new_state = terms->term_bad; because that would consistently handle the three cases. Of course you _could_ do new_state = (res == 125) ? "skip" : (res > 0) ? terms->term_bad : terms->term_good; instead, but that would be harder to read. > + temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666); Can this open fail, and if it fails, what do we want to do? > + 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); Hmph, now you lost me. Whose output are we working around here with the redirection? ... goes and looks ... Ahh, OK. bisect_next_all() to bisect_checkout() all assume that they only need to write to the standard output, so we need to do this dance (unless we are willing to update the bisect.c functions to accept FILE * as parameter, that is). However, they use not just write(2) but stdio to do their output, no? Don't we need to fflush(stdout) around the redirection dance, one to empty the output that was associated with the real standard output stream before asking bisect_state() to write to fd #1 via stdio, and one more time to flush out what bisect_state() wrote to the stdio after the call returns before closing the fd we opened to the BISECT_RUN file? > + print_file_to_stdout(git_path_bisect_run()); OK. So this corresponds to the "write bisect-state to ./git/BISECT_RUN and then cat it" in the scripted version. > + if (res == BISECT_ONLY_SKIPPED_LEFT) > + error(_("bisect run cannot continue any more")); > + else if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) { > + printf(_("bisect run success")); > + res = BISECT_OK; > + } else if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) { > + printf(_("bisect found first bad commit")); > + 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); > + } else { > + continue; > + } > + > + strbuf_release(&command); > + strvec_clear(&args); > + strvec_clear(&run_args); > + return res; > + } > +} OK, the "res to diag" and clearing the resources at the end of the function looks good to me. Thanks.
Hi Junio & Miriam, On Thu, 2 Sep 2021, Junio C Hamano wrote: > Miriam Rubio <mirucam@gmail.com> writes: > > [...] > > @@ -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; > > It is easier to follow the code if you spelled out this part as > > else if (!res) > new_state = terms->term_good; > else > new_state = terms->term_bad; > > because that would consistently handle the three cases. Of course > you _could_ do > > new_state = (res == 125) > ? "skip" > : (res > 0) > ? terms->term_bad > : terms->term_good; > > instead, but that would be harder to read. FWIW I agree with this, after seeing the resulting code. > > + temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666); > > Can this open fail, and if it fails, what do we want to do? > > > + 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); > > Hmph, now you lost me. Whose output are we working around here with > the redirection? > > ... goes and looks ... > > Ahh, OK. bisect_next_all() to bisect_checkout() all assume that > they only need to write to the standard output, so we need to do > this dance (unless we are willing to update the bisect.c functions > to accept FILE * as parameter, that is). > > However, they use not just write(2) but stdio to do their output, > no? Don't we need to fflush(stdout) around the redirection dance, > one to empty the output that was associated with the real standard > output stream before asking bisect_state() to write to fd #1 via > stdio, and one more time to flush out what bisect_state() wrote to > the stdio after the call returns before closing the fd we opened to > the BISECT_RUN file? Yes, we would have to `fflush(stdout)`. However, I still don't like that we play such a `dup2()` game. I gave it a quick try to avoid it (see the diff below, which corresponds to the commit I pushed up as `git-bisect-work-part4-v7` to https://github.com/dscho/git), which still could benefit from a bit of polishing (maybe we should rethink the object model and extend/rename `bisect_terms` to `bisect_state` and accumulate more fields, such as `out_fd`. Obviously this will need to be cleaned up, and while I would _love_ to see this make it into your next iteration, ultimately it is up to you, Miriam, to decide whether you want to build on my diff (quite possibly making the entire object model of the bisect part of Git's code more elegant and more maintainable), and up to you, Junio, to decide whether you would be willing to accept the patch series without this refactoring. -- 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)
Hi Johannes, El lun, 6 sept 2021 a las 9:33, Johannes Schindelin (<Johannes.Schindelin@gmx.de>) escribió: > > Hi Junio & Miriam, > > On Thu, 2 Sep 2021, Junio C Hamano wrote: > > > Miriam Rubio <mirucam@gmail.com> writes: > > > > [...] > > > @@ -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; > > > > It is easier to follow the code if you spelled out this part as > > > > else if (!res) > > new_state = terms->term_good; > > else > > new_state = terms->term_bad; > > > > because that would consistently handle the three cases. Of course > > you _could_ do > > > > new_state = (res == 125) > > ? "skip" > > : (res > 0) > > ? terms->term_bad > > : terms->term_good; > > > > instead, but that would be harder to read. > > FWIW I agree with this, after seeing the resulting code. > > > > + temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666); > > > > Can this open fail, and if it fails, what do we want to do? > > > > > + 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); > > > > Hmph, now you lost me. Whose output are we working around here with > > the redirection? > > > > ... goes and looks ... > > > > Ahh, OK. bisect_next_all() to bisect_checkout() all assume that > > they only need to write to the standard output, so we need to do > > this dance (unless we are willing to update the bisect.c functions > > to accept FILE * as parameter, that is). > > > > However, they use not just write(2) but stdio to do their output, > > no? Don't we need to fflush(stdout) around the redirection dance, > > one to empty the output that was associated with the real standard > > output stream before asking bisect_state() to write to fd #1 via > > stdio, and one more time to flush out what bisect_state() wrote to > > the stdio after the call returns before closing the fd we opened to > > the BISECT_RUN file? > > Yes, we would have to `fflush(stdout)`. > > However, I still don't like that we play such a `dup2()` game. I gave it a > quick try to avoid it (see the diff below, which corresponds to the commit > I pushed up as `git-bisect-work-part4-v7` to > https://github.com/dscho/git), which still could benefit from a bit of > polishing (maybe we should rethink the object model and extend/rename > `bisect_terms` to `bisect_state` and accumulate more fields, such as > `out_fd`. > > Obviously this will need to be cleaned up, and while I would _love_ to see > this make it into your next iteration, ultimately it is up to you, Miriam, > to decide whether you want to build on my diff (quite possibly making the > entire object model of the bisect part of Git's code more elegant and more > maintainable), and up to you, Junio, to decide whether you would be > willing to accept the patch series without this refactoring. > I also don’t love this `dup2()` game but I implemented it as a possible solution to recreate the cat command as it is in the shell script, without changing behavior or parameters in other functions. Also thank you for your solution, I agree that it is more elegant and maintainable. If Junio accepts the patch series with my `dup2()` solution, I can implement your suggestion as an improvement after finishing the porting of git bisect to C. Because after this patch series, there will be only one last patch series left and I believe rethinking the object model and extend/rename `bisect_terms` to `bisect_state` and accumulate more fields, such as `out_fd` should be better separated of the porting project. Best, Miriam. > -- 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)
"Miriam R." <mirucam@gmail.com> writes: >> However, I still don't like that we play such a `dup2()` game. I gave it a >> quick try to avoid it (see the diff below, which corresponds to the commit >> I pushed up as `git-bisect-work-part4-v7` to >> https://github.com/dscho/git), which still could benefit from a bit of >> polishing (maybe we should rethink the object model and extend/rename >> `bisect_terms` to `bisect_state` and accumulate more fields, such as >> `out_fd`. >> >> Obviously this will need to be cleaned up, and while I would _love_ to see >> this make it into your next iteration, ultimately it is up to you, Miriam, >> to decide whether you want to build on my diff (quite possibly making the >> entire object model of the bisect part of Git's code more elegant and more >> maintainable), and up to you, Junio, to decide whether you would be >> willing to accept the patch series without this refactoring. If the code paths involved are shallow and narrow enough that not too many existing callers need to start passing FILE *stdout down (from the looks of your illustration patch, it does not seem to be too bad), I do not mind a series that is a bit longer than the current 6-patch series that has a preliminary enhancement step that allows callers to pass their own "FILE *" for output destination before the main part of the topic. Thanks.
Hi Junio, On Tue, 7 Sep 2021, Junio C Hamano wrote: > "Miriam R." <mirucam@gmail.com> writes: > > >> However, I still don't like that we play such a `dup2()` game. I gave it a > >> quick try to avoid it (see the diff below, which corresponds to the commit > >> I pushed up as `git-bisect-work-part4-v7` to > >> https://github.com/dscho/git), which still could benefit from a bit of > >> polishing (maybe we should rethink the object model and extend/rename > >> `bisect_terms` to `bisect_state` and accumulate more fields, such as > >> `out_fd`. > >> > >> Obviously this will need to be cleaned up, and while I would _love_ to see > >> this make it into your next iteration, ultimately it is up to you, Miriam, > >> to decide whether you want to build on my diff (quite possibly making the > >> entire object model of the bisect part of Git's code more elegant and more > >> maintainable), and up to you, Junio, to decide whether you would be > >> willing to accept the patch series without this refactoring. > > If the code paths involved are shallow and narrow enough that not > too many existing callers need to start passing FILE *stdout down > (from the looks of your illustration patch, it does not seem to be > too bad), I do not mind a series that is a bit longer than the > current 6-patch series that has a preliminary enhancement step that > allows callers to pass their own "FILE *" for output destination > before the main part of the topic. My impression, from the diff that I sent, is that this is too deep and wide, and indeed needs a follow-up patch series as indicated by Miriam. My preference would be (as I hinted at) to accumulate relevant data (such as the terms and, yes, the `FILE *`) into a `struct bisect_state` and pass that around. Sort of a light-weight object-oriented design, similar to how we do things in `builtin/am.c` with `struct am_state`. Thanks, Dscho
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; + + temporary_stdout_fd = open(git_path_bisect_run(), O_CREAT | O_WRONLY | O_TRUNC, 0666); + 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); + + print_file_to_stdout(git_path_bisect_run()); + + if (res == BISECT_ONLY_SKIPPED_LEFT) + error(_("bisect run cannot continue any more")); + else if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) { + printf(_("bisect run success")); + res = BISECT_OK; + } else if (res == BISECT_INTERNAL_SUCCESS_1ST_BAD_FOUND) { + printf(_("bisect found first bad commit")); + 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); + } else { + continue; + } + + strbuf_release(&command); + strvec_clear(&args); + strvec_clear(&run_args); + return res; + } +} + int cmd_bisect__helper(int argc, const char **argv, const char *prefix) { enum { @@ -1089,6 +1177,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) BISECT_REPLAY, BISECT_SKIP, BISECT_VISUALIZE, + BISECT_RUN, } cmdmode = 0; int res = 0, nolog = 0; struct option options[] = { @@ -1112,6 +1201,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) N_("skip some commits for checkout"), BISECT_SKIP), OPT_CMDMODE(0, "bisect-visualize", &cmdmode, N_("visualize the bisection"), BISECT_VISUALIZE), + OPT_CMDMODE(0, "bisect-run", &cmdmode, + N_("use <cmd>... to automatically bisect."), BISECT_RUN), OPT_BOOL(0, "no-log", &nolog, N_("no log for BISECT_WRITE")), OPT_END() @@ -1177,6 +1268,12 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) get_terms(&terms); res = bisect_visualize(&terms, argv, argc); break; + case BISECT_RUN: + if (!argc) + return error(_("bisect run failed: no command provided.")); + get_terms(&terms); + res = bisect_run(&terms, argv, argc); + break; default: BUG("unknown subcommand %d", cmdmode); } diff --git a/git-bisect.sh b/git-bisect.sh index 95f7f3fb8c..e83d011e17 100755 --- a/git-bisect.sh +++ b/git-bisect.sh @@ -39,66 +39,6 @@ _x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40" TERM_BAD=bad TERM_GOOD=good -bisect_run () { - git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit - - test -n "$*" || die "$(gettext "bisect run failed: no command provided.")" - - while true - do - command="$@" - eval_gettextln "running \$command" - "$@" - res=$? - - # Check for really bad run error. - if [ $res -lt 0 -o $res -ge 128 ] - then - eval_gettextln "bisect run failed: -exit code \$res from '\$command' is < 0 or >= 128" >&2 - exit $res - fi - - # Find current state depending on run success or failure. - # A special exit code of 125 means cannot test. - if [ $res -eq 125 ] - then - state='skip' - elif [ $res -gt 0 ] - then - state="$TERM_BAD" - else - state="$TERM_GOOD" - fi - - git bisect--helper --bisect-state $state >"$GIT_DIR/BISECT_RUN" - res=$? - - cat "$GIT_DIR/BISECT_RUN" - - if sane_grep "first $TERM_BAD commit could be any of" "$GIT_DIR/BISECT_RUN" \ - >/dev/null - then - gettextln "bisect run cannot continue any more" >&2 - exit $res - fi - - if [ $res -ne 0 ] - then - eval_gettextln "bisect run failed: -'bisect-state \$state' exited with error code \$res" >&2 - exit $res - fi - - if sane_grep "is the first $TERM_BAD commit" "$GIT_DIR/BISECT_RUN" >/dev/null - then - gettextln "bisect run success" - exit 0; - fi - - done -} - get_terms () { if test -s "$GIT_DIR/BISECT_TERMS" then @@ -137,7 +77,7 @@ case "$#" in log) git bisect--helper --bisect-log || exit ;; run) - bisect_run "$@" ;; + git bisect--helper --bisect-run "$@" || exit;; terms) git bisect--helper --bisect-terms "$@" || exit;; *)