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

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

[Martin Erik Werner]

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(-)

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

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

[Jonathan Nieder]

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.

> ---
>  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
> -- 

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

[Martin Erik Werner]

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've changed to using [] && [] and rerolled the patch.


> 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>

>         
>         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
>                 -- 
> 

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

[martinerikwerner]

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(-)

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

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

[Junio C Hamano]

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

> 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.

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

[Jonathan Nieder]

Martin Erik Werner wrote:

> 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.

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

[Simon Oosthoek]

On 19/02/13 00:07, Junio C Hamano wrote:
> 
> 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" )

> 
> 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

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

[Junio C Hamano]

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.