threads / patch / 39662

patch, 2 partsredo fix for test-lib.sh color support

Subject: [PATCH 0/2] redo fix for test-lib.sh color support

## tl;dr

13 messages between Jun 17, 2015 and Jun 17, 2015. Diffs are folded; open one to read it.

replies: 12people: 3as markdown or json

Richard Hansen· Jun 17, 2015, 19:06 UTC · lore

Commit 102fc80d fixed a bug where tput was failing because it needed to read ~/.terminfo after HOME was changed. However, that commit is buggy, and it unnecessarily disables color support when tput needs to read from ~/.terminfo.

This series does two things:

  * revert the buggy fix
  * fix it properly, I hope :)

Richard Hansen (2):
  Revert "test-lib.sh: do tests for color support after changing HOME"
  test-lib.sh: fix color support when tput needs ~/.terminfo

 t/test-lib.sh | 103 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 51 insertions(+), 52 deletions(-)

-- 
2.4.3

Richard Hansen· Jun 17, 2015, 19:06 UTC · re: Richard Hansen · lore

[PATCH 1/2] Revert "test-lib.sh: do tests for color support after changing HOME"

This reverts commit 102fc80d32094ad6598b17ab9d607516ee8edc4a.

There are two issues with that commit:

  * It is buggy.  In pseudocode, it is doing:

       color is set || TERM != dumb && color works && color=t

    when it should be doing:

       color is set || { TERM != dumb && color works && color=t }

  * It unnecessarily disables color when tput needs to read
    ~/.terminfo to get the control sequences.
---
 t/test-lib.sh | 90 ++++++++++++++++++++++++++++-------------------------------
 1 file changed, 43 insertions(+), 47 deletions(-)

Show changes to t/test-lib.sh +43 −47

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 39da9c2..57212ec 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -181,8 +181,16 @@ export _x05 _x40 _z40 LF u200c
 # This test checks if command xyzzy does the right thing...
 # '
 # . ./test-lib.sh
+test "x$ORIGINAL_TERM" != "xdumb" && (
+		TERM=$ORIGINAL_TERM &&
+		export TERM &&
+		test -t 1 &&
+		tput bold >/dev/null 2>&1 &&
+		tput setaf 1 >/dev/null 2>&1 &&
+		tput sgr0 >/dev/null 2>&1
+	) &&
+	color=t
 
-unset color
 while test "$#" -ne 0
 do
 	case "$1" in
@@ -253,6 +261,40 @@ then
 	verbose=t
 fi
 
+if test -n "$color"
+then
+	say_color () {
+		(
+		TERM=$ORIGINAL_TERM
+		export TERM
+		case "$1" in
+		error)
+			tput bold; tput setaf 1;; # bold red
+		skip)
+			tput setaf 4;; # blue
+		warn)
+			tput setaf 3;; # brown/yellow
+		pass)
+			tput setaf 2;; # green
+		info)
+			tput setaf 6;; # cyan
+		*)
+			test -n "$quiet" && return;;
+		esac
+		shift
+		printf "%s" "$*"
+		tput sgr0
+		echo
+		)
+	}
+else
+	say_color() {
+		test -z "$1" && test -n "$quiet" && return
+		shift
+		printf "%s\n" "$*"
+	}
+fi
+
 error () {
 	say_color error "error: $*"
 	GIT_EXIT_OK=t
@@ -829,52 +871,6 @@ HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"
 export HOME GNUPGHOME
 
-# run the tput tests *after* changing HOME (in case ncurses needs
-# ~/.terminfo for $TERM)
-test -n "${color+set}" || test "x$ORIGINAL_TERM" != "xdumb" && (
-		TERM=$ORIGINAL_TERM &&
-		export TERM &&
-		test -t 1 &&
-		tput bold >/dev/null 2>&1 &&
-		tput setaf 1 >/dev/null 2>&1 &&
-		tput sgr0 >/dev/null 2>&1
-	) &&
-	color=t
-
-if test -n "$color"
-then
-	say_color () {
-		(
-		TERM=$ORIGINAL_TERM
-		export TERM
-		case "$1" in
-		error)
-			tput bold; tput setaf 1;; # bold red
-		skip)
-			tput setaf 4;; # blue
-		warn)
-			tput setaf 3;; # brown/yellow
-		pass)
-			tput setaf 2;; # green
-		info)
-			tput setaf 6;; # cyan
-		*)
-			test -n "$quiet" && return;;
-		esac
-		shift
-		printf "%s" "$*"
-		tput sgr0
-		echo
-		)
-	}
-else
-	say_color() {
-		test -z "$1" && test -n "$quiet" && return
-		shift
-		printf "%s\n" "$*"
-	}
-fi
-
 if test -z "$TEST_NO_CREATE_REPO"
 then
 	test_create_repo "$TRASH_DIRECTORY"

-- 
2.4.3

Richard Hansen· Jun 17, 2015, 19:06 UTC · re: Richard Hansen · lore

[PATCH 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo

If tput needs ~/.terminfo for the current $TERM, then tput will succeed before HOME is changed to $TRASH_DIRECTORY (causing color to be set to 't') but fail afterward.

One possible way to fix this is to treat HOME like TERM: back up the original value and temporarily restore it before say_color() runs tput.

Instead, pre-compute and save the color control sequences before changing either TERM or HOME. Use the saved control sequences in say_color() rather than call tput each time. This avoids the need to back up and restore the TERM and HOME variables, and it avoids the overhead of a subshell and two invocations of tput per call to say_color().

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
 t/test-lib.sh | 53 ++++++++++++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 25 deletions(-)

Show changes to t/test-lib.sh +28 −25

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 57212ec..4a59bfb 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -15,9 +15,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see http://www.gnu.org/licenses/ .
 
-# Keep the original TERM for say_color
-ORIGINAL_TERM=$TERM
-
 # Test the binaries we have just built.  The tests are kept in
 # t/ subdirectory and are run in 'trash directory' subdirectory.
 if test -z "$TEST_DIRECTORY"
@@ -68,12 +65,12 @@ done,*)
 esac
 
 # For repeatability, reset the environment to known value.
+# TERM is sanitized below, after saving color control sequences.
 LANG=C
 LC_ALL=C
 PAGER=cat
 TZ=UTC
-TERM=dumb
-export LANG LC_ALL PAGER TERM TZ
+export LANG LC_ALL PAGER TZ
 EDITOR=:
 # A call to "unset" with no arguments causes at least Solaris 10
 # /usr/xpg4/bin/sh and /bin/ksh to bail out.  So keep the unsets
@@ -181,9 +178,7 @@ export _x05 _x40 _z40 LF u200c
 # This test checks if command xyzzy does the right thing...
 # '
 # . ./test-lib.sh
-test "x$ORIGINAL_TERM" != "xdumb" && (
-		TERM=$ORIGINAL_TERM &&
-		export TERM &&
+test "x$TERM" != "xdumb" && (
 		test -t 1 &&
 		tput bold >/dev/null 2>&1 &&
 		tput setaf 1 >/dev/null 2>&1 &&
@@ -263,29 +258,34 @@ fi
 
 if test -n "$color"
 then
+	# Save the color control sequences now rather than run tput
+	# each time say_color() is called.  This is done for two
+	# reasons:
+	#   * TERM will be changed to dumb
+	#   * HOME will be changed to a temporary directory and tput
+	#     might need to read ~/.terminfo from the original HOME
+	#     directory to get the control sequences
+	# Note:  This approach assumes the control sequences don't end
+	# in a newline for any terminal of interest (command
+	# substitutions strip trailing newlines).  Given that most
+	# (all?) terminals in common use are related to ECMA-48, this
+	# shouldn't be a problem.
+	say_color_error=$(tput bold; tput setaf 1) # bold red
+	say_color_skip=$(tput setaf 4) # blue
+	say_color_warn=$(tput setaf 3) # brown/yellow
+	say_color_pass=$(tput setaf 2) # green
+	say_color_info=$(tput setaf 6) # cyan
+	say_color_sgr0=$(tput sgr0)
 	say_color () {
-		(
-		TERM=$ORIGINAL_TERM
-		export TERM
+		say_color_color=
 		case "$1" in
-		error)
-			tput bold; tput setaf 1;; # bold red
-		skip)
-			tput setaf 4;; # blue
-		warn)
-			tput setaf 3;; # brown/yellow
-		pass)
-			tput setaf 2;; # green
-		info)
-			tput setaf 6;; # cyan
+		error|skip|warn|pass|info)
+			eval "say_color_color=\$say_color_$1";;
 		*)
 			test -n "$quiet" && return;;
 		esac
 		shift
-		printf "%s" "$*"
-		tput sgr0
-		echo
-		)
+		printf "%s\\n" "$say_color_color$*$say_color_sgr0"
 	}
 else
 	say_color() {
@@ -295,6 +295,9 @@ else
 	}
 fi
 
+TERM=dumb
+export TERM
+
 error () {
 	say_color error "error: $*"
 	GIT_EXIT_OK=t

-- 
2.4.3

Jeff King· Jun 17, 2015, 19:43 UTC · re: Richard Hansen · lore

Re: [PATCH 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo

On Wed, Jun 17, 2015 at 03:06:26PM -0400, Richard Hansen wrote:

Show 16 quoted lines

> If tput needs ~/.terminfo for the current $TERM, then tput will
> succeed before HOME is changed to $TRASH_DIRECTORY (causing color to
> be set to 't') but fail afterward.
> 
> One possible way to fix this is to treat HOME like TERM: back up the
> original value and temporarily restore it before say_color() runs
> tput.
> 
> Instead, pre-compute and save the color control sequences before
> changing either TERM or HOME.  Use the saved control sequences in
> say_color() rather than call tput each time.  This avoids the need to
> back up and restore the TERM and HOME variables, and it avoids the
> overhead of a subshell and two invocations of tput per call to
> say_color().
> 
> Signed-off-by: Richard Hansen <rhansen@bbn.com>

Nice, I like it.

Show 12 quoted lines

> +	# Save the color control sequences now rather than run tput
> +	# each time say_color() is called.  This is done for two
> +	# reasons:
> +	#   * TERM will be changed to dumb
> +	#   * HOME will be changed to a temporary directory and tput
> +	#     might need to read ~/.terminfo from the original HOME
> +	#     directory to get the control sequences
> +	# Note:  This approach assumes the control sequences don't end
> +	# in a newline for any terminal of interest (command
> +	# substitutions strip trailing newlines).  Given that most
> +	# (all?) terminals in common use are related to ECMA-48, this
> +	# shouldn't be a problem.

Yeah, that was my first thought, but I agree it probably isn't going to be a big deal in practice.

Show 11 quoted lines

> +	say_color_error=$(tput bold; tput setaf 1) # bold red
> +	say_color_skip=$(tput setaf 4) # blue
> +	say_color_warn=$(tput setaf 3) # brown/yellow
> +	say_color_pass=$(tput setaf 2) # green
> +	say_color_info=$(tput setaf 6) # cyan
> +	say_color_sgr0=$(tput sgr0)
> [...]
> +		error|skip|warn|pass|info)
> +			eval "say_color_color=\$say_color_$1";;
>  		*)
>  			test -n "$quiet" && return;;

I think you could dispense with this case statement entirely and do:

  eval "say_color_color=\$say_color_$1"
  if test -z "$say_color_color"; then
          test -n "$quiet" && return
  fi

I guess that is making the assumption that all colors have non-zero sizes, but that seems reasonable. I do not mind it so much as you have it, but it does mean adding a new field needs to update two spots.

-Peff

Richard Hansen· Jun 17, 2015, 19:55 UTC · re: Jeff King · lore

Re: [PATCH 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo

On 2015-06-17 15:43, Jeff King wrote:

Show 22 quoted lines

> On Wed, Jun 17, 2015 at 03:06:26PM -0400, Richard Hansen wrote:
>> +	say_color_error=$(tput bold; tput setaf 1) # bold red
>> +	say_color_skip=$(tput setaf 4) # blue
>> +	say_color_warn=$(tput setaf 3) # brown/yellow
>> +	say_color_pass=$(tput setaf 2) # green
>> +	say_color_info=$(tput setaf 6) # cyan
>> +	say_color_sgr0=$(tput sgr0)
>> [...]
>> +		error|skip|warn|pass|info)
>> +			eval "say_color_color=\$say_color_$1";;
>>  		*)
>>  			test -n "$quiet" && return;;
> 
> I think you could dispense with this case statement entirely and do:
> 
>   eval "say_color_color=\$say_color_$1"
>   if test -z "$say_color_color"; then
>           test -n "$quiet" && return
>   fi
> 
> I guess that is making the assumption that all colors have non-zero
> sizes, but that seems reasonable.

We could test if the variable is set first (test -n "${foo+set}"), at the cost of a bit more complexity.

> I do not mind it so much as you have
> it, but it does mean adding a new field needs to update two spots.

I also don't like the duplicate list of color types, and I considered
doing something similar to what you suggested, but I decided against it.
 I'm a bit worried about bizarre syntax errors or code execution if
say_color() is used improperly.  ('eval' with uncontrolled variables
makes me nervous.)

Thanks for reviewing, Richard

Junio C Hamano· Jun 17, 2015, 20:15 UTC · re: Richard Hansen · lore

Re: [PATCH 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo

Richard Hansen <rhansen@bbn.com> writes:

Show 11 quoted lines

> We could test if the variable is set first (test -n "${foo+set}"), at
> the cost of a bit more complexity.
>
>> I do not mind it so much as you have
>> it, but it does mean adding a new field needs to update two spots.
>
> I also don't like the duplicate list of color types, and I considered
> doing something similar to what you suggested, but I decided against it.
> I'm a bit worried about bizarre syntax errors or code execution if
> say_color() is used improperly.  ('eval' with uncontrolled variables
> makes me nervous.)

I originally had the same reaction to your use of `eval` (with or without being guarded by the case to limit to known 5 ones). But the uncontrolled-ness of this use of eval is to the same degree of uncontrolled-ness of any test_expect_{success,failure} scriptlet, so...

I like this "save to variables instead of using tput" approach very much either way. Well done.

Thanks.

Richard Hansen· Jun 17, 2015, 21:11 UTC · re: Junio C Hamano · lore

[PATCH v2 0/2] redo fix for test-lib.sh color support

Changes from v1:
  * Eliminate the case statement and assume the user passed a sane
    value for $1.
  * Use the same test as the non-colorized version of say_color() when
    determining whether to suppress the output:  assume that a message
    can only be suppresed if $1 is the empty string.  This avoids the
    need to test whether the variable say_color_$1 is set.
  * Rename say_color_sgr0 to say_color_reset.
  * Add a new variable say_color_ (set to the empty string) as a way
    of documenting that $1 is expected to be the empty string for
    normal text.

Richard Hansen (2):
  Revert "test-lib.sh: do tests for color support after changing HOME"
  test-lib.sh: fix color support when tput needs ~/.terminfo

 t/test-lib.sh | 99 ++++++++++++++++++++++++++++-------------------------------
 1 file changed, 47 insertions(+), 52 deletions(-)

-- 
2.4.3

Richard Hansen· Jun 17, 2015, 21:11 UTC · re: Richard Hansen · lore

[PATCH v2 1/2] Revert "test-lib.sh: do tests for color support after changing HOME"

This reverts commit 102fc80d32094ad6598b17ab9d607516ee8edc4a.

There are two issues with that commit:

  * It is buggy.  In pseudocode, it is doing:

       color is set || TERM != dumb && color works && color=t

    when it should be doing:

       color is set || { TERM != dumb && color works && color=t }

  * It unnecessarily disables color when tput needs to read
    ~/.terminfo to get the control sequences.
---
 t/test-lib.sh | 90 ++++++++++++++++++++++++++++-------------------------------
 1 file changed, 43 insertions(+), 47 deletions(-)

Show changes to t/test-lib.sh +43 −47

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 39da9c2..57212ec 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -181,8 +181,16 @@ export _x05 _x40 _z40 LF u200c
 # This test checks if command xyzzy does the right thing...
 # '
 # . ./test-lib.sh
+test "x$ORIGINAL_TERM" != "xdumb" && (
+		TERM=$ORIGINAL_TERM &&
+		export TERM &&
+		test -t 1 &&
+		tput bold >/dev/null 2>&1 &&
+		tput setaf 1 >/dev/null 2>&1 &&
+		tput sgr0 >/dev/null 2>&1
+	) &&
+	color=t
 
-unset color
 while test "$#" -ne 0
 do
 	case "$1" in
@@ -253,6 +261,40 @@ then
 	verbose=t
 fi
 
+if test -n "$color"
+then
+	say_color () {
+		(
+		TERM=$ORIGINAL_TERM
+		export TERM
+		case "$1" in
+		error)
+			tput bold; tput setaf 1;; # bold red
+		skip)
+			tput setaf 4;; # blue
+		warn)
+			tput setaf 3;; # brown/yellow
+		pass)
+			tput setaf 2;; # green
+		info)
+			tput setaf 6;; # cyan
+		*)
+			test -n "$quiet" && return;;
+		esac
+		shift
+		printf "%s" "$*"
+		tput sgr0
+		echo
+		)
+	}
+else
+	say_color() {
+		test -z "$1" && test -n "$quiet" && return
+		shift
+		printf "%s\n" "$*"
+	}
+fi
+
 error () {
 	say_color error "error: $*"
 	GIT_EXIT_OK=t
@@ -829,52 +871,6 @@ HOME="$TRASH_DIRECTORY"
 GNUPGHOME="$HOME/gnupg-home-not-used"
 export HOME GNUPGHOME
 
-# run the tput tests *after* changing HOME (in case ncurses needs
-# ~/.terminfo for $TERM)
-test -n "${color+set}" || test "x$ORIGINAL_TERM" != "xdumb" && (
-		TERM=$ORIGINAL_TERM &&
-		export TERM &&
-		test -t 1 &&
-		tput bold >/dev/null 2>&1 &&
-		tput setaf 1 >/dev/null 2>&1 &&
-		tput sgr0 >/dev/null 2>&1
-	) &&
-	color=t
-
-if test -n "$color"
-then
-	say_color () {
-		(
-		TERM=$ORIGINAL_TERM
-		export TERM
-		case "$1" in
-		error)
-			tput bold; tput setaf 1;; # bold red
-		skip)
-			tput setaf 4;; # blue
-		warn)
-			tput setaf 3;; # brown/yellow
-		pass)
-			tput setaf 2;; # green
-		info)
-			tput setaf 6;; # cyan
-		*)
-			test -n "$quiet" && return;;
-		esac
-		shift
-		printf "%s" "$*"
-		tput sgr0
-		echo
-		)
-	}
-else
-	say_color() {
-		test -z "$1" && test -n "$quiet" && return
-		shift
-		printf "%s\n" "$*"
-	}
-fi
-
 if test -z "$TEST_NO_CREATE_REPO"
 then
 	test_create_repo "$TRASH_DIRECTORY"

-- 
2.4.3

Richard Hansen· Jun 17, 2015, 21:11 UTC · re: Richard Hansen · lore

[PATCH v2 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo

If tput needs ~/.terminfo for the current $TERM, then tput will succeed before HOME is changed to $TRASH_DIRECTORY (causing color to be set to 't') but fail afterward.

One possible way to fix this is to treat HOME like TERM: back up the original value and temporarily restore it before say_color() runs tput.

Signed-off-by: Richard Hansen <rhansen@bbn.com>
---
 t/test-lib.sh | 57 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

Show changes to t/test-lib.sh +28 −29

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 57212ec..cea6cda 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -15,9 +15,6 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see http://www.gnu.org/licenses/ .
 
-# Keep the original TERM for say_color
-ORIGINAL_TERM=$TERM
-
 # Test the binaries we have just built.  The tests are kept in
 # t/ subdirectory and are run in 'trash directory' subdirectory.
 if test -z "$TEST_DIRECTORY"
@@ -68,12 +65,12 @@ done,*)
 esac
 
 # For repeatability, reset the environment to known value.
+# TERM is sanitized below, after saving color control sequences.
 LANG=C
 LC_ALL=C
 PAGER=cat
 TZ=UTC
-TERM=dumb
-export LANG LC_ALL PAGER TERM TZ
+export LANG LC_ALL PAGER TZ
 EDITOR=:
 # A call to "unset" with no arguments causes at least Solaris 10
 # /usr/xpg4/bin/sh and /bin/ksh to bail out.  So keep the unsets
@@ -181,9 +178,7 @@ export _x05 _x40 _z40 LF u200c
 # This test checks if command xyzzy does the right thing...
 # '
 # . ./test-lib.sh
-test "x$ORIGINAL_TERM" != "xdumb" && (
-		TERM=$ORIGINAL_TERM &&
-		export TERM &&
+test "x$TERM" != "xdumb" && (
 		test -t 1 &&
 		tput bold >/dev/null 2>&1 &&
 		tput setaf 1 >/dev/null 2>&1 &&
@@ -263,29 +258,30 @@ fi
 
 if test -n "$color"
 then
+	# Save the color control sequences now rather than run tput
+	# each time say_color() is called.  This is done for two
+	# reasons:
+	#   * TERM will be changed to dumb
+	#   * HOME will be changed to a temporary directory and tput
+	#     might need to read ~/.terminfo from the original HOME
+	#     directory to get the control sequences
+	# Note:  This approach assumes the control sequences don't end
+	# in a newline for any terminal of interest (command
+	# substitutions strip trailing newlines).  Given that most
+	# (all?) terminals in common use are related to ECMA-48, this
+	# shouldn't be a problem.
+	say_color_error=$(tput bold; tput setaf 1) # bold red
+	say_color_skip=$(tput setaf 4) # blue
+	say_color_warn=$(tput setaf 3) # brown/yellow
+	say_color_pass=$(tput setaf 2) # green
+	say_color_info=$(tput setaf 6) # cyan
+	say_color_reset=$(tput sgr0)
+	say_color_="" # no formatting for normal text
 	say_color () {
-		(
-		TERM=$ORIGINAL_TERM
-		export TERM
-		case "$1" in
-		error)
-			tput bold; tput setaf 1;; # bold red
-		skip)
-			tput setaf 4;; # blue
-		warn)
-			tput setaf 3;; # brown/yellow
-		pass)
-			tput setaf 2;; # green
-		info)
-			tput setaf 6;; # cyan
-		*)
-			test -n "$quiet" && return;;
-		esac
+		test -z "$1" && test -n "$quiet" && return
+		eval "say_color_color=\$say_color_$1"
 		shift
-		printf "%s" "$*"
-		tput sgr0
-		echo
-		)
+		printf "%s\\n" "$say_color_color$*$say_color_reset"
 	}
 else
 	say_color() {
@@ -295,6 +291,9 @@ else
 	}
 fi
 
+TERM=dumb
+export TERM
+
 error () {
 	say_color error "error: $*"
 	GIT_EXIT_OK=t

-- 
2.4.3

Jeff King· Jun 17, 2015, 22:13 UTC · re: Richard Hansen · lore

Re: [PATCH v2 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo

On Wed, Jun 17, 2015 at 05:11:21PM -0400, Richard Hansen wrote:

> +		test -z "$1" && test -n "$quiet" && return
> +		eval "say_color_color=\$say_color_$1"

Thanks, this looks much simpler.

In the non-quiet case, you will eval $say_color_, even though we know it to be bogus. I guess we need to make sure say_color_color is blank, though. The alternative would be:

  if test -z "$1"; then
    test -n "$quiet" && return
    say_color_color=
  else
    eval "say_color_color=\$say_color_$1"
  fi

I dunno if that makes the intent more clear or not. I am OK with it either way.

-Peff

Junio C Hamano· Jun 17, 2015, 22:23 UTC · re: Jeff King · lore

Re: [PATCH v2 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

> On Wed, Jun 17, 2015 at 05:11:21PM -0400, Richard Hansen wrote:
>
>> +		test -z "$1" && test -n "$quiet" && return
>> +		eval "say_color_color=\$say_color_$1"
>
> Thanks, this looks much simpler.
>
> In the non-quiet case, you will eval $say_color_, even though we know it
> to be bogus.

Yeah, but there is this gem in this patch:

+ ... + say_color_info=$(tput setaf 6) # cyan + say_color_reset=$(tput sgr0) + say_color_="" # no formatting for normal text

In other words, the patch handles these two in the same mechanism:

	say_color error "this is my error message"
	say_color "" "ok this is just a regular message"

and treating an empy string just one of the supported "colors", i.e. "error", "skip", "warn", "pass", "info" "reset" and "" are the colors.

Show 12 quoted lines

> I guess we need to make sure say_color_color is blank,
> though. The alternative would be:
>
>   if test -z "$1"; then
>     test -n "$quiet" && return
>     say_color_color=
>   else
>     eval "say_color_color=\$say_color_$1"
>   fi
>
> I dunno if that makes the intent more clear or not. I am OK with it
> either way.

I am OK with it either way, too.

Jeff King· Jun 17, 2015, 22:26 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo

On Wed, Jun 17, 2015 at 03:23:49PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > On Wed, Jun 17, 2015 at 05:11:21PM -0400, Richard Hansen wrote:
> >
> >> +		test -z "$1" && test -n "$quiet" && return
> >> +		eval "say_color_color=\$say_color_$1"
> >
> > Thanks, this looks much simpler.
> >
> > In the non-quiet case, you will eval $say_color_, even though we know it
> > to be bogus.
> 
> Yeah, but there is this gem in this patch:
> 
> +	...
> +	say_color_info=$(tput setaf 6) # cyan
> +	say_color_reset=$(tput sgr0)
> +	say_color_="" # no formatting for normal text

Oh, sorry, I was so focused on the later part that I totally missed that. That is rather elegant, and nicer than what I wrote.

-Peff

Jeff King· Jun 17, 2015, 20:25 UTC · re: Richard Hansen · lore

Re: [PATCH 2/2] test-lib.sh: fix color support when tput needs ~/.terminfo

On Wed, Jun 17, 2015 at 03:55:05PM -0400, Richard Hansen wrote:

Show 8 quoted lines

> > I do not mind it so much as you have
> > it, but it does mean adding a new field needs to update two spots.
> 
> I also don't like the duplicate list of color types, and I considered
> doing something similar to what you suggested, but I decided against it.
>  I'm a bit worried about bizarre syntax errors or code execution if
> say_color() is used improperly.  ('eval' with uncontrolled variables
> makes me nervous.)

As Junio pointed out, I think all bets are off in the test scripts. They are running tons of arbitrary code. :)

But for the record, I am fine with your patch as-is. Thanks for looking into it.

-Peff