[PATCH v6 00/16] Finish converting git bisect into a built-in

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

After three GSoC/Outreachy students spent an incredible effort on this, it
is finally time to put a neat little bow on it, or maybe more like a big
bow, maybe even a very large one, seeing as it takes quite a while to tie
(half a year at the time of writing)...

Changes since v5:

 * Fixed the commit message of "bisect run: fix the error message" that had
   become stale as of v3 due to 80c2e9657f2 "overtaking" it and doing half
   of its job.
 * Skipped translation usage strings in "bisect--helper: migrate to
   OPT_SUBCOMMAND()" that do not contain translatable parts.
 * Minor white-space clean-up in "bisect--helper: migrate to
   OPT_SUBCOMMAND()".

Changes since v4:

 * rebased onto sg/parse-options-subcommand
 * migrated to OPT_SUBCOMMAND().
 * As a consequence, this patch series is now unfortunately very large. And
   the range-diff is much less useful than I'd like because of the extensive
   changes that were de facto made a precondition to moving this patch
   series further. Junio, I would have liked to keep the scope (and burden
   for the reviewers) substantially smaller, maybe you can help with the
   review?

Changes since v3:

 * Rebased because of merge conflicts with ab/plug-leak-in-revisions.
 * Fixed the bug that git bisect --bisect-terms 1 2 wanted to auto-start a
   bisection if running with a git executable built at the in-between state
   at patch "bisect: move even the command-line parsing to bisect--helper".
   Since this bug was "fixed" in v3 by the very next patch, "bisect: teach
   the bisect--helper command to show the correct usage strings", v4 avoids
   introducing this bug simply by letting these two patches trade places.
   The range-diff admittedly looks quite awful because both patches overlap
   quite a bit in the lines they modify. The end result is the same, though,
   the diff between v3's and v4's builtin/bisect.c would be empty if I
   hadn't been forced to rebase.
 * Added a test case to ensure that this bug won't be introduced again. This
   test case is the only actual difference relative to v3 of this patch
   series.

Changes since v2:

 * We're now careful to provide identical usage strings upon git bisect -h
   and git bisect bogus.
 * When a bogus command is provided, we now error out instead of trying to
   start a git bisect run.
 * Rebased onto main to avoid plenty of merge conflicts with
   rs/bisect-executable-not-found, ac/usage-string-fixups and with
   cd/bisect-messages-from-pre-flight-states.

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 (16):
  bisect--helper: retire the --no-log option
  bisect--helper: really retire --bisect-next-check
  bisect--helper: really retire `--bisect-autostart`
  bisect--helper: simplify exit code computation
  bisect--helper: make `terms` an explicit singleton
  bisect--helper: make the order consistently `argc, argv`
  bisect--helper: migrate to OPT_SUBCOMMAND()
  bisect: verify that a bogus option won't try to start a bisection
  bisect run: fix the error message
  bisect: avoid double-quoting when printing the failed command
  bisect--helper: calling `bisect_state()` without an argument is a bug
  bisect--helper: make `state` optional
  bisect: move even the command-line 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} | 679 ++++++++++++++-----------
 git-bisect.sh                          |  84 ---
 git.c                                  |   2 +-
 t/t6030-bisect-porcelain.sh            |  21 +-
 7 files changed, 406 insertions(+), 388 deletions(-)
 rename builtin/{bisect--helper.c => bisect.c} (67%)
 delete mode 100755 git-bisect.sh


base-commit: 8f9d80f6c06369b563c76ec46c462e740a1a2cf0
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1132%2Fdscho%2Fbisect-in-c-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1132/dscho/bisect-in-c-v6
Pull-Request: https://github.com/gitgitgadget/git/pull/1132

Range-diff vs v5:

  1:  05262b6a7d1 =  1:  05262b6a7d1 bisect--helper: retire the --no-log option
  2:  1e43148864a =  2:  1e43148864a bisect--helper: really retire --bisect-next-check
  3:  1a1649d9d0d =  3:  1a1649d9d0d bisect--helper: really retire `--bisect-autostart`
  4:  9ab30552c6a =  4:  9ab30552c6a bisect--helper: simplify exit code computation
  5:  92b3b116ef8 =  5:  92b3b116ef8 bisect--helper: make `terms` an explicit singleton
  6:  c9dc0281e38 =  6:  c9dc0281e38 bisect--helper: make the order consistently `argc, argv`
  7:  5b7a3d58b4f !  7:  e97e187bbec bisect--helper: migrate to OPT_SUBCOMMAND()
     @@ Commit message
          `bisect--helper` learns about it in preparation for replacing the
          `git-bisect.sh` script altogether.
      
     +    As a consequence, the usage strings are copied over from the scripted
     +    version of the `git bisect` command. To avoid regressing on 959d670d1a4
     +    (i18n: remove from i18n strings that do not hold translatable parts,
     +    2022-01-31), we specifically do not enclose usage strings in `N_(...)`
     +    that do not contain any translatable parts.
     +
          Note that a couple of `bisect_*()` functions are not converted into
          `cmd_bisect_*()` functions directly, as they have callers other than the
          `OPT_SUBCOMMAND()` one (and the original functions did not expect
     @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA
      +	   "    [<pathspec>...]"),
      +	N_("git bisect (bad|new) [<rev>]"),
      +	N_("git bisect (good|old) [<rev>...]"),
     -+	N_("git bisect terms [--term-good | --term-bad]"),
     ++	"git bisect terms [--term-good | --term-bad]",
      +	N_("git bisect skip [(<rev>|<range>)...]"),
     -+	N_("git bisect next"),
     ++	"git bisect next",
      +	N_("git bisect reset [<commit>]"),
     -+	N_("git bisect (visualize|view)"),
     ++	"git bisect (visualize|view)",
      +	N_("git bisect replay <logfile>"),
     -+	N_("git bisect log"),
     ++	"git bisect log",
      +	N_("git bisect run <cmd>..."),
      +	NULL
      +};
     @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA
      +};
      +
      +static const char * const bisect_terms_usage[] = {
     -+	N_("git bisect terms [--term-good | --term-bad]"),
     ++	"git bisect terms [--term-good | --term-bad]",
      +	NULL
      +};
      +
     @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA
      +};
      +
      +static const char * const bisect_next_usage[] = {
     -+	N_("git bisect next"),
     ++	"git bisect next",
      +	NULL
      +};
      +
     @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA
      +};
      +
      +static const char * const bisect_visualize_usage[] = {
     -+	N_("git bisect visualize"),
     ++	"git bisect visualize",
      +	NULL
      +};
      +
     @@ builtin/bisect--helper.c: static GIT_PATH_FUNC(git_path_bisect_names, "BISECT_NA
      +};
      +
      +static const char * const bisect_log_usage[] = {
     -+	N_("git bisect log"),
     ++	"git bisect log",
      +	NULL
      +};
      +
     @@ builtin/bisect--helper.c: static int bisect_reset(const char *commit)
      +		OPT_END()
      +	};
      +	argc = parse_options(argc, argv, prefix, options,
     -+			     bisect_reset_usage,  PARSE_OPT_STOP_AT_NON_OPTION);
     ++			     bisect_reset_usage, PARSE_OPT_STOP_AT_NON_OPTION);
      +
      +	if (argc > 1)
      +		usage_msg_opt(_("requires either no argument or a commit"),
     @@ builtin/bisect--helper.c: finish:
      +	};
      +
      +	argc = parse_options(argc, argv, prefix, options,
     -+			     bisect_terms_usage,  PARSE_OPT_STOP_AT_NON_OPTION);
     ++			     bisect_terms_usage, PARSE_OPT_STOP_AT_NON_OPTION);
      +
      +	if (argc > 1)
      +		usage_msg_opt(_("terms: requires 0 or 1 argument"),
     @@ builtin/bisect--helper.c: static enum bisect_error bisect_state(int argc, const
      +	get_terms();
      +
      +	argc = parse_options(argc, argv, prefix, options,
     -+			     bisect_state_usage,  PARSE_OPT_STOP_AT_NON_OPTION);
     ++			     bisect_state_usage, PARSE_OPT_STOP_AT_NON_OPTION);
      +
      +	if (!argc)
      +		usage_msg_opt(_("need at least one argument"),
     @@ builtin/bisect--helper.c: static enum bisect_error bisect_state(int argc, const
       	const char* filename = git_path_bisect_log();
       
      +	argc = parse_options(argc, argv, prefix, options,
     -+			     bisect_log_usage,  PARSE_OPT_STOP_AT_NON_OPTION);
     ++			     bisect_log_usage, PARSE_OPT_STOP_AT_NON_OPTION);
      +
      +	if (argc > 0)
      +		usage_msg_opt(_("require 0 argument"), bisect_log_usage,
     @@ builtin/bisect--helper.c: static int process_replay_line(struct strbuf *line)
       
      +	set_terms("bad", "good");
      +	argc = parse_options(argc, argv, prefix, options,
     -+			     bisect_reset_usage,  PARSE_OPT_STOP_AT_NON_OPTION);
     ++			     bisect_reset_usage, PARSE_OPT_STOP_AT_NON_OPTION);
      +
      +	if (argc != 1)
      +		usage_msg_opt(_("no logfile given"), bisect_replay_usage,
     @@ builtin/bisect--helper.c: static enum bisect_error bisect_replay(const char *fil
      +	set_terms("bad", "good");
      +	get_terms();
      +	argc = parse_options(argc, argv, prefix, options,
     -+			     bisect_skip_usage,  PARSE_OPT_STOP_AT_NON_OPTION);
     ++			     bisect_skip_usage, PARSE_OPT_STOP_AT_NON_OPTION);
      +
       	strvec_push(&argv_state, "skip");
       
     @@ builtin/bisect--helper.c: static int bisect_run(int argc, const char **argv)
       
      +	get_terms();
      +	argc = parse_options(argc, argv, prefix, options,
     -+			     bisect_run_usage,  PARSE_OPT_STOP_AT_NON_OPTION);
     ++			     bisect_run_usage, PARSE_OPT_STOP_AT_NON_OPTION);
      +
       	if (bisect_next_check(NULL))
       		return BISECT_FAILED;
  8:  ba537af7066 =  8:  30c87f2e92e bisect: verify that a bogus option won't try to start a bisection
  9:  409492ad830 !  9:  4696652b99c bisect run: fix the error message
     @@ Commit message
      
          However, the error message was supposed to print out whether the state
          was "good" or "bad", but used a bogus (because non-populated) `args`
     -    variable for it.
     +    variable for it. This was fixed in 80c2e9657f2 (bisect--helper: report
     +    actual bisect_state() argument on error, 2022-01-18), but the error
     +    message still talks about `bisect--helper`, which is an implementation
     +    detail that should not concern end users.
     +
     +    Fix that, and add a regression test to ensure that the intended form of
     +    the error message.
      
          Helped-by: Elijah Newren <newren@gmail.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
 10:  bc5efc8fbfe = 10:  b202a0e386c bisect: avoid double-quoting when printing the failed command
 11:  8a0adfe3867 = 11:  3376b450867 bisect--helper: calling `bisect_state()` without an argument is a bug
 12:  189d2b3ba46 = 12:  e7623508f90 bisect--helper: make `state` optional
 13:  32bf74e3050 = 13:  3f052580c95 bisect: move even the command-line parsing to `bisect--helper`
 14:  a8f08f5e0cb = 14:  a83fe3dc3c2 Turn `git bisect` into a full built-in
 15:  a96489310d3 = 15:  f2132b61ff7 bisect: remove Cogito-related code
 16:  bfa7aa19f03 = 16:  4f93692e071 bisect: no longer try to clean up left-over `.git/head-name` files

-- 
gitgitgadget