Subject: bug#12445: ls --color does not color symlinks when not resolving them

From: Pádraig Brady <[email-masked]>
Date: Sun, 16 Sep 2012 04:46:46 +0100

On 09/15/2012 11:40 AM, Pádraig Brady wrote:
> reopen 12445
> stop
>
> On 09/15/2012 03:28 AM, Matei David wrote:
>> On Sat, 15 Sep 2012 00:55:12 +0100
>> Pádraig Brady<[email-masked]> wrote:
>>
>>> tag 12445 + notabug
>>> close 12445
>>> stop
>>>
>>> On 09/14/2012 05:54 PM, Matei David wrote:
>>>> When the ORPHAN property is disabled in dircolors, ls --color does not
>>>> stat() symlinks. However, in this case symlinks are not colored using the
>>>> LINK property.
>>>>
>>>> How to reproduce:
>>>> $ eval $(dircolors -p | sed 's/^ORPHAN.*/ORPHAN 00/' | dircolors -)
>>>> $ mkdir /tmp/xxx
>>>> $ ln -s /tmp/nowhere /tmp/xxx/yyy
>>>> $ ls --color /tmp/xxx
>>>>
>>>> Expected output:
>>>> yyy, colored cyan (default for LINK)
>>>>
>>>> Actual output:
>>>> yyy, not colored
>>>>
>>>> Fix attached. Basically, in gobble_file(), assume a symlink is ok if stat()
>>>> is not run on it.
>>>
>>> It seems this was fixed over 5 years ago in version 6.9
>>> http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=0a744370
>>> Are you using an older version than that?
>>>
>>> I'm marking as done for now.
>>>
>>> thanks,
>>> Pádraig.
>>
>> I'm using coreutils 8.19. There are 2 lines which ever modify f->linkok (both
>> set it to true): 2700 and 2720. Both are inside the if block starting at
>> line 2560. By design, the if block is _not_ executed under the conditions I laid
>> out above in order to avoid both fstat() and stat() calls on the symlink (which
>> is ok!) However, the coloring is missing. Did you try my code?
>>
>> The bug you mention was about avoiding the stat() call, i.e. following the
>> link. There is also this one, related:
>> http://git.savannah.gnu.org/cgit/coreutils.git/commit/src/ls.c?id=a085b6fc6c8fa03dac20f01f2f2b64b50b8bdf66
>>
>> The behaviour I observe is: no fstat(), no stat(), (great so far...) but no
>> color either- that's the bug.
>
> Drats, I was too hasty sorry.
>
> ls 8.10 doesn't have the issue, and when I tested the latest version
> I only did so with -l which also doesn't show the issue.
>
> So this regression was added since 8.10 and only when -l is not used.
>
> I'll add tests etc. and apply a fix in your name.

So the regression was introduced in coreutils 8.14 in:
http://git.sv.gnu.org/gitweb/?p=coreutils.git;a=commit;h=84457c49

I dug a little deeper and was confused as to the need
for the simulated "linkok" at all. That was originally introduced with:
http://thread.gmane.org/gmane.comp.gnu.coreutils.bugs/10608/focus=10927

I also noticed that is_colored() was not used with C_ORPHAN,
thus meaning any of or=00, or=0, or= in LS_COLORS was considered significant.
One could argue that for any attribute, 00 should mean color as 00,
rather than don't color, but for consistency with all other color checks
I changed that to use is_colored(). As that was the only part of the code inspecting
the "linkok" variable, I removed the simulated linkok referenced
in the above link.

All tests pass with the new code.

cheers,
Pádraig.
From ebf697b623435d46dd48998408e1c7cf3eba1623 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Sun, 16 Sep 2012 04:10:50 +0100
Subject: [PATCH] ls: fix coloring of dangling symlinks in default listing
 mode

When listing a directory containing dangling symlinks,
and not outputting a long format listing, and orphaned links
are set to no coloring in LS_COLORS, then the symlinks
would get no color rather than reverting to the standard
symlink color.  The issue was introduced in v8.13-19-g84457c4

* src/ls.c (print_color_indicator): Use the standard method
to check if coloring is specified for orphaned symlinks.
The existing method would consider 'or=00' or 'or=0' as significant
in LS_COLORS. Even 'or=' was significant as in that case the
string='or=' and the length=0.  Also apply the same change
for missing symlinks for consistency.
(gobble_file): Remove the simulation of linkok, which is only
tested in print_color_indicator() which now handles this directly
by keying on the LS_COLORS values correctly.
* tests/misc/ls-misc.pl: Add a test case.
* THANKS: Add the reporter.
* NEWS: Mention the fix.
Reported-by: David Matei
---
 NEWS                  |    4 ++++
 THANKS.in             |    1 +
 src/ls.c              |   11 ++---------
 tests/misc/ls-misc.pl |   15 +++++++++++++++
 4 files changed, 22 insertions(+), 9 deletions(-)

diff --git a/NEWS b/NEWS
index 2e69391..ab205f5 100644
--- a/NEWS
+++ b/NEWS
@@ -21,6 +21,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   numbers, and up to 8 times slower for some worst-case individual numbers.
   [bug introduced in coreutils-7.0, with GNU MP support]
 
+  ls now correctly colors dangling symlinks when listing their containing
+  directories, with orphaned symlink coloring disabled in LS_COLORS.
+  [bug introduced in coreutils-8.14]
+
   rm -i -d now prompts the user then removes an empty directory, rather
   than ignoring the -d option and failing with an 'Is a directory' error.
   [bug introduced in coreutils-8.19, with the addition of --dir (-d)]
diff --git a/THANKS.in b/THANKS.in
index 2c3f83c..b0061b3 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -153,6 +153,7 @@ David Godfrey                       [email-masked]
 David Luyer                         [email-masked]
 David Madore                        [email-masked]
 David Malone                        [email-masked]
+David Matei                         [email-masked]
 Davide Canova                       [email-masked]
 Dawson Engler                       [email-masked]
 Dean Gaudet                         [email-masked]
diff --git a/src/ls.c b/src/ls.c
index 9494ae9..106d234 100644
--- a/src/ls.c
+++ b/src/ls.c
@@ -3064,12 +3064,6 @@ gobble_file (char const *name, enum filetype type, ino_t inode,
           free (linkname);
         }
 
-      /* When not distinguishing types of symlinks, pretend we know that
-         it is stat'able, so that it will be colored as a regular symlink,
-         and not as an orphan.  */
-      if (S_ISLNK (f->stat.st_mode) && !check_symlink_color)
-        f->linkok = true;
-
       if (S_ISLNK (f->stat.st_mode))
         f->filetype = symbolic_link;
       else if (S_ISDIR (f->stat.st_mode))
@@ -4293,7 +4287,7 @@ print_color_indicator (const struct fileinfo *f, bool symlink_target)
 
   /* Is this a nonexistent file?  If so, linkok == -1.  */
 
-  if (linkok == -1 && color_indicator[C_MISSING].string != NULL)
+  if (linkok == -1 && is_colored (C_MISSING))
     type = C_MISSING;
   else if (!f->stat_ok)
     {
@@ -4368,8 +4362,7 @@ print_color_indicator (const struct fileinfo *f, bool symlink_target)
   /* Adjust the color for orphaned symlinks.  */
   if (type == C_LINK && !linkok)
     {
-      if (color_symlink_as_referent
-          || color_indicator[C_ORPHAN].string)
+      if (color_symlink_as_referent || is_colored (C_ORPHAN))
         type = C_ORPHAN;
     }
 
diff --git a/tests/misc/ls-misc.pl b/tests/misc/ls-misc.pl
index 71647f9..04d8016 100755
--- a/tests/misc/ls-misc.pl
+++ b/tests/misc/ls-misc.pl
@@ -263,6 +263,21 @@ my @Tests =
       {POST => sub {unlink 's' or die "s: $!\n";
                     restore_ls_colors; }},
      ],
+     # The code related to the above fixes introduced a regression,
+     # that was fixed after coreutils-8.19.  This is the case when
+     # listing a dir containing dangling symlinks, but with orphans uncolored.
+     # I.E. the same as the previous test, but listing the directory
+     # rather than the symlink directly.
+     ['sl-dangle9', '--color=always d',
+      {OUT => "$e\e[1;36ms$e\n"},
+      {PRE => sub {mkdir 'd',0755 or die "d: $!\n";
+                   symlink 'dangle', 'd/s' or die "d/s: $!\n";
+                   push_ls_colors('ln=1;36:or=:')
+       }},
+      {POST => sub {unlink 'd/s' or die "d/s: $!\n";
+                    rmdir 'd' or die "d: $!\n";
+                    restore_ls_colors; }},
+     ],
 
      # Test for a bug that was introduced in coreutils-4.5.4; fixed in 4.5.5.
      # To demonstrate it, the file in question (with executable bit set)
-- 
1.7.6.4