{"thread":{"id":"32926","subject":"[PATCH] shell-prompt: clean up nested if-then","startedAt":"2013-02-18T16:23:21Z","lastAt":"2013-02-19T16:28:35Z","messageCount":8,"participants":["Martin Erik Werner","Jonathan Nieder","martinerikwerner@gmail.com","Junio C Hamano","Simon Oosthoek"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"209687","messageId":"1361204601-4573-1-git-send-email-martinerikwerner@gmail.com","threadId":"32926","inReplyTo":"1361204512.4758.10.camel@mas","subject":"[PATCH] shell-prompt: clean up nested if-then","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2013-02-18T16:23:21Z","receivedAt":"2013-02-18T16:23:21Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"Minor clean up of if-then nesting in checks for environment variables\nand config options. No functional changes.\n---\n contrib/completion/git-prompt.sh |   27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 9b2eec2..e29694d 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -320,26 +320,25 @@ __git_ps1 ()\n \t\t\t\tb=\"GIT_DIR!\"\n \t\t\tfi\n \t\telif [ \"true\" = \"$(git rev-parse --is-inside-work-tree 2>/dev/null)\" ]; then\n-\t\t\tif [ -n \"${GIT_PS1_SHOWDIRTYSTATE-}\" ]; then\n-\t\t\t\tif [ \"$(git config --bool bash.showDirtyState)\" != \"false\" ]; then\n-\t\t\t\t\tgit diff --no-ext-diff --quiet --exit-code || w=\"*\"\n-\t\t\t\t\tif git rev-parse --quiet --verify HEAD >/dev/null; then\n-\t\t\t\t\t\tgit diff-index --cached --quiet HEAD -- || i=\"+\"\n-\t\t\t\t\telse\n-\t\t\t\t\t\ti=\"#\"\n-\t\t\t\t\tfi\n+\t\t\tif test -n \"${GIT_PS1_SHOWDIRTYSTATE-}\" &&\n+\t\t\t   test \"$(git config --bool bash.showDirtyState)\" != \"false\"\n+\t\t\tthen\n+\t\t\t\tgit diff --no-ext-diff --quiet --exit-code || w=\"*\"\n+\t\t\t\tif git rev-parse --quiet --verify HEAD >/dev/null; then\n+\t\t\t\t\tgit diff-index --cached --quiet HEAD -- || i=\"+\"\n+\t\t\t\telse\n+\t\t\t\t\ti=\"#\"\n \t\t\t\tfi\n \t\t\tfi\n \t\t\tif [ -n \"${GIT_PS1_SHOWSTASHSTATE-}\" ]; then\n \t\t\t\tgit rev-parse --verify refs/stash >/dev/null 2>&1 && s=\"$\"\n \t\t\tfi\n \n-\t\t\tif [ -n \"${GIT_PS1_SHOWUNTRACKEDFILES-}\" ]; then\n-\t\t\t\tif [ \"$(git config --bool bash.showUntrackedFiles)\" != \"false\" ]; then\n-\t\t\t\t\tif [ -n \"$(git ls-files --others --exclude-standard)\" ]; then\n-\t\t\t\t\t\tu=\"%\"\n-\t\t\t\t\tfi\n-\t\t\t\tfi\n+\t\t\tif test -n \"${GIT_PS1_SHOWUNTRACKEDFILES-}\" &&\n+\t\t\t   test \"$(git config --bool bash.showUntrackedFiles)\" != \"false\" &&\n+\t\t\t   test -n \"$(git ls-files --others --exclude-standard)\"\n+\t\t\tthen\n+\t\t\t\tu=\"%\"\n \t\t\tfi\n \n \t\t\tif [ -n \"${GIT_PS1_SHOWUPSTREAM-}\" ]; then\n-- \n1.7.10.4\n"},{"id":"209701","messageId":"20130218191040.GB3234@elie.Belkin","threadId":"32926","inReplyTo":"1361204601-4573-1-git-send-email-martinerikwerner@gmail.com","subject":"Re: [PATCH] shell-prompt: clean up nested if-then","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T19:10:40Z","receivedAt":"2013-02-18T19:10:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Martin,\n\nMartin Erik Werner wrote:\n\n> Minor clean up of if-then nesting in checks for environment variables\n> and config options. No functional changes.\n\nYeah, the nesting was getting a little deep.  Thanks for the cleanup.\nMay we have your sign-off?\n\nOnce this is signed off,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nPatch left unsnipped for reference.\n\n> ---\n>  contrib/completion/git-prompt.sh |   27 +++++++++++++--------------\n>  1 file changed, 13 insertions(+), 14 deletions(-)\n> \n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index 9b2eec2..e29694d 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -320,26 +320,25 @@ __git_ps1 ()\n>  \t\t\t\tb=\"GIT_DIR!\"\n>  \t\t\tfi\n>  \t\telif [ \"true\" = \"$(git rev-parse --is-inside-work-tree 2>/dev/null)\" ]; then\n> -\t\t\tif [ -n \"${GIT_PS1_SHOWDIRTYSTATE-}\" ]; then\n> -\t\t\t\tif [ \"$(git config --bool bash.showDirtyState)\" != \"false\" ]; then\n> -\t\t\t\t\tgit diff --no-ext-diff --quiet --exit-code || w=\"*\"\n> -\t\t\t\t\tif git rev-parse --quiet --verify HEAD >/dev/null; then\n> -\t\t\t\t\t\tgit diff-index --cached --quiet HEAD -- || i=\"+\"\n> -\t\t\t\t\telse\n> -\t\t\t\t\t\ti=\"#\"\n> -\t\t\t\t\tfi\n> +\t\t\tif test -n \"${GIT_PS1_SHOWDIRTYSTATE-}\" &&\n> +\t\t\t   test \"$(git config --bool bash.showDirtyState)\" != \"false\"\n> +\t\t\tthen\n> +\t\t\t\tgit diff --no-ext-diff --quiet --exit-code || w=\"*\"\n> +\t\t\t\tif git rev-parse --quiet --verify HEAD >/dev/null; then\n> +\t\t\t\t\tgit diff-index --cached --quiet HEAD -- || i=\"+\"\n> +\t\t\t\telse\n> +\t\t\t\t\ti=\"#\"\n>  \t\t\t\tfi\n>  \t\t\tfi\n>  \t\t\tif [ -n \"${GIT_PS1_SHOWSTASHSTATE-}\" ]; then\n>  \t\t\t\tgit rev-parse --verify refs/stash >/dev/null 2>&1 && s=\"$\"\n>  \t\t\tfi\n>  \n> -\t\t\tif [ -n \"${GIT_PS1_SHOWUNTRACKEDFILES-}\" ]; then\n> -\t\t\t\tif [ \"$(git config --bool bash.showUntrackedFiles)\" != \"false\" ]; then\n> -\t\t\t\t\tif [ -n \"$(git ls-files --others --exclude-standard)\" ]; then\n> -\t\t\t\t\t\tu=\"%\"\n> -\t\t\t\t\tfi\n> -\t\t\t\tfi\n> +\t\t\tif test -n \"${GIT_PS1_SHOWUNTRACKEDFILES-}\" &&\n> +\t\t\t   test \"$(git config --bool bash.showUntrackedFiles)\" != \"false\" &&\n> +\t\t\t   test -n \"$(git ls-files --others --exclude-standard)\"\n> +\t\t\tthen\n> +\t\t\t\tu=\"%\"\n>  \t\t\tfi\n>  \n>  \t\t\tif [ -n \"${GIT_PS1_SHOWUPSTREAM-}\" ]; then\n> -- \n"},{"id":"209749","messageId":"1361228206.17734.4.camel@mas","threadId":"32926","inReplyTo":"0c94f24b-f7ee-4699-87a7-6861b927cea4@email.android.com","subject":"Re: [PATCH] shell-prompt: clean up nested if-then","fromName":"Martin Erik Werner","fromEmail":"martinerikwerner@gmail.com","sentAt":"2013-02-18T22:56:46Z","receivedAt":"2013-02-18T22:56:46Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"\nOn Mon, 2013-02-18 at 21:31 +0100, Simon vanaf Telefoon wrote:\n> Hi all, sorry for top posting :-( blame the phone and k9\n> \n> I have a small issue with the use of test instead of [\n> If that only applies to this section of the entire file. \n> Coding style has some value.\n> \n> Combining nested ifs with && seems harmless enough, though should be\n> well tested.\n> \n> Cheers\n> Simon \n> \n\nAh, indeed, I looked around a bit more, and as per\nhttp://mywiki.wooledge.org/BashPitfalls it seems like 'test' is bad to use with multiple &&'s anyways.\n\nI've changed to using [] && [] and rerolled the patch.\n\n\n> Jonathan Nieder <jrnieder@gmail.com> wrote:\n>         Hi Martin,\n>         \n>         Martin Erik Werner wrote:\n>         \n>                 Minor clean up of if-then nesting in checks for environment variables\n>                 and config options. No functional changes.\n>         \n>         Yeah, the nesting was getting a little deep.  Thanks for the cleanup.\n>         May we have your sign-off?\n>         \n>         Once this is signed off,\n>         Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nMeh, I keep on missing that :/\nOld (and new) patch is:\nSigned-off-by: Martin Erik Werner <martinerikwerner@gmail.com>\n\n>         \n>         Patch left unsnipped for reference.\n>         \n>                 ---\n>                 contrib/completion/git-prompt.sh |   27 +++++++++++++--------------\n>                 1 file changed, 13 insertions(+), 14 deletions(-)\n>                 \n>                 diff --git\n>                 a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n>                 index 9b2eec2..e29694d 100644\n>                 --- a/contrib/completion/git-prompt.sh\n>                 +++ b/contrib/completion/git-prompt.sh\n>                 @@ -320,26 +320,25 @@ __git_ps1 ()\n>                     b=\"GIT_DIR!\"\n>                    fi\n>                   elif [ \"true\" = \"$(git rev-parse --is-inside-work-tree 2>/dev/null)\" ]; then\n>                 -   if [ -n \"${GIT_PS1_SHOWDIRTYSTATE-}\" ]; then\n>                 -    if [ \"$(git config --bool bash.showDirtyState)\" != \"false\" ]; then\n>                 -     git diff --no-ext-diff --quiet --exit-code || w=\"*\"\n>                 -     if git rev-parse --quiet --verify HEAD >/dev/null; then\n>                 -      git diff-index --cached --quiet HEAD -- || i=\"+\"\n>                 -     else\n>                 -      i=\"#\"\n>                 -     fi\n>                 +   if test -n \"${GIT_PS1_SHOWDIRTYSTATE-}\" &&\n>                 +      test \"$(git config --bool bash.showDirtyState)\" !=\n>                 \"false\"\n>                 +   then\n>                 +    git diff --no-ext-diff --quiet --exit-code || w=\"*\"\n>                 +    if git rev-parse --quiet --verify HEAD >/dev/null; then\n>                 +     git diff-index --cached --quiet HEAD -- || i=\"+\"\n>                 +    else\n>                 +     i=\"#\"\n>                     fi\n>                    fi\n>                    if [ -n \"${GIT_PS1_SHOWSTASHSTATE-}\" ]; then\n>                     git rev-parse --verify refs/stash >/dev/null 2>&1 && s=\"$\"\n>                    fi\n>                 \n>                 -   if [ -n \"${GIT_PS1_SHOWUNTRACKEDFILES-}\" ]; then\n>                 -    if [ \"$(git config --bool bash.showUntrackedFiles)\" != \"false\" ]; then\n>                 -     if [ -n \"$(git ls-files --others --exclude-standard)\" ]; then\n>                 -      u=\"%\"\n>                 -     fi\n>                 -    fi\n>                 +   if test -n \"${GIT_PS1_SHOWUNTRACKEDFILES-}\" &&\n>                 +      test \"$(git config --bool bash.showUntrackedFiles)\" != \"false\" &&\n>                 +      test -n \"$(git ls-files --others --exclude-standard)\"\n>                 +   then\n>                 +    u=\"%\"\n>                    fi\n>                 \n>                    if [ -n \"${GIT_PS1_SHOWUPSTREAM-}\" ];!\n>                   then\n>                 -- \n> \n"},{"id":"209750","messageId":"5122b23b.512e980a.32b9.ffffa580@mx.google.com","threadId":"32926","inReplyTo":"1361228206.17734.4.camel@mas","subject":"[PATCH v2] shell-prompt: clean up nested if-then","fromName":"","fromEmail":"martinerikwerner@gmail.com","sentAt":"2013-02-18T22:59:03Z","receivedAt":"2013-02-18T22:59:03Z","isPatch":true,"sender":{"key":"martinerikwerner@gmail.com","avatar":"https://avatars.githubusercontent.com/u/732190?v=4"},"body":"From: Martin Erik Werner <martinerikwerner@gmail.com>\n\nMinor clean up of if-then nesting in checks for environment variables\nand config options. No functional changes.\n\nSigned-off-by: Martin Erik Werner <martinerikwerner@gmail.com>\n---\n contrib/completion/git-prompt.sh |   27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 9b2eec2..341422a 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -320,26 +320,25 @@ __git_ps1 ()\n \t\t\t\tb=\"GIT_DIR!\"\n \t\t\tfi\n \t\telif [ \"true\" = \"$(git rev-parse --is-inside-work-tree 2>/dev/null)\" ]; then\n-\t\t\tif [ -n \"${GIT_PS1_SHOWDIRTYSTATE-}\" ]; then\n-\t\t\t\tif [ \"$(git config --bool bash.showDirtyState)\" != \"false\" ]; then\n-\t\t\t\t\tgit diff --no-ext-diff --quiet --exit-code || w=\"*\"\n-\t\t\t\t\tif git rev-parse --quiet --verify HEAD >/dev/null; then\n-\t\t\t\t\t\tgit diff-index --cached --quiet HEAD -- || i=\"+\"\n-\t\t\t\t\telse\n-\t\t\t\t\t\ti=\"#\"\n-\t\t\t\t\tfi\n+\t\t\tif [ -n \"${GIT_PS1_SHOWDIRTYSTATE-}\" ] &&\n+\t\t\t   [ \"$(git config --bool bash.showDirtyState)\" != \"false\" ]\n+\t\t\tthen\n+\t\t\t\tgit diff --no-ext-diff --quiet --exit-code || w=\"*\"\n+\t\t\t\tif git rev-parse --quiet --verify HEAD >/dev/null; then\n+\t\t\t\t\tgit diff-index --cached --quiet HEAD -- || i=\"+\"\n+\t\t\t\telse\n+\t\t\t\t\ti=\"#\"\n \t\t\t\tfi\n \t\t\tfi\n \t\t\tif [ -n \"${GIT_PS1_SHOWSTASHSTATE-}\" ]; then\n \t\t\t\tgit rev-parse --verify refs/stash >/dev/null 2>&1 && s=\"$\"\n \t\t\tfi\n \n-\t\t\tif [ -n \"${GIT_PS1_SHOWUNTRACKEDFILES-}\" ]; then\n-\t\t\t\tif [ \"$(git config --bool bash.showUntrackedFiles)\" != \"false\" ]; then\n-\t\t\t\t\tif [ -n \"$(git ls-files --others --exclude-standard)\" ]; then\n-\t\t\t\t\t\tu=\"%\"\n-\t\t\t\t\tfi\n-\t\t\t\tfi\n+\t\t\tif [ -n \"${GIT_PS1_SHOWUNTRACKEDFILES-}\" ] &&\n+\t\t\t   [ \"$(git config --bool bash.showUntrackedFiles)\" != \"false\" ] &&\n+\t\t\t   [ -n \"$(git ls-files --others --exclude-standard)\" ]\n+\t\t\tthen\n+\t\t\t\tu=\"%\"\n \t\t\tfi\n \n \t\t\tif [ -n \"${GIT_PS1_SHOWUPSTREAM-}\" ]; then\n-- \n1.7.10.4\n"},{"id":"209752","messageId":"7vtxp98bmx.fsf@alter.siamese.dyndns.org","threadId":"32926","inReplyTo":"1361228206.17734.4.camel@mas","subject":"Re: [PATCH] shell-prompt: clean up nested if-then","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-18T23:07:02Z","receivedAt":"2013-02-18T23:07:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Erik Werner <martinerikwerner@gmail.com> writes:\n\n> On Mon, 2013-02-18 at 21:31 +0100, Simon vanaf Telefoon wrote:\n>> Hi all, sorry for top posting :-( blame the phone and k9\n>> \n>> I have a small issue with the use of test instead of [\n>> If that only applies to this section of the entire file. \n>> Coding style has some value.\n>> \n>> Combining nested ifs with && seems harmless enough, though should be\n>> well tested.\n>> \n>> Cheers\n>> Simon \n>> \n>\n> Ah, indeed, I looked around a bit more, and as per\n> http://mywiki.wooledge.org/BashPitfalls it seems like 'test' is bad to use with multiple &&'s anyways.\n\nI think you are misreading a suggestion that is somewhat misguided\n(yes \"[ <condition> && <another> ]\" does not make sense, but that is\nnot applicable to \"test <conditon> && test <another>\"); ignore it.\n\nIt is fine to write \"test <condition> && test <another>\" and that\nworks portably to even pre-posix systems.\n\nBut the existing code the patch touches favors [] over test\nconsistently; that alone is a good reason to stick with [] in _this_\nscript, even though it is against Git's overall shell script style.\n"},{"id":"209754","messageId":"20130218232816.GE4022@elie.Belkin","threadId":"32926","inReplyTo":"5122b23b.512e980a.32b9.ffffa580@mx.google.com","subject":"Re: [PATCH v2] shell-prompt: clean up nested if-then","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-18T23:28:16Z","receivedAt":"2013-02-18T23:28:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Martin Erik Werner wrote:\n\n> Minor clean up of if-then nesting in checks for environment variables\n> and config options. No functional changes.\n>\n> Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>\n> ---\n>  contrib/completion/git-prompt.sh |   27 +++++++++++++--------------\n>  1 file changed, 13 insertions(+), 14 deletions(-)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for the quick turnaround.\n"},{"id":"209780","messageId":"5123350D.4010802@xs4all.nl","threadId":"32926","inReplyTo":"7vtxp98bmx.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] shell-prompt: clean up nested if-then","fromName":"Simon Oosthoek","fromEmail":"s.oosthoek@xs4all.nl","sentAt":"2013-02-19T08:17:17Z","receivedAt":"2013-02-19T08:17:17Z","isPatch":true,"sender":{"key":"s.oosthoek@xs4all.nl","avatar":"https://avatars.githubusercontent.com/u/11677720?v=4"},"body":"On 19/02/13 00:07, Junio C Hamano wrote:\n> \n> I think you are misreading a suggestion that is somewhat misguided\n> (yes \"[ <condition> && <another> ]\" does not make sense, but that is\n> not applicable to \"test <conditon> && test <another>\"); ignore it.\n> \n> It is fine to write \"test <condition> && test <another>\" and that\n> works portably to even pre-posix systems.\n\n(that's like doing \"ls file && rm file\" )\n\n> \n> But the existing code the patch touches favors [] over test\n> consistently; that alone is a good reason to stick with [] in _this_\n> script, even though it is against Git's overall shell script style.\n> \n\nI suppose it would be fine if a patch was sent to update the entire\ngit-prompt.sh code to be more in line with the Git shell script style...\n\nMy original gripe was just with doing it in one place while leaving all\nthe others unchanged. It makes for messy reading and leads to confusion.\n\nCheers\n\nSimon\n"},{"id":"209833","messageId":"7vy5ek6zf0.fsf@alter.siamese.dyndns.org","threadId":"32926","inReplyTo":"5123350D.4010802@xs4all.nl","subject":"Re: [PATCH] shell-prompt: clean up nested if-then","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-19T16:28:35Z","receivedAt":"2013-02-19T16:28:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Oosthoek <s.oosthoek@xs4all.nl> writes:\n\n> I suppose it would be fine if a patch was sent to update the entire\n> git-prompt.sh code to be more in line with the Git shell script style...\n\nPlease don't.  We do not want a \"style conversion\" for the sole\npurpose of conversion, especially when a subsystem is already\ninternally consistent.\n\nBesides, the git-prompt.sh thing needs to be fairly bash specific so\nthe usual \"Git Porcelain scripts targetted for POSIX/Bourne shells\"\nrules does not apply there.\n\n> My original gripe was just with doing it in one place while leaving all\n> the others unchanged. It makes for messy reading and leads to confusion.\n\nYes, it is always preferred to match the _local_ convention.\n"}]}