Re: [PATCH v3 1/5] completion: complete new old actions, start opts

From: Patrick Steinhardt <ps@pks.im>
Date: Jan 19, 2024, 07:04 UTC
Message-ID: <ZaofDZIwMkmII174@tanuki>
In-Reply-To: <20240118204323.1113859-2-britton.kerin@gmail.com>

On Thu, Jan 18, 2024 at 11:43:19AM -0900, Britton Leo Kerin wrote:

I feel like the commit message can be improved. First, you don't mention that you're adding completion for git-bisect(1), which one needs to infer from the patch. Second, the message is missing an explanation what is wrong about the current code and how you're fixing it.

Show 15 quoted lines

> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 185b47d802..15d22ff7d9 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1449,7 +1449,7 @@ _git_bisect ()
>  {
>  	__git_has_doubledash && return
>  
> -	local subcommands="start bad good skip reset visualize replay log run"
> +	local subcommands="start bad new good old terms skip reset visualize replay log run help"

I'd propose to move the addition of subcommands into one or more additional commits. "bad", "old" and "help" all already work perfectly fine, so these can easily go into a single commit. But I think that it would make sense to introduce "terms" in a standalone commit and then extend the below case statement to handle its argumens "--term-good" and "--term-bad".

Show 16 quoted lines

>  	local subcommand="$(__git_find_on_cmdline "$subcommands")"
>  	if [ -z "$subcommand" ]; then
>  		__git_find_repo_path
> @@ -1462,7 +1462,20 @@ _git_bisect ()
>  	fi
>  
>  	case "$subcommand" in
> -	bad|good|reset|skip|start)
> +	start)
> +		case "$cur" in
> +		--*)
> +			__gitcomp "--term-new --term-bad --term-old --term-good --first-parent --no-checkout"
> +			return
> +			;;
> +		*)
> +			;;

If you also called `__git_complete_refs here` then you could merge the two case statements for "$subcommand". It's a bit of duplicate code, but I think the end result would be easier to read.

Patrick

Show 12 quoted lines

> +		esac
> +		;;
> +	esac
> +
> +	case "$subcommand" in
> +	bad|new|good|old|reset|skip|start)
>  		__git_complete_refs
>  		;;
>  	*)
> -- 
> 2.43.0
>