{"thread":{"id":"54545","subject":"[PATCH] git-prompt.sh: make `option` a local variable","startedAt":"2020-10-31T22:09:51Z","lastAt":"2020-11-02T20:40:42Z","messageCount":4,"participants":["Sibo Dong via GitGitGadget","Junio C Hamano","Sibo Dong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"408802","messageId":"pull.775.git.1604182187195.gitgitgadget@gmail.com","threadId":"54545","inReplyTo":null,"subject":"[PATCH] git-prompt.sh: make `option` a local variable","fromName":"Sibo Dong via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-10-31T22:09:46Z","receivedAt":"2020-10-31T22:09:51Z","isPatch":true,"sender":{"key":"sibo.dong@mail.utoronto.ca","avatar":null},"body":"From: Sibo Dong \n\nOther variables like `key` and `value` are local to this completion\nscript, so I think the same should be done with `option`.\n\nSigned-off-by: Sibo Dong \n---\n git-prompt.sh: make option a local variable\n \n This is very trivial, but variables like key and value are local to \n git-prompt.sh, so I think the same should be done with option. I have \n GIT_PS1_SHOW_UPSTREAM set to verbose in my ~/.bashrc, so option gets set\n to verbose in my Bash session whenever I change directories to a local\n repository with a remote.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-775%2Fdongsibo%2Ffix-git-prompt-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-775/dongsibo/fix-git-prompt-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/775\n\n contrib/completion/git-prompt.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 16260bab73..5116016d39 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -137,6 +137,7 @@ __git_ps1_show_upstream ()\n \tdone <<< \"$output\"\n \n \t# parse configuration values\n+\tlocal option\n \tfor option in ${GIT_PS1_SHOWUPSTREAM}; do\n \t\tcase \"$option\" in\n \t\tgit|svn) upstream=\"$option\" ;;\n\nbase-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6\n-- \ngitgitgadget\n"},{"id":"408804","messageId":"xmqqmu02xbe6.fsf@gitster.c.googlers.com","threadId":"54545","inReplyTo":"pull.775.git.1604182187195.gitgitgadget@gmail.com","subject":"Re: [PATCH] git-prompt.sh: make `option` a local variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-10-31T22:45:37Z","receivedAt":"2020-10-31T22:48:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Sibo Dong via GitGitGadget\" writes:\n\n> From: Sibo Dong \n>\n> Other variables like `key` and `value` are local to this completion\n> script, so I think the same should be done with `option`.\n>\n> Signed-off-by: Sibo Dong \n> ---\n> git-prompt.sh: make option a local variable\n> \n> This is very trivial, but variables like key and value are local to \n> git-prompt.sh, so I think the same should be done with option.\n\nThat explains why we might consider localizing option; another thing\nto consider is if it is safe to just blindly localize it.\n\nHere is how I would explain and justify the change, if I were\nwriting it:\n\n git-prompt.sh: localize `option` in __git_ps1_show_upstream\n\n The variable 'option' is used in __git_ps1_show_upstream()\n without being localized.\n\n This clobbers the variable the user may be using for other\n purposes, which is bad. Luckily, $option is not used to carry\n information around in the script as a global variable. The use\n of it in this script has very limited scope (namely, only inside\n this function), so just declare that it is \"local\".\n\n\nfollowing the usual pattern:\n\n - start with the observation on the codebase before your change,\n - explain what the problem is with the current state,\n - outline the solution and explain why it is the right thing to do, and\n - order the codebase to \"be like so\".\n\n\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index 16260bab73..5116016d39 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -137,6 +137,7 @@ __git_ps1_show_upstream ()\n> \tdone <<< \"$output\"\n> \n> \t# parse configuration values\n> +\tlocal option\n> \tfor option in ${GIT_PS1_SHOWUPSTREAM}; do\n> \t\tcase \"$option\" in\n> \t\tgit|svn) upstream=\"$option\" ;;\n\nLooks OK to me.\n"},{"id":"408807","messageId":"1d19cdf35c12b1079cee0861469dc1a652642c82.camel@mail.utoronto.ca","threadId":"54545","inReplyTo":"xmqqmu02xbe6.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] git-prompt.sh: make `option` a local variable","fromName":"Sibo Dong","fromEmail":"sibo.dong@mail.utoronto.ca","sentAt":"2020-11-01T08:04:04Z","receivedAt":"2020-11-01T08:15:10Z","isPatch":true,"sender":{"key":"sibo.dong@mail.utoronto.ca","avatar":null},"body":"Thank you for the feedback Junio; I'm a bit new to contributing. Since\nyou've justified this change much better than I have, is there any need\nfor further action on my part?\n-- \nSibo Dong \n\n"},{"id":"408904","messageId":"xmqqlffjtrvm.fsf@gitster.c.googlers.com","threadId":"54545","inReplyTo":"1d19cdf35c12b1079cee0861469dc1a652642c82.camel@mail.utoronto.ca","subject":"Re: [PATCH] git-prompt.sh: make `option` a local variable","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-02T20:39:57Z","receivedAt":"2020-11-02T20:40:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sibo Dong writes:\n\n> Thank you for the feedback Junio; I'm a bit new to contributing. Since\n> you've justified this change much better than I have, is there any need\n> for further action on my part?\n\nThe reviews by others are offered and taken as input to improve the\npatch(es), but they are merely sugestions. It is up to the original\ncontributors to either disagree and not take them with explanation,\nor agree with and take them to improve their patch(es) before\nsending the next iteration.\n\nSo, at least you'd need to say if you agree with the suggested\nchange and to what degree. Since you've done so, I can save one\nroundtrip by rewriting the proposed commit log message myself while\nqueuing your patch ;-)\n\nThanks.\n\n"}]}