Strip ANSI escape codes when not writing to a terminal

[fredrikfoss]

ANSI color escape codes are stripped from job output when standard output is redirected to a pipe or file, unless CLICOLOR_FORCE is set to a value other than "0", or TERM is set to "dumb". This matches ninja's behavior.

See #20 (comment). I'll just open the pull request before I forget.

[eli-schwartz]

I'm highly opposed to this in general but I'm also highly opposed to this in the specific implementation details. Please do NOT use dumb generic envvars like "CLICOLOR_FORCE" as this (dumb) local convention is contaminatively contagious and affects ALL applications recursively throughout the process tree.

That includes ls (unbelievable but true). Heaven help you if any program used in your build happens to be a badly written shell script that parses the output of ls -- which nobody should do, but many people nonetheless manifestly do regardless.

The idea of "one variable to rule them all" is dumb and ninja only supports it because the variable is the pet hobby horse of the primary ninja developer. As a matter of basic safety hygiene one must never actually use this functionality, which is another way of saying: ninja does NOT support disabling stripping of color.

If one actually wished to add such a feature to samurai, it must use a correctly namespaced environment variable so that people can guarantee it affects what it's supposed to affect (and nothing else). But you might as well just control this via a cli argument and not mess with environment variables at all.

[fredrikfoss]

I hadn't heard about CLICOLOR_FORCE before reading the ninja docs either. I'm not opposed to using a flag instead; in fact I would much prefer that. Figured I'd wait until hearing if this feature is wanted or not.

I don't really have any strong opinions on this, although it is quite nice being able to run samu from the terminal and get colors, but then have them stripped when called from inside my editor without having to manually sed them out or set some non-samu option first. It's what I expect when running a terminal program in general.

Anyway, you described the idea as extremely bad in #20. I can image a few small cons, but not anything that extreme (assuming a way to disable/enable stripping)? But yeah I can see it being a bit dirty.

[michaelforney]

@eli-schwartz I think you are being a bit extreme here, could you please be a bit more respectful?

Personally, I don't use colors much in my terminal, so I'm not really aware of the history/politics of color-controlling environment variables (it seems there is also FORCE_COLOR and NO_COLOR, and https://bixense.com/clicolors/ recommends that). But, even assuming there is some agreed-upon way to indicate that the user wants the application to produce colored output, I don't think the filtering belongs in samurai.

samurai itself doesn't produce any colored output; it just writes non-colored status messages and the output of other tools. My understanding is that if the tools that are actually producing colored output supported this variable, samurai wouldn't have to do anything. In your normal terminal session, you can indicate that you want colors, and something that is saving to a log can set it based on whether it wants colors in the log.

I could see an argument that it should be more automatic based on whether you're connected to a terminal, but this could easily be solved by setting the variable when connected to a TTY. Ideally, outside of samurai, like a small wrapper script

test -t 1 && export FORCE_COLOR=1
exec samu "$@"

Basically, my view is that post-processing command output is the wrong place to fix this problem; it's an attempt to fix-up color control of other tools. Most tools probably control color output based on isatty(1) without any extra flags or environment variables. So for this to do anything, you've already had to already tell those tools "I want color output", but now are saying "nevermind, I changed my mind" after they already wrote their output.

That said, I think your implementation of escape sequence stripping is pretty good. I'm open to putting it in a branch if people want this behavior.

[eli-schwartz]

@eli-schwartz I think you are being a bit extreme here, could you please be a bit more respectful?

Personally, I don't use colors much in my terminal, so I'm not really aware of the history/politics of color-controlling environment variables (it seems there is also FORCE_COLOR and NO_COLOR, and https://bixense.com/clicolors/ recommends that). But, even assuming there is some agreed-upon way to indicate that the user wants the application to produce colored output, I don't think the filtering belongs in samurai.

Sorry, I am a bit passionate about the fact that the author of that website also implemented it in his projects and then goes around filing mass bugs against random projects saying "it's a standard, please follow it".

[sertonix]

I don't think the filtering belongs in samurai.

I think that is the most important part. Tools like less(1) seem to me like a better place to mess with ansi escape sequences.

[fredrikfoss]

That said, I think your implementation of escape sequence stripping is pretty good. I'm open to putting it in a branch if people want this behavior.

Thanks, but in that case I think I would personally just stick to using the default version. I'm assuming it's the one package managers will ship.

By the way, I was pretty conservative in the implementation as it is based on ninja's. It can be hardened quite a bit more. For example, right now it ignores OSC sequences so hyperlinks won't be stripped (although I'm not sure any compilers emit those?) It shouldn't require much change, I have a working draft.