* [PATCH] shell-prompt: clean up nested if-then [not found] <1361204512.4758.10.camel@mas> @ 2013-02-18 16:23 ` Martin Erik Werner 2013-02-18 19:10 ` Jonathan Nieder 0 siblings, 1 reply; 8+ messages in thread From: Martin Erik Werner @ 2013-02-18 16:23 UTC (permalink / raw) To: gitster; +Cc: git, trsten, 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] shell-prompt: clean up nested if-then 2013-02-18 16:23 ` [PATCH] shell-prompt: clean up nested if-then Martin Erik Werner @ 2013-02-18 19:10 ` Jonathan Nieder [not found] ` <0c94f24b-f7ee-4699-87a7-6861b927cea4@email.android.com> 0 siblings, 1 reply; 8+ messages in thread From: Jonathan Nieder @ 2013-02-18 19:10 UTC (permalink / raw) To: Martin Erik Werner; +Cc: gitster, git, trsten, Simon Oosthoek, Felipe Contreras 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 > -- ^ permalink raw reply [flat|nested] 8+ messages in thread
[parent not found: <0c94f24b-f7ee-4699-87a7-6861b927cea4@email.android.com>]
* Re: [PATCH] shell-prompt: clean up nested if-then [not found] ` <0c94f24b-f7ee-4699-87a7-6861b927cea4@email.android.com> @ 2013-02-18 22:56 ` Martin Erik Werner 2013-02-18 22:59 ` [PATCH v2] " martinerikwerner 2013-02-18 23:07 ` [PATCH] " Junio C Hamano 0 siblings, 2 replies; 8+ messages in thread From: Martin Erik Werner @ 2013-02-18 22:56 UTC (permalink / raw) To: Simon vanaf Telefoon Cc: Jonathan Nieder, gitster, git, trsten, Felipe Contreras 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 > -- > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] shell-prompt: clean up nested if-then 2013-02-18 22:56 ` Martin Erik Werner @ 2013-02-18 22:59 ` martinerikwerner 2013-02-18 23:28 ` Jonathan Nieder 2013-02-18 23:07 ` [PATCH] " Junio C Hamano 1 sibling, 1 reply; 8+ messages in thread From: martinerikwerner @ 2013-02-18 22:59 UTC (permalink / raw) To: s.oosthoek, jrnieder; +Cc: git, trsten, felipe.contreras, Martin Erik Werner 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] shell-prompt: clean up nested if-then 2013-02-18 22:59 ` [PATCH v2] " martinerikwerner @ 2013-02-18 23:28 ` Jonathan Nieder 0 siblings, 0 replies; 8+ messages in thread From: Jonathan Nieder @ 2013-02-18 23:28 UTC (permalink / raw) To: martinerikwerner; +Cc: s.oosthoek, git, trsten, felipe.contreras 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] shell-prompt: clean up nested if-then 2013-02-18 22:56 ` Martin Erik Werner 2013-02-18 22:59 ` [PATCH v2] " martinerikwerner @ 2013-02-18 23:07 ` Junio C Hamano 2013-02-19 8:17 ` Simon Oosthoek 1 sibling, 1 reply; 8+ messages in thread From: Junio C Hamano @ 2013-02-18 23:07 UTC (permalink / raw) To: Martin Erik Werner Cc: Simon vanaf Telefoon, Jonathan Nieder, git, trsten, Felipe Contreras 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] shell-prompt: clean up nested if-then 2013-02-18 23:07 ` [PATCH] " Junio C Hamano @ 2013-02-19 8:17 ` Simon Oosthoek 2013-02-19 16:28 ` Junio C Hamano 0 siblings, 1 reply; 8+ messages in thread From: Simon Oosthoek @ 2013-02-19 8:17 UTC (permalink / raw) To: Junio C Hamano Cc: Martin Erik Werner, Jonathan Nieder, git, trsten, Felipe Contreras 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] shell-prompt: clean up nested if-then 2013-02-19 8:17 ` Simon Oosthoek @ 2013-02-19 16:28 ` Junio C Hamano 0 siblings, 0 replies; 8+ messages in thread From: Junio C Hamano @ 2013-02-19 16:28 UTC (permalink / raw) To: Simon Oosthoek Cc: Martin Erik Werner, Jonathan Nieder, git, trsten, Felipe Contreras 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. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2013-02-19 16:29 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1361204512.4758.10.camel@mas>
2013-02-18 16:23 ` [PATCH] shell-prompt: clean up nested if-then Martin Erik Werner
2013-02-18 19:10 ` Jonathan Nieder
[not found] ` <0c94f24b-f7ee-4699-87a7-6861b927cea4@email.android.com>
2013-02-18 22:56 ` Martin Erik Werner
2013-02-18 22:59 ` [PATCH v2] " martinerikwerner
2013-02-18 23:28 ` Jonathan Nieder
2013-02-18 23:07 ` [PATCH] " Junio C Hamano
2013-02-19 8:17 ` Simon Oosthoek
2013-02-19 16:28 ` Junio C Hamano
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 NNTP newsgroup(s).