Eric Blake <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> --- 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;[EMAIL PROTECTED]"}, $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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> --- 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 _______________________________________________ Bug-coreutils mailing list Bug-coreutils@gnu.org http://lists.gnu.org/mailman/listinfo/bug-coreutils