mbox series

[v3,00/15] Finish converting git bisect into a built-in

Message ID pull.1132.v3.git.1653144546.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Finish converting git bisect into a built-in | expand

Message

Victoria Dye via GitGitGadget May 21, 2022, 2:48 p.m. UTC
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 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 (15):
  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 command-line parsing to `bisect--helper`
  bisect: teach the `bisect--helper` command to show the correct usage
    strings
  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} | 202 +++++++++++--------------
 git-bisect.sh                          |  84 ----------
 git.c                                  |   2 +-
 t/t6030-bisect-porcelain.sh            |  11 +-
 7 files changed, 100 insertions(+), 207 deletions(-)
 rename builtin/{bisect--helper.c => bisect.c} (89%)
 delete mode 100755 git-bisect.sh


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

Range-diff vs v2:

  1:  81ca0d68cde !  1:  cf6034625dd bisect run: fix the error message
     @@ Commit message
          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, 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;
     - 	const char *new_state;
     - 	int temporary_stdout_fd, saved_stdout;
     -@@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
     - 	strvec_push(&run_args, command.buf);
     - 
     - 	while (1) {
     --		strvec_clear(&args);
     --
     - 		printf(_("running %s\n"), command.buf);
     - 		res = run_command_v_opt(run_args.v, RUN_USING_SHELL);
     - 
      @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
       			printf(_("bisect found first bad commit"));
       			res = BISECT_OK;
       		} else if (res) {
      -			error(_("bisect run failed: 'git bisect--helper --bisect-state"
     --			" %s' exited with error code %d"), args.v[0], res);
      +			error(_("bisect run failed: 'git bisect"
     -+			" %s' exited with error code %d"), new_state, res);
     + 			" %s' exited with error code %d"), new_state, res);
       		} else {
       			continue;
     - 		}
     - 
     - 		strbuf_release(&command);
     --		strvec_clear(&args);
     - 		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"
     +@@ t/t6030-bisect-porcelain.sh: test_expect_success 'bisect state output with bad commit' '
     + 	grep -F "waiting for good commit(s), bad commit known" output
       '
       
     -+test_expect_success 'testing' '
     ++test_expect_success 'verify correct error message' '
      +	git bisect reset &&
      +	git bisect start $HASH4 $HASH1 &&
      +	write_script test_script.sh <<-\EOF &&
  2:  4320101f2e0 !  2:  955ccd4d8c8 bisect: avoid double-quoting when printing the failed command
     @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons
       			error(_("bisect run failed: exit code %d from"
      -				" '%s' is < 0 or >= 128"), res, command.buf);
      +				" %s is < 0 or >= 128"), res, command.buf);
     - 			strbuf_release(&command);
     - 			return res;
     + 			break;
       		}
     + 
  3:  88d7173c86b !  3:  abcbc25966c bisect--helper: retire the --no-log option
     @@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, co
      @@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
       			 N_("visualize the bisection"), BISECT_VISUALIZE),
       		OPT_CMDMODE(0, "bisect-run", &cmdmode,
     - 			 N_("use <cmd>... to automatically bisect."), BISECT_RUN),
     + 			 N_("use <cmd>... to automatically bisect"), BISECT_RUN),
      -		OPT_BOOL(0, "no-log", &nolog,
      -			 N_("no log for BISECT_WRITE")),
       		OPT_END()
  4:  b914fe64dda =  4:  af60ef1b5a4 bisect--helper: really retire --bisect-next-check
  5:  0d3db63bda6 =  5:  07a92c58f8e bisect--helper: really retire `--bisect-autostart`
  6:  a345cf3e0e4 =  6:  04ba0950b85 bisect--helper: using `--bisect-state` without an argument is a bug
  7:  0487701220b !  7:  6847af9d485 bisect--helper: align the sub-command order with git-bisect.sh
     @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons
      +		OPT_CMDMODE(0, "bisect-log", &cmdmode,
      +			 N_("list the bisection steps so far"), BISECT_LOG),
       		OPT_CMDMODE(0, "bisect-run", &cmdmode,
     - 			 N_("use <cmd>... to automatically bisect."), BISECT_RUN),
     + 			 N_("use <cmd>... to automatically bisect"), BISECT_RUN),
       		OPT_END()
      @@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
       		usage_with_options(git_bisect_helper_usage, options);
  8:  d8b2767c148 !  8:  b7bc53b9cb6 bisect--helper: make `--bisect-state` optional
     @@ builtin/bisect--helper.c
      @@ builtin/bisect--helper.c: static const char * const git_bisect_helper_usage[] = {
       	N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]"
       					    " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
     - 	N_("git bisect--helper --bisect-next"),
     + 	"git bisect--helper --bisect-next",
      -	N_("git bisect--helper --bisect-state (bad|new) [<rev>]"),
      -	N_("git bisect--helper --bisect-state (good|old) [<rev>...]"),
      +	N_("git bisect--helper [--bisect-state] (bad|new) [<rev>]"),
      +	N_("git bisect--helper [--bisect-state] (good|old) [<rev>...]"),
       	N_("git bisect--helper --bisect-replay <filename>"),
       	N_("git bisect--helper --bisect-skip [(<rev>|<range>)...]"),
     - 	N_("git bisect--helper --bisect-visualize"),
     + 	"git bisect--helper --bisect-visualize",
      @@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
       			     git_bisect_helper_usage,
       			     PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN);
  9:  e8904db81c5 =  9:  1919237a819 bisect--helper: move the `BISECT_STATE` case to the end
 10:  208f8fa4851 = 10:  1236a731903 bisect--helper: return only correct exit codes in `cmd_*()`
 11:  dc04b06206b ! 11:  4ae78d37d04 bisect: move even the option parsing to `bisect--helper`
     @@ Metadata
      Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
      
       ## Commit message ##
     -    bisect: move even the option parsing to `bisect--helper`
     +    bisect: move even the command-line parsing to `bisect--helper`
      
          On our journey to a fully built-in `git bisect`, this is the
     -    second-to-last step.
     +    one of the last steps.
      
     -    Side note: The `if (!strcmp(...)) ... else if (!strcmp(...)) ... else if
     -    (!strcmp(...)) ...` chain seen in this patch was not actually the first
     -    idea how to convert the command modes to sub-commands. Since the
     -    `bisect--helper` command already used the `parse-opions` API with neatly
     -    set-up command modes, naturally the idea was to use `PARSE_OPT_NODASH`
     -    to support proper sub-commands instead. However, the `parse-options` API
     -    is not set up for that, and even after making that option work with long
     -    options, it turned out that `STOP_AT_NON_OPTION` and `KEEP_UNKNOWN`
     -    would have to be used but these options were not designed to work
     -    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.
     +    Side note: The `parse-options` API is not at all set up to parse
     +    subcommands such as `git bisect start`, `git bisect reset`, etc.
     +    Instead of fighting an up-hill battle trying to "fix" that, we simply
     +    roll the same type of manual subcommand parsing as we already do e.g.
     +    in `builtin/bundle.c`.
      
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## builtin/bisect--helper.c ##
     -@@ builtin/bisect--helper.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_helper_usage[] = {
     --	N_("git bisect--helper --bisect-reset [<commit>]"),
     --	N_("git bisect--helper --bisect-terms [--term-good | --term-old | --term-bad | --term-new]"),
     --	N_("git bisect--helper --bisect-start [--term-{new,bad}=<term> --term-{old,good}=<term>]"
     --					    " [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<paths>...]"),
     --	N_("git bisect--helper --bisect-next"),
     --	N_("git bisect--helper [--bisect-state] (bad|new) [<rev>]"),
     --	N_("git bisect--helper [--bisect-state] (good|old) [<rev>...]"),
     --	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>..."),
     -+static const char * const git_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"
     -+	   "\t\t [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...]\n"
     -+	   "\treset bisect state and start bisection."),
     -+	N_("git bisect (bad|new) [<rev>]\n"
     -+	   "\tmark <rev> a known-bad revision/\n"
     -+	   "\t\ta revision after change in a given property."),
     -+	N_("git bisect (good|old) [<rev>...]\n"
     -+	   "\tmark <rev>... known-good revisions/\n"
     -+	   "\t\trevisions before change in a given property."),
     -+	N_("git bisect terms [--term-good | --term-bad]\n"
     -+	   "\tshow the terms used for old and new commits (default: bad, good)"),
     -+	N_("git bisect skip [(<rev>|<range>)...]\n"
     -+	   "\tmark <rev>... untestable revisions."),
     -+	N_("git bisect next\n"
     -+	   "\tfind next bisection to test and check it out."),
     -+	N_("git bisect reset [<commit>]\n"
     -+	   "\tfinish bisection search and go back to commit."),
     -+	N_("git bisect (visualize|view)\n"
     -+	   "\tshow bisect status in gitk."),
     -+	N_("git bisect replay <logfile>\n"
     -+	   "\treplay bisection log."),
     -+	N_("git bisect log\n"
     -+	   "\tshow bisect log."),
     -+	N_("git bisect run <cmd>...\n"
     -+	   "\tuse <cmd>... to automatically bisect."),
     - 	NULL
     - };
     - 
      @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, const char **argv, int argc)
       
       int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
     @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons
      -		OPT_CMDMODE(0, "bisect-log", &cmdmode,
      -			 N_("list the bisection steps so far"), BISECT_LOG),
      -		OPT_CMDMODE(0, "bisect-run", &cmdmode,
     --			 N_("use <cmd>... to automatically bisect."), BISECT_RUN),
     +-			 N_("use <cmd>... to automatically bisect"), BISECT_RUN),
       		OPT_END()
       	};
       	struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL };
     @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons
      -			     git_bisect_helper_usage,
      -			     PARSE_OPT_KEEP_DASHDASH | PARSE_OPT_KEEP_UNKNOWN);
      +	if (!strcmp("-h", command) || !strcmp("help", command))
     -+		usage_with_options(git_bisect_usage, options);
     ++		usage_with_options(git_bisect_helper_usage, options);
       
      -	switch (cmdmode ? cmdmode : BISECT_STATE) {
      -	case BISECT_START:
     @@ builtin/bisect--helper.c: static int bisect_run(struct bisect_terms *terms, cons
      -		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);
     + 			usage_msg_opt(msg, git_bisect_helper_usage, options);
       		}
      +		/* shift the `command` back in */
      +		argc++;
  -:  ----------- > 12:  ac472aefb6a bisect: teach the `bisect--helper` command to show the correct usage strings
 12:  7db4b03b668 ! 13:  85f5c256ae3 Turn `git bisect` into a full built-in
     @@ 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)
     - 	}
     + 	return res;
       }
       
      -int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
      +int cmd_bisect(int argc, const char **argv, const char *prefix)
       {
       	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++;
     + 	struct bisect_terms terms = { .term_good = NULL, .term_bad = NULL };
      
       ## git-bisect.sh (deleted) ##
      @@
 13:  0611d16f772 = 14:  289917e96af bisect: remove Cogito-related code
 14:  e2fa11a819e = 15:  8f8d2ba0fe4 bisect: no longer try to clean up left-over `.git/head-name` files

Comments

Bagas Sanjaya May 22, 2022, 3:07 a.m. UTC | #1
On 5/21/22 21:48, Johannes Schindelin via GitGitGadget wrote:
>  * When a bogus command is provided, we now error out instead of trying to
>    start a git bisect run.

Ah! git bisect now behave like other shell commands where bogus
command is given, instead of "accidentally" start the bisection.

BTW, do the current version of git bisect exhibit that accidental
behavior?
Ævar Arnfjörð Bjarmason May 23, 2022, 10:22 a.m. UTC | #2
On Sun, May 22 2022, Bagas Sanjaya wrote:

> On 5/21/22 21:48, Johannes Schindelin via GitGitGadget wrote:
>>  * When a bogus command is provided, we now error out instead of trying to
>>    start a git bisect run.
>
> Ah! git bisect now behave like other shell commands where bogus
> command is given, instead of "accidentally" start the bisection.
>
> BTW, do the current version of git bisect exhibit that accidental
> behavior?

No, this is a fix for a bug introduced in an earlier version of this
topic, but the more general issue remains. See
https://lore.kernel.org/git/220521.86zgjazuy4.gmgdl@evledraar.gmail.com/

Rather than a surgical fix for a specific issue I noted, with the
initial report being:
https://lore.kernel.org/git/220223.86v8x56g7g.gmgdl@evledraar.gmail.com/

This topic could really use some confidence building by adding new "git
bisect" tests. I.e. let's have tests for what exit codes etc. we emit on
various valid and invalid "git bisect" usage.

We clearly don't have that since the proposed v2 would have introduced a
regression, and this v3 would also have done that in other cases (while
fixing the one specific case of "-h").