Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

using color (or not) depending on -t 1 seems wrong #69

Closed
mstreuhofer opened this issue Dec 1, 2016 · 1 comment
Closed

using color (or not) depending on -t 1 seems wrong #69

mstreuhofer opened this issue Dec 1, 2016 · 1 comment

Comments

@mstreuhofer
Copy link
Collaborator

if [ "${NO_COLOR}" = "true" ] || [[ "${TERM:-}" != "xterm"* ]] || [ -t 1 ]; then

The line

if [ "${NO_COLOR}" = "true" ] || [[ "${TERM:-}" != "xterm"* ]] || [ -t 1 ]; then

seems to be wrong really in at least 2 different ways.

  1. It checks STDOUT to be connected to a terminal while logging happens on STDERR only.
  2. It turns OFF colors if STDOUT is actually connected to a terminal.

I'm guessing here but a quick check seems to suggest that nobody noticed until now because __b3bp_log always got called in a subshell which actually was not connected to a terminal. If the function would be just called - as suggested by shellcheck - the code would not work as expected.

Now I can fix that, but before that I wanted to ask if there is something about this code, or the decisions leading to this code, which I overlooked or I don't understand.

@kvz
Copy link
Owner

kvz commented Dec 2, 2016

I think you just caught a stupid mistake by me @mstreuhofer - so kudos to you 👍

kvz pushed a commit that referenced this issue Dec 9, 2016
turn off colors if NO_COLOR is "true" or TERM is not "xterm*" or STDERR
is not connected to a terminal, but ignore TERM and STDERR if NO_COLOR
is set to "false" explicitly.

Fixes #69
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants