* [PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM
@ 2024-04-25 18:59 Thomas via GitGitGadget
2024-04-26 1:08 ` brian m. carlson
0 siblings, 1 reply; 3+ messages in thread
From: Thomas via GitGitGadget @ 2024-04-25 18:59 UTC (permalink / raw)
To: git; +Cc: Thomas, Thomas Queiroz
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(-)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM
2024-04-25 18:59 [PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM Thomas via GitGitGadget
@ 2024-04-26 1:08 ` brian m. carlson
2024-04-26 5:39 ` Junio C Hamano
0 siblings, 1 reply; 3+ messages in thread
From: brian m. carlson @ 2024-04-26 1:08 UTC (permalink / raw)
To: Thomas via GitGitGadget; +Cc: git, Thomas
[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]
On 2024-04-25 at 18:59:51, Thomas via GitGitGadget wrote:
> 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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM
2024-04-26 1:08 ` brian m. carlson
@ 2024-04-26 5:39 ` Junio C Hamano
0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2024-04-26 5:39 UTC (permalink / raw)
To: brian m. carlson; +Cc: Thomas via GitGitGadget, git, Thomas
"brian m. carlson" <sandals@crustytoothpaste.net> 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.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-26 5:39 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-25 18:59 [PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM Thomas via GitGitGadget
2024-04-26 1:08 ` brian m. carlson
2024-04-26 5:39 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).