threads / patch / 61370

patchcompletion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM

Subject: [PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM

## tl;dr

3 messages between Apr 25, 2024 and Apr 26, 2024. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Thomas via GitGitGadget· Apr 25, 2024, 18:59 UTC · lore

From: Thomas Queiroz <thomasqueirozb@gmail.com>

Since GIT_PS1_SHOWUPSTREAM is a variable with space separated values and zsh for loops do no split by space by default, parsing of the options wasn't actually being done. The `-d' '` is a hacky solution that works in both bash and zsh. The correct way to do that in zsh would be do use read -rA and loop over the resulting array but -A isn't defined in bash.

Signed-off-by: Thomas Queiroz <thomasqueirozb@gmail.com>
---
    completion: Fix zsh parsing $GIT_PS1_SHOWUPSTREAM

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1710%2Fthomasqueirozb%2Fzsh-completion-fix-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1710/thomasqueirozb/zsh-completion-fix-v1
Pull-Request: https://github.com/git/git/pull/1710

 contrib/completion/git-prompt.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Show changes to contrib/completion/git-prompt.sh +2 −2

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 5330e769a72..9c25ec1e965 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -141,14 +141,14 @@ __git_ps1_show_upstream ()
 
 	# parse configuration values
 	local option
-	for option in ${GIT_PS1_SHOWUPSTREAM-}; do
+	while read -r -d' ' option; do
 		case "$option" in
 		git|svn) upstream_type="$option" ;;
 		verbose) verbose=1 ;;
 		legacy)  legacy=1  ;;
 		name)    name=1 ;;
 		esac
-	done
+	done <<< "${GIT_PS1_SHOWUPSTREAM-} "
 
 	# Find our upstream type
 	case "$upstream_type" in

base-commit: 21306a098c3f174ad4c2a5cddb9069ee27a548b0

-- 
gitgitgadget

brian m. carlson· Apr 26, 2024, 01:08 UTC · re: Thomas via GitGitGadget · lore

Re: [PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM

On 2024-04-25 at 18:59:51, Thomas via GitGitGadget wrote:

Show 7 quoted lines

> From: Thomas Queiroz <thomasqueirozb@gmail.com>
> 
> Since GIT_PS1_SHOWUPSTREAM is a variable with space separated values and
> zsh for loops do no split by space by default, parsing of the options
> wasn't actually being done. The `-d' '` is a hacky solution that works
> in both bash and zsh. The correct way to do that in zsh would be do use
> read -rA and loop over the resulting array but -A isn't defined in bash.

I wonder if it might actually be better to adjust the shell options when we call into __git_ps1. We could write this like so:

	[ -z "${ZSH_VERSION-}" ] || setopt localoptions shwordsplit

That will turn on shell word splitting for just that function (and the functions it calls), so the existing code will work fine and we won't tamper with the user's preferred shell options.

My concern is that changing the way we write the code here might result in someone unintentionally changing it back because it's less intuitive. By specifically asking zsh to use shell word splitting, we get consistent behaviour between bash and zsh, which is really what we want anyway.

I use the above syntax (minus the shell check) in my zsh prompt and can confirm it works as expected.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA

Junio C Hamano· Apr 26, 2024, 05:39 UTC · re: brian m. carlson · lore

Re: [PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

Show 8 quoted lines

> I wonder if it might actually be better to adjust the shell options when
> we call into __git_ps1.  We could write this like so:
>
> 	[ -z "${ZSH_VERSION-}" ] || setopt localoptions shwordsplit
>
> That will turn on shell word splitting for just that function (and the
> functions it calls), so the existing code will work fine and we won't
> tamper with the user's preferred shell options.

Nice.  I did

    $ git grep -e 'for [a-z0-9_]* in ' contrib/completion/

and wondered why other hits were OK. The completion one seems to have "emulate" all over the place to hide zsh-ness from functions it borrows from git-completion.bash, but git-prompt side seems to lack necessary "compatibility" stuff.

Show 5 quoted lines

> My concern is that changing the way we write the code here might result
> in someone unintentionally changing it back because it's less intuitive.
> By specifically asking zsh to use shell word splitting, we get
> consistent behaviour between bash and zsh, which is really what we want
> anyway.

Very well said.

> I use the above syntax (minus the shell check) in my zsh prompt and can
> confirm it works as expected.

Thanks.

By the way, I notice that the title of the patch talks about "completion", but this is about a prompt. It needs to be updated in a future iteration.