threads / patch / 54545

patchgit-prompt.sh: make `option` a local variable

Subject: [PATCH] git-prompt.sh: make `option` a local variable

## tl;dr

4 messages between Oct 31, 2020 and Nov 2, 2020. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Sibo Dong via GitGitGadget· Oct 31, 2020, 22:09 UTC · lore
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(+)
Show changes to contrib/completion/git-prompt.sh +1 −0
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
Junio C Hamano· Oct 31, 2020, 22:45 UTC · re: Sibo Dong via GitGitGadget · lore

Re: [PATCH] git-prompt.sh: make `option` a local variable

"Sibo Dong via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 11 quoted lines
> 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".
Show 12 quoted lines
> 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.
Sibo Dong· Nov 1, 2020, 08:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-prompt.sh: make `option` a local variable

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>
Junio C Hamano· Nov 2, 2020, 20:39 UTC · re: Sibo Dong · lore

Re: [PATCH] git-prompt.sh: make `option` a local variable

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.

← back to recent threads