| Message ID | pull.1132.v2.git.1645547423.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Finish converting git bisect into a built-in | expand |
On Tue, Feb 22, 2022 at 8:30 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote: > > After three GSoC/Outreachy students spent an incredible effort on this, it > is finally time to put a neat little bow on it. > > Changes since v1: > > * Added a regression test to "bisect run: fix the error message". > * Added a patch to address an error message that double-single-quoted the > command. > * Reworked the logic in "bisect--helper: make --bisect-state optional" to > delay showing the usage upon an unknown command, which should make the > code a lot less confusing. > * Split out the change that moved the BISECT_STATE case to the end of the > switch block. > * Added a patch that replaces the return error() calls in > cmd_bisect_helper() with die() calls, to avoid returning -1 as an exit > code. > * Dropped the use of parse_options() for the single purpose of handling -h; > This is now done explicitly. > * Simplified the diff of "bisect: move even the option parsing to > bisect--helper" by modifying argc and argv instead of modifying all the > function calls using those variables. > * In the "Turn git bisect into a full built-in" patch, changed the name of > the variable holding the usage to use the builtin_ prefix used in other > built-ins, too. > * Removed the trailing dot from the commit message of "Turn git bisect into > a full built-in". > > Johannes Schindelin (14): > bisect run: fix the error message > bisect: avoid double-quoting when printing the failed command > bisect--helper: retire the --no-log option > bisect--helper: really retire --bisect-next-check > bisect--helper: really retire `--bisect-autostart` > bisect--helper: using `--bisect-state` without an argument is a bug > bisect--helper: align the sub-command order with git-bisect.sh > bisect--helper: make `--bisect-state` optional > bisect--helper: move the `BISECT_STATE` case to the end > bisect--helper: return only correct exit codes in `cmd_*()` > bisect: move even the option parsing to `bisect--helper` > Turn `git bisect` into a full built-in > bisect: remove Cogito-related code > bisect: no longer try to clean up left-over `.git/head-name` files > > Makefile | 3 +- > bisect.c | 3 - > builtin.h | 2 +- > builtin/{bisect--helper.c => bisect.c} | 189 ++++++++++--------------- > git-bisect.sh | 84 ----------- > git.c | 2 +- > t/t6030-bisect-porcelain.sh | 11 +- > 7 files changed, 88 insertions(+), 206 deletions(-) > rename builtin/{bisect--helper.c => bisect.c} (88%) > delete mode 100755 git-bisect.sh > > > base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1132%2Fdscho%2Fbisect-in-c-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1132/dscho/bisect-in-c-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/1132 > > Range-diff vs v1: > > 1: 93d19d85ee3 ! 1: 81ca0d68cde bisect run: fix the error message > @@ Commit message > was "good" or "bad", but used a bogus (because non-populated) `args` > variable for it. > > + Helped-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > ## builtin/bisect--helper.c ## > @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons > strvec_clear(&run_args); > return res; > } > + > + ## t/t6030-bisect-porcelain.sh ## > +@@ t/t6030-bisect-porcelain.sh: test_expect_success 'bisect visualize with a filename with dash and space' ' > + git bisect visualize -p -- "-hello 2" > + ' > + > ++test_expect_success 'testing' ' > ++ git bisect reset && > ++ git bisect start $HASH4 $HASH1 && > ++ write_script test_script.sh <<-\EOF && > ++ rm .git/BISECT* > ++ EOF > ++ test_must_fail git bisect run ./test_script.sh 2>error && > ++ grep "git bisect good.*exited with error code" error > ++' > ++ > + test_done > -: ----------- > 2: 4320101f2e0 bisect: avoid double-quoting when printing the failed command > 2: 8e0e5559980 = 3: 88d7173c86b bisect--helper: retire the --no-log option > 3: 996a7099bf8 = 4: b914fe64dda bisect--helper: really retire --bisect-next-check > 4: 3de4c48b66d = 5: 0d3db63bda6 bisect--helper: really retire `--bisect-autostart` > 8: 1b14ed3d797 = 6: a345cf3e0e4 bisect--helper: using `--bisect-state` without an argument is a bug > 5: 6afc6e0eece = 7: 0487701220b bisect--helper: align the sub-command order with git-bisect.sh > 6: eddbdde222a ! 8: d8b2767c148 bisect--helper: make `--bisect-state` optional > @@ Commit message > `git bisect--helper bad`, i.e. do not require the `--bisect-state` > option to be passed explicitly. > > - To prepare for converting `bisect--helper` to a full built-in > - implementation of `git bisect` (which must not require the > - `--bisect-state` option to be specified at all), let's move the handling > - toward the end of the `switch (cmdmode)` block. > - > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > ## builtin/bisect--helper.c ## > @@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, co > git_bisect_helper_usage, > PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); > > -+ if (!cmdmode && argc > 0) { > -+ set_terms(&terms, "bad", "good"); > -+ get_terms(&terms); > -+ if (!check_and_set_terms(&terms, argv[0])) > -+ cmdmode = BISECT_STATE; > -+ } > -+ > - if (!cmdmode) > - usage_with_options(git_bisect_helper_usage, options); > - > -@@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > +- if (!cmdmode) > +- usage_with_options(git_bisect_helper_usage, options); > +- > +- switch (cmdmode) { > ++ switch (cmdmode ? cmdmode : BISECT_STATE) { > + case BISECT_START: > set_terms(&terms, "bad", "good"); > res = bisect_start(&terms, argv, argc); > - break; > -- case BISECT_STATE: > -- set_terms(&terms, "bad", "good"); > -- get_terms(&terms); > -- res = bisect_state(&terms, argv, argc); > -- break; > - case BISECT_TERMS: > - if (argc > 1) > - return error(_("--bisect-terms requires 0 or 1 argument")); > @@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > + case BISECT_STATE: > + set_terms(&terms, "bad", "good"); > get_terms(&terms); > - res = bisect_run(&terms, argv, argc); > - break; > -+ case BISECT_STATE: > -+ if (!terms.term_good) { > -+ set_terms(&terms, "bad", "good"); > -+ get_terms(&terms); > ++ if (!cmdmode && > ++ (!argc || check_and_set_terms(&terms, argv[0]))) { > ++ char *msg = xstrfmt(_("unknown command: '%s'"), argv[0]); > ++ usage_msg_opt(msg, git_bisect_helper_usage, options); > + } > -+ res = bisect_state(&terms, argv, argc); > -+ break; > - default: > - BUG("unknown subcommand %d", cmdmode); > - } > + res = bisect_state(&terms, argv, argc); > + break; > + case BISECT_TERMS: > + > + ## git-bisect.sh ## > +@@ git-bisect.sh: case "$#" in > + start) > + git bisect--helper --bisect-start "$@" ;; > + bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") > +- git bisect--helper --bisect-state "$cmd" "$@" ;; > ++ git bisect--helper "$cmd" "$@" ;; > + skip) > + git bisect--helper --bisect-skip "$@" || exit;; > + next) > -: ----------- > 9: e8904db81c5 bisect--helper: move the `BISECT_STATE` case to the end > -: ----------- > 10: 208f8fa4851 bisect--helper: return only correct exit codes in `cmd_*()` > 7: 515e86e2075 ! 11: dc04b06206b bisect: move even the option parsing to `bisect--helper` > @@ Commit message > together. So it would appear as if a lot of work would have to be done > just to be able to use `parse_options()` just to parse the sub-command, > instead of a simple `if...else if` chain, the latter being a > - dramatically simpler implementation. Therefore, we now keep the > - `parse_options()` call primarily to support `-h` and little else. > + dramatically simpler implementation. > > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > > @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons > - argc = parse_options(argc, argv, prefix, options, > - git_bisect_helper_usage, > - PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); > -+ /* Handle -h and invalid options */ > -+ parse_options(argc - 1, argv + 1, prefix, options, > -+ git_bisect_usage, > -+ PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN | > -+ PARSE_OPT_ONE_SHOT | PARSE_OPT_STOP_AT_NON_OPTION); > - > -- if (!cmdmode && argc > 0) { > -+ if (!strcmp("help", command)) > ++ if (!strcmp("-h", command) || !strcmp("help", command)) > + usage_with_options(git_bisect_usage, options); > -+ else if (!strcmp("start", command)) { > - set_terms(&terms, "bad", "good"); > -- get_terms(&terms); > -- if (!check_and_set_terms(&terms, argv[0])) > -- cmdmode = BISECT_STATE; > -- } > -- > -- if (!cmdmode) > -- usage_with_options(git_bisect_helper_usage, options); > -- > -- switch (cmdmode) { > + > +- switch (cmdmode ? cmdmode : BISECT_STATE) { > - case BISECT_START: > -- set_terms(&terms, "bad", "good"); > -- res = bisect_start(&terms, argv, argc); > ++ argc -= 2; > ++ argv += 2; > ++ > ++ if (!strcmp("start", command)) { > + set_terms(&terms, "bad", "good"); > + res = bisect_start(&terms, argv, argc); > - break; > - case BISECT_TERMS: > -- if (argc > 1) > -- return error(_("--bisect-terms requires 0 or 1 argument")); > -- res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL); > ++ } else if (!strcmp("terms", command)) { > + if (argc > 1) > +- die(_("--bisect-terms requires 0 or 1 argument")); > ++ die(_("'terms' requires 0 or 1 argument")); > + res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL); > - break; > - case BISECT_SKIP: > -+ res = bisect_start(&terms, argv + 2, argc - 2); > -+ } else if (!strcmp("terms", command)) { > -+ if (argc > 3) > -+ return error(_("'terms' requires 0 or 1 argument")); > -+ res = bisect_terms(&terms, argc == 3 ? argv[2] : NULL); > + } else if (!strcmp("skip", command)) { > set_terms(&terms, "bad", "good"); > get_terms(&terms); > -- res = bisect_skip(&terms, argv, argc); > + res = bisect_skip(&terms, argv, argc); > - break; > - case BISECT_NEXT: > -- if (argc) > -- return error(_("--bisect-next requires 0 arguments")); > -+ res = bisect_skip(&terms, argv + 2, argc - 2); > + } else if (!strcmp("next", command)) { > -+ if (argc != 2) > -+ return error(_("'next' requires 0 arguments")); > + if (argc) > +- die(_("--bisect-next requires 0 arguments")); > ++ die(_("'next' requires 0 arguments")); > get_terms(&terms); > res = bisect_next(&terms, prefix); > - break; > - case BISECT_RESET: > -- if (argc > 1) > -- return error(_("--bisect-reset requires either no argument or a commit")); > -- res = bisect_reset(argc ? argv[0] : NULL); > ++ } else if (!strcmp("reset", command)) { > + if (argc > 1) > +- die(_("--bisect-reset requires either no argument or a commit")); > ++ die(_("'reset' requires either no argument or a commit")); > + res = bisect_reset(argc ? argv[0] : NULL); > - break; > - case BISECT_VISUALIZE: > -+ } else if (!strcmp("reset", command)) { > -+ if (argc > 3) > -+ return error(_("'reset' requires either no argument or a commit")); > -+ res = bisect_reset(argc > 2 ? argv[2] : NULL); > + } else if (one_of(command, "visualize", "view", NULL)) { > get_terms(&terms); > -- res = bisect_visualize(&terms, argv, argc); > + res = bisect_visualize(&terms, argv, argc); > - break; > - case BISECT_REPLAY: > -- if (argc != 1) > -+ res = bisect_visualize(&terms, argv + 2, argc - 2); > + } else if (!strcmp("replay", command)) { > -+ if (argc != 3) > - return error(_("no logfile given")); > + if (argc != 1) > + die(_("no logfile given")); > set_terms(&terms, "bad", "good"); > -- res = bisect_replay(&terms, argv[0]); > + res = bisect_replay(&terms, argv[0]); > - break; > - case BISECT_LOG: > -- if (argc) > -- return error(_("--bisect-log requires 0 arguments")); > -+ res = bisect_replay(&terms, argv[2]); > + } else if (!strcmp("log", command)) { > -+ if (argc > 2) > -+ return error(_("'log' requires 0 arguments")); > + if (argc) > +- die(_("--bisect-log requires 0 arguments")); > ++ die(_("'log' requires 0 arguments")); > res = bisect_log(); > - break; > - case BISECT_RUN: > -- if (!argc) > + } else if (!strcmp("run", command)) { > -+ if (argc < 3) > - return error(_("bisect run failed: no command provided.")); > + if (!argc) > + die(_("bisect run failed: no command provided.")); > get_terms(&terms); > -- res = bisect_run(&terms, argv, argc); > + res = bisect_run(&terms, argv, argc); > - break; > - case BISECT_STATE: > -- if (!terms.term_good) { > -- set_terms(&terms, "bad", "good"); > -- get_terms(&terms); > -+ res = bisect_run(&terms, argv + 2, argc - 2); > + } else { > -+ set_terms(&terms, "bad", "good"); > -+ get_terms(&terms); > -+ if (!check_and_set_terms(&terms, command)) > -+ res = bisect_state(&terms, argv + 1, argc - 1); > -+ else { > + set_terms(&terms, "bad", "good"); > + get_terms(&terms); > +- if (!cmdmode && > +- (!argc || check_and_set_terms(&terms, argv[0]))) { > +- char *msg = xstrfmt(_("unknown command: '%s'"), argv[0]); > +- usage_msg_opt(msg, git_bisect_helper_usage, options); > ++ if (check_and_set_terms(&terms, command)) { > + char *msg = xstrfmt(_("unknown command: '%s'"), command); > + usage_msg_opt(msg, git_bisect_usage, options); > } > -- res = bisect_state(&terms, argv, argc); > ++ /* shift the `command` back in */ > ++ argc++; > ++ argv--; > + res = bisect_state(&terms, argv, argc); > - break; > - default: > - BUG("unknown subcommand %d", cmdmode); > @@ git-bisect.sh: Please use "git help bisect" to get the full man page.' > - start) > - git bisect--helper --bisect-start "$@" ;; > - bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") > -- git bisect--helper --bisect-state "$cmd" "$@" ;; > +- git bisect--helper "$cmd" "$@" ;; > - skip) > - git bisect--helper --bisect-skip "$@" || exit;; > - next) > 9: 1c0bd8a326f ! 12: 7db4b03b668 Turn `git bisect` into a full built-in. > @@ Metadata > Author: Johannes Schindelin <Johannes.Schindelin@gmx.de> > > ## Commit message ## > - Turn `git bisect` into a full built-in. > + Turn `git bisect` into a full built-in > > Now that the shell script hands off to the `bisect--helper` to do > _anything_ (except to show the help), it is but a tiny step to let the > @@ builtin.h: int cmd_am(int argc, const char **argv, const char *prefix); > int cmd_bugreport(int argc, const char **argv, const char *prefix); > > ## builtin/bisect--helper.c => builtin/bisect.c ## > +@@ builtin/bisect.c: 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_usage[] = { > ++static const char * const builtin_bisect_usage[] = { > + N_("git bisect help\n" > + "\tprint this long help message."), > + N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]\n" > @@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) > } > } > @@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, const char * > { > int res = 0; > struct option options[] = { > +@@ builtin/bisect.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > + const char *command = argc > 1 ? argv[1] : "help"; > + > + if (!strcmp("-h", command) || !strcmp("help", command)) > +- usage_with_options(git_bisect_usage, options); > ++ usage_with_options(builtin_bisect_usage, options); > + > + argc -= 2; > + argv += 2; > +@@ builtin/bisect.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > + get_terms(&terms); > + if (check_and_set_terms(&terms, command)) { > + char *msg = xstrfmt(_("unknown command: '%s'"), command); > +- usage_msg_opt(msg, git_bisect_usage, options); > ++ usage_msg_opt(msg, builtin_bisect_usage, options); > + } > + /* shift the `command` back in */ > + argc++; > > ## git-bisect.sh (deleted) ## > @@ > 10: cce533486db = 13: 0611d16f772 bisect: remove Cogito-related code > 11: dc77297c676 = 14: e2fa11a819e bisect: no longer try to clean up left-over `.git/head-name` files > > -- > gitgitgadget You've addressed all my feedback from v1, and I've looked over the changes and they look good to me: Reviewed-by: Elijah Newren <newren@gmail.com>
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes: > After three GSoC/Outreachy students spent an incredible effort on this, it > is finally time to put a neat little bow on it. Thanks for tying the loose ends. I've read the previous one and also this round, and except for the parse-options bit I am not sure about (read: I do not have a strong objection, but I do have to wonder if there is a better way to achieve the goal), everything else looks good. Will queue.
Hi Elijah, On Tue, 22 Feb 2022, Elijah Newren wrote: > You've addressed all my feedback from v1, and I've looked over the > changes and they look good to me: > > Reviewed-by: Elijah Newren <newren@gmail.com> Thank you for helping me improve the patch series! Dscho
Hi Junio, On Wed, 23 Feb 2022, Junio C Hamano wrote: > [...] the parse-options bit I am not sure about (read: I do not have a > strong objection, but I do have to wonder if there is a better way to > achieve the goal) [...] After spending a lot of time to try to make it work, I realized that it's in the name of `parse_options()` that it's not a `handle_subcommand()` API. So yes, I agree there is probably a better way (and e.g. `git bundle` and `scalar` would benefit from it, too): designing a proper API for handling subcommands. That's safely outside the scope of the patch series, of course. > Will queue. Thanks, Dscho
On Fri, Feb 25 2022, Johannes Schindelin wrote: > Hi Junio, > > On Wed, 23 Feb 2022, Junio C Hamano wrote: > >> [...] the parse-options bit I am not sure about (read: I do not have a >> strong objection, but I do have to wonder if there is a better way to >> achieve the goal) [...] > > After spending a lot of time to try to make it work, I realized that it's > in the name of `parse_options()` that it's not a `handle_subcommand()` > API. > > So yes, I agree there is probably a better way (and e.g. `git bundle` > and `scalar` would benefit from it, too): designing a proper API for > handling subcommands. > > That's safely outside the scope of the patch series, of course. I think SZEDER has some out-of-tree patches to do that, for what it's worth. But it's not emit_usage_info() either (at least not directly), and we also use it for that. So this seems like a distraction from the feedback I provided on your patch. Which for one is that due to bugs in the conversion things that emitted usage errors before are now silently accepted. And that whatever we do to make things *easier* for bundle/stash/commit-graph etc. when it comes to this, doesn't explain why you feel the need to introduce yet another pattern of doing this, when the existing "handle subcommands" built-ins do it rather uniformly, and AFAICT there's nothing in the "git bisect" case that's special enough that it wouldn't be served by pretty much copying that template.
After three GSoC/Outreachy students spent an incredible effort on this, it is finally time to put a neat little bow on it. Changes since v1: * Added a regression test to "bisect run: fix the error message". * Added a patch to address an error message that double-single-quoted the command. * Reworked the logic in "bisect--helper: make --bisect-state optional" to delay showing the usage upon an unknown command, which should make the code a lot less confusing. * Split out the change that moved the BISECT_STATE case to the end of the switch block. * Added a patch that replaces the return error() calls in cmd_bisect_helper() with die() calls, to avoid returning -1 as an exit code. * Dropped the use of parse_options() for the single purpose of handling -h; This is now done explicitly. * Simplified the diff of "bisect: move even the option parsing to bisect--helper" by modifying argc and argv instead of modifying all the function calls using those variables. * In the "Turn git bisect into a full built-in" patch, changed the name of the variable holding the usage to use the builtin_ prefix used in other built-ins, too. * Removed the trailing dot from the commit message of "Turn git bisect into a full built-in". Johannes Schindelin (14): bisect run: fix the error message bisect: avoid double-quoting when printing the failed command bisect--helper: retire the --no-log option bisect--helper: really retire --bisect-next-check bisect--helper: really retire `--bisect-autostart` bisect--helper: using `--bisect-state` without an argument is a bug bisect--helper: align the sub-command order with git-bisect.sh bisect--helper: make `--bisect-state` optional bisect--helper: move the `BISECT_STATE` case to the end bisect--helper: return only correct exit codes in `cmd_*()` bisect: move even the option parsing to `bisect--helper` Turn `git bisect` into a full built-in bisect: remove Cogito-related code bisect: no longer try to clean up left-over `.git/head-name` files Makefile | 3 +- bisect.c | 3 - builtin.h | 2 +- builtin/{bisect--helper.c => bisect.c} | 189 ++++++++++--------------- git-bisect.sh | 84 ----------- git.c | 2 +- t/t6030-bisect-porcelain.sh | 11 +- 7 files changed, 88 insertions(+), 206 deletions(-) rename builtin/{bisect--helper.c => bisect.c} (88%) delete mode 100755 git-bisect.sh base-commit: 89bece5c8c96f0b962cfc89e63f82d603fd60bed Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1132%2Fdscho%2Fbisect-in-c-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1132/dscho/bisect-in-c-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1132 Range-diff vs v1: 1: 93d19d85ee3 ! 1: 81ca0d68cde bisect run: fix the error message @@ Commit message was "good" or "bad", but used a bogus (because non-populated) `args` variable for it. + Helped-by: Elijah Newren <newren@gmail.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> ## builtin/bisect--helper.c ## @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons strvec_clear(&run_args); return res; } + + ## t/t6030-bisect-porcelain.sh ## +@@ t/t6030-bisect-porcelain.sh: test_expect_success 'bisect visualize with a filename with dash and space' ' + git bisect visualize -p -- "-hello 2" + ' + ++test_expect_success 'testing' ' ++ git bisect reset && ++ git bisect start $HASH4 $HASH1 && ++ write_script test_script.sh <<-\EOF && ++ rm .git/BISECT* ++ EOF ++ test_must_fail git bisect run ./test_script.sh 2>error && ++ grep "git bisect good.*exited with error code" error ++' ++ + test_done -: ----------- > 2: 4320101f2e0 bisect: avoid double-quoting when printing the failed command 2: 8e0e5559980 = 3: 88d7173c86b bisect--helper: retire the --no-log option 3: 996a7099bf8 = 4: b914fe64dda bisect--helper: really retire --bisect-next-check 4: 3de4c48b66d = 5: 0d3db63bda6 bisect--helper: really retire `--bisect-autostart` 8: 1b14ed3d797 = 6: a345cf3e0e4 bisect--helper: using `--bisect-state` without an argument is a bug 5: 6afc6e0eece = 7: 0487701220b bisect--helper: align the sub-command order with git-bisect.sh 6: eddbdde222a ! 8: d8b2767c148 bisect--helper: make `--bisect-state` optional @@ Commit message `git bisect--helper bad`, i.e. do not require the `--bisect-state` option to be passed explicitly. - To prepare for converting `bisect--helper` to a full built-in - implementation of `git bisect` (which must not require the - `--bisect-state` option to be specified at all), let's move the handling - toward the end of the `switch (cmdmode)` block. - Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> ## builtin/bisect--helper.c ## @@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, co git_bisect_helper_usage, PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); -+ if (!cmdmode && argc > 0) { -+ set_terms(&terms, "bad", "good"); -+ get_terms(&terms); -+ if (!check_and_set_terms(&terms, argv[0])) -+ cmdmode = BISECT_STATE; -+ } -+ - if (!cmdmode) - usage_with_options(git_bisect_helper_usage, options); - -@@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix) +- if (!cmdmode) +- usage_with_options(git_bisect_helper_usage, options); +- +- switch (cmdmode) { ++ switch (cmdmode ? cmdmode : BISECT_STATE) { + case BISECT_START: set_terms(&terms, "bad", "good"); res = bisect_start(&terms, argv, argc); - break; -- case BISECT_STATE: -- set_terms(&terms, "bad", "good"); -- get_terms(&terms); -- res = bisect_state(&terms, argv, argc); -- break; - case BISECT_TERMS: - if (argc > 1) - return error(_("--bisect-terms requires 0 or 1 argument")); @@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix) + case BISECT_STATE: + set_terms(&terms, "bad", "good"); get_terms(&terms); - res = bisect_run(&terms, argv, argc); - break; -+ case BISECT_STATE: -+ if (!terms.term_good) { -+ set_terms(&terms, "bad", "good"); -+ get_terms(&terms); ++ if (!cmdmode && ++ (!argc || check_and_set_terms(&terms, argv[0]))) { ++ char *msg = xstrfmt(_("unknown command: '%s'"), argv[0]); ++ usage_msg_opt(msg, git_bisect_helper_usage, options); + } -+ res = bisect_state(&terms, argv, argc); -+ break; - default: - BUG("unknown subcommand %d", cmdmode); - } + res = bisect_state(&terms, argv, argc); + break; + case BISECT_TERMS: + + ## git-bisect.sh ## +@@ git-bisect.sh: case "$#" in + start) + git bisect--helper --bisect-start "$@" ;; + bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") +- git bisect--helper --bisect-state "$cmd" "$@" ;; ++ git bisect--helper "$cmd" "$@" ;; + skip) + git bisect--helper --bisect-skip "$@" || exit;; + next) -: ----------- > 9: e8904db81c5 bisect--helper: move the `BISECT_STATE` case to the end -: ----------- > 10: 208f8fa4851 bisect--helper: return only correct exit codes in `cmd_*()` 7: 515e86e2075 ! 11: dc04b06206b bisect: move even the option parsing to `bisect--helper` @@ Commit message together. So it would appear as if a lot of work would have to be done just to be able to use `parse_options()` just to parse the sub-command, instead of a simple `if...else if` chain, the latter being a - dramatically simpler implementation. Therefore, we now keep the - `parse_options()` call primarily to support `-h` and little else. + dramatically simpler implementation. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons - argc = parse_options(argc, argv, prefix, options, - git_bisect_helper_usage, - PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN); -+ /* Handle -h and invalid options */ -+ parse_options(argc - 1, argv + 1, prefix, options, -+ git_bisect_usage, -+ PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN | -+ PARSE_OPT_ONE_SHOT | PARSE_OPT_STOP_AT_NON_OPTION); - -- if (!cmdmode && argc > 0) { -+ if (!strcmp("help", command)) ++ if (!strcmp("-h", command) || !strcmp("help", command)) + usage_with_options(git_bisect_usage, options); -+ else if (!strcmp("start", command)) { - set_terms(&terms, "bad", "good"); -- get_terms(&terms); -- if (!check_and_set_terms(&terms, argv[0])) -- cmdmode = BISECT_STATE; -- } -- -- if (!cmdmode) -- usage_with_options(git_bisect_helper_usage, options); -- -- switch (cmdmode) { + +- switch (cmdmode ? cmdmode : BISECT_STATE) { - case BISECT_START: -- set_terms(&terms, "bad", "good"); -- res = bisect_start(&terms, argv, argc); ++ argc -= 2; ++ argv += 2; ++ ++ if (!strcmp("start", command)) { + set_terms(&terms, "bad", "good"); + res = bisect_start(&terms, argv, argc); - break; - case BISECT_TERMS: -- if (argc > 1) -- return error(_("--bisect-terms requires 0 or 1 argument")); -- res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL); ++ } else if (!strcmp("terms", command)) { + if (argc > 1) +- die(_("--bisect-terms requires 0 or 1 argument")); ++ die(_("'terms' requires 0 or 1 argument")); + res = bisect_terms(&terms, argc == 1 ? argv[0] : NULL); - break; - case BISECT_SKIP: -+ res = bisect_start(&terms, argv + 2, argc - 2); -+ } else if (!strcmp("terms", command)) { -+ if (argc > 3) -+ return error(_("'terms' requires 0 or 1 argument")); -+ res = bisect_terms(&terms, argc == 3 ? argv[2] : NULL); + } else if (!strcmp("skip", command)) { set_terms(&terms, "bad", "good"); get_terms(&terms); -- res = bisect_skip(&terms, argv, argc); + res = bisect_skip(&terms, argv, argc); - break; - case BISECT_NEXT: -- if (argc) -- return error(_("--bisect-next requires 0 arguments")); -+ res = bisect_skip(&terms, argv + 2, argc - 2); + } else if (!strcmp("next", command)) { -+ if (argc != 2) -+ return error(_("'next' requires 0 arguments")); + if (argc) +- die(_("--bisect-next requires 0 arguments")); ++ die(_("'next' requires 0 arguments")); get_terms(&terms); res = bisect_next(&terms, prefix); - break; - case BISECT_RESET: -- if (argc > 1) -- return error(_("--bisect-reset requires either no argument or a commit")); -- res = bisect_reset(argc ? argv[0] : NULL); ++ } else if (!strcmp("reset", command)) { + if (argc > 1) +- die(_("--bisect-reset requires either no argument or a commit")); ++ die(_("'reset' requires either no argument or a commit")); + res = bisect_reset(argc ? argv[0] : NULL); - break; - case BISECT_VISUALIZE: -+ } else if (!strcmp("reset", command)) { -+ if (argc > 3) -+ return error(_("'reset' requires either no argument or a commit")); -+ res = bisect_reset(argc > 2 ? argv[2] : NULL); + } else if (one_of(command, "visualize", "view", NULL)) { get_terms(&terms); -- res = bisect_visualize(&terms, argv, argc); + res = bisect_visualize(&terms, argv, argc); - break; - case BISECT_REPLAY: -- if (argc != 1) -+ res = bisect_visualize(&terms, argv + 2, argc - 2); + } else if (!strcmp("replay", command)) { -+ if (argc != 3) - return error(_("no logfile given")); + if (argc != 1) + die(_("no logfile given")); set_terms(&terms, "bad", "good"); -- res = bisect_replay(&terms, argv[0]); + res = bisect_replay(&terms, argv[0]); - break; - case BISECT_LOG: -- if (argc) -- return error(_("--bisect-log requires 0 arguments")); -+ res = bisect_replay(&terms, argv[2]); + } else if (!strcmp("log", command)) { -+ if (argc > 2) -+ return error(_("'log' requires 0 arguments")); + if (argc) +- die(_("--bisect-log requires 0 arguments")); ++ die(_("'log' requires 0 arguments")); res = bisect_log(); - break; - case BISECT_RUN: -- if (!argc) + } else if (!strcmp("run", command)) { -+ if (argc < 3) - return error(_("bisect run failed: no command provided.")); + if (!argc) + die(_("bisect run failed: no command provided.")); get_terms(&terms); -- res = bisect_run(&terms, argv, argc); + res = bisect_run(&terms, argv, argc); - break; - case BISECT_STATE: -- if (!terms.term_good) { -- set_terms(&terms, "bad", "good"); -- get_terms(&terms); -+ res = bisect_run(&terms, argv + 2, argc - 2); + } else { -+ set_terms(&terms, "bad", "good"); -+ get_terms(&terms); -+ if (!check_and_set_terms(&terms, command)) -+ res = bisect_state(&terms, argv + 1, argc - 1); -+ else { + set_terms(&terms, "bad", "good"); + get_terms(&terms); +- if (!cmdmode && +- (!argc || check_and_set_terms(&terms, argv[0]))) { +- char *msg = xstrfmt(_("unknown command: '%s'"), argv[0]); +- usage_msg_opt(msg, git_bisect_helper_usage, options); ++ if (check_and_set_terms(&terms, command)) { + char *msg = xstrfmt(_("unknown command: '%s'"), command); + usage_msg_opt(msg, git_bisect_usage, options); } -- res = bisect_state(&terms, argv, argc); ++ /* shift the `command` back in */ ++ argc++; ++ argv--; + res = bisect_state(&terms, argv, argc); - break; - default: - BUG("unknown subcommand %d", cmdmode); @@ git-bisect.sh: Please use "git help bisect" to get the full man page.' - start) - git bisect--helper --bisect-start "$@" ;; - bad|good|new|old|"$TERM_BAD"|"$TERM_GOOD") -- git bisect--helper --bisect-state "$cmd" "$@" ;; +- git bisect--helper "$cmd" "$@" ;; - skip) - git bisect--helper --bisect-skip "$@" || exit;; - next) 9: 1c0bd8a326f ! 12: 7db4b03b668 Turn `git bisect` into a full built-in. @@ Metadata Author: Johannes Schindelin <Johannes.Schindelin@gmx.de> ## Commit message ## - Turn `git bisect` into a full built-in. + Turn `git bisect` into a full built-in Now that the shell script hands off to the `bisect--helper` to do _anything_ (except to show the help), it is but a tiny step to let the @@ builtin.h: int cmd_am(int argc, const char **argv, const char *prefix); int cmd_bugreport(int argc, const char **argv, const char *prefix); ## builtin/bisect--helper.c => builtin/bisect.c ## +@@ builtin/bisect.c: 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_usage[] = { ++static const char * const builtin_bisect_usage[] = { + N_("git bisect help\n" + "\tprint this long help message."), + N_("git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]\n" @@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, const char **argv, int argc) } } @@ builtin/bisect.c: static int bisect_run(struct bisect_terms *terms, const char * { int res = 0; struct option options[] = { +@@ builtin/bisect.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix) + const char *command = argc > 1 ? argv[1] : "help"; + + if (!strcmp("-h", command) || !strcmp("help", command)) +- usage_with_options(git_bisect_usage, options); ++ usage_with_options(builtin_bisect_usage, options); + + argc -= 2; + argv += 2; +@@ builtin/bisect.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix) + get_terms(&terms); + if (check_and_set_terms(&terms, command)) { + char *msg = xstrfmt(_("unknown command: '%s'"), command); +- usage_msg_opt(msg, git_bisect_usage, options); ++ usage_msg_opt(msg, builtin_bisect_usage, options); + } + /* shift the `command` back in */ + argc++; ## git-bisect.sh (deleted) ## @@ 10: cce533486db = 13: 0611d16f772 bisect: remove Cogito-related code 11: dc77297c676 = 14: e2fa11a819e bisect: no longer try to clean up left-over `.git/head-name` files