Re: [PATCH] shell-prompt: clean up nested if-then
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] shell-prompt: clean up nested if-then
Simon Oosthoek s.oosth...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 jrnie...@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
21 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
--
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] shell-prompt: clean up nested if-then
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 jrnie...@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 jrnie...@gmail.com
Meh, I keep on missing that :/
Old (and new) patch is:
Signed-off-by: Martin Erik Werner martinerikwer...@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 21 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
--
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] shell-prompt: clean up nested if-then
Martin Erik Werner martinerikwer...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
