Re: [PATCH 1/2] completion: add optional ignore-case when matching refs

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Sat, Nov 05, 2022 at 05:28:34PM +0000, Alison Winters via GitGitGadget wrote:
> From: Alison Winters <alisonatwork@outlook.com>
> 
> If GIT_COMPLETION_IGNORE_CASE=1 is set, --ignore-case will be added to
> git for-each-ref calls so that branches and tags can be matched case
> insensitively.
> 
> Signed-off-by: Alison Winters <alisonatwork@outlook.com>
> ---
>  contrib/completion/git-completion.bash | 41 ++++++++++++++++++++++++++
>  t/t9902-completion.sh                  | 16 ++++++++++
>  2 files changed, 57 insertions(+)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index ba5c395d2d8..8ed96a5b8b6 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -58,6 +58,11 @@
>  #
>  #     When set to "1" suggest all options, including options which are
>  #     typically hidden (e.g. '--allow-empty' for 'git commit').
> +#
> +#   GIT_COMPLETION_IGNORE_CASE
> +#
> +#     When set to "1", suggest refs that match case insensitively (e.g.,
> +#     completing "FOO" on "git checkout f<TAB>").

I wish the commit message and this comment would be more explicit
about only the matching being case insensitive, but the words listed
for completion will all have the same case as the files/refs/etc. that
are being completed.

I've already started writing a reply along the lines of "does this
only work on case-insensitive filesystems?!" before finally realizing
that's not how it works...

>  case "$COMP_WORDBREAKS" in
>  *:*) : great ;;
> @@ -644,8 +649,15 @@ __git_complete_index_file ()
>  __git_heads ()
>  {
>  	local pfx="${1-}" cur_="${2-}" sfx="${3-}"
> +	local ignore_case=""
> +
> +	if test "${GIT_COMPLETION_IGNORE_CASE-}" = "1"
> +	then
> +		ignore_case="--ignore-case"
> +	fi

I find these six lines a bit too verbose for what there are doing,
especially since the same six lines are added a couple of times.  I
think it could be shortened to just a single line with the "use
alternate value" parameter expansion like this:

    local ignore_case=${GIT_COMPLETION_IGNORE_CASE+--ignore-case}

>  	__git for-each-ref --format="${pfx//\%/%%}%(refname:strip=2)$sfx" \
> +			$ignore_case \

In fact, we could eliminate that new $ignore_case local variable
entirely by adding that parameter expansion right here.

>  			"refs/heads/$cur_*" "refs/heads/$cur_*/**"
>  }
>  
> @@ -657,8 +669,15 @@ __git_heads ()
>  __git_remote_heads ()
>  {
>  	local pfx="${1-}" cur_="${2-}" sfx="${3-}"
> +	local ignore_case=""
> +
> +	if test "${GIT_COMPLETION_IGNORE_CASE-}" = "1"
> +	then
> +		ignore_case="--ignore-case"
> +	fi
>  
>  	__git for-each-ref --format="${pfx//\%/%%}%(refname:strip=2)$sfx" \
> +			$ignore_case \
>  			"refs/remotes/$cur_*" "refs/remotes/$cur_*/**"
>  }
>  
> @@ -667,8 +686,15 @@ __git_remote_heads ()
>  __git_tags ()
>  {
>  	local pfx="${1-}" cur_="${2-}" sfx="${3-}"
> +	local ignore_case=""
> +
> +	if test "${GIT_COMPLETION_IGNORE_CASE-}" = "1"
> +	then
> +		ignore_case="--ignore-case"
> +	fi
>  
>  	__git for-each-ref --format="${pfx//\%/%%}%(refname:strip=2)$sfx" \
> +			$ignore_case \
>  			"refs/tags/$cur_*" "refs/tags/$cur_*/**"
>  }
>  
> @@ -682,12 +708,19 @@ __git_dwim_remote_heads ()
>  {
>  	local pfx="${1-}" cur_="${2-}" sfx="${3-}"
>  	local fer_pfx="${pfx//\%/%%}" # "escape" for-each-ref format specifiers
> +	local ignore_case=""
> +
> +	if test "${GIT_COMPLETION_IGNORE_CASE-}" = "1"
> +	then
> +		ignore_case="--ignore-case"
> +	fi
>  
>  	# employ the heuristic used by git checkout and git switch
>  	# Try to find a remote branch that cur_es the completion word
>  	# but only output if the branch name is unique
>  	__git for-each-ref --format="$fer_pfx%(refname:strip=3)$sfx" \
>  		--sort="refname:strip=3" \
> +		$ignore_case \
>  		"refs/remotes/*/$cur_*" "refs/remotes/*/$cur_*/**" | \
>  	uniq -u
>  }
> @@ -713,6 +746,7 @@ __git_refs ()
>  	local pfx="${3-}" cur_="${4-$cur}" sfx="${5-}"
>  	local match="${4-}"
>  	local fer_pfx="${pfx//\%/%%}" # "escape" for-each-ref format specifiers
> +	local ignore_case=""
>  
>  	__git_find_repo_path
>  	dir="$__git_repo_path"
> @@ -735,6 +769,11 @@ __git_refs ()
>  		fi
>  	fi
>  
> +	if test "${GIT_COMPLETION_IGNORE_CASE-}" = "1"
> +	then
> +		ignore_case="--ignore-case"
> +	fi
> +
>  	if [ "$list_refs_from" = path ]; then
>  		if [[ "$cur_" == ^* ]]; then
>  			pfx="$pfx^"
> @@ -765,6 +804,7 @@ __git_refs ()
>  			;;
>  		esac
>  		__git_dir="$dir" __git for-each-ref --format="$fer_pfx%($format)$sfx" \
> +			$ignore_case \
>  			"${refs[@]}"
>  		if [ -n "$track" ]; then
>  			__git_dwim_remote_heads "$pfx" "$match" "$sfx"
> @@ -787,6 +827,7 @@ __git_refs ()
>  			$match*)	echo "${pfx}HEAD$sfx" ;;
>  			esac
>  			__git for-each-ref --format="$fer_pfx%(refname:strip=3)$sfx" \
> +				$ignore_case \
>  				"refs/remotes/$remote/$match*" \
>  				"refs/remotes/$remote/$match*/**"
>  		else
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 43de868b800..f62a395d827 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2255,6 +2255,22 @@ test_expect_success 'checkout completes ref names' '
>  	EOF
>  '
>  
> +test_expect_success 'checkout does not match ref names of a different case' '
> +	test_completion "git checkout M" ""
> +'
> +
> +test_expect_success 'checkout matches case insensitively with GIT_COMPLETION_IGNORE_CASE' '
> +	(
> +		. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&

I don't think it's necessary to source the completion script here,
because the test script's main shell process has already sourced it,
and its functions are visible in this test's subshell as well.

(Yeah, there are a few test cases near the end that do (re-)source the
completion script, but they do so because they must invalidate the
cache variables used by the completion script; this case-insensitive
match feature, however, doesn't involve any of those cache variables.)

> +		GIT_COMPLETION_IGNORE_CASE=1 && export GIT_COMPLETION_IGNORE_CASE &&

There is no need to export this variable, because only a handful of
completion helper functions look at it, which are invoked in this same
shell process (or perhaps in one of its subshells, but this variable
will be visible there are all the same).

> +		test_completion "git checkout M" <<-\EOF
> +		main Z
> +		mybranch Z
> +		mytag Z
> +		EOF
> +	)
> +'
> +
>  test_expect_success 'git -C <path> checkout uses the right repo' '
>  	test_completion "git -C subdir -C subsubdir -C .. -C ../otherrepo checkout b" <<-\EOF
>  	branch-in-other Z
> -- 
> gitgitgadget
>