ls(1): Add --color=when

[kevans]

Summary

--color may be set to one of: 'auto', 'always', and 'never'.

'auto' is the default behavior- output colors only if -G or COLORTERM are set, and only if stdout is a tty.

'always' is a new behavior- output colors (as strictly ANSI escape sequences, rather than lookup of termcap(5)) regardless of whether stdout is a tty or not.

'never' to turn off any environment variable and -G usage.

'always' was decidedly most useful if it ignored TERM/termcap(5), which may or may not have color availability but the user really wants colors. Presumably they know what they're doing, and the appropriate escape sequences will work when reading the output on a terminal that supports colors.

[danfe]

As Rod had pointed out, we should measure and document if enabling colors would cause a significant performance impact due to extra stat(2) calls.

bin/ls/extern.h

75 ↗

(On Diff #46757)

Why this extra line added?

bin/ls/ls.c

111 ↗

(On Diff #46757)

Superfluous blank line.

194 ↗

(On Diff #46757)

I don't really understand why this line was removed, but it's generally better not to mix style and functional changes together.

199 ↗

(On Diff #46757)

Shouldn't do_color_from_env() call be inside #ifdef COLORLS?

[kevans]

Address nits by @danfe - leftovers from previous iterations of this diff.

[kevans]

bin/ls/ls.c

199 ↗

(On Diff #46757)

It's not, in order to match previous behavior- it checked if the environment has requested colored ls(1), and warns if it's impossible because COLORLS bits have been compiled out.

[cem]

LGTM modulo a few nits below.

bin/ls/ls.1

214 ↗

(On Diff #46774)

Spurious ?

bin/ls/ls.c

107 ↗

(On Diff #46774)

GNU ls' --color is optional argument — if no argument is provided, it is treated the same as --color=always. Probably worth emulating for compatibility.

I'd also suggest hiding the long opt under #ifdef COLORLS given the getopt handling below is similarly ifdefed.

415 ↗

(On Diff #46774)

This should be errx, and print valid options.

430 ↗

(On Diff #46774)

Comment is now partially accurate. Kill it or update it, but don't leave it as-is :-). I have no preference either way.

437 ↗

(On Diff #46774)

style(9) nit: put the ending */ on its own line

439–440 ↗

(On Diff #46774)

I don't understand why always does not also consult TERM, if set. If absent, sure, force ANSI codes, but it is perfectly valid for someone to e.g. alias ls to ls --color=always in their shell rc and then use a variety of terms.

bin/ls/print.c

568 ↗

(On Diff #46774)

Kind of sucks we don't have defines for these anywhere. (I looked in /usr/include anyway.)

printf is gratuitous for some of these but the compiler is generally smart enough to replace printf with fputs(). Doesn't need to be changed.

570–578 ↗

(On Diff #46774)

I'd restructure the semi-colons to simplify

if (bold) print(1);
if (num0...) print(";3%d" ...);
if (num1...) print(";4%d" ...);

(If bold is set but no color, I don't think we want "1;m" — just "1m".)

[kevans]

bin/ls/ls.c

107 ↗

(On Diff #46774)

I goofed the ifdef in the case. I was aiming to be consistent with the -G case, where we'll accept it as a valid option whether or not we're doing a COLORLS build -- it just won't have any effect. I'm not sure how important this consistency is, though.

439–440 ↗

(On Diff #46774)

I was rather torn on this one, but I have concerns of end-user setting their TERM to one that does not support color and expecting ls --color=always > somefile to actually populate somefile with the colored output.

[cem]

bin/ls/ls.c

439–440 ↗

(On Diff #46774)

I don't believe this is a popular enough use-case to bother.

[kevans]

Address concerns raised by @cem:

• Don't bother with the long option if we're not COLORLS
• warn -> err, enumerate possible values, default to "always" if omitted
• Editorial changes
• Don't force ANSI unless TERM is unset or set to something unrecognized by termcap -- this is done by just setting another bit in colorflag, after colorflag's already otherwise processed.
• Move semicolons around to simplify printcolor_ansi

I might be missing one or two...

[cem]

LGTM, just a few minor nits:

bin/ls/extern.h

68–73 ↗

(On Diff #46795)

Just use a separate variable for the forceansi boolean. We don't need to pack the enum and boolean value together.

bin/ls/ls.c

438 ↗

(On Diff #46795)

Should we invoke tgetent if getenv(TERM) is NULL?

[kevans]

• Split the forceansi flag out; actually renamed to explicitansi, since it's all ANSI -- it's just a matter of whether it gets filtered through termcap(5) or not
• Don't invoke tgetent if TERM isn't set, to be more explicit in our intentions.

[kevans]

bin/ls/ls.c

438 ↗

(On Diff #46795)

It doesn't necessarily matter -- tgetent will do the right thing. OTOH, being explicit here is probably for the better.

[0mp]

The manpage part seems OK.

bin/ls/ls.1

218 ↗

(On Diff #46799)

I think that all those auto, always and never words should be stylized with the Cm macro, which is exactly for command modifiers.

226 ↗

(On Diff #46799)

The Tn macro is deprecated. Although we might remove them all at once in another commit.

[kevans]

bin/ls/ls.1

218 ↗

(On Diff #46799)

Will change pre-commit. Thanks!

226 ↗

(On Diff #46799)

Right- I think I'll keep this for now to keep this mention of ANSI consistent with others in the same file.