{"thread":{"id":"61370","subject":"[PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM","startedAt":"2024-04-25T18:59:55Z","lastAt":"2024-04-26T05:39:18Z","messageCount":3,"participants":["Thomas via GitGitGadget","brian m. carlson","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"493479","messageId":"pull.1710.git.git.1714071592035.gitgitgadget@gmail.com","threadId":"61370","inReplyTo":null,"subject":"[PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM","fromName":"Thomas via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-04-25T18:59:51Z","receivedAt":"2024-04-25T18:59:55Z","isPatch":true,"sender":{"key":"th.acker@arcor.de","avatar":"https://avatars.githubusercontent.com/u/1358536?v=4"},"body":"From: Thomas Queiroz <thomasqueirozb@gmail.com>\n\nSince GIT_PS1_SHOWUPSTREAM is a variable with space separated values and\nzsh for loops do no split by space by default, parsing of the options\nwasn't actually being done. The `-d' '` is a hacky solution that works\nin both bash and zsh. The correct way to do that in zsh would be do use\nread -rA and loop over the resulting array but -A isn't defined in bash.\n\nSigned-off-by: Thomas Queiroz <thomasqueirozb@gmail.com>\n---\n    completion: Fix zsh parsing $GIT_PS1_SHOWUPSTREAM\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1710%2Fthomasqueirozb%2Fzsh-completion-fix-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1710/thomasqueirozb/zsh-completion-fix-v1\nPull-Request: https://github.com/git/git/pull/1710\n\n contrib/completion/git-prompt.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 5330e769a72..9c25ec1e965 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -141,14 +141,14 @@ __git_ps1_show_upstream ()\n \n \t# parse configuration values\n \tlocal option\n-\tfor option in ${GIT_PS1_SHOWUPSTREAM-}; do\n+\twhile read -r -d' ' option; do\n \t\tcase \"$option\" in\n \t\tgit|svn) upstream_type=\"$option\" ;;\n \t\tverbose) verbose=1 ;;\n \t\tlegacy)  legacy=1  ;;\n \t\tname)    name=1 ;;\n \t\tesac\n-\tdone\n+\tdone <<< \"${GIT_PS1_SHOWUPSTREAM-} \"\n \n \t# Find our upstream type\n \tcase \"$upstream_type\" in\n\nbase-commit: 21306a098c3f174ad4c2a5cddb9069ee27a548b0\n-- \ngitgitgadget\n"},{"id":"493505","messageId":"Zir-eeK0CZxVLhcR@tapette.crustytoothpaste.net","threadId":"61370","inReplyTo":"pull.1710.git.git.1714071592035.gitgitgadget@gmail.com","subject":"Re: [PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-04-26T01:08:09Z","receivedAt":"2024-04-26T01:08:17Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-04-25 at 18:59:51, Thomas via GitGitGadget wrote:\n> From: Thomas Queiroz <thomasqueirozb@gmail.com>\n> \n> Since GIT_PS1_SHOWUPSTREAM is a variable with space separated values and\n> zsh for loops do no split by space by default, parsing of the options\n> wasn't actually being done. The `-d' '` is a hacky solution that works\n> in both bash and zsh. The correct way to do that in zsh would be do use\n> read -rA and loop over the resulting array but -A isn't defined in bash.\n\nI wonder if it might actually be better to adjust the shell options when\nwe call into __git_ps1.  We could write this like so:\n\n\t[ -z \"${ZSH_VERSION-}\" ] || setopt localoptions shwordsplit\n\nThat will turn on shell word splitting for just that function (and the\nfunctions it calls), so the existing code will work fine and we won't\ntamper with the user's preferred shell options.\n\nMy concern is that changing the way we write the code here might result\nin someone unintentionally changing it back because it's less intuitive.\nBy specifically asking zsh to use shell word splitting, we get\nconsistent behaviour between bash and zsh, which is really what we want\nanyway.\n\nI use the above syntax (minus the shell check) in my zsh prompt and can\nconfirm it works as expected.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"493507","messageId":"xmqqr0esbs3l.fsf@gitster.g","threadId":"61370","inReplyTo":"Zir-eeK0CZxVLhcR@tapette.crustytoothpaste.net","subject":"Re: [PATCH] completion: fix zsh parsing $GIT_PS1_SHOWUPSTREAM","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-04-26T05:39:10Z","receivedAt":"2024-04-26T05:39:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> I wonder if it might actually be better to adjust the shell options when\n> we call into __git_ps1.  We could write this like so:\n>\n> \t[ -z \"${ZSH_VERSION-}\" ] || setopt localoptions shwordsplit\n>\n> That will turn on shell word splitting for just that function (and the\n> functions it calls), so the existing code will work fine and we won't\n> tamper with the user's preferred shell options.\n\nNice.  I did\n\n    $ git grep -e 'for [a-z0-9_]* in ' contrib/completion/\n\nand wondered why other hits were OK.  The completion one seems to\nhave \"emulate\" all over the place to hide zsh-ness from functions it\nborrows from git-completion.bash, but git-prompt side seems to lack\nnecessary \"compatibility\" stuff.\n\n> My concern is that changing the way we write the code here might result\n> in someone unintentionally changing it back because it's less intuitive.\n> By specifically asking zsh to use shell word splitting, we get\n> consistent behaviour between bash and zsh, which is really what we want\n> anyway.\n\nVery well said.\n\n> I use the above syntax (minus the shell check) in my zsh prompt and can\n> confirm it works as expected.\n\nThanks.\n\nBy the way, I notice that the title of the patch talks about\n\"completion\", but this is about a prompt.  It needs to be updated in\na future iteration.\n\n"}]}