Re: [PATCH v3 2/5] completion: git-log opts to bisect visualize

[Patrick Steinhardt <ps@pks.im>]

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

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

Proposal for the commit subject: "completion: complete log opts for
git-bisect visualize".

> To do this the majority of _git_log has been factored out into the new
> __git_complete_log_opts.

We typically do not continue the commit message as if the commit subject
was the first line of the message. An introduction like the following
would help to set the stage:

    Arguments passed to the "visualize" subcommand of git-bisect(1) get
    forwarded to git-log(1). It thus supports the same options as
    git-log(1) would, but our Bash completion script does not know to
    handle this.

> This is needed because the visualize command
> accepts git-log options but not rev arguments (they are fixed to the
> commits under bisection).
> 
> __git_complete_log_opts has a precondition that COMPREPLY be empty.  In
> a completion context it doesn't seem advisable to implement
> preconditions as noisy or hard failures, so instead it becomes a no-op
> on violation.  This should be detectable and quick to debug for devels,
> without ever aggravating a user (besides completion failure).
> 
> Signed-off-by: Britton Leo Kerin <britton.kerin@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 30 +++++++++++++++++++++++---
>  1 file changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 15d22ff7d9..c16aded36c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -1472,6 +1472,16 @@ _git_bisect ()
>  			;;
>  		esac
>  		;;
> +	visualize)
> +		case "$cur" in
> +		-*)
> +			__git_complete_log_opts
> +			return
> +			;;
> +		*)
> +			;;
> +		esac
> +		;;

Is this switch even needed? Can't we call `__git_complete_log_opts`
directly?

>  	esac
>  
>  	case "$subcommand" in
> @@ -2074,10 +2084,14 @@ __git_diff_merges_opts="off none on first-parent 1 separate m combined c dense-c
>  __git_log_pretty_formats="oneline short medium full fuller reference email raw format: tformat: mboxrd"
>  __git_log_date_formats="relative iso8601 iso8601-strict rfc2822 short local default human raw unix auto: format:"
>  
> -_git_log ()
> +
> +# Check for only porcelain (i.e. not git-rev-list) option (not argument)
> +# and selected option argument completions for git-log options and if any
> +# are found put them in COMPREPLY.  COMPREPLY must be empty at the start,
> +# and will be empty on return if no candidates are found.

Why do we need to enforce that COMPREPLY is empty? None of the other
`__git_complete_*` helpers do this, so I think it's fair to expect that
the variable woulld get clobbered when calling the new function. Thus, I
don't think there's a need for this precondition.

> +__git_complete_log_opts ()
>  {
> -	__git_has_doubledash && return
> -	__git_find_repo_path
> +	[ -z "$COMPREPLY" ] || return 1   # Precondition
>  
>  	local merge=""
>  	if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
> @@ -2171,6 +2185,16 @@ _git_log ()
>  		return
>  		;;
>  	esac
> +}
> +
> +_git_log ()
> +{
> +	__git_has_doubledash && return
> +	__git_find_repo_path

I was about to say that it would make more sense to call
`__git_find_repo_path` in `__git_complete_log_opts` so that all prereqs
are fulfilled whenever the latter is called. But `__git_complete_relist`
doesn't know to find the repo path in all cases, so that wouldn't quite
work alright.

Patrick

> +        __git_complete_log_opts
> +        [ -z "$COMPREPLY" ] || return
> +
>  	__git_complete_revlist
>  }
>  
> -- 
> 2.43.0
> 

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