[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: the `--color' option of `ls' shadows the shortcut evaluation scheme
From: |
Jim Meyering |
Subject: |
Re: the `--color' option of `ls' shadows the shortcut evaluation scheme of `&&' and `||' |
Date: |
Tue, 12 Feb 2008 19:30:54 +0100 |
Eric Blake <address@hidden> wrote:
> According to Day on 2/11/2008 8:35 PM:
> |
> | [ "$(ls -A --color ./dir)" ] && echo "Not Empty" || echo "Empty"
> |
> | the echoed message is always "Not Empty", regardless of the fact that
> ./dir is actually EMPTY. Below is the tested result.
>
> Thanks for the report. However, this is not necessarily a bug. By
> itself, --color is documented to mean --color=always, in which case ls
> outputs command sequences to initialize and restore colors back to normal:
>
> $ ls -A --color | od -tx1z
> 0000000 1b 5b 30 6d 1b 5b 6d >.[0m.[m<
> 0000007
>
> And since that is output (even though it does not show up on the
> terminal), the string is non-empty, hence the result of the [] test. You
> really want the color output to be supressed in this context (in $()
> command substitution, the output is not a terminal, so color escape
> sequences don't normally make sense to the downstream client), so you
> should consider using --color=auto instead:
>
> $ ls -A --color=auto | od -tx1z
> 0000000
>
> It may be worth considering a patch to coreutils, such that plain --color
> implies --color=auto rather than --color=always. For example, this would
> match how 'git config' reacts when interpreting color options (where
> 'auto' and 'true' are synonyms, and 'always' must be explicit).
Sounds ok.
Any other opinions?
> It may also be worth a patch to ls to omit color sequences altogether if
> there is no other output, although this may be more difficult to code
> and/or justify.
Yes, that is worthwhile.
Ed Avis wrote a patch to do just that a few years ago.
It was held up due to copyright assignment processing delays,
and then I forgot about it.
I've just dug up the patch, merged it, and added one more test
along with the usual administrivia.
Here's the result I'll push in a day or two:
ls --color no longer outputs unnecessary escape sequences
In --color mode, plain files do not get any color, not even white.
When no highlighting is required, ls outputs no escape sequence at all.
* src/ls.c (print_with_color):
(used_color): New global.
(indicator_no) [C_RESET]: New enum value.
(indicator_name) ["rs"]: Corresponding new string.
(color_indicator): Make the 'normal' and 'file' markers be NULL.
Use "rs" (C_RESET) to reset to ordinary colors.
(process_signals): Restore default colors only if necessary.
(main): Don't call prep_non_filename_text here.
(print_name_with_quoting): Call it here, instead.
(prep_non_filename_text): Use C_RESET, not C_NORM.
(print_color_indicator): Return bool, not void.
Print nothing, when possible.
(put_indicator): Call prep_non_filename_text the first time.
* tests/misc/ls-misc: Test for above.
* tests/ls/color-dtype-dir: Adapt: no escapes around regular file name.
* TODO: Remove item.
* NEWS: Mention this.
Signed-off-by: Jim Meyering <address@hidden>
---
NEWS | 4 +++
TODO | 3 --
src/ls.c | 60 +++++++++++++++++++++++++++++++++-------------
tests/ls/color-dtype-dir | 4 +-
tests/misc/ls-misc | 17 ++++++++++--
5 files changed, 63 insertions(+), 25 deletions(-)
diff --git a/NEWS b/NEWS
index e05e1c3..b9f25e6 100644
--- a/NEWS
+++ b/NEWS
@@ -10,6 +10,10 @@ GNU coreutils NEWS -*-
outline -*-
"rmdir --ignore-fail-on-non-empty" detects and ignores the failure
in more cases when a directory is empty.
+** Improvements
+
+ ls --color no longer outputs unnecessary escape sequences
+
** Consistency
mkdir and split now write --verbose output to stdout, not stderr.
diff --git a/TODO b/TODO
index 3cec054..f8cc66a 100644
--- a/TODO
+++ b/TODO
@@ -133,9 +133,6 @@ Changes expected to go in, someday.
Pending copyright papers:
------------------------
- ls --color: Ed Avis' patch to suppress escape sequences for
- non-highlighted files
-
getpwnam from Bruce Korb
pb (progress bar) from Miika Pekkarinen
diff --git a/src/ls.c b/src/ls.c
index 46713f2..bf94e8f 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -207,7 +207,7 @@ static bool file_ignored (char const *name);
static uintmax_t gobble_file (char const *name, enum filetype type,
ino_t inode, bool command_line_arg,
char const *dirname);
-static void print_color_indicator (const char *name, mode_t mode, int linkok,
+static bool print_color_indicator (const char *name, mode_t mode, int linkok,
bool stat_ok, enum filetype type);
static void put_indicator (const struct bin_str *ind);
static void add_ignore_pattern (const char *pattern);
@@ -489,6 +489,12 @@ ARGMATCH_VERIFY (indicator_style_args,
indicator_style_types);
static bool print_with_color;
+/* Whether we used any colors in the output so far. If so, we will
+ need to restore the default color later. If not, we will need to
+ call prep_non_filename_text before using color for the first time. */
+
+static bool used_color = false;
+
enum color_type
{
color_never, /* 0: default or --color=never */
@@ -507,14 +513,15 @@ enum Dereference_symlink
enum indicator_no
{
- C_LEFT, C_RIGHT, C_END, C_NORM, C_FILE, C_DIR, C_LINK, C_FIFO, C_SOCK,
+ C_LEFT, C_RIGHT, C_END, C_RESET, C_NORM, C_FILE, C_DIR, C_LINK,
+ C_FIFO, C_SOCK,
C_BLK, C_CHR, C_MISSING, C_ORPHAN, C_EXEC, C_DOOR, C_SETUID, C_SETGID,
C_STICKY, C_OTHER_WRITABLE, C_STICKY_OTHER_WRITABLE
};
static const char *const indicator_name[]=
{
- "lc", "rc", "ec", "no", "fi", "di", "ln", "pi", "so",
+ "lc", "rc", "ec", "rs", "no", "fi", "di", "ln", "pi", "so",
"bd", "cd", "mi", "or", "ex", "do", "su", "sg", "st",
"ow", "tw", NULL
};
@@ -531,8 +538,9 @@ static struct bin_str color_indicator[] =
{ LEN_STR_PAIR ("\033[") }, /* lc: Left of color sequence */
{ LEN_STR_PAIR ("m") }, /* rc: Right of color sequence */
{ 0, NULL }, /* ec: End color (replaces lc+no+rc) */
- { LEN_STR_PAIR ("0") }, /* no: Normal */
- { LEN_STR_PAIR ("0") }, /* fi: File: default */
+ { LEN_STR_PAIR ("0") }, /* rs: Reset to ordinary colors */
+ { 0, NULL }, /* no: Normal */
+ { 0, NULL }, /* fi: File: default */
{ LEN_STR_PAIR ("01;34") }, /* di: Directory: bright blue */
{ LEN_STR_PAIR ("01;36") }, /* ln: Symlink: bright cyan */
{ LEN_STR_PAIR ("33") }, /* pi: Pipe: yellow/brown */
@@ -1072,7 +1080,8 @@ process_signals (void)
int stops;
sigset_t oldset;
- restore_default_color ();
+ if (used_color)
+ restore_default_color ();
fflush (stdout);
sigprocmask (SIG_BLOCK, &caught_signals, &oldset);
@@ -1209,8 +1218,6 @@ main (int argc, char **argv)
}
#endif
}
-
- prep_non_filename_text ();
}
if (dereference == DEREF_UNDEFINED)
@@ -1325,7 +1332,8 @@ main (int argc, char **argv)
{
int j;
- restore_default_color ();
+ if (used_color)
+ restore_default_color ();
fflush (stdout);
/* Restore the default signal handling. */
@@ -3826,8 +3834,9 @@ print_name_with_quoting (const char *p, mode_t mode, int
linkok,
bool stat_ok, enum filetype type,
struct obstack *stack)
{
- if (print_with_color)
- print_color_indicator (p, mode, linkok, stat_ok, type);
+ bool used_color_this_time
+ = (print_with_color
+ && print_color_indicator (p, mode, linkok, stat_ok, type));
if (stack)
PUSH_CURRENT_DIRED_POS (stack);
@@ -3837,7 +3846,7 @@ print_name_with_quoting (const char *p, mode_t mode, int
linkok,
if (stack)
PUSH_CURRENT_DIRED_POS (stack);
- if (print_with_color)
+ if (used_color_this_time)
{
process_signals ();
prep_non_filename_text ();
@@ -3852,7 +3861,7 @@ prep_non_filename_text (void)
else
{
put_indicator (&color_indicator[C_LEFT]);
- put_indicator (&color_indicator[C_NORM]);
+ put_indicator (&color_indicator[C_RESET]);
put_indicator (&color_indicator[C_RIGHT]);
}
}
@@ -3927,7 +3936,8 @@ print_type_indicator (bool stat_ok, mode_t mode, enum
filetype type)
DIRED_PUTCHAR (c);
}
-static void
+/* Returns whether any color sequence was printed. */
+static bool
print_color_indicator (const char *name, mode_t mode, int linkok,
bool stat_ok, enum filetype filetype)
{
@@ -4004,9 +4014,19 @@ print_color_indicator (const char *name, mode_t mode,
int linkok,
}
}
- put_indicator (&color_indicator[C_LEFT]);
- put_indicator (ext ? &(ext->seq) : &color_indicator[type]);
- put_indicator (&color_indicator[C_RIGHT]);
+ {
+ const struct bin_str *const s
+ = ext ? &(ext->seq) : &color_indicator[type];
+ if (s->string != NULL)
+ {
+ put_indicator (&color_indicator[C_LEFT]);
+ put_indicator (s);
+ put_indicator (&color_indicator[C_RIGHT]);
+ return true;
+ }
+ else
+ return false;
+ }
}
/* Output a color indicator (which may contain nulls). */
@@ -4016,6 +4036,12 @@ put_indicator (const struct bin_str *ind)
size_t i;
const char *p;
+ if (! used_color)
+ {
+ used_color = true;
+ prep_non_filename_text ();
+ }
+
p = ind->string;
for (i = ind->len; i != 0; --i)
diff --git a/tests/ls/color-dtype-dir b/tests/ls/color-dtype-dir
index 7a25c06..b969701 100755
--- a/tests/ls/color-dtype-dir
+++ b/tests/ls/color-dtype-dir
@@ -4,7 +4,7 @@
# directories the same as the first one -- but only on a file system
# with dirent.d_type support.
-# Copyright (C) 2006-2007 Free Software Foundation, Inc.
+# Copyright (C) 2006-2008 Free Software Foundation, Inc.
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -43,7 +43,7 @@ mv o1 out || fail=1
cat <<\EOF > exp || fail=1
^[[0m^[[01;34md^[[0m$
^[[34;42mother-writable^[[0m$
-^[[0mout^[[0m$
+out$
^[[37;44msticky^[[0m$
^[[m
EOF
diff --git a/tests/misc/ls-misc b/tests/misc/ls-misc
index 9290eb4..4182fa1 100755
--- a/tests/misc/ls-misc
+++ b/tests/misc/ls-misc
@@ -1,7 +1,7 @@
#!/bin/sh
# -*- perl -*-
-# Copyright (C) 1998, 2000-2007 Free Software Foundation, Inc.
+# Copyright (C) 1998, 2000-2008 Free Software Foundation, Inc.
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
@@ -55,8 +55,14 @@ system (qq(touch setuid && chmod u+s setuid && $test -u
setuid &&
or (warn "$program_name: cannot create setuid/setgid/sticky files,"
. "so can't run this test\n"), exit 77;
-my $mkdir = {PRE => sub {mkdir 'd',0755 or die "d: $!\n"}};
-my $rmdir = {POST => sub {rmdir 'd' or die "d: $!\n"}};
+sub mkdir_d {mkdir 'd',0755 or die "d: $!\n"}
+sub rmdir_d {rmdir 'd' or die "d: $!\n"}
+my $mkdir = {PRE => sub {mkdir_d}};
+my $rmdir = {POST => sub {rmdir_d}};
+my $mkdir_reg = {PRE => sub {mkdir_d; open (FH, '>d/f') && close FH
+ or die "d/f: $!\n" }};
+my $rmdir_reg = {POST => sub {unlink 'd/f' or die "d/f: $!\n";
+ rmdir 'd' or die "d: $!\n"}};
my $mkdir2 = {PRE => sub {mkdir 'd',0755 or die "d: $!\n";
mkdir 'd/e',0755 or die "d/e: $!\n" }};
@@ -132,6 +138,11 @@ my @Tests =
{OUT => "\e[0m\e[01;address@hidden"},
$slink_d, $unlink_d],
+ # A listing with no output should have no color sequences at all.
+ ['no-c-empty', '--color=always d', {OUT => ""}, $mkdir, $rmdir],
+ # A listing with only regular files should have no color sequences at all.
+ ['no-c-reg', '--color=always d', {OUT => "f\n"}, $mkdir_reg, $rmdir_reg],
+
# Test for a bug fixed after coreutils-6.9.
['sl-target', '--color=always d',
{OUT => "\e[0m\e[01;34mX\e[0m\n\e[m"}, $target, $target2],
--
1.5.4.1.98.gf3293
>From afeb251602c3d5ec9cffdadde25c4173dad498b0 Mon Sep 17 00:00:00 2001
From: Jim Meyering <address@hidden>
Date: Tue, 12 Feb 2008 18:13:09 +0100
Subject: [PATCH] * src/ls.c (put_indicator): Use fwrite, not a loop.
Signed-off-by: Jim Meyering <address@hidden>
---
src/ls.c | 8 +-------
1 files changed, 1 insertions(+), 7 deletions(-)
diff --git a/src/ls.c b/src/ls.c
index bf94e8f..d5c6efb 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -4033,19 +4033,13 @@ print_color_indicator (const char *name, mode_t mode,
int linkok,
static void
put_indicator (const struct bin_str *ind)
{
- size_t i;
- const char *p;
-
if (! used_color)
{
used_color = true;
prep_non_filename_text ();
}
- p = ind->string;
-
- for (i = ind->len; i != 0; --i)
- putchar (*(p++));
+ fwrite (ind->string, ind->len, 1, stdout);
}
static size_t
--
1.5.4.1.98.gf3293