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@xxxxxxxxx>
Signed-off-by: Johannes Schindelin <johannes.schindelin@xxxxxx>
## 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@xxxxxx>
## 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@xxxxxx>
@@ 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@xxxxxx>
## 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
