threads / patch / 32926

patchshell-prompt: clean up nested if-then

Subject: [PATCH] shell-prompt: clean up nested if-then

## tl;dr

8 messages between Feb 18, 2013 and Feb 19, 2013. Diffs are folded; open one to read it.

replies: 7people: 4as markdown or json

Martin Erik Werner· Feb 18, 2013, 16:23 UTC · lore
Minor clean up of if-then nesting in checks for environment variables
and config options. No functional changes.
---
 contrib/completion/git-prompt.sh |   27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)
Show changes to contrib/completion/git-prompt.sh +13 −14
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9b2eec2..e29694d 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -320,26 +320,25 @@ __git_ps1 ()
 				b="GIT_DIR!"
 			fi
 		elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
-			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
-				if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
-					git diff --no-ext-diff --quiet --exit-code || w="*"
-					if git rev-parse --quiet --verify HEAD >/dev/null; then
-						git diff-index --cached --quiet HEAD -- || i="+"
-					else
-						i="#"
-					fi
+			if test -n "${GIT_PS1_SHOWDIRTYSTATE-}" &&
+			   test "$(git config --bool bash.showDirtyState)" != "false"
+			then
+				git diff --no-ext-diff --quiet --exit-code || w="*"
+				if git rev-parse --quiet --verify HEAD >/dev/null; then
+					git diff-index --cached --quiet HEAD -- || i="+"
+				else
+					i="#"
 				fi
 			fi
 			if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
 				git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
 			fi
 
-			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
-				if [ "$(git config --bool bash.showUntrackedFiles)" != "false" ]; then
-					if [ -n "$(git ls-files --others --exclude-standard)" ]; then
-						u="%"
-					fi
-				fi
+			if test -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" &&
+			   test "$(git config --bool bash.showUntrackedFiles)" != "false" &&
+			   test -n "$(git ls-files --others --exclude-standard)"
+			then
+				u="%"
 			fi
 
 			if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
-- 
1.7.10.4
Jonathan Nieder· Feb 18, 2013, 19:10 UTC · re: Martin Erik Werner · lore

Re: [PATCH] shell-prompt: clean up nested if-then

Hi Martin,
Martin Erik Werner wrote:
> Minor clean up of if-then nesting in checks for environment variables
> and config options. No functional changes.

Yeah, the nesting was getting a little deep. Thanks for the cleanup. May we have your sign-off?

Once this is signed off,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Patch left unsnipped for reference.
Show 49 quoted lines
> ---
>  contrib/completion/git-prompt.sh |   27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
> 
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 9b2eec2..e29694d 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -320,26 +320,25 @@ __git_ps1 ()
>  				b="GIT_DIR!"
>  			fi
>  		elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
> -			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
> -				if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
> -					git diff --no-ext-diff --quiet --exit-code || w="*"
> -					if git rev-parse --quiet --verify HEAD >/dev/null; then
> -						git diff-index --cached --quiet HEAD -- || i="+"
> -					else
> -						i="#"
> -					fi
> +			if test -n "${GIT_PS1_SHOWDIRTYSTATE-}" &&
> +			   test "$(git config --bool bash.showDirtyState)" != "false"
> +			then
> +				git diff --no-ext-diff --quiet --exit-code || w="*"
> +				if git rev-parse --quiet --verify HEAD >/dev/null; then
> +					git diff-index --cached --quiet HEAD -- || i="+"
> +				else
> +					i="#"
>  				fi
>  			fi
>  			if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
>  				git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
>  			fi
>  
> -			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
> -				if [ "$(git config --bool bash.showUntrackedFiles)" != "false" ]; then
> -					if [ -n "$(git ls-files --others --exclude-standard)" ]; then
> -						u="%"
> -					fi
> -				fi
> +			if test -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" &&
> +			   test "$(git config --bool bash.showUntrackedFiles)" != "false" &&
> +			   test -n "$(git ls-files --others --exclude-standard)"
> +			then
> +				u="%"
>  			fi
>  
>  			if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
> -- 
Martin Erik Werner· Feb 18, 2013, 22:56 UTC · lore

Re: [PATCH] shell-prompt: clean up nested if-then

On Mon, 2013-02-18 at 21:31 +0100, Simon vanaf Telefoon wrote:
Show 12 quoted lines
> Hi all, sorry for top posting :-( blame the phone and k9
> 
> I have a small issue with the use of test instead of [
> If that only applies to this section of the entire file. 
> Coding style has some value.
> 
> Combining nested ifs with && seems harmless enough, though should be
> well tested.
> 
> Cheers
> Simon 
> 

Ah, indeed, I looked around a bit more, and as per http://mywiki.wooledge.org/BashPitfalls it seems like 'test' is bad to use with multiple &&'s anyways.

I've changed to using [] && [] and rerolled the patch.
Show 13 quoted lines
> Jonathan Nieder <jrnieder@gmail.com> wrote:
>         Hi Martin,
>         
>         Martin Erik Werner wrote:
>         
>                 Minor clean up of if-then nesting in checks for environment variables
>                 and config options. No functional changes.
>         
>         Yeah, the nesting was getting a little deep.  Thanks for the cleanup.
>         May we have your sign-off?
>         
>         Once this is signed off,
>         Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Meh, I keep on missing that :/
Old (and new) patch is:
Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
Show 56 quoted lines
>         
>         Patch left unsnipped for reference.
>         
>                 ---
>                 contrib/completion/git-prompt.sh |   27 +++++++++++++--------------
>                 1 file changed, 13 insertions(+), 14 deletions(-)
>                 
>                 diff --git
>                 a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
>                 index 9b2eec2..e29694d 100644
>                 --- a/contrib/completion/git-prompt.sh
>                 +++ b/contrib/completion/git-prompt.sh
>                 @@ -320,26 +320,25 @@ __git_ps1 ()
>                     b="GIT_DIR!"
>                    fi
>                   elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
>                 -   if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
>                 -    if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
>                 -     git diff --no-ext-diff --quiet --exit-code || w="*"
>                 -     if git rev-parse --quiet --verify HEAD >/dev/null; then
>                 -      git diff-index --cached --quiet HEAD -- || i="+"
>                 -     else
>                 -      i="#"
>                 -     fi
>                 +   if test -n "${GIT_PS1_SHOWDIRTYSTATE-}" &&
>                 +      test "$(git config --bool bash.showDirtyState)" !=
>                 "false"
>                 +   then
>                 +    git diff --no-ext-diff --quiet --exit-code || w="*"
>                 +    if git rev-parse --quiet --verify HEAD >/dev/null; then
>                 +     git diff-index --cached --quiet HEAD -- || i="+"
>                 +    else
>                 +     i="#"
>                     fi
>                    fi
>                    if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
>                     git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
>                    fi
>                 
>                 -   if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
>                 -    if [ "$(git config --bool bash.showUntrackedFiles)" != "false" ]; then
>                 -     if [ -n "$(git ls-files --others --exclude-standard)" ]; then
>                 -      u="%"
>                 -     fi
>                 -    fi
>                 +   if test -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" &&
>                 +      test "$(git config --bool bash.showUntrackedFiles)" != "false" &&
>                 +      test -n "$(git ls-files --others --exclude-standard)"
>                 +   then
>                 +    u="%"
>                    fi
>                 
>                    if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ];!
>                   then
>                 -- 
> 
martinerikwerner@gmail.com· Feb 18, 2013, 22:59 UTC · re: Martin Erik Werner · lore

[PATCH v2] shell-prompt: clean up nested if-then

From: Martin Erik Werner <martinerikwerner@gmail.com>

Minor clean up of if-then nesting in checks for environment variables and config options. No functional changes.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 contrib/completion/git-prompt.sh |   27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)
Show changes to contrib/completion/git-prompt.sh +13 −14
diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9b2eec2..341422a 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -320,26 +320,25 @@ __git_ps1 ()
 				b="GIT_DIR!"
 			fi
 		elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
-			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
-				if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
-					git diff --no-ext-diff --quiet --exit-code || w="*"
-					if git rev-parse --quiet --verify HEAD >/dev/null; then
-						git diff-index --cached --quiet HEAD -- || i="+"
-					else
-						i="#"
-					fi
+			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ] &&
+			   [ "$(git config --bool bash.showDirtyState)" != "false" ]
+			then
+				git diff --no-ext-diff --quiet --exit-code || w="*"
+				if git rev-parse --quiet --verify HEAD >/dev/null; then
+					git diff-index --cached --quiet HEAD -- || i="+"
+				else
+					i="#"
 				fi
 			fi
 			if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
 				git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
 			fi
 
-			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
-				if [ "$(git config --bool bash.showUntrackedFiles)" != "false" ]; then
-					if [ -n "$(git ls-files --others --exclude-standard)" ]; then
-						u="%"
-					fi
-				fi
+			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ] &&
+			   [ "$(git config --bool bash.showUntrackedFiles)" != "false" ] &&
+			   [ -n "$(git ls-files --others --exclude-standard)" ]
+			then
+				u="%"
 			fi
 
 			if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
-- 
1.7.10.4
Jonathan Nieder· Feb 18, 2013, 23:28 UTC · re: martinerikwerner@gmail.com · lore

Re: [PATCH v2] shell-prompt: clean up nested if-then

Martin Erik Werner wrote:
Show 7 quoted lines
> Minor clean up of if-then nesting in checks for environment variables
> and config options. No functional changes.
>
> Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
> ---
>  contrib/completion/git-prompt.sh |   27 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 14 deletions(-)
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for the quick turnaround.
Junio C Hamano· Feb 18, 2013, 23:07 UTC · re: Martin Erik Werner · lore

Re: [PATCH] shell-prompt: clean up nested if-then

Martin Erik Werner <martinerikwerner@gmail.com> writes:
Show 16 quoted lines
> On Mon, 2013-02-18 at 21:31 +0100, Simon vanaf Telefoon wrote:
>> Hi all, sorry for top posting :-( blame the phone and k9
>> 
>> I have a small issue with the use of test instead of [
>> If that only applies to this section of the entire file. 
>> Coding style has some value.
>> 
>> Combining nested ifs with && seems harmless enough, though should be
>> well tested.
>> 
>> Cheers
>> Simon 
>> 
>
> Ah, indeed, I looked around a bit more, and as per
> http://mywiki.wooledge.org/BashPitfalls it seems like 'test' is bad to use with multiple &&'s anyways.

I think you are misreading a suggestion that is somewhat misguided (yes "[ <condition> && <another> ]" does not make sense, but that is not applicable to "test <conditon> && test <another>"); ignore it.

It is fine to write "test <condition> && test <another>" and that works portably to even pre-posix systems.

But the existing code the patch touches favors [] over test consistently; that alone is a good reason to stick with [] in _this_ script, even though it is against Git's overall shell script style.

Simon Oosthoek· Feb 19, 2013, 08:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] shell-prompt: clean up nested if-then

On 19/02/13 00:07, Junio C Hamano wrote:
Show 7 quoted lines
> 
> I think you are misreading a suggestion that is somewhat misguided
> (yes "[ <condition> && <another> ]" does not make sense, but that is
> not applicable to "test <conditon> && test <another>"); ignore it.
> 
> It is fine to write "test <condition> && test <another>" and that
> works portably to even pre-posix systems.
(that's like doing "ls file && rm file" )
Show 5 quoted lines
> 
> But the existing code the patch touches favors [] over test
> consistently; that alone is a good reason to stick with [] in _this_
> script, even though it is against Git's overall shell script style.
> 

I suppose it would be fine if a patch was sent to update the entire git-prompt.sh code to be more in line with the Git shell script style...

My original gripe was just with doing it in one place while leaving all the others unchanged. It makes for messy reading and leads to confusion.

Cheers
Simon
Junio C Hamano· Feb 19, 2013, 16:28 UTC · re: Simon Oosthoek · lore

Re: [PATCH] shell-prompt: clean up nested if-then

Simon Oosthoek <s.oosthoek@xs4all.nl> writes:
> I suppose it would be fine if a patch was sent to update the entire
> git-prompt.sh code to be more in line with the Git shell script style...

Please don't. We do not want a "style conversion" for the sole purpose of conversion, especially when a subsystem is already internally consistent.

Besides, the git-prompt.sh thing needs to be fairly bash specific so the usual "Git Porcelain scripts targetted for POSIX/Bourne shells" rules does not apply there.

> My original gripe was just with doing it in one place while leaving all
> the others unchanged. It makes for messy reading and leads to confusion.
Yes, it is always preferred to match the _local_ convention.

← back to recent threads