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

From: Jeff King <peff@peff.net>
Date: Jun 17, 2015, 19:43 UTC
Message-ID: <20150617194315.GE25304@peff.net>
In-Reply-To: <1434567986-23552-3-git-send-email-rhansen@bbn.com>

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