ls: Improve error handling and other improvements (#2809)

*  print error in the correct order by flushing the stdout buffer before printing an error
*  print correct GNU error codes
*  correct formatting for config.inode, and for dangling links
*  correct padding for Format::Long
*  remove colors after the -> link symbol as this doesn't match GNU
*  correct the major, minor #s for char devices, and correct padding
*  improve speed for all metadata intensive ops by not allocating metadata unless in a Sort mode
*  new tests, have struggled with how to deal with stderr, stdout ordering in a test though
*  tried to implement UIoError, but am still having issues matching the formatting of GNU


Co-authored-by: electricboogie <32370782+electricboogie@users.noreply.github.com>

src/uu/ls/src/ls.rs

@@ -29,7 +29,7 @@ use std::{
     error::Error,
     fmt::Display,
     fs::{self, DirEntry, FileType, Metadata},
-    io::{stdout, BufWriter, Stdout, Write},
+    io::{stdout, BufWriter, ErrorKind, Stdout, Write},
     path::{Path, PathBuf},
     time::{SystemTime, UNIX_EPOCH},
 };
@@ -142,14 +142,16 @@ const DEFAULT_TERM_WIDTH: u16 = 80;
 #[derive(Debug)]
 enum LsError {
     InvalidLineWidth(String),
-    NoMetadata(PathBuf),
+    IOError(std::io::Error),
+    IOErrorContext(std::io::Error, PathBuf),
 }
 
 impl UError for LsError {
     fn code(&self) -> i32 {
         match self {
             LsError::InvalidLineWidth(_) => 2,
-            LsError::NoMetadata(_) => 1,
+            LsError::IOError(_) => 1,
+            LsError::IOErrorContext(_, _) => 1,
         }
     }
 }
@@ -160,7 +162,39 @@ impl Display for LsError {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
         match self {
             LsError::InvalidLineWidth(s) => write!(f, "invalid line width: {}", s.quote()),
-            LsError::NoMetadata(p) => write!(f, "could not open file: {}", p.quote()),
+            LsError::IOError(e) => write!(f, "general io error: {}", e),
+            LsError::IOErrorContext(e, p) => {
+                let error_kind = e.kind();
+
+                match error_kind {
+                    ErrorKind::NotFound => write!(
+                        f,
+                        "cannot access '{}': No such file or directory",
+                        p.to_string_lossy()
+                    ),
+                    ErrorKind::PermissionDenied => {
+                        if p.is_dir() {
+                            write!(
+                                f,
+                                "cannot open directory '{}': Permission denied",
+                                p.to_string_lossy()
+                            )
+                        } else {
+                            write!(
+                                f,
+                                "cannot open file '{}': Permission denied",
+                                p.to_string_lossy()
+                            )
+                        }
+                    }
+                    _ => write!(
+                        f,
+                        "unknown io error: '{:?}', '{:?}'",
+                        p.to_string_lossy(),
+                        e
+                    ),
+                }
+            }
         }
     }
 }
@@ -259,6 +293,8 @@ struct LongFormat {
 }
 
 struct PaddingCollection {
+    #[cfg(unix)]
+    longest_inode_len: usize,
     longest_link_count_len: usize,
     longest_uname_len: usize,
     longest_group_len: usize,
@@ -633,6 +669,7 @@ pub fn uumain(args: impl uucore::Args) -> UResult<()> {
     let matches = app.get_matches_from(args);
 
     let config = Config::from(&matches)?;
+
     let locs = matches
         .values_of_os(options::PATHS)
         .map(|v| v.map(Path::new).collect())
@@ -1205,6 +1242,7 @@ only ignore '.' and '..'.",
 /// Represents a Path along with it's associated data
 /// Any data that will be reused several times makes sense to be added to this structure
 /// Caching data here helps eliminate redundant syscalls to fetch same information
+#[derive(Debug)]
 struct PathData {
     // Result<MetaData> got from symlink_metadata() or metadata() based on config
     md: OnceCell<Option<Metadata>>,
@@ -1253,6 +1291,7 @@ impl PathData {
             }
             Dereference::None => false,
         };
+
         let ft = match file_type {
             Some(ft) => OnceCell::from(ft.ok()),
             None => OnceCell::new(),
@@ -1290,24 +1329,22 @@ impl PathData {
 fn list(locs: Vec<&Path>, config: Config) -> UResult<()> {
     let mut files = Vec::<PathData>::new();
     let mut dirs = Vec::<PathData>::new();
-
     let mut out = BufWriter::new(stdout());
+    let initial_locs_len = locs.len();
 
-    for loc in &locs {
-        let p = PathBuf::from(loc);
-        let path_data = PathData::new(p, None, None, &config, true);
+    for loc in locs {
+        let path_data = PathData::new(PathBuf::from(loc), None, None, &config, true);
 
+        // Getting metadata here is no big deal as it's just the CWD
+        // and we really just want to know if the strings exist as files/dirs
         if path_data.md().is_none() {
-            // FIXME: Would be nice to use the actual error instead of hardcoding it
-            // Presumably other errors can happen too?
-            show_error!(
-                "cannot access {}: No such file or directory",
-                path_data.p_buf.quote()
-            );
-            set_exit_code(1);
-            // We found an error, no need to continue the execution
+            let _ = out.flush();
+            show!(LsError::IOErrorContext(
+                std::io::Error::new(ErrorKind::NotFound, "NotFound"),
+                path_data.p_buf
+            ));
             continue;
-        }
+        };
 
         let show_dir_contents = match path_data.file_type() {
             Some(ft) => !config.directory && ft.is_dir(),
@@ -1323,16 +1360,14 @@ fn list(locs: Vec<&Path>, config: Config) -> UResult<()> {
             files.push(path_data);
         }
     }
+
     sort_entries(&mut files, &config);
+    sort_entries(&mut dirs, &config);
+
     display_items(&files, &config, &mut out);
 
-    sort_entries(&mut dirs, &config);
-    for dir in dirs {
-        if locs.len() > 1 || config.recursive {
-            // FIXME: This should use the quoting style and propagate errors
-            let _ = writeln!(out, "\n{}:", dir.p_buf.display());
-        }
-        enter_directory(&dir, &config, &mut out);
+    for dir in &dirs {
+        enter_directory(dir, &config, initial_locs_len, &mut out);
     }
 
     Ok(())
@@ -1347,9 +1382,7 @@ fn sort_entries(entries: &mut Vec<PathData>, config: &Config) {
                     .unwrap_or(UNIX_EPOCH),
             )
         }),
-        Sort::Size => {
-            entries.sort_by_key(|k| Reverse(k.md().as_ref().map(|md| md.len()).unwrap_or(0)))
-        }
+        Sort::Size => entries.sort_by_key(|k| Reverse(k.md().map(|md| md.len()).unwrap_or(0))),
         // The default sort in GNU ls is case insensitive
         Sort::Name => entries.sort_by(|a, b| a.display_name.cmp(&b.display_name)),
         Sort::Version => entries
@@ -1376,7 +1409,7 @@ fn is_hidden(file_path: &DirEntry) -> bool {
         let attr = metadata.file_attributes();
         (attr & 0x2) > 0
     }
-    #[cfg(unix)]
+    #[cfg(not(windows))]
     {
         file_path
             .file_name()
@@ -1399,43 +1432,90 @@ fn should_display(entry: &DirEntry, config: &Config) -> bool {
         };
         continue;
     }
+
     // else default to display
     true
 }
 
-fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter<Stdout>) {
-    let mut entries: Vec<_> = if config.files == Files::All {
+fn enter_directory(
+    dir: &PathData,
+    config: &Config,
+    initial_locs_len: usize,
+    out: &mut BufWriter<Stdout>,
+) {
+    // Create vec of entries with initial dot files
+    let mut entries: Vec<PathData> = if config.files == Files::All {
         vec![
-            PathData::new(
-                dir.p_buf.clone(),
-                Some(Ok(*dir.file_type().unwrap())),
-                Some(".".into()),
-                config,
-                false,
-            ),
+            PathData::new(dir.p_buf.clone(), None, Some(".".into()), config, false),
             PathData::new(dir.p_buf.join(".."), None, Some("..".into()), config, false),
         ]
     } else {
         vec![]
     };
 
-    let mut temp: Vec<_> = crash_if_err!(1, fs::read_dir(&dir.p_buf))
-        .map(|res| crash_if_err!(1, res))
-        .filter(|res| should_display(res, config))
-        .map(|res| {
-            PathData::new(
-                DirEntry::path(&res),
-                Some(res.file_type()),
-                None,
-                config,
-                false,
-            )
-        })
-        .collect();
+    // Convert those entries to the PathData struct
+    let mut vec_path_data = Vec::new();
 
-    sort_entries(&mut temp, config);
+    // check for errors early, and ignore entries with errors
+    let read_dir = match fs::read_dir(&dir.p_buf) {
+        Err(err) => {
+            // flush buffer because the error may get printed in the wrong order
+            let _ = out.flush();
+            show!(LsError::IOErrorContext(err, dir.p_buf.clone()));
+            return;
+        }
+        Ok(res) => res,
+    };
 
-    entries.append(&mut temp);
+    for entry in read_dir {
+        let unwrapped = match entry {
+            Ok(path) => path,
+            Err(error) => {
+                let _ = out.flush();
+                show!(LsError::IOError(error));
+                continue;
+            }
+        };
+        if should_display(&unwrapped, config) {
+            // why check the DirEntry file_type()?  B/c the call is
+            // nearly free compared to a metadata() or file_type() call on a dir/file
+            let path_data = match unwrapped.file_type() {
+                Err(_err) => {
+                    let _ = out.flush();
+                    show!(LsError::IOErrorContext(
+                        std::io::Error::new(ErrorKind::NotFound, "NotFound"),
+                        unwrapped.path()
+                    ));
+                    continue;
+                }
+                Ok(dir_ft) => {
+                    let res =
+                        PathData::new(unwrapped.path(), Some(Ok(dir_ft)), None, config, false);
+                    if dir_ft.is_symlink() && res.md().is_none() {
+                        let _ = out.flush();
+                        show!(LsError::IOErrorContext(
+                            std::io::Error::new(ErrorKind::NotFound, "NotFound"),
+                            unwrapped.path()
+                        ));
+                    }
+                    res
+                }
+            };
+            vec_path_data.push(path_data);
+        }
+    }
+
+    sort_entries(&mut vec_path_data, config);
+    entries.append(&mut vec_path_data);
+
+    // Print dir heading - name...
+    if initial_locs_len > 1 || config.recursive {
+        let _ = writeln!(out, "\n{}:", dir.p_buf.display());
+    }
+    // ...and total
+    if config.format == Format::Long {
+        display_total(&entries, config, out);
+    }
 
     display_items(&entries, config, out);
 
@@ -1445,21 +1525,23 @@ fn enter_directory(dir: &PathData, config: &Config, out: &mut BufWriter<Stdout>)
             .skip(if config.files == Files::All { 2 } else { 0 })
             .filter(|p| p.file_type().map(|ft| ft.is_dir()).unwrap_or(false))
         {
-            let _ = writeln!(out, "\n{}:", e.p_buf.display());
-            enter_directory(e, config, out);
+            enter_directory(e, config, 0, out);
         }
     }
 }
 
-fn get_metadata(entry: &Path, dereference: bool) -> std::io::Result<Metadata> {
+fn get_metadata(p_buf: &Path, dereference: bool) -> std::io::Result<Metadata> {
     if dereference {
-        entry.metadata()
+        p_buf.metadata()
     } else {
-        entry.symlink_metadata()
+        p_buf.symlink_metadata()
     }
 }
 
-fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize, usize, usize) {
+fn display_dir_entry_size(
+    entry: &PathData,
+    config: &Config,
+) -> (usize, usize, usize, usize, usize) {
     // TODO: Cache/memorize the display_* results so we don't have to recalculate them.
     if let Some(md) = entry.md() {
         (
@@ -1467,9 +1549,10 @@ fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize, u
             display_uname(md, config).len(),
             display_group(md, config).len(),
             display_size_or_rdev(md, config).len(),
+            display_inode(md).len(),
         )
     } else {
-        (0, 0, 0, 0)
+        (0, 0, 0, 0, 0)
     }
 }
 
@@ -1481,12 +1564,34 @@ fn pad_right(string: &str, count: usize) -> String {
     format!("{:<width$}", string, width = count)
 }
 
+fn display_total(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout>) {
+    let mut total_size = 0;
+    for item in items {
+        total_size += item
+            .md()
+            .as_ref()
+            .map_or(0, |md| get_block_size(md, config));
+    }
+    let _ = writeln!(out, "total {}", display_size(total_size, config));
+}
+
 fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout>) {
     // `-Z`, `--context`:
     // Display the SELinux security context or '?' if none is found. When used with the `-l`
     // option, print the security context to the left of the size column.
 
     if config.format == Format::Long {
+        #[cfg(unix)]
+        let (
+            mut longest_inode_len,
+            mut longest_link_count_len,
+            mut longest_uname_len,
+            mut longest_group_len,
+            mut longest_context_len,
+            mut longest_size_len,
+        ) = (1, 1, 1, 1, 1, 1);
+
+        #[cfg(not(unix))]
         let (
             mut longest_link_count_len,
             mut longest_uname_len,
@@ -1494,11 +1599,27 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
             mut longest_context_len,
             mut longest_size_len,
         ) = (1, 1, 1, 1, 1);
-        let mut total_size = 0;
 
+        #[cfg(unix)]
         for item in items {
             let context_len = item.security_context.len();
-            let (link_count_len, uname_len, group_len, size_len) =
+            let (link_count_len, uname_len, group_len, size_len, inode_len) =
+                display_dir_entry_size(item, config);
+            longest_inode_len = inode_len.max(longest_inode_len);
+            longest_link_count_len = link_count_len.max(longest_link_count_len);
+            longest_size_len = size_len.max(longest_size_len);
+            longest_uname_len = uname_len.max(longest_uname_len);
+            longest_group_len = group_len.max(longest_group_len);
+            if config.context {
+                longest_context_len = context_len.max(longest_context_len);
+            }
+            longest_size_len = size_len.max(longest_size_len);
+        }
+
+        #[cfg(not(unix))]
+        for item in items {
+            let context_len = item.security_context.len();
+            let (link_count_len, uname_len, group_len, size_len, _inode_len) =
                 display_dir_entry_size(item, config);
             longest_link_count_len = link_count_len.max(longest_link_count_len);
             longest_size_len = size_len.max(longest_size_len);
@@ -1508,17 +1629,14 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
                 longest_context_len = context_len.max(longest_context_len);
             }
             longest_size_len = size_len.max(longest_size_len);
-            total_size += item.md().map_or(0, |md| get_block_size(md, config));
-        }
-
-        if total_size > 0 {
-            let _ = writeln!(out, "total {}", display_size(total_size, config));
         }
 
         for item in items {
             display_item_long(
                 item,
                 PaddingCollection {
+                    #[cfg(unix)]
+                    longest_inode_len,
                     longest_link_count_len,
                     longest_uname_len,
                     longest_group_len,
@@ -1540,9 +1658,12 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
         } else {
             None
         };
-        let names = items
+
+        let names: std::vec::IntoIter<Cell> = items
             .iter()
-            .filter_map(|i| display_file_name(i, config, prefix_context));
+            .map(|i| display_file_name(i, config, prefix_context, out))
+            .collect::<Vec<Cell>>()
+            .into_iter();
 
         match config.format {
             Format::Columns => display_grid(names, config.width, Direction::TopToBottom, out),
@@ -1671,85 +1792,138 @@ fn display_grid(
 ///    longest_size_len: usize,
 /// ```
 /// that decide the maximum possible character count of each field.
+#[allow(clippy::write_literal)]
 fn display_item_long(
     item: &PathData,
     padding: PaddingCollection,
     config: &Config,
     out: &mut BufWriter<Stdout>,
 ) {
-    let md = match item.md() {
-        None => {
-            show!(LsError::NoMetadata(item.p_buf.clone()));
-            return;
+    if let Some(md) = item.md() {
+        #[cfg(unix)]
+        {
+            if config.inode {
+                let _ = write!(
+                    out,
+                    "{} ",
+                    pad_left(&get_inode(md), padding.longest_inode_len),
+                );
+            }
         }
-        Some(md) => md,
-    };
 
-    #[cfg(unix)]
-    {
-        if config.inode {
-            let _ = write!(out, "{} ", get_inode(md));
+        let _ = write!(
+            out,
+            "{}{} {}",
+            display_permissions(md, true),
+            if item.security_context.len() > 1 {
+                // GNU `ls` uses a "." character to indicate a file with a security context,
+                // but not other alternate access method.
+                "."
+            } else {
+                ""
+            },
+            pad_left(&display_symlink_count(md), padding.longest_link_count_len),
+        );
+
+        if config.long.owner {
+            let _ = write!(
+                out,
+                " {}",
+                pad_right(&display_uname(md, config), padding.longest_uname_len),
+            );
         }
-    }
 
-    let _ = write!(
-        out,
-        "{}{} {}",
-        display_permissions(md, true),
-        if item.security_context.len() > 1 {
-            // GNU `ls` uses a "." character to indicate a file with a security context,
-            // but not other alternate access method.
-            "."
-        } else {
-            ""
-        },
-        pad_left(&display_symlink_count(md), padding.longest_link_count_len),
-    );
+        if config.long.group {
+            let _ = write!(
+                out,
+                " {}",
+                pad_right(&display_group(md, config), padding.longest_group_len),
+            );
+        }
+
+        if config.context {
+            let _ = write!(
+                out,
+                " {}",
+                pad_right(&item.security_context, padding.longest_context_len),
+            );
+        }
+
+        // Author is only different from owner on GNU/Hurd, so we reuse
+        // the owner, since GNU/Hurd is not currently supported by Rust.
+        if config.long.author {
+            let _ = write!(
+                out,
+                " {}",
+                pad_right(&display_uname(md, config), padding.longest_uname_len),
+            );
+        }
+
+        let dfn = display_file_name(item, config, None, out).contents;
+
+        let _ = writeln!(
+            out,
+            " {} {} {}",
+            pad_left(&display_size_or_rdev(md, config), padding.longest_size_len),
+            display_date(md, config),
+            dfn,
+        );
+    } else {
+        // this 'else' is expressly for the case of a dangling symlink
+        #[cfg(unix)]
+        {
+            if config.inode {
+                let _ = write!(out, "{} ", pad_left("?", padding.longest_inode_len),);
+            }
+        }
 
-    if config.long.owner {
         let _ = write!(
             out,
-            " {}",
-            pad_right(&display_uname(md, config), padding.longest_uname_len)
+            "{}{} {}",
+            "l?????????".to_string(),
+            if item.security_context.len() > 1 {
+                // GNU `ls` uses a "." character to indicate a file with a security context,
+                // but not other alternate access method.
+                "."
+            } else {
+                ""
+            },
+            pad_left("", padding.longest_link_count_len),
         );
-    }
 
-    if config.long.group {
-        let _ = write!(
+        if config.long.owner {
+            let _ = write!(out, " {}", pad_right("?", padding.longest_uname_len));
+        }
+
+        if config.long.group {
+            let _ = write!(out, " {}", pad_right("?", padding.longest_group_len));
+        }
+
+        if config.context {
+            let _ = write!(
+                out,
+                " {}",
+                pad_right(&item.security_context, padding.longest_context_len)
+            );
+        }
+
+        // Author is only different from owner on GNU/Hurd, so we reuse
+        // the owner, since GNU/Hurd is not currently supported by Rust.
+        if config.long.author {
+            let _ = write!(out, " {}", pad_right("?", padding.longest_uname_len));
+        }
+
+        let dfn = display_file_name(item, config, None, out).contents;
+        let date_len = 12;
+
+        let _ = writeln!(
             out,
-            " {}",
-            pad_right(&display_group(md, config), padding.longest_group_len)
+            " {} {} {}",
+            pad_left("?", padding.longest_size_len),
+            pad_left("?", date_len),
+            dfn,
         );
     }
-
-    if config.context {
-        let _ = write!(
-            out,
-            " {}",
-            pad_right(&item.security_context, padding.longest_context_len)
-        );
-    }
-
-    // Author is only different from owner on GNU/Hurd, so we reuse
-    // the owner, since GNU/Hurd is not currently supported by Rust.
-    if config.long.author {
-        let _ = write!(
-            out,
-            " {}",
-            pad_right(&display_uname(md, config), padding.longest_uname_len)
-        );
-    }
-
-    let _ = writeln!(
-        out,
-        " {} {} {}",
-        pad_left(&display_size_or_rdev(md, config), padding.longest_size_len),
-        display_date(md, config),
-        // unwrap is fine because it fails when metadata is not available
-        // but we already know that it is because it's checked at the
-        // start of the function.
-        display_file_name(item, config, None).unwrap().contents,
-    );
 }
 
 #[cfg(unix)]
@@ -1911,7 +2085,7 @@ fn display_size_or_rdev(metadata: &Metadata, config: &Config) -> String {
             let dev: u64 = metadata.rdev();
             let major = (dev >> 8) as u8;
             let minor = dev as u8;
-            return format!("{}, {}", major, minor);
+            return format!("{}, {}", major, minor,);
         }
     }
 
@@ -1952,7 +2126,7 @@ fn classify_file(path: &PathData) -> Option<char> {
                 Some('=')
             } else if file_type.is_fifo() {
                 Some('|')
-            } else if file_type.is_file() && file_is_executable(path.md()?) {
+            } else if file_type.is_file() && file_is_executable(path.md().as_ref().unwrap()) {
                 Some('*')
             } else {
                 None
@@ -1976,27 +2150,44 @@ fn classify_file(path: &PathData) -> Option<char> {
 ///
 /// Note that non-unicode sequences in symlink targets are dealt with using
 /// [`std::path::Path::to_string_lossy`].
+#[allow(unused_variables)]
 fn display_file_name(
     path: &PathData,
     config: &Config,
     prefix_context: Option<usize>,
-) -> Option<Cell> {
+    out: &mut BufWriter<Stdout>,
+) -> Cell {
     // This is our return value. We start by `&path.display_name` and modify it along the way.
     let mut name = escape_name(&path.display_name, &config.quoting_style);
 
-    #[cfg(unix)]
-    {
-        if config.format != Format::Long && config.inode {
-            name = path.md().map_or_else(|| "?".to_string(), get_inode) + " " + &name;
-        }
-    }
-
     // We need to keep track of the width ourselves instead of letting term_grid
     // infer it because the color codes mess up term_grid's width calculation.
     let mut width = name.width();
 
     if let Some(ls_colors) = &config.color {
-        name = color_name(ls_colors, &path.p_buf, name, path.md()?);
+        if let Ok(metadata) = path.p_buf.symlink_metadata() {
+            name = color_name(ls_colors, &path.p_buf, name, &metadata);
+        }
+    }
+
+    #[cfg(unix)]
+    {
+        if config.inode && config.format != Format::Long {
+            let inode = if let Some(md) = path.md() {
+                get_inode(md)
+            } else {
+                let _ = out.flush();
+                let show_error = show!(LsError::IOErrorContext(
+                    std::io::Error::new(ErrorKind::NotFound, "NotFound"),
+                    path.p_buf.clone(),
+                ));
+                "?".to_string()
+            };
+            // increment width here b/c name was given colors and name.width() is now the wrong
+            // size for display
+            width += inode.width();
+            name = inode + " " + &name;
+        }
     }
 
     if config.indicator_style != IndicatorStyle::None {
@@ -2027,7 +2218,10 @@ fn display_file_name(
         }
     }
 
-    if config.format == Format::Long && path.file_type()?.is_symlink() {
+    if config.format == Format::Long
+        && path.file_type().is_some()
+        && path.file_type().unwrap().is_symlink()
+    {
         if let Ok(target) = path.p_buf.read_link() {
             name.push_str(" -> ");
 
@@ -2050,17 +2244,21 @@ fn display_file_name(
                 // Because we use an absolute path, we can assume this is guaranteed to exist.
                 // Otherwise, we use path.md(), which will guarantee we color to the same
                 // color of non-existent symlinks according to style_for_path_with_metadata.
-                let target_metadata = match target_data.md() {
-                    Some(md) => md,
-                    None => path.md()?,
-                };
+                if path.md().is_none() && target_data.md().is_none() {
+                    name.push_str(&path.p_buf.read_link().unwrap().to_string_lossy());
+                } else {
+                    let target_metadata = match target_data.md() {
+                        Some(md) => md,
+                        None => path.md().unwrap(),
+                    };
 
-                name.push_str(&color_name(
-                    ls_colors,
-                    &target_data.p_buf,
-                    target.to_string_lossy().into_owned(),
-                    target_metadata,
-                ));
+                    name.push_str(&color_name(
+                        ls_colors,
+                        &target_data.p_buf,
+                        target.to_string_lossy().into_owned(),
+                        target_metadata,
+                    ));
+                }
             } else {
                 // If no coloring is required, we just use target as is.
                 name.push_str(&target.to_string_lossy());
@@ -2082,10 +2280,10 @@ fn display_file_name(
         }
     }
 
-    Some(Cell {
+    Cell {
         contents: name,
         width,
-    })
+    }
 }
 
 fn color_name(ls_colors: &LsColors, path: &Path, name: String, md: &Metadata) -> String {
@@ -2107,6 +2305,18 @@ fn display_symlink_count(metadata: &Metadata) -> String {
     metadata.nlink().to_string()
 }
 
+#[allow(unused_variables)]
+fn display_inode(metadata: &Metadata) -> String {
+    #[cfg(unix)]
+    {
+        get_inode(metadata)
+    }
+    #[cfg(not(unix))]
+    {
+        "".to_string()
+    }
+}
+
 // This returns the SELinux security context as UTF8 `String`.
 // In the long term this should be changed to `OsStr`, see discussions at #2621/#2656
 #[allow(unused_variables)]
@@ -2115,7 +2325,7 @@ fn get_security_context(config: &Config, p_buf: &Path, must_dereference: bool) -
     if config.selinux_supported {
         #[cfg(feature = "selinux")]
         {
-            match selinux::SecurityContext::of_path(p_buf, must_dereference, false) {
+            match selinux::SecurityContext::of_path(p_buf, must_dereference.to_owned(), false) {
                 Err(_r) => {
                     // TODO: show the actual reason why it failed
                     show_warning!("failed to get security context of: {}", p_buf.quote());

tests/by-util/test_ls.rs

@@ -11,7 +11,6 @@ use std::collections::HashMap;
 use std::path::Path;
 use std::thread::sleep;
 use std::time::Duration;
-
 #[cfg(not(windows))]
 extern crate libc;
 #[cfg(not(windows))]
@@ -39,6 +38,75 @@ fn test_ls_i() {
     new_ucmd!().arg("-il").succeeds();
 }
 
+#[test]
+fn test_ls_ordering() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+    at.mkdir("some-dir1");
+    at.mkdir("some-dir2");
+    at.mkdir("some-dir3");
+    at.mkdir("some-dir4");
+    at.mkdir("some-dir5");
+    at.mkdir("some-dir6");
+
+    scene
+        .ucmd()
+        .arg("-Rl")
+        .succeeds()
+        .stdout_matches(&Regex::new("some-dir1:\\ntotal 0").unwrap());
+}
+
+#[cfg(all(feature = "chmod"))]
+#[test]
+fn test_ls_io_errors() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+    at.mkdir("some-dir1");
+    at.mkdir("some-dir2");
+    at.symlink_file("does_not_exist", "some-dir2/dangle");
+    at.mkdir("some-dir3");
+    at.mkdir("some-dir3/some-dir4");
+    at.mkdir("some-dir3/some-dir5");
+    at.mkdir("some-dir3/some-dir6");
+    at.mkdir("some-dir3/some-dir7");
+    at.mkdir("some-dir3/some-dir8");
+
+    scene.ccmd("chmod").arg("000").arg("some-dir1").succeeds();
+
+    scene
+        .ucmd()
+        .arg("-1")
+        .arg("some-dir1")
+        .fails()
+        .stderr_contains("cannot open directory")
+        .stderr_contains("Permission denied");
+
+    scene
+        .ucmd()
+        .arg("-Li")
+        .arg("some-dir2")
+        .fails()
+        .stderr_contains("cannot access")
+        .stderr_contains("No such file or directory")
+        .stdout_contains(if cfg!(windows) { "dangle" } else { "? dangle" });
+
+    scene
+        .ccmd("chmod")
+        .arg("000")
+        .arg("some-dir3/some-dir4")
+        .succeeds();
+
+    scene
+        .ucmd()
+        .arg("-laR")
+        .arg("some-dir3")
+        .fails()
+        .stderr_contains("some-dir4")
+        .stderr_contains("cannot open directory")
+        .stderr_contains("Permission denied")
+        .stdout_contains("some-dir4");
+}
+
 #[test]
 fn test_ls_walk_glob() {
     let scene = TestScenario::new(util_name!());
@@ -2303,8 +2371,16 @@ fn test_ls_dangling_symlinks() {
         .ucmd()
         .arg("-Li")
         .arg("temp_dir")
-        .succeeds() // this should fail, though at the moment, ls lacks a way to propagate errors encountered during display
+        .fails()
+        .stderr_contains("cannot access")
         .stdout_contains(if cfg!(windows) { "dangle" } else { "? dangle" });
+
+    scene
+        .ucmd()
+        .arg("-Ll")
+        .arg("temp_dir")
+        .fails()
+        .stdout_contains("l?????????");
 }
 
 #[test]