Subject: Re: [PATCH 2/2] completion: add case-insensitive match of pseudorefs

From: SZEDER Gábor <[email-masked]>
Date: Sun, 20 Nov 2022 21:42:32 +0100

On Sat, Nov 05, 2022 at 05:28:35PM +0000, Alison Winters via GitGitGadget wrote:
> From: Alison Winters <[email-masked]>
> 
> When GIT_COMPLETION_IGNORE_CASE=1, also allow lowercase completion text
> like "head" to match HEAD and other pseudorefs.
> 
> Signed-off-by: Alison Winters <[email-masked]>
> ---
>  contrib/completion/git-completion.bash | 10 +++++++---
>  t/t9902-completion.sh                  | 16 ++++++++++++++++
>  2 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 8ed96a5b8b6..161327057da 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -745,6 +745,7 @@ __git_refs ()
>  	local format refs
>  	local pfx="${3-}" cur_="${4-$cur}" sfx="${5-}"
>  	local match="${4-}"
> +	local umatch="${4-}"

Why 'umatch' and not 'imatch'?

>  	local fer_pfx="${pfx//\%/%%}" # "escape" for-each-ref format specifiers
>  	local ignore_case=""
>  
> @@ -772,6 +773,8 @@ __git_refs ()
>  	if test "${GIT_COMPLETION_IGNORE_CASE-}" = "1"
>  	then
>  		ignore_case="--ignore-case"
> +		# use tr instead of ${match,^^} to preserve bash 3.2 compatibility

Thank you for keeping compatibility with old versions in mind!

> +		umatch=$(echo "$match" | tr a-z A-Z 2> /dev/null || echo "$match")

Style nit: we usually don't add a space between the redirection
operator and the filename.

>  	fi
>  
>  	if [ "$list_refs_from" = path ]; then
> @@ -780,6 +783,7 @@ __git_refs ()
>  			fer_pfx="$fer_pfx^"
>  			cur_=${cur_#^}
>  			match=${match#^}
> +			umatch=${umatch#^}
>  		fi
>  		case "$cur_" in
>  		refs|refs/*)
> @@ -790,7 +794,7 @@ __git_refs ()
>  		*)
>  			for i in HEAD FETCH_HEAD ORIG_HEAD MERGE_HEAD REBASE_HEAD CHERRY_PICK_HEAD; do
>  				case "$i" in
> -				$match*)
> +				$match*|$umatch*)

I find the two patterns here weird.  I suppose it works as intended,
because with GIT_COMPLETION_IGNORE_CASE being unset both $match and
$umatch have the same value, so there is only one pattern after all,
but when GIT_COMPLETION_IGNORE_CASE is set then $umatch contains the
case insensitive pattern...  but I still find it a bit weird :) but I
don't know how to make it less weird without introducing code
duplication, so it's still better than the alternatives.

>  					if [ -e "$dir/$i" ]; then
>  						echo "$pfx$i$sfx"
>  					fi
> @@ -824,7 +828,7 @@ __git_refs ()
>  	*)
>  		if [ "$list_refs_from" = remote ]; then
>  			case "HEAD" in
> -			$match*)	echo "${pfx}HEAD$sfx" ;;
> +			$match*|$umatch*)	echo "${pfx}HEAD$sfx" ;;
>  			esac
>  			__git for-each-ref --format="$fer_pfx%(refname:strip=3)$sfx" \
>  				$ignore_case \
> @@ -833,7 +837,7 @@ __git_refs ()
>  		else
>  			local query_symref
>  			case "HEAD" in
> -			$match*)	query_symref="HEAD" ;;
> +			$match*|$umatch*)	query_symref="HEAD" ;;
>  			esac
>  			__git ls-remote "$remote" $query_symref \
>  				"refs/tags/$match*" "refs/heads/$match*" \
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index f62a395d827..b4c46567fa4 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2271,6 +2271,22 @@ test_expect_success 'checkout matches case insensitively with GIT_COMPLETION_IGN
>  	)
>  '
>  
> +test_expect_success 'checkout completes pseudo refs' '
> +	test_completion "git checkout H" <<-\EOF
> +	HEAD Z
> +	EOF
> +'
> +
> +test_expect_success 'checkout completes pseudo refs case insensitively with GIT_COMPLETION_IGNORE_CASE' '
> +	(
> +		. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
> +		GIT_COMPLETION_IGNORE_CASE=1 && export GIT_COMPLETION_IGNORE_CASE &&

The same comment about the unnecessary sourcing and exporting on the
previous patch applies here as well.

> +		test_completion "git checkout h" <<-\EOF
> +		HEAD 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