Bug 230101

Summary: [PATCH] Enable ls colors when COLORTERM is defined
Product: Base System Reporter: D Green <dfrg>
Component: binAssignee: Kyle Evans <kevans>
Status: Closed FIXED    
Severity: Affects Some People CC: cem, kevans, yuripv
Priority: --- Keywords: patch
Version: CURRENTFlags: kevans: mfc-stable11+
kevans: mfc-stable10-
Hardware: Any   
OS: Any   
Attachments:
Description Flags
Enable colors when COLORTERM defined none

Description D Green 2018-07-28 09:51:07 UTC
Created attachment 195523 [details]
Enable colors when COLORTERM defined

Color output is currently supported for ls via the CLICOLOR environment variable which is unique to both the FreeBSD project and that particular binary.

The defacto standard is COLORTERM and is set by all terminal emulators derived from rxvt or those based on vte (the majority).

Attached patch enables color output for interactive shells started within the above terminal emulators. All others, in particular system console and non-interactive shells, remain unchanged.

It's my first patch so apologies if the format isn't exactly as required.
Comment 1 Conrad Meyer freebsd_committer freebsd_triage 2018-07-28 20:05:19 UTC
Patch LGTM.
Comment 2 Yuri Pankov 2018-07-28 20:14:32 UTC
I think there's a difference between CLICOLOR and COLORTERM in that the former *needs* to be set by the user, and the latter is likely to be set by the software (ie, terminal emulator) itself, so this change will mean that we are forcing colored ls output on those whose terminal emulator did set COLORTERM on its own.
Comment 3 Conrad Meyer freebsd_committer freebsd_triage 2018-07-28 20:20:19 UTC
(In reply to Yuri Pankov from comment #2)
I guess there is that distinction, yes.  Perhaps it is possible for users to configure their terminal to disable setting the color variable, or explicitly unset it in .shellrc.  Nevertheless, it doesn't really make sense for ls to have its own variable that nothing else uses.
Comment 4 D Green 2018-07-28 20:47:42 UTC
(In reply to Yuri Pankov from comment #2)

Yes, colors would be automatic for any terminal setting COLORTERM wheres CLICOLOR requires explicit configuration. The existing check that TERM is suitable is unchanged.

I can't think of a non-contrived reason for a user of a color terminal not wanting that behavior but if so then unsetting COLORTERM via .shellrc is trivial and already necessary as many other binaries external to the project enable colors automatically when it is defined.

As with Conrad, I think it's better to follow the defacto standard rather than use a unique variable (or two if you count CLICOLOR_FORCE). Currently you either know to set the non-portable CLICOLOR or forgo color functionality.
Comment 5 commit-hook freebsd_committer freebsd_triage 2018-08-08 21:51:53 UTC
A commit references this bug:

Author: kevans
Date: Wed Aug  8 21:51:20 UTC 2018
New revision: 337506
URL: https://svnweb.freebsd.org/changeset/base/337506

Log:
  ls(1): Enable colors with COLORTERM is set in the environment

  COLORTERM is the de facto standard, while CLICOLOR is generally specific to
  FreeBSD and ls(1).

  PR:		230101
  Submitted by:	D Green <dfrg@xsmail.com> (with manpage additions by myself)
  Reviewed by:	cem ("LGTM" in PR; pre-manpage changes)
  MFC after:	1 week

Changes:
  head/bin/ls/ls.1
  head/bin/ls/ls.c
Comment 6 Kyle Evans freebsd_committer freebsd_triage 2018-08-08 21:55:06 UTC
Committed, thanks!
Comment 7 commit-hook freebsd_committer freebsd_triage 2018-08-15 01:29:36 UTC
A commit references this bug:

Author: kevans
Date: Wed Aug 15 01:29:02 UTC 2018
New revision: 337826
URL: https://svnweb.freebsd.org/changeset/base/337826

Log:
  MFC r337506: ls(1): Enable colors with COLORTERM is set in the environment

  COLORTERM is the de facto standard, while CLICOLOR is generally specific to
  FreeBSD and ls(1).

  PR:		230101

Changes:
_U  stable/11/
  stable/11/bin/ls/ls.1
  stable/11/bin/ls/ls.c