Subject: Re: [PATCH 09/11] Turn `git bisect` into a full built-in.

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Sat, 29 Jan 2022 08:09:28 +0100

On Fri, Jan 28 2022, Johannes Schindelin via GitGitGadget wrote:

> From: Johannes Schindelin <[email-masked]>
>
> 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
> helper implement the actual `git bisect` command instead.
>
> This retires `git-bisect.sh`, concluding a multi-year journey that many
> hands helped with, in particular Pranit Bauna, Tanushree Tumane and
> Miriam Rubio.
>
> Signed-off-by: Johannes Schindelin <[email-masked]>
> ---
>  Makefile                               |  3 +--
>  builtin.h                              |  2 +-
>  builtin/{bisect--helper.c => bisect.c} |  2 +-
>  git-bisect.sh                          | 37 --------------------------
>  git.c                                  |  2 +-
>  5 files changed, 4 insertions(+), 42 deletions(-)
>  rename builtin/{bisect--helper.c => bisect.c} (99%)
>  delete mode 100755 git-bisect.sh
>
> diff --git a/Makefile b/Makefile
> index 5580859afdb..ce2eabeca58 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -595,7 +595,6 @@ THIRD_PARTY_SOURCES =
>  # interactive shell sessions without exporting it.
>  unexport CDPATH
>  
> -SCRIPT_SH += git-bisect.sh
>  SCRIPT_SH += git-difftool--helper.sh
>  SCRIPT_SH += git-filter-branch.sh
>  SCRIPT_SH += git-merge-octopus.sh
> @@ -1064,7 +1063,7 @@ BUILTIN_OBJS += builtin/am.o
>  BUILTIN_OBJS += builtin/annotate.o
>  BUILTIN_OBJS += builtin/apply.o
>  BUILTIN_OBJS += builtin/archive.o
> -BUILTIN_OBJS += builtin/bisect--helper.o
> +BUILTIN_OBJS += builtin/bisect.o
>  BUILTIN_OBJS += builtin/blame.o
>  BUILTIN_OBJS += builtin/branch.o
>  BUILTIN_OBJS += builtin/bugreport.o
> diff --git a/builtin.h b/builtin.h
> index 8a58743ed63..b5567ea3a9d 100644
> --- a/builtin.h
> +++ b/builtin.h
> @@ -116,7 +116,7 @@ int cmd_am(int argc, const char **argv, const char *prefix);
>  int cmd_annotate(int argc, const char **argv, const char *prefix);
>  int cmd_apply(int argc, const char **argv, const char *prefix);
>  int cmd_archive(int argc, const char **argv, const char *prefix);
> -int cmd_bisect__helper(int argc, const char **argv, const char *prefix);
> +int cmd_bisect(int argc, const char **argv, const char *prefix);
>  int cmd_blame(int argc, const char **argv, const char *prefix);
>  int cmd_branch(int argc, const char **argv, const char *prefix);
>  int cmd_bugreport(int argc, const char **argv, const char *prefix);
> diff --git a/builtin/bisect--helper.c b/builtin/bisect.c
> similarity index 99%
> rename from builtin/bisect--helper.c
> rename to builtin/bisect.c
> index 009c919a989..189aca9dd22 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect.c
> @@ -1182,7 +1182,7 @@ 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)
> +int cmd_bisect(int argc, const char **argv, const char *prefix)
>  {
>  	int res = 0;
>  	struct option options[] = {
> diff --git a/git-bisect.sh b/git-bisect.sh
> deleted file mode 100755
> index 028d39cd9ce..00000000000
> --- a/git-bisect.sh
> +++ /dev/null
> @@ -1,37 +0,0 @@
> -#!/bin/sh
> -
> -USAGE='[help|start|bad|good|new|old|terms|skip|next|reset|visualize|view|replay|log|run]'
> -LONG_USAGE='git bisect help
> -	print this long help message.
> -git bisect start [--term-{new,bad}=<term> --term-{old,good}=<term>]
> -		 [--no-checkout] [--first-parent] [<bad> [<good>...]] [--] [<pathspec>...]
> -	reset bisect state and start bisection.
> -git bisect (bad|new) [<rev>]
> -	mark <rev> a known-bad revision/
> -		a revision after change in a given property.
> -git bisect (good|old) [<rev>...]
> -	mark <rev>... known-good revisions/
> -		revisions before change in a given property.
> -git bisect terms [--term-good | --term-bad]
> -	show the terms used for old and new commits (default: bad, good)
> -git bisect skip [(<rev>|<range>)...]
> -	mark <rev>... untestable revisions.
> -git bisect next
> -	find next bisection to test and check it out.
> -git bisect reset [<commit>]
> -	finish bisection search and go back to commit.
> -git bisect (visualize|view)
> -	show bisect status in gitk.
> -git bisect replay <logfile>
> -	replay bisection log.
> -git bisect log
> -	show bisect log.
> -git bisect run <cmd>...
> -	use <cmd>... to automatically bisect.
> -
> -Please use "git help bisect" to get the full man page.'
> -
> -OPTIONS_SPEC=
> -. git-sh-setup
> -
> -exec git bisect--helper "$@"
> diff --git a/git.c b/git.c
> index edda922ce6d..a8500132a28 100644
> --- a/git.c
> +++ b/git.c
> @@ -490,7 +490,7 @@ static struct cmd_struct commands[] = {
>  	{ "annotate", cmd_annotate, RUN_SETUP | NO_PARSEOPT },
>  	{ "apply", cmd_apply, RUN_SETUP_GENTLY },
>  	{ "archive", cmd_archive, RUN_SETUP_GENTLY },
> -	{ "bisect--helper", cmd_bisect__helper, RUN_SETUP },
> +	{ "bisect", cmd_bisect, RUN_SETUP },
>  	{ "blame", cmd_blame, RUN_SETUP },
>  	{ "branch", cmd_branch, RUN_SETUP | DELAY_PAGER_CONFIG },
>  	{ "bugreport", cmd_bugreport, RUN_SETUP_GENTLY },

This should really be squashed into 07/11, and I think shows an unstated
reason for why you probably ditched parse_options.

I.e. you've stuck the descriptions of the command into what we'd in
parse_options() put after a "" item. See builtin/blame.c for such a use.

So having this squashed would really make it clear how the "-h" output
is different after this series (which is OK), and what strings are
re-used or not etc.