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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 2198 bytes --]

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.

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

>  	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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]