ls: add already listed message (#3707)

* ls: handle looping symlinks infinite printing

* ls: better coloring and printing symlinks when dereferenced

* tests/ls: add dereferencing and symlink loop tests

* ls: reformat changed using rustfmt

* ls: follow clippy advice for cleaner code

* uucore/fs: fix FileInformation to open directory handles in Windows as
well

src/uu/ls/src/ls.rs

@@ -18,6 +18,7 @@ use glob::Pattern;
 use lscolors::LsColors;
 use number_prefix::NumberPrefix;
 use once_cell::unsync::OnceCell;
+use std::collections::HashSet;
 #[cfg(windows)]
 use std::os::windows::fs::MetadataExt;
 use std::{
@@ -151,6 +152,7 @@ enum LsError {
     IOError(std::io::Error),
     IOErrorContext(std::io::Error, PathBuf),
     BlockSizeParseError(String),
+    AlreadyListedError(PathBuf),
 }
 
 impl UError for LsError {
@@ -160,6 +162,7 @@ impl UError for LsError {
             LsError::IOError(_) => 1,
             LsError::IOErrorContext(_, _) => 1,
             LsError::BlockSizeParseError(_) => 1,
+            LsError::AlreadyListedError(_) => 2,
         }
     }
 }
@@ -236,6 +239,13 @@ impl Display for LsError {
                     },
                 }
             }
+            LsError::AlreadyListedError(path) => {
+                write!(
+                    f,
+                    "{}: not listing already-listed directory",
+                    path.to_string_lossy()
+                )
+            }
         }
     }
 }
@@ -1488,16 +1498,26 @@ impl PathData {
 
         // Why prefer to check the DirEntry file_type()?  B/c the call is
         // nearly free compared to a metadata() call on a Path
-        let ft = match de {
-            Some(ref de) => {
-                if let Ok(ft_de) = de.file_type() {
-                    OnceCell::from(Some(ft_de))
-                } else if let Ok(md_pb) = p_buf.metadata() {
-                    OnceCell::from(Some(md_pb.file_type()))
-                } else {
-                    OnceCell::new()
+        fn get_file_type(
+            de: &DirEntry,
+            p_buf: &Path,
+            must_dereference: bool,
+        ) -> OnceCell<Option<FileType>> {
+            if must_dereference {
+                if let Ok(md_pb) = p_buf.metadata() {
+                    return OnceCell::from(Some(md_pb.file_type()));
                 }
             }
+            if let Ok(ft_de) = de.file_type() {
+                OnceCell::from(Some(ft_de))
+            } else if let Ok(md_pb) = p_buf.symlink_metadata() {
+                OnceCell::from(Some(md_pb.file_type()))
+            } else {
+                OnceCell::new()
+            }
+        }
+        let ft = match de {
+            Some(ref de) => get_file_type(de, &p_buf, must_dereference),
             None => OnceCell::new(),
         };
 
@@ -1619,7 +1639,12 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> {
                 writeln!(out, "\n{}:", path_data.p_buf.display())?;
             }
         }
-        enter_directory(path_data, read_dir, config, &mut out)?;
+        let mut listed_ancestors = HashSet::new();
+        listed_ancestors.insert(FileInformation::from_path(
+            &path_data.p_buf,
+            path_data.must_dereference,
+        )?);
+        enter_directory(path_data, read_dir, config, &mut out, &mut listed_ancestors)?;
     }
 
     Ok(())
@@ -1715,6 +1740,7 @@ fn enter_directory(
     read_dir: ReadDir,
     config: &Config,
     out: &mut BufWriter<Stdout>,
+    listed_ancestors: &mut HashSet<FileInformation>,
 ) -> UResult<()> {
     // Create vec of entries with initial dot files
     let mut entries: Vec<PathData> = if config.files == Files::All {
@@ -1780,8 +1806,17 @@ fn enter_directory(
                     continue;
                 }
                 Ok(rd) => {
-                    writeln!(out, "\n{}:", e.p_buf.display())?;
-                    enter_directory(e, rd, config, out)?;
+                    if !listed_ancestors
+                        .insert(FileInformation::from_path(&e.p_buf, e.must_dereference)?)
+                    {
+                        out.flush()?;
+                        show!(LsError::AlreadyListedError(e.p_buf.clone()));
+                    } else {
+                        writeln!(out, "\n{}:", e.p_buf.display())?;
+                        enter_directory(e, rd, config, out, listed_ancestors)?;
+                        listed_ancestors
+                            .remove(&FileInformation::from_path(&e.p_buf, e.must_dereference)?);
+                    }
                 }
             }
         }
@@ -2254,6 +2289,7 @@ fn get_inode(metadata: &Metadata) -> String {
 use std::sync::Mutex;
 #[cfg(unix)]
 use uucore::entries;
+use uucore::fs::FileInformation;
 use uucore::quoting_style;
 
 #[cfg(unix)]
@@ -2518,12 +2554,17 @@ fn display_file_name(
     let mut width = name.width();
 
     if let Some(ls_colors) = &config.color {
-        name = color_name(
-            name,
-            &path.p_buf,
-            path.p_buf.symlink_metadata().ok().as_ref(),
-            ls_colors,
-        );
+        let md = path.md(out);
+        name = if md.is_some() {
+            color_name(name, &path.p_buf, md, ls_colors)
+        } else {
+            color_name(
+                name,
+                &path.p_buf,
+                path.p_buf.symlink_metadata().ok().as_ref(),
+                ls_colors,
+            )
+        };
     }
 
     if config.format != Format::Long && !more_info.is_empty() {
@@ -2564,6 +2605,7 @@ fn display_file_name(
     if config.format == Format::Long
         && path.file_type(out).is_some()
         && path.file_type(out).unwrap().is_symlink()
+        && !path.must_dereference
     {
         if let Ok(target) = path.p_buf.read_link() {
             name.push_str(" -> ");

src/uucore/src/lib/features/fs.rs

@@ -79,9 +79,12 @@ impl FileInformation {
             use std::fs::OpenOptions;
             use std::os::windows::prelude::*;
             let mut open_options = OpenOptions::new();
+            let mut custom_flags = 0;
             if !dereference {
-                open_options.custom_flags(winapi::um::winbase::FILE_FLAG_OPEN_REPARSE_POINT);
+                custom_flags |= winapi::um::winbase::FILE_FLAG_OPEN_REPARSE_POINT;
             }
+            custom_flags |= winapi::um::winbase::FILE_FLAG_BACKUP_SEMANTICS;
+            open_options.custom_flags(custom_flags);
             let file = open_options.read(true).open(path.as_ref())?;
             Self::from_file(&file)
         }

tests/by-util/test_ls.rs

@@ -3065,3 +3065,62 @@ fn test_ls_quoting_color() {
         .succeeds()
         .stdout_contains("\'need quoting\'");
 }
+
+#[test]
+fn test_ls_dereference_looped_symlinks_recursive() {
+    let (at, mut ucmd) = at_and_ucmd!();
+
+    at.mkdir("loop");
+    at.relative_symlink_dir("../loop", "loop/sub");
+
+    ucmd.args(&["-RL", "loop"])
+        .fails()
+        .stderr_contains("not listing already-listed directory");
+}
+
+#[test]
+fn test_dereference_dangling_color() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.relative_symlink_file("wat", "nonexistent");
+    let out_exp = ucmd.args(&["--color"]).run().stdout_move_str();
+
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.relative_symlink_file("wat", "nonexistent");
+    ucmd.args(&["-L", "--color"])
+        .fails()
+        .code_is(1)
+        .stderr_contains("No such file or directory")
+        .stdout_is(out_exp);
+}
+
+#[test]
+fn test_dereference_symlink_dir_color() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.mkdir("dir1");
+    at.mkdir("dir1/link");
+    let out_exp = ucmd.args(&["--color", "dir1"]).run().stdout_move_str();
+
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.mkdir("dir1");
+    at.mkdir("dir2");
+    at.relative_symlink_dir("../dir2", "dir1/link");
+    ucmd.args(&["-L", "--color", "dir1"])
+        .succeeds()
+        .stdout_is(out_exp);
+}
+
+#[test]
+fn test_dereference_symlink_file_color() {
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.mkdir("dir1");
+    at.touch("dir1/link");
+    let out_exp = ucmd.args(&["--color", "dir1"]).run().stdout_move_str();
+
+    let (at, mut ucmd) = at_and_ucmd!();
+    at.mkdir("dir1");
+    at.touch("file");
+    at.relative_symlink_file("../file", "dir1/link");
+    ucmd.args(&["-L", "--color", "dir1"])
+        .succeeds()
+        .stdout_is(out_exp);
+}