| Message ID | 20210407173334.68222-4-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> If I am reading the patch correctly, this removes the need for the $GIT_DIR/BISECT_RUN file that used to be used to keep track of the state? If that is true, it is worth noting in the proposed log message. As far as I can see, nobody creates $GIT_DIR/BISECT_RUN anymore. $ git grep -e path_bisect_run -e BISECT_RUN bisect.c:static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") bisect.c: unlink_or_warn(git_path_bisect_run()); builtin/bisect--helper.c: BISECT_RUN, builtin/bisect--helper.c: N_("use <cmd>... to automatical... builtin/bisect--helper.c: case BISECT_RUN: t/t6030-bisect-porcelain.sh: test_path_is_missing ".git/BISECT_RUN" && What if a run script tried to read from (or checked the presence of) the file for its correct operation (e.g. I would imagine that "do this operation when run interactively, but do the same operation silently when run from the git-bisect machinery" may be a reasonable thing to do)? This change just unintendedly broke such a script, didn't it? The change makes me a bit worried. > + if (bisect_next_check(terms, NULL)) > + return BISECT_FAILED; > + > + if (argc) > + sq_quote_argv(&command, argv); > + else > + return BISECT_FAILED; > + > + run_args.v[0] = xstrdup(command.buf); > + run_args.nr = 1; > + > + while (1) { > + strvec_clear(&args); > + > + printf(_("running %s"), command.buf); > + res = run_command_v_opt(run_args.v, RUN_USING_SHELL); Nicely used sq_quote_argv() with RUN_USING_SHELL here. Goodl. > + if (res < 0 && res >= 128) { > + error(_("bisect run failed: exit code %d from" > + " '%s' is < 0 or >= 128"), res, command.buf); > + strbuf_release(&command); > + return res; > + } > + > + if (res == 125) > + strvec_push(&args, "skip"); > + else if (res > 0) > + strvec_push(&args, terms->term_bad); > + else > + strvec_push(&args, terms->term_good); > + bisect_state() does so much that it was a bit hard to follow for me (who hasn't been following the bisect-in-C topic very closely), but the code around here roughly corresponds to the following snippet in the original scripted version. > - 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 I see that the contents of the file BISECT_RUN is shown to the user in the original but is that part of what bisect_state() does, or did we lose it during this round of conversion? > + res = bisect_state(terms, args.v, args.nr); > + if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) { > + printf(_("bisect run success")); > + res = BISECT_OK; > + } else if (res == BISECT_ONLY_SKIPPED_LEFT) > + error(_("bisect run cannot continue any more")); > + else if (res) > + error(_("bisect run failed:'git bisect--helper --bisect-state" > + " %s' exited with error code %d"), args.v[0], res); > + else > + continue; In any case, being able to check the return value from bisect_state() and switch is so much nicer than having to sane_grep in BISECT_RUN. > + 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 { > @@ -1086,7 +1146,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > BISECT_LOG, > BISECT_REPLAY, > BISECT_SKIP, > - BISECT_VISUALIZE > + BISECT_VISUALIZE, > + BISECT_RUN, Now this new one has the trailing comma. I'd suggest doing so in the previous step. Thanks.
Miriam Rubio <mirucam@gmail.com> writes: > +static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) > +{ > ... > + while (1) { > + strvec_clear(&args); > + > + printf(_("running %s"), command.buf); > + res = run_command_v_opt(run_args.v, RUN_USING_SHELL); > + > + if (res < 0 && res >= 128) { Sorry for not noticing this total nonsense during my earlier review, but this condition will never trigger. I would have noticed it if the style consistently used "order quantities from left to right, as if on a number line" convention, i.e. if (res < 0 && 128 <= res) { Because there are only two valid range notation around a single variable, which are: (res < 0 || 128 <= res) - meaning 'res' is outside a span, or (0 <= res && res < 128) - meaning 'res' is inside a span. and the above rewriten form is neither, it would have stood out as an error immediately.
El jue, 8 abr 2021 a las 0:09, Junio C Hamano (<gitster@pobox.com>) escribió: > > 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> > > If I am reading the patch correctly, this removes the need for the > $GIT_DIR/BISECT_RUN file that used to be used to keep track of the > state? If that is true, it is worth noting in the proposed log > message. > > As far as I can see, nobody creates $GIT_DIR/BISECT_RUN anymore. > > $ git grep -e path_bisect_run -e BISECT_RUN > bisect.c:static GIT_PATH_FUNC(git_path_bisect_run, "BISECT_RUN") > bisect.c: unlink_or_warn(git_path_bisect_run()); > builtin/bisect--helper.c: BISECT_RUN, > builtin/bisect--helper.c: N_("use <cmd>... to automatical... > builtin/bisect--helper.c: case BISECT_RUN: > t/t6030-bisect-porcelain.sh: test_path_is_missing ".git/BISECT_RUN" && > > What if a run script tried to read from (or checked the presence of) > the file for its correct operation (e.g. I would imagine that "do > this operation when run interactively, but do the same operation > silently when run from the git-bisect machinery" may be a reasonable > thing to do)? > > This change just unintendedly broke such a script, didn't it? The > change makes me a bit worried. Hi, thank you for reviewing!. I don't know why the need for the $GIT_DIR/BISECT_RUN file was removed, so in the last patch series version I have just sent, I have added the creation of the file and it contains bisect_state() output as in original shell script version. Regards, Miriam > > > + if (bisect_next_check(terms, NULL)) > > + return BISECT_FAILED; > > + > > + if (argc) > > + sq_quote_argv(&command, argv); > > + else > > + return BISECT_FAILED; > > + > > + run_args.v[0] = xstrdup(command.buf); > > + run_args.nr = 1; > > + > > + while (1) { > > + strvec_clear(&args); > > + > > + printf(_("running %s"), command.buf); > > + res = run_command_v_opt(run_args.v, RUN_USING_SHELL); > > Nicely used sq_quote_argv() with RUN_USING_SHELL here. Goodl. > > > + if (res < 0 && res >= 128) { > > + error(_("bisect run failed: exit code %d from" > > + " '%s' is < 0 or >= 128"), res, command.buf); > > + strbuf_release(&command); > > + return res; > > + } > > + > > + if (res == 125) > > + strvec_push(&args, "skip"); > > + else if (res > 0) > > + strvec_push(&args, terms->term_bad); > > + else > > + strvec_push(&args, terms->term_good); > > + > > bisect_state() does so much that it was a bit hard to follow for me > (who hasn't been following the bisect-in-C topic very closely), but > the code around here roughly corresponds to the following snippet in > the original scripted version. > > > - 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 > > I see that the contents of the file BISECT_RUN is shown to the user > in the original but is that part of what bisect_state() does, or did > we lose it during this round of conversion? > > > + res = bisect_state(terms, args.v, args.nr); > > + if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) { > > + printf(_("bisect run success")); > > + res = BISECT_OK; > > + } else if (res == BISECT_ONLY_SKIPPED_LEFT) > > + error(_("bisect run cannot continue any more")); > > + else if (res) > > + error(_("bisect run failed:'git bisect--helper --bisect-state" > > + " %s' exited with error code %d"), args.v[0], res); > > + else > > + continue; > > In any case, being able to check the return value from bisect_state() > and switch is so much nicer than having to sane_grep in BISECT_RUN. > > > + 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 { > > @@ -1086,7 +1146,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > > BISECT_LOG, > > BISECT_REPLAY, > > BISECT_SKIP, > > - BISECT_VISUALIZE > > + BISECT_VISUALIZE, > > + BISECT_RUN, > > Now this new one has the trailing comma. I'd suggest doing so in > the previous step. > > Thanks.
diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c index b77c4f6b29..31c5f99660 100644 --- a/builtin/bisect--helper.c +++ b/builtin/bisect--helper.c @@ -31,6 +31,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 }; @@ -1073,6 +1074,65 @@ 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; + + if (bisect_next_check(terms, NULL)) + return BISECT_FAILED; + + if (argc) + sq_quote_argv(&command, argv); + else + return BISECT_FAILED; + + run_args.v[0] = xstrdup(command.buf); + run_args.nr = 1; + + while (1) { + strvec_clear(&args); + + printf(_("running %s"), command.buf); + res = run_command_v_opt(run_args.v, RUN_USING_SHELL); + + if (res < 0 && res >= 128) { + error(_("bisect run failed: exit code %d from" + " '%s' is < 0 or >= 128"), res, command.buf); + strbuf_release(&command); + return res; + } + + if (res == 125) + strvec_push(&args, "skip"); + else if (res > 0) + strvec_push(&args, terms->term_bad); + else + strvec_push(&args, terms->term_good); + + res = bisect_state(terms, args.v, args.nr); + + if (res == BISECT_INTERNAL_SUCCESS_MERGE_BASE) { + printf(_("bisect run success")); + res = BISECT_OK; + } else if (res == BISECT_ONLY_SKIPPED_LEFT) + error(_("bisect run cannot continue any more")); + 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 { @@ -1086,7 +1146,8 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) BISECT_LOG, BISECT_REPLAY, BISECT_SKIP, - BISECT_VISUALIZE + BISECT_VISUALIZE, + BISECT_RUN, } cmdmode = 0; int res = 0, nolog = 0; struct option options[] = { @@ -1110,6 +1171,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() @@ -1174,6 +1237,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;; *)