* [PATCH] git-prompt.sh: make `option` a local variable
@ 2020-10-31 22:09 Sibo Dong via GitGitGadget
2020-10-31 22:45 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Sibo Dong via GitGitGadget @ 2020-10-31 22:09 UTC (permalink / raw)
To: git; +Cc: Sibo Dong, Sibo Dong
From: Sibo Dong <sibo.dong@outlook.com>
Other variables like `key` and `value` are local to this completion
script, so I think the same should be done with `option`.
Signed-off-by: Sibo Dong <sibo.dong@outlook.com>
---
git-prompt.sh: make option a local variable
This is very trivial, but variables like key and value are local to
git-prompt.sh, so I think the same should be done with option. I have
GIT_PS1_SHOW_UPSTREAM set to verbose in my ~/.bashrc, so option gets set
to verbose in my Bash session whenever I change directories to a local
repository with a remote.
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-775%2Fdongsibo%2Ffix-git-prompt-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-775/dongsibo/fix-git-prompt-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/775
contrib/completion/git-prompt.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 16260bab73..5116016d39 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -137,6 +137,7 @@ __git_ps1_show_upstream ()
done <<< "$output"
# parse configuration values
+ local option
for option in ${GIT_PS1_SHOWUPSTREAM}; do
case "$option" in
git|svn) upstream="$option" ;;
base-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6
--
gitgitgadget
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] git-prompt.sh: make `option` a local variable
2020-10-31 22:09 [PATCH] git-prompt.sh: make `option` a local variable Sibo Dong via GitGitGadget
@ 2020-10-31 22:45 ` Junio C Hamano
2020-11-01 8:04 ` Sibo Dong
0 siblings, 1 reply; 4+ messages in thread
From: Junio C Hamano @ 2020-10-31 22:45 UTC (permalink / raw)
To: Sibo Dong via GitGitGadget; +Cc: git, Sibo Dong
"Sibo Dong via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Sibo Dong <sibo.dong@outlook.com>
>
> Other variables like `key` and `value` are local to this completion
> script, so I think the same should be done with `option`.
>
> Signed-off-by: Sibo Dong <sibo.dong@outlook.com>
> ---
> git-prompt.sh: make option a local variable
>
> This is very trivial, but variables like key and value are local to
> git-prompt.sh, so I think the same should be done with option.
That explains why we might consider localizing option; another thing
to consider is if it is safe to just blindly localize it.
Here is how I would explain and justify the change, if I were
writing it:
git-prompt.sh: localize `option` in __git_ps1_show_upstream
The variable 'option' is used in __git_ps1_show_upstream()
without being localized.
This clobbers the variable the user may be using for other
purposes, which is bad. Luckily, $option is not used to carry
information around in the script as a global variable. The use
of it in this script has very limited scope (namely, only inside
this function), so just declare that it is "local".
following the usual pattern:
- start with the observation on the codebase before your change,
- explain what the problem is with the current state,
- outline the solution and explain why it is the right thing to do, and
- order the codebase to "be like so".
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 16260bab73..5116016d39 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -137,6 +137,7 @@ __git_ps1_show_upstream ()
> done <<< "$output"
>
> # parse configuration values
> + local option
> for option in ${GIT_PS1_SHOWUPSTREAM}; do
> case "$option" in
> git|svn) upstream="$option" ;;
Looks OK to me.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] git-prompt.sh: make `option` a local variable
2020-10-31 22:45 ` Junio C Hamano
@ 2020-11-01 8:04 ` Sibo Dong
2020-11-02 20:39 ` Junio C Hamano
0 siblings, 1 reply; 4+ messages in thread
From: Sibo Dong @ 2020-11-01 8:04 UTC (permalink / raw)
To: gitster@pobox.com
Cc: git@vger.kernel.org, gitgitgadget@gmail.com,
sibo.dong@outlook.com
Thank you for the feedback Junio; I'm a bit new to contributing. Since
you've justified this change much better than I have, is there any need
for further action on my part?
--
Sibo Dong <sibo.dong@mail.utoronto.ca>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] git-prompt.sh: make `option` a local variable
2020-11-01 8:04 ` Sibo Dong
@ 2020-11-02 20:39 ` Junio C Hamano
0 siblings, 0 replies; 4+ messages in thread
From: Junio C Hamano @ 2020-11-02 20:39 UTC (permalink / raw)
To: Sibo Dong
Cc: git@vger.kernel.org, gitgitgadget@gmail.com,
sibo.dong@outlook.com
Sibo Dong <sibo.dong@mail.utoronto.ca> writes:
> Thank you for the feedback Junio; I'm a bit new to contributing. Since
> you've justified this change much better than I have, is there any need
> for further action on my part?
The reviews by others are offered and taken as input to improve the
patch(es), but they are merely sugestions. It is up to the original
contributors to either disagree and not take them with explanation,
or agree with and take them to improve their patch(es) before
sending the next iteration.
So, at least you'd need to say if you agree with the suggested
change and to what degree. Since you've done so, I can save one
roundtrip by rewriting the proposed commit log message myself while
queuing your patch ;-)
Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-11-02 20:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-31 22:09 [PATCH] git-prompt.sh: make `option` a local variable Sibo Dong via GitGitGadget
2020-10-31 22:45 ` Junio C Hamano
2020-11-01 8:04 ` Sibo Dong
2020-11-02 20:39 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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).