{"thread":{"id":"42434","subject":"[PATCH] git-prompt.sh: Don't error on null ${ZSH,BASH}_VERSION","startedAt":"2016-05-24T08:31:53Z","lastAt":"2016-06-06T16:31:38Z","messageCount":5,"participants":["Ville Skyttä","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"287400","messageId":"1464078713-3220-1-git-send-email-ville.skytta@iki.fi","threadId":"42434","inReplyTo":null,"subject":"[PATCH] git-prompt.sh: Don't error on null ${ZSH,BASH}_VERSION","fromName":"Ville Skyttä","fromEmail":"ville.skytta@iki.fi","sentAt":"2016-05-24T08:31:53Z","receivedAt":"2016-05-24T08:31:53Z","isPatch":true,"sender":{"key":"ville.skytta@iki.fi","avatar":"https://avatars.githubusercontent.com/u/109152?v=4"},"body":"When the shell is in \"nounset\" or \"set -u\" mode, referencing unset or\nnull variables results in an error. Protect $ZSH_VERSION and\n$BASH_VERSION against that.\n\nSigned-off-by: Ville Skyttä <ville.skytta@iki.fi>\n---\n contrib/completion/git-prompt.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 64219e6..341cf6b 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -355,8 +355,8 @@ __git_ps1 ()\n \t# incorrect.)\n \t#\n \tlocal ps1_expanded=yes\n-\t[ -z \"$ZSH_VERSION\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n-\t[ -z \"$BASH_VERSION\" ] || shopt -q promptvars || ps1_expanded=no\n+\t[ -z \"${ZSH_VERSION-}\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n+\t[ -z \"${BASH_VERSION-}\" ] || shopt -q promptvars || ps1_expanded=no\n \n \tlocal repo_info rev_parse_exit_code\n \trepo_info=\"$(git rev-parse --git-dir --is-inside-git-dir \\\n-- \n2.5.5\n"},{"id":"287427","messageId":"xmqqwpmj5qjx.fsf@gitster.mtv.corp.google.com","threadId":"42434","inReplyTo":"1464078713-3220-1-git-send-email-ville.skytta@iki.fi","subject":"Re: [PATCH] git-prompt.sh: Don't error on null ${ZSH,BASH}_VERSION","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-24T17:11:46Z","receivedAt":"2016-05-24T17:11:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ville Skyttä <ville.skytta@iki.fi> writes:\n\n> When the shell is in \"nounset\" or \"set -u\" mode, referencing unset or\n> null variables results in an error. Protect $ZSH_VERSION and\n> $BASH_VERSION against that.\n>\n> Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>\n> ---\n\nMakes sense from a cursory look.  Thanks.\n\n>  contrib/completion/git-prompt.sh | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\n> index 64219e6..341cf6b 100644\n> --- a/contrib/completion/git-prompt.sh\n> +++ b/contrib/completion/git-prompt.sh\n> @@ -355,8 +355,8 @@ __git_ps1 ()\n>  \t# incorrect.)\n>  \t#\n>  \tlocal ps1_expanded=yes\n> -\t[ -z \"$ZSH_VERSION\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n> -\t[ -z \"$BASH_VERSION\" ] || shopt -q promptvars || ps1_expanded=no\n> +\t[ -z \"${ZSH_VERSION-}\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n> +\t[ -z \"${BASH_VERSION-}\" ] || shopt -q promptvars || ps1_expanded=no\n>  \n>  \tlocal repo_info rev_parse_exit_code\n>  \trepo_info=\"$(git rev-parse --git-dir --is-inside-git-dir \\\n"},{"id":"287429","messageId":"xmqqshx75psw.fsf@gitster.mtv.corp.google.com","threadId":"42434","inReplyTo":"1464078713-3220-1-git-send-email-ville.skytta@iki.fi","subject":"Re: [PATCH] git-prompt.sh: Don't error on null ${ZSH,BASH}_VERSION","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-24T17:27:59Z","receivedAt":"2016-05-24T17:27:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ville Skyttä <ville.skytta@iki.fi> writes:\n\n> When the shell is in \"nounset\" or \"set -u\" mode, referencing unset or\n> null variables results in an error. Protect $ZSH_VERSION and\n> $BASH_VERSION against that.\n>  \tlocal ps1_expanded=yes\n> -\t[ -z \"$ZSH_VERSION\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n> -\t[ -z \"$BASH_VERSION\" ] || shopt -q promptvars || ps1_expanded=no\n> +\t[ -z \"${ZSH_VERSION-}\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n> +\t[ -z \"${BASH_VERSION-}\" ] || shopt -q promptvars || ps1_expanded=no\n\nI wonder if we can teach t9903 to run (at least) some parts of the\ncompletion script under \"set -u\" to catch these automatically without\na report from an end user.\n\nWith the attached \"just started to scratch the surface\" patch to\ntest-lib.sh and fix by Ville to the git-prompt.sh script, we find\nanother one.\n\n    ...contrib/completion/git-prompt.sh: line 481: short_sha: unbound variable\n\nVille, I think you want to take the \"git-prompt.sh\" part from the\nfollowing and replace it with your patch.\n\nOthers, I am not sure how seriously we would want to support those\nwho use \"set -u\"; the damage to test-lib.sh so far (see below) does\nnot look _too_ bad, but the reason why I have \"set -u\" after\nincluding lib-bash.sh is because it will trigger a lot more errors\nwhile lib-bash and test-lib do their preparation.\n\n contrib/completion/git-prompt.sh | 5 +++--\n t/t9903-bash-prompt.sh           | 2 ++\n t/test-lib.sh                    | 8 +++++++-\n 3 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh\nindex 64219e6..ed994a9 100644\n--- a/contrib/completion/git-prompt.sh\n+++ b/contrib/completion/git-prompt.sh\n@@ -355,8 +355,8 @@ __git_ps1 ()\n \t# incorrect.)\n \t#\n \tlocal ps1_expanded=yes\n-\t[ -z \"$ZSH_VERSION\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n-\t[ -z \"$BASH_VERSION\" ] || shopt -q promptvars || ps1_expanded=no\n+\t[ -z \"${ZSH_VERSION-}\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n+\t[ -z \"${BASH_VERSION-}\" ] || shopt -q promptvars || ps1_expanded=no\n \n \tlocal repo_info rev_parse_exit_code\n \trepo_info=\"$(git rev-parse --git-dir --is-inside-git-dir \\\n@@ -369,6 +369,7 @@ __git_ps1 ()\n \tfi\n \n \tlocal short_sha\n+\tshort_sha=\n \tif [ \"$rev_parse_exit_code\" = \"0\" ]; then\n \t\tshort_sha=\"${repo_info##*$'\\n'}\"\n \t\trepo_info=\"${repo_info%$'\\n'*}\"\ndiff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh\nindex 0db4469..96b5ff8 100755\n--- a/t/t9903-bash-prompt.sh\n+++ b/t/t9903-bash-prompt.sh\n@@ -7,6 +7,8 @@ test_description='test git-specific bash prompt functions'\n \n . ./lib-bash.sh\n \n+set -u\n+\n . \"$GIT_BUILD_DIR/contrib/completion/git-prompt.sh\"\n \n actual=\"$TRASH_DIRECTORY/actual\"\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0055ebb..36ca13e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -186,6 +186,9 @@ test \"x$TERM\" != \"xdumb\" && (\n \t) &&\n \tcolor=t\n \n+HARNESS_ACTIVE= quiet= verbose_only= run_list= trace=\n+: ${GIT_SKIP_TESTS=}\n+\n while test \"$#\" -ne 0\n do\n \tcase \"$1\" in\n@@ -585,7 +588,7 @@ test_eval_ () {\n \n test_run_ () {\n \ttest_cleanup=:\n-\texpecting_failure=$2\n+\texpecting_failure=${2-}\n \n \tif test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0; then\n \t\t# turn off tracing for this test-eval, as it simply creates\n@@ -750,6 +753,8 @@ test_done () {\n \tesac\n }\n \n+GIT_VALGRIND=\n+\n if test -n \"$valgrind\"\n then\n \tmake_symlink () {\n@@ -902,6 +907,7 @@ fi\n # in subprocesses like git equals our $PWD (for pathname comparisons).\n cd -P \"$TRASH_DIRECTORY\" || exit 1\n \n+skip_all=\n this_test=${0##*/}\n this_test=${this_test%%-*}\n if match_pattern_list \"$this_test\" $GIT_SKIP_TESTS\n"},{"id":"287450","messageId":"xmqq7fej44iq.fsf@gitster.mtv.corp.google.com","threadId":"42434","inReplyTo":"xmqqshx75psw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-prompt.sh: Don't error on null ${ZSH,BASH}_VERSION","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-24T19:53:01Z","receivedAt":"2016-05-24T19:53:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Others, I am not sure how seriously we would want to support those\n> who use \"set -u\"; the damage to test-lib.sh so far (see below) does\n> not look _too_ bad, but the reason why I have \"set -u\" after\n> including lib-bash.sh is because it will trigger a lot more errors\n> while lib-bash and test-lib do their preparation.\n\nEhh, this didn't come out very well as I hoped, primarily because my\nthinking behind it was muddy X-<.\n\nI do want to see us keep supporting those who \"set -u\" in their\ninteractive shell environment, and we should strive to make sure\nthat the \"prompt\" and the \"completion\", which are part of their\ninteractive shell experience, work well with \"set -u\".\n\nIt does not mean \"cd t && sh t$anything.sh\" must be \"set -u\" clean\nat all (this is where my thinking was muddy); it is sufficient if we\ncan successfully run t9903 (prompt) with \"set -u\" before the script\nincludes \"git-prompt\".  We do not have to be able to insert \"set -u\"\nat the beginning of test-lib.sh and cleanly run other tests.\n\nHaving said that, some parts of \"minimum\" change shown in my \"how\nabout this\" patch would probably be worth doing anyway.  They make\nsure that:\n\n (1) variables the test framework uses will always be initialized\n     properly, without getting affected a stray environment variable\n     the user may have.  E.g. \"trace\".\n\n (2) environment variables the test framework may accept to change\n     the behaviour, e.g. GIT_SKIP_TESTS, are clearly documented by\n     using the idiom \"set to an empty string if not set\".\n\nSo in that sense, even though we may not add \"set -u\" at the\nbeginning of test-lib.sh in the production code, doing so may be a\ngood developer aid to catch other variables the attached patch did\nnot catch and complete the above two points.\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 0055ebb..36ca13e 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -186,6 +186,9 @@ test \"x$TERM\" != \"xdumb\" && (\n>  \t) &&\n>  \tcolor=t\n>  \n> +HARNESS_ACTIVE= quiet= verbose_only= run_list= trace=\n> +: ${GIT_SKIP_TESTS=}\n> +\n>  while test \"$#\" -ne 0\n>  do\n>  \tcase \"$1\" in\n> @@ -585,7 +588,7 @@ test_eval_ () {\n>  \n>  test_run_ () {\n>  \ttest_cleanup=:\n> -\texpecting_failure=$2\n> +\texpecting_failure=${2-}\n>  \n>  \tif test \"${GIT_TEST_CHAIN_LINT:-1}\" != 0; then\n>  \t\t# turn off tracing for this test-eval, as it simply creates\n> @@ -750,6 +753,8 @@ test_done () {\n>  \tesac\n>  }\n>  \n> +GIT_VALGRIND=\n> +\n>  if test -n \"$valgrind\"\n>  then\n>  \tmake_symlink () {\n> @@ -902,6 +907,7 @@ fi\n>  # in subprocesses like git equals our $PWD (for pathname comparisons).\n>  cd -P \"$TRASH_DIRECTORY\" || exit 1\n>  \n> +skip_all=\n>  this_test=${0##*/}\n>  this_test=${this_test%%-*}\n>  if match_pattern_list \"$this_test\" $GIT_SKIP_TESTS\n"},{"id":"288451","messageId":"CABr9L5CQeL6iaTh-=G8sfWuygqgpj50mxBaOSL6ihS6x91F3UA@mail.gmail.com","threadId":"42434","inReplyTo":"xmqqshx75psw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] git-prompt.sh: Don't error on null ${ZSH,BASH}_VERSION","fromName":"Ville Skyttä","fromEmail":"ville.skytta@iki.fi","sentAt":"2016-06-06T16:31:38Z","receivedAt":"2016-06-06T16:31:38Z","isPatch":true,"sender":{"key":"ville.skytta@iki.fi","avatar":"https://avatars.githubusercontent.com/u/109152?v=4"},"body":"On Tue, May 24, 2016 at 8:27 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ville Skyttä <ville.skytta@iki.fi> writes:\n>\n>> When the shell is in \"nounset\" or \"set -u\" mode, referencing unset or\n>> null variables results in an error. Protect $ZSH_VERSION and\n>> $BASH_VERSION against that.\n>>       local ps1_expanded=yes\n>> -     [ -z \"$ZSH_VERSION\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n>> -     [ -z \"$BASH_VERSION\" ] || shopt -q promptvars || ps1_expanded=no\n>> +     [ -z \"${ZSH_VERSION-}\" ] || [[ -o PROMPT_SUBST ]] || ps1_expanded=no\n>> +     [ -z \"${BASH_VERSION-}\" ] || shopt -q promptvars || ps1_expanded=no\n>\n> I wonder if we can teach t9903 to run (at least) some parts of the\n> completion script under \"set -u\" to catch these automatically without\n> a report from an end user.\n>\n> With the attached \"just started to scratch the surface\" patch to\n> test-lib.sh and fix by Ville to the git-prompt.sh script, we find\n> another one.\n>\n>     ...contrib/completion/git-prompt.sh: line 481: short_sha: unbound variable\n>\n> Ville, I think you want to take the \"git-prompt.sh\" part from the\n> following and replace it with your patch.\n\nHm, I'm not entirely sure what I was expected to do, but I took the\nshort_sha bit, added it to my patch (modifying to be in line with the\nstyle of the rest of the file), and submitted a superseding patch with\nthe subject \"[PATCH 1/2] git-prompt.sh: Don't error on null\n${ZSH,BASH}_VERSION, $short_sha\" (and there's no part 2/2 coming, that\nwas a user error).\n"}]}