Re: [PATCH v2 00/14] Finish converting git bisect into a built-in

>
> 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