completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM

Commit Message

Thomas Queiroz April 25, 2024, 6:59 p.m. UTC

From: Thomas Queiroz <[e-mail]>

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 <[e-mail]>
---
    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(-)


base-commit: 21306a098c3f174ad4c2a5cddb9069ee27a548b0

Comments

brian m. carlson April 26, 2024, 1:08 a.m. UTC | #1

On 2024-04-25 at 18:59:51, Thomas via GitGitGadget wrote:
> From: Thomas Queiroz <[e-mail]>
> 
> 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.

Junio C Hamano April 26, 2024, 5:39 a.m. UTC | #2

"brian m. carlson" <[e-mail]> writes:

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

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

Patch

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