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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Nov 20, 2022, 20:42 UTC
Message-ID: <20221120204232.GC4039@szeder.dev>
In-Reply-To: <c455e855395dcc9215ea4ce3181eb3cbaff4000b.1667669315.git.gitgitgadget@gmail.com>

On Sat, Nov 05, 2022 at 05:28:35PM +0000, Alison Winters via GitGitGadget wrote:

Show 20 quoted lines

> From: Alison Winters <alisonatwork@outlook.com>
> 
> When GIT_COMPLETION_IGNORE_CASE=1, also allow lowercase completion text
> like "head" to match HEAD and other pseudorefs.
> 
> Signed-off-by: Alison Winters <alisonatwork@outlook.com>
> ---
>  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'?

Show 8 quoted lines

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

Show 17 quoted lines

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

Show 39 quoted lines

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

Show 11 quoted lines

> +		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