{"thread":{"id":"39662","subject":"[PATCH 0/2] redo fix for test-lib.sh color support","startedAt":"2015-06-17T19:06:24Z","lastAt":"2015-06-17T22:26:02Z","messageCount":13,"participants":["Richard Hansen","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"264063","messageId":"1434567986-23552-1-git-send-email-rhansen@bbn.com","threadId":"39662","inReplyTo":null,"subject":"[PATCH 0/2] redo fix for test-lib.sh color support","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-06-17T19:06:24Z","receivedAt":"2015-06-17T19:06:24Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"Commit 102fc80d fixed a bug where tput was failing because it needed\nto read ~/.terminfo after HOME was changed.  However, that commit is\nbuggy, and it unnecessarily disables color support when tput needs to\nread from ~/.terminfo.\n\nThis series does two things:\n\n  * revert the buggy fix\n  * fix it properly, I hope :)\n\nRichard Hansen (2):\n  Revert \"test-lib.sh: do tests for color support after changing HOME\"\n  test-lib.sh: fix color support when tput needs ~/.terminfo\n\n t/test-lib.sh | 103 +++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 51 insertions(+), 52 deletions(-)\n\n-- \n2.4.3\n"},{"id":"264065","messageId":"1434567986-23552-2-git-send-email-rhansen@bbn.com","threadId":"39662","inReplyTo":"1434567986-23552-1-git-send-email-rhansen@bbn.com","subject":"[PATCH 1/2] Revert \"test-lib.sh: do tests for color support after changing HOME\"","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-06-17T19:06:25Z","receivedAt":"2015-06-17T19:06:25Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"This reverts commit 102fc80d32094ad6598b17ab9d607516ee8edc4a.\n\nThere are two issues with that commit:\n\n  * It is buggy.  In pseudocode, it is doing:\n\n       color is set || TERM != dumb && color works && color=t\n\n    when it should be doing:\n\n       color is set || { TERM != dumb && color works && color=t }\n\n  * It unnecessarily disables color when tput needs to read\n    ~/.terminfo to get the control sequences.\n---\n t/test-lib.sh | 90 ++++++++++++++++++++++++++++-------------------------------\n 1 file changed, 43 insertions(+), 47 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 39da9c2..57212ec 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -181,8 +181,16 @@ export _x05 _x40 _z40 LF u200c\n # This test checks if command xyzzy does the right thing...\n # '\n # . ./test-lib.sh\n+test \"x$ORIGINAL_TERM\" != \"xdumb\" && (\n+\t\tTERM=$ORIGINAL_TERM &&\n+\t\texport TERM &&\n+\t\ttest -t 1 &&\n+\t\ttput bold >/dev/null 2>&1 &&\n+\t\ttput setaf 1 >/dev/null 2>&1 &&\n+\t\ttput sgr0 >/dev/null 2>&1\n+\t) &&\n+\tcolor=t\n \n-unset color\n while test \"$#\" -ne 0\n do\n \tcase \"$1\" in\n@@ -253,6 +261,40 @@ then\n \tverbose=t\n fi\n \n+if test -n \"$color\"\n+then\n+\tsay_color () {\n+\t\t(\n+\t\tTERM=$ORIGINAL_TERM\n+\t\texport TERM\n+\t\tcase \"$1\" in\n+\t\terror)\n+\t\t\ttput bold; tput setaf 1;; # bold red\n+\t\tskip)\n+\t\t\ttput setaf 4;; # blue\n+\t\twarn)\n+\t\t\ttput setaf 3;; # brown/yellow\n+\t\tpass)\n+\t\t\ttput setaf 2;; # green\n+\t\tinfo)\n+\t\t\ttput setaf 6;; # cyan\n+\t\t*)\n+\t\t\ttest -n \"$quiet\" && return;;\n+\t\tesac\n+\t\tshift\n+\t\tprintf \"%s\" \"$*\"\n+\t\ttput sgr0\n+\t\techo\n+\t\t)\n+\t}\n+else\n+\tsay_color() {\n+\t\ttest -z \"$1\" && test -n \"$quiet\" && return\n+\t\tshift\n+\t\tprintf \"%s\\n\" \"$*\"\n+\t}\n+fi\n+\n error () {\n \tsay_color error \"error: $*\"\n \tGIT_EXIT_OK=t\n@@ -829,52 +871,6 @@ HOME=\"$TRASH_DIRECTORY\"\n GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n export HOME GNUPGHOME\n \n-# run the tput tests *after* changing HOME (in case ncurses needs\n-# ~/.terminfo for $TERM)\n-test -n \"${color+set}\" || test \"x$ORIGINAL_TERM\" != \"xdumb\" && (\n-\t\tTERM=$ORIGINAL_TERM &&\n-\t\texport TERM &&\n-\t\ttest -t 1 &&\n-\t\ttput bold >/dev/null 2>&1 &&\n-\t\ttput setaf 1 >/dev/null 2>&1 &&\n-\t\ttput sgr0 >/dev/null 2>&1\n-\t) &&\n-\tcolor=t\n-\n-if test -n \"$color\"\n-then\n-\tsay_color () {\n-\t\t(\n-\t\tTERM=$ORIGINAL_TERM\n-\t\texport TERM\n-\t\tcase \"$1\" in\n-\t\terror)\n-\t\t\ttput bold; tput setaf 1;; # bold red\n-\t\tskip)\n-\t\t\ttput setaf 4;; # blue\n-\t\twarn)\n-\t\t\ttput setaf 3;; # brown/yellow\n-\t\tpass)\n-\t\t\ttput setaf 2;; # green\n-\t\tinfo)\n-\t\t\ttput setaf 6;; # cyan\n-\t\t*)\n-\t\t\ttest -n \"$quiet\" && return;;\n-\t\tesac\n-\t\tshift\n-\t\tprintf \"%s\" \"$*\"\n-\t\ttput sgr0\n-\t\techo\n-\t\t)\n-\t}\n-else\n-\tsay_color() {\n-\t\ttest -z \"$1\" && test -n \"$quiet\" && return\n-\t\tshift\n-\t\tprintf \"%s\\n\" \"$*\"\n-\t}\n-fi\n-\n if test -z \"$TEST_NO_CREATE_REPO\"\n then\n \ttest_create_repo \"$TRASH_DIRECTORY\"\n-- \n2.4.3\n"},{"id":"264066","messageId":"1434567986-23552-3-git-send-email-rhansen@bbn.com","threadId":"39662","inReplyTo":"1434567986-23552-1-git-send-email-rhansen@bbn.com","subject":"[PATCH 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-06-17T19:06:26Z","receivedAt":"2015-06-17T19:06:26Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"If tput needs ~/.terminfo for the current $TERM, then tput will\nsucceed before HOME is changed to $TRASH_DIRECTORY (causing color to\nbe set to 't') but fail afterward.\n\nOne possible way to fix this is to treat HOME like TERM: back up the\noriginal value and temporarily restore it before say_color() runs\ntput.\n\nInstead, pre-compute and save the color control sequences before\nchanging either TERM or HOME.  Use the saved control sequences in\nsay_color() rather than call tput each time.  This avoids the need to\nback up and restore the TERM and HOME variables, and it avoids the\noverhead of a subshell and two invocations of tput per call to\nsay_color().\n\nSigned-off-by: Richard Hansen <rhansen@bbn.com>\n---\n t/test-lib.sh | 53 ++++++++++++++++++++++++++++-------------------------\n 1 file changed, 28 insertions(+), 25 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 57212ec..4a59bfb 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -15,9 +15,6 @@\n # You should have received a copy of the GNU General Public License\n # along with this program.  If not, see http://www.gnu.org/licenses/ .\n \n-# Keep the original TERM for say_color\n-ORIGINAL_TERM=$TERM\n-\n # Test the binaries we have just built.  The tests are kept in\n # t/ subdirectory and are run in 'trash directory' subdirectory.\n if test -z \"$TEST_DIRECTORY\"\n@@ -68,12 +65,12 @@ done,*)\n esac\n \n # For repeatability, reset the environment to known value.\n+# TERM is sanitized below, after saving color control sequences.\n LANG=C\n LC_ALL=C\n PAGER=cat\n TZ=UTC\n-TERM=dumb\n-export LANG LC_ALL PAGER TERM TZ\n+export LANG LC_ALL PAGER TZ\n EDITOR=:\n # A call to \"unset\" with no arguments causes at least Solaris 10\n # /usr/xpg4/bin/sh and /bin/ksh to bail out.  So keep the unsets\n@@ -181,9 +178,7 @@ export _x05 _x40 _z40 LF u200c\n # This test checks if command xyzzy does the right thing...\n # '\n # . ./test-lib.sh\n-test \"x$ORIGINAL_TERM\" != \"xdumb\" && (\n-\t\tTERM=$ORIGINAL_TERM &&\n-\t\texport TERM &&\n+test \"x$TERM\" != \"xdumb\" && (\n \t\ttest -t 1 &&\n \t\ttput bold >/dev/null 2>&1 &&\n \t\ttput setaf 1 >/dev/null 2>&1 &&\n@@ -263,29 +258,34 @@ fi\n \n if test -n \"$color\"\n then\n+\t# Save the color control sequences now rather than run tput\n+\t# each time say_color() is called.  This is done for two\n+\t# reasons:\n+\t#   * TERM will be changed to dumb\n+\t#   * HOME will be changed to a temporary directory and tput\n+\t#     might need to read ~/.terminfo from the original HOME\n+\t#     directory to get the control sequences\n+\t# Note:  This approach assumes the control sequences don't end\n+\t# in a newline for any terminal of interest (command\n+\t# substitutions strip trailing newlines).  Given that most\n+\t# (all?) terminals in common use are related to ECMA-48, this\n+\t# shouldn't be a problem.\n+\tsay_color_error=$(tput bold; tput setaf 1) # bold red\n+\tsay_color_skip=$(tput setaf 4) # blue\n+\tsay_color_warn=$(tput setaf 3) # brown/yellow\n+\tsay_color_pass=$(tput setaf 2) # green\n+\tsay_color_info=$(tput setaf 6) # cyan\n+\tsay_color_sgr0=$(tput sgr0)\n \tsay_color () {\n-\t\t(\n-\t\tTERM=$ORIGINAL_TERM\n-\t\texport TERM\n+\t\tsay_color_color=\n \t\tcase \"$1\" in\n-\t\terror)\n-\t\t\ttput bold; tput setaf 1;; # bold red\n-\t\tskip)\n-\t\t\ttput setaf 4;; # blue\n-\t\twarn)\n-\t\t\ttput setaf 3;; # brown/yellow\n-\t\tpass)\n-\t\t\ttput setaf 2;; # green\n-\t\tinfo)\n-\t\t\ttput setaf 6;; # cyan\n+\t\terror|skip|warn|pass|info)\n+\t\t\teval \"say_color_color=\\$say_color_$1\";;\n \t\t*)\n \t\t\ttest -n \"$quiet\" && return;;\n \t\tesac\n \t\tshift\n-\t\tprintf \"%s\" \"$*\"\n-\t\ttput sgr0\n-\t\techo\n-\t\t)\n+\t\tprintf \"%s\\\\n\" \"$say_color_color$*$say_color_sgr0\"\n \t}\n else\n \tsay_color() {\n@@ -295,6 +295,9 @@ else\n \t}\n fi\n \n+TERM=dumb\n+export TERM\n+\n error () {\n \tsay_color error \"error: $*\"\n \tGIT_EXIT_OK=t\n-- \n2.4.3\n"},{"id":"264070","messageId":"20150617194315.GE25304@peff.net","threadId":"39662","inReplyTo":"1434567986-23552-3-git-send-email-rhansen@bbn.com","subject":"Re: [PATCH 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-17T19:43:15Z","receivedAt":"2015-06-17T19:43:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 17, 2015 at 03:06:26PM -0400, Richard Hansen wrote:\n\n> If tput needs ~/.terminfo for the current $TERM, then tput will\n> succeed before HOME is changed to $TRASH_DIRECTORY (causing color to\n> be set to 't') but fail afterward.\n> \n> One possible way to fix this is to treat HOME like TERM: back up the\n> original value and temporarily restore it before say_color() runs\n> tput.\n> \n> Instead, pre-compute and save the color control sequences before\n> changing either TERM or HOME.  Use the saved control sequences in\n> say_color() rather than call tput each time.  This avoids the need to\n> back up and restore the TERM and HOME variables, and it avoids the\n> overhead of a subshell and two invocations of tput per call to\n> say_color().\n> \n> Signed-off-by: Richard Hansen <rhansen@bbn.com>\n\nNice, I like it.\n\n> +\t# Save the color control sequences now rather than run tput\n> +\t# each time say_color() is called.  This is done for two\n> +\t# reasons:\n> +\t#   * TERM will be changed to dumb\n> +\t#   * HOME will be changed to a temporary directory and tput\n> +\t#     might need to read ~/.terminfo from the original HOME\n> +\t#     directory to get the control sequences\n> +\t# Note:  This approach assumes the control sequences don't end\n> +\t# in a newline for any terminal of interest (command\n> +\t# substitutions strip trailing newlines).  Given that most\n> +\t# (all?) terminals in common use are related to ECMA-48, this\n> +\t# shouldn't be a problem.\n\nYeah, that was my first thought, but I agree it probably isn't going to\nbe a big deal in practice.\n\n> +\tsay_color_error=$(tput bold; tput setaf 1) # bold red\n> +\tsay_color_skip=$(tput setaf 4) # blue\n> +\tsay_color_warn=$(tput setaf 3) # brown/yellow\n> +\tsay_color_pass=$(tput setaf 2) # green\n> +\tsay_color_info=$(tput setaf 6) # cyan\n> +\tsay_color_sgr0=$(tput sgr0)\n> [...]\n> +\t\terror|skip|warn|pass|info)\n> +\t\t\teval \"say_color_color=\\$say_color_$1\";;\n>  \t\t*)\n>  \t\t\ttest -n \"$quiet\" && return;;\n\nI think you could dispense with this case statement entirely and do:\n\n  eval \"say_color_color=\\$say_color_$1\"\n  if test -z \"$say_color_color\"; then\n          test -n \"$quiet\" && return\n  fi\n\nI guess that is making the assumption that all colors have non-zero\nsizes, but that seems reasonable. I do not mind it so much as you have\nit, but it does mean adding a new field needs to update two spots.\n\n-Peff\n"},{"id":"264073","messageId":"5581D099.7090200@bbn.com","threadId":"39662","inReplyTo":"20150617194315.GE25304@peff.net","subject":"Re: [PATCH 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-06-17T19:55:05Z","receivedAt":"2015-06-17T19:55:05Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"On 2015-06-17 15:43, Jeff King wrote:\n> On Wed, Jun 17, 2015 at 03:06:26PM -0400, Richard Hansen wrote:\n>> +\tsay_color_error=$(tput bold; tput setaf 1) # bold red\n>> +\tsay_color_skip=$(tput setaf 4) # blue\n>> +\tsay_color_warn=$(tput setaf 3) # brown/yellow\n>> +\tsay_color_pass=$(tput setaf 2) # green\n>> +\tsay_color_info=$(tput setaf 6) # cyan\n>> +\tsay_color_sgr0=$(tput sgr0)\n>> [...]\n>> +\t\terror|skip|warn|pass|info)\n>> +\t\t\teval \"say_color_color=\\$say_color_$1\";;\n>>  \t\t*)\n>>  \t\t\ttest -n \"$quiet\" && return;;\n> \n> I think you could dispense with this case statement entirely and do:\n> \n>   eval \"say_color_color=\\$say_color_$1\"\n>   if test -z \"$say_color_color\"; then\n>           test -n \"$quiet\" && return\n>   fi\n> \n> I guess that is making the assumption that all colors have non-zero\n> sizes, but that seems reasonable.\n\nWe could test if the variable is set first (test -n \"${foo+set}\"), at\nthe cost of a bit more complexity.\n\n> I do not mind it so much as you have\n> it, but it does mean adding a new field needs to update two spots.\n\nI also don't like the duplicate list of color types, and I considered\ndoing something similar to what you suggested, but I decided against it.\n I'm a bit worried about bizarre syntax errors or code execution if\nsay_color() is used improperly.  ('eval' with uncontrolled variables\nmakes me nervous.)\n\nThanks for reviewing,\nRichard\n"},{"id":"264080","messageId":"xmqqvbem5bx9.fsf@gitster.dls.corp.google.com","threadId":"39662","inReplyTo":"5581D099.7090200@bbn.com","subject":"Re: [PATCH 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-17T20:15:30Z","receivedAt":"2015-06-17T20:15:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Richard Hansen <rhansen@bbn.com> writes:\n\n> We could test if the variable is set first (test -n \"${foo+set}\"), at\n> the cost of a bit more complexity.\n>\n>> I do not mind it so much as you have\n>> it, but it does mean adding a new field needs to update two spots.\n>\n> I also don't like the duplicate list of color types, and I considered\n> doing something similar to what you suggested, but I decided against it.\n> I'm a bit worried about bizarre syntax errors or code execution if\n> say_color() is used improperly.  ('eval' with uncontrolled variables\n> makes me nervous.)\n\nI originally had the same reaction to your use of `eval` (with or\nwithout being guarded by the case to limit to known 5 ones).  But\nthe uncontrolled-ness of this use of eval is to the same degree of\nuncontrolled-ness of any test_expect_{success,failure} scriptlet,\nso...\n\nI like this \"save to variables instead of using tput\" approach very\nmuch either way.  Well done.\n\nThanks.\n"},{"id":"264081","messageId":"20150617202507.GA25234@peff.net","threadId":"39662","inReplyTo":"5581D099.7090200@bbn.com","subject":"Re: [PATCH 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-17T20:25:08Z","receivedAt":"2015-06-17T20:25:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 17, 2015 at 03:55:05PM -0400, Richard Hansen wrote:\n\n> > I do not mind it so much as you have\n> > it, but it does mean adding a new field needs to update two spots.\n> \n> I also don't like the duplicate list of color types, and I considered\n> doing something similar to what you suggested, but I decided against it.\n>  I'm a bit worried about bizarre syntax errors or code execution if\n> say_color() is used improperly.  ('eval' with uncontrolled variables\n> makes me nervous.)\n\nAs Junio pointed out, I think all bets are off in the test scripts. They\nare running tons of arbitrary code. :)\n\nBut for the record, I am fine with your patch as-is. Thanks for looking\ninto it.\n\n-Peff\n"},{"id":"264088","messageId":"1434575481-24604-1-git-send-email-rhansen@bbn.com","threadId":"39662","inReplyTo":"xmqqvbem5bx9.fsf@gitster.dls.corp.google.com","subject":"[PATCH v2 0/2] redo fix for test-lib.sh color support","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-06-17T21:11:19Z","receivedAt":"2015-06-17T21:11:19Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"Changes from v1:\n  * Eliminate the case statement and assume the user passed a sane\n    value for $1.\n  * Use the same test as the non-colorized version of say_color() when\n    determining whether to suppress the output:  assume that a message\n    can only be suppresed if $1 is the empty string.  This avoids the\n    need to test whether the variable say_color_$1 is set.\n  * Rename say_color_sgr0 to say_color_reset.\n  * Add a new variable say_color_ (set to the empty string) as a way\n    of documenting that $1 is expected to be the empty string for\n    normal text.\n\nRichard Hansen (2):\n  Revert \"test-lib.sh: do tests for color support after changing HOME\"\n  test-lib.sh: fix color support when tput needs ~/.terminfo\n\n t/test-lib.sh | 99 ++++++++++++++++++++++++++++-------------------------------\n 1 file changed, 47 insertions(+), 52 deletions(-)\n\n-- \n2.4.3\n"},{"id":"264087","messageId":"1434575481-24604-2-git-send-email-rhansen@bbn.com","threadId":"39662","inReplyTo":"1434575481-24604-1-git-send-email-rhansen@bbn.com","subject":"[PATCH v2 1/2] Revert \"test-lib.sh: do tests for color support after changing HOME\"","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-06-17T21:11:20Z","receivedAt":"2015-06-17T21:11:20Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"This reverts commit 102fc80d32094ad6598b17ab9d607516ee8edc4a.\n\nThere are two issues with that commit:\n\n  * It is buggy.  In pseudocode, it is doing:\n\n       color is set || TERM != dumb && color works && color=t\n\n    when it should be doing:\n\n       color is set || { TERM != dumb && color works && color=t }\n\n  * It unnecessarily disables color when tput needs to read\n    ~/.terminfo to get the control sequences.\n---\n t/test-lib.sh | 90 ++++++++++++++++++++++++++++-------------------------------\n 1 file changed, 43 insertions(+), 47 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 39da9c2..57212ec 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -181,8 +181,16 @@ export _x05 _x40 _z40 LF u200c\n # This test checks if command xyzzy does the right thing...\n # '\n # . ./test-lib.sh\n+test \"x$ORIGINAL_TERM\" != \"xdumb\" && (\n+\t\tTERM=$ORIGINAL_TERM &&\n+\t\texport TERM &&\n+\t\ttest -t 1 &&\n+\t\ttput bold >/dev/null 2>&1 &&\n+\t\ttput setaf 1 >/dev/null 2>&1 &&\n+\t\ttput sgr0 >/dev/null 2>&1\n+\t) &&\n+\tcolor=t\n \n-unset color\n while test \"$#\" -ne 0\n do\n \tcase \"$1\" in\n@@ -253,6 +261,40 @@ then\n \tverbose=t\n fi\n \n+if test -n \"$color\"\n+then\n+\tsay_color () {\n+\t\t(\n+\t\tTERM=$ORIGINAL_TERM\n+\t\texport TERM\n+\t\tcase \"$1\" in\n+\t\terror)\n+\t\t\ttput bold; tput setaf 1;; # bold red\n+\t\tskip)\n+\t\t\ttput setaf 4;; # blue\n+\t\twarn)\n+\t\t\ttput setaf 3;; # brown/yellow\n+\t\tpass)\n+\t\t\ttput setaf 2;; # green\n+\t\tinfo)\n+\t\t\ttput setaf 6;; # cyan\n+\t\t*)\n+\t\t\ttest -n \"$quiet\" && return;;\n+\t\tesac\n+\t\tshift\n+\t\tprintf \"%s\" \"$*\"\n+\t\ttput sgr0\n+\t\techo\n+\t\t)\n+\t}\n+else\n+\tsay_color() {\n+\t\ttest -z \"$1\" && test -n \"$quiet\" && return\n+\t\tshift\n+\t\tprintf \"%s\\n\" \"$*\"\n+\t}\n+fi\n+\n error () {\n \tsay_color error \"error: $*\"\n \tGIT_EXIT_OK=t\n@@ -829,52 +871,6 @@ HOME=\"$TRASH_DIRECTORY\"\n GNUPGHOME=\"$HOME/gnupg-home-not-used\"\n export HOME GNUPGHOME\n \n-# run the tput tests *after* changing HOME (in case ncurses needs\n-# ~/.terminfo for $TERM)\n-test -n \"${color+set}\" || test \"x$ORIGINAL_TERM\" != \"xdumb\" && (\n-\t\tTERM=$ORIGINAL_TERM &&\n-\t\texport TERM &&\n-\t\ttest -t 1 &&\n-\t\ttput bold >/dev/null 2>&1 &&\n-\t\ttput setaf 1 >/dev/null 2>&1 &&\n-\t\ttput sgr0 >/dev/null 2>&1\n-\t) &&\n-\tcolor=t\n-\n-if test -n \"$color\"\n-then\n-\tsay_color () {\n-\t\t(\n-\t\tTERM=$ORIGINAL_TERM\n-\t\texport TERM\n-\t\tcase \"$1\" in\n-\t\terror)\n-\t\t\ttput bold; tput setaf 1;; # bold red\n-\t\tskip)\n-\t\t\ttput setaf 4;; # blue\n-\t\twarn)\n-\t\t\ttput setaf 3;; # brown/yellow\n-\t\tpass)\n-\t\t\ttput setaf 2;; # green\n-\t\tinfo)\n-\t\t\ttput setaf 6;; # cyan\n-\t\t*)\n-\t\t\ttest -n \"$quiet\" && return;;\n-\t\tesac\n-\t\tshift\n-\t\tprintf \"%s\" \"$*\"\n-\t\ttput sgr0\n-\t\techo\n-\t\t)\n-\t}\n-else\n-\tsay_color() {\n-\t\ttest -z \"$1\" && test -n \"$quiet\" && return\n-\t\tshift\n-\t\tprintf \"%s\\n\" \"$*\"\n-\t}\n-fi\n-\n if test -z \"$TEST_NO_CREATE_REPO\"\n then\n \ttest_create_repo \"$TRASH_DIRECTORY\"\n-- \n2.4.3\n"},{"id":"264086","messageId":"1434575481-24604-3-git-send-email-rhansen@bbn.com","threadId":"39662","inReplyTo":"1434575481-24604-1-git-send-email-rhansen@bbn.com","subject":"[PATCH v2 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo","fromName":"Richard Hansen","fromEmail":"rhansen@bbn.com","sentAt":"2015-06-17T21:11:21Z","receivedAt":"2015-06-17T21:11:21Z","isPatch":true,"sender":{"key":"rhansen@rhansen.org","avatar":null},"body":"If tput needs ~/.terminfo for the current $TERM, then tput will\nsucceed before HOME is changed to $TRASH_DIRECTORY (causing color to\nbe set to 't') but fail afterward.\n\nOne possible way to fix this is to treat HOME like TERM: back up the\noriginal value and temporarily restore it before say_color() runs\ntput.\n\nInstead, pre-compute and save the color control sequences before\nchanging either TERM or HOME.  Use the saved control sequences in\nsay_color() rather than call tput each time.  This avoids the need to\nback up and restore the TERM and HOME variables, and it avoids the\noverhead of a subshell and two invocations of tput per call to\nsay_color().\n\nSigned-off-by: Richard Hansen <rhansen@bbn.com>\n---\n t/test-lib.sh | 57 ++++++++++++++++++++++++++++-----------------------------\n 1 file changed, 28 insertions(+), 29 deletions(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 57212ec..cea6cda 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -15,9 +15,6 @@\n # You should have received a copy of the GNU General Public License\n # along with this program.  If not, see http://www.gnu.org/licenses/ .\n \n-# Keep the original TERM for say_color\n-ORIGINAL_TERM=$TERM\n-\n # Test the binaries we have just built.  The tests are kept in\n # t/ subdirectory and are run in 'trash directory' subdirectory.\n if test -z \"$TEST_DIRECTORY\"\n@@ -68,12 +65,12 @@ done,*)\n esac\n \n # For repeatability, reset the environment to known value.\n+# TERM is sanitized below, after saving color control sequences.\n LANG=C\n LC_ALL=C\n PAGER=cat\n TZ=UTC\n-TERM=dumb\n-export LANG LC_ALL PAGER TERM TZ\n+export LANG LC_ALL PAGER TZ\n EDITOR=:\n # A call to \"unset\" with no arguments causes at least Solaris 10\n # /usr/xpg4/bin/sh and /bin/ksh to bail out.  So keep the unsets\n@@ -181,9 +178,7 @@ export _x05 _x40 _z40 LF u200c\n # This test checks if command xyzzy does the right thing...\n # '\n # . ./test-lib.sh\n-test \"x$ORIGINAL_TERM\" != \"xdumb\" && (\n-\t\tTERM=$ORIGINAL_TERM &&\n-\t\texport TERM &&\n+test \"x$TERM\" != \"xdumb\" && (\n \t\ttest -t 1 &&\n \t\ttput bold >/dev/null 2>&1 &&\n \t\ttput setaf 1 >/dev/null 2>&1 &&\n@@ -263,29 +258,30 @@ fi\n \n if test -n \"$color\"\n then\n+\t# Save the color control sequences now rather than run tput\n+\t# each time say_color() is called.  This is done for two\n+\t# reasons:\n+\t#   * TERM will be changed to dumb\n+\t#   * HOME will be changed to a temporary directory and tput\n+\t#     might need to read ~/.terminfo from the original HOME\n+\t#     directory to get the control sequences\n+\t# Note:  This approach assumes the control sequences don't end\n+\t# in a newline for any terminal of interest (command\n+\t# substitutions strip trailing newlines).  Given that most\n+\t# (all?) terminals in common use are related to ECMA-48, this\n+\t# shouldn't be a problem.\n+\tsay_color_error=$(tput bold; tput setaf 1) # bold red\n+\tsay_color_skip=$(tput setaf 4) # blue\n+\tsay_color_warn=$(tput setaf 3) # brown/yellow\n+\tsay_color_pass=$(tput setaf 2) # green\n+\tsay_color_info=$(tput setaf 6) # cyan\n+\tsay_color_reset=$(tput sgr0)\n+\tsay_color_=\"\" # no formatting for normal text\n \tsay_color () {\n-\t\t(\n-\t\tTERM=$ORIGINAL_TERM\n-\t\texport TERM\n-\t\tcase \"$1\" in\n-\t\terror)\n-\t\t\ttput bold; tput setaf 1;; # bold red\n-\t\tskip)\n-\t\t\ttput setaf 4;; # blue\n-\t\twarn)\n-\t\t\ttput setaf 3;; # brown/yellow\n-\t\tpass)\n-\t\t\ttput setaf 2;; # green\n-\t\tinfo)\n-\t\t\ttput setaf 6;; # cyan\n-\t\t*)\n-\t\t\ttest -n \"$quiet\" && return;;\n-\t\tesac\n+\t\ttest -z \"$1\" && test -n \"$quiet\" && return\n+\t\teval \"say_color_color=\\$say_color_$1\"\n \t\tshift\n-\t\tprintf \"%s\" \"$*\"\n-\t\ttput sgr0\n-\t\techo\n-\t\t)\n+\t\tprintf \"%s\\\\n\" \"$say_color_color$*$say_color_reset\"\n \t}\n else\n \tsay_color() {\n@@ -295,6 +291,9 @@ else\n \t}\n fi\n \n+TERM=dumb\n+export TERM\n+\n error () {\n \tsay_color error \"error: $*\"\n \tGIT_EXIT_OK=t\n-- \n2.4.3\n"},{"id":"264094","messageId":"20150617221331.GA26069@peff.net","threadId":"39662","inReplyTo":"1434575481-24604-3-git-send-email-rhansen@bbn.com","subject":"Re: [PATCH v2 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-17T22:13:31Z","receivedAt":"2015-06-17T22:13:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 17, 2015 at 05:11:21PM -0400, Richard Hansen wrote:\n\n> +\t\ttest -z \"$1\" && test -n \"$quiet\" && return\n> +\t\teval \"say_color_color=\\$say_color_$1\"\n\nThanks, this looks much simpler.\n\nIn the non-quiet case, you will eval $say_color_, even though we know it\nto be bogus. I guess we need to make sure say_color_color is blank,\nthough. The alternative would be:\n\n  if test -z \"$1\"; then\n    test -n \"$quiet\" && return\n    say_color_color=\n  else\n    eval \"say_color_color=\\$say_color_$1\"\n  fi\n\nI dunno if that makes the intent more clear or not. I am OK with it\neither way.\n\n-Peff\n"},{"id":"264095","messageId":"xmqq7fr255ze.fsf@gitster.dls.corp.google.com","threadId":"39662","inReplyTo":"20150617221331.GA26069@peff.net","subject":"Re: [PATCH v2 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-06-17T22:23:49Z","receivedAt":"2015-06-17T22:23:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jun 17, 2015 at 05:11:21PM -0400, Richard Hansen wrote:\n>\n>> +\t\ttest -z \"$1\" && test -n \"$quiet\" && return\n>> +\t\teval \"say_color_color=\\$say_color_$1\"\n>\n> Thanks, this looks much simpler.\n>\n> In the non-quiet case, you will eval $say_color_, even though we know it\n> to be bogus.\n\nYeah, but there is this gem in this patch:\n\n+\t...\n+\tsay_color_info=$(tput setaf 6) # cyan\n+\tsay_color_reset=$(tput sgr0)\n+\tsay_color_=\"\" # no formatting for normal text\n\nIn other words, the patch handles these two in the same mechanism:\n\n\tsay_color error \"this is my error message\"\n\tsay_color \"\" \"ok this is just a regular message\"\n\nand treating an empy string just one of the supported \"colors\",\ni.e. \"error\", \"skip\", \"warn\", \"pass\", \"info\" \"reset\" and \"\" are the\ncolors.\n\n> I guess we need to make sure say_color_color is blank,\n> though. The alternative would be:\n>\n>   if test -z \"$1\"; then\n>     test -n \"$quiet\" && return\n>     say_color_color=\n>   else\n>     eval \"say_color_color=\\$say_color_$1\"\n>   fi\n>\n> I dunno if that makes the intent more clear or not. I am OK with it\n> either way.\n\nI am OK with it either way, too.\n"},{"id":"264096","messageId":"20150617222602.GA31735@peff.net","threadId":"39662","inReplyTo":"xmqq7fr255ze.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v2 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-06-17T22:26:02Z","receivedAt":"2015-06-17T22:26:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 17, 2015 at 03:23:49PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > On Wed, Jun 17, 2015 at 05:11:21PM -0400, Richard Hansen wrote:\n> >\n> >> +\t\ttest -z \"$1\" && test -n \"$quiet\" && return\n> >> +\t\teval \"say_color_color=\\$say_color_$1\"\n> >\n> > Thanks, this looks much simpler.\n> >\n> > In the non-quiet case, you will eval $say_color_, even though we know it\n> > to be bogus.\n> \n> Yeah, but there is this gem in this patch:\n> \n> +\t...\n> +\tsay_color_info=$(tput setaf 6) # cyan\n> +\tsay_color_reset=$(tput sgr0)\n> +\tsay_color_=\"\" # no formatting for normal text\n\nOh, sorry, I was so focused on the later part that I totally missed\nthat. That is rather elegant, and nicer than what I wrote.\n\n-Peff\n"}]}