ls: Fix device display (#2855)

src/uu/ls/src/ls.rs

@@ -319,6 +319,10 @@ struct PaddingCollection {
     longest_group_len: usize,
     longest_context_len: usize,
     longest_size_len: usize,
+    #[cfg(unix)]
+    longest_major_len: usize,
+    #[cfg(unix)]
+    longest_minor_len: usize,
 }
 
 impl Config {
@@ -1560,18 +1564,28 @@ fn display_dir_entry_size(
     entry: &PathData,
     config: &Config,
     out: &mut BufWriter<std::io::Stdout>,
-) -> (usize, usize, usize, usize, usize) {
+) -> (usize, usize, 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(out) {
+        let (size_len, major_len, minor_len) = match display_size_or_rdev(md, config) {
+            SizeOrDeviceId::Device(major, minor) => (
+                (major.len() + minor.len() + 2usize),
+                major.len(),
+                minor.len(),
+            ),
+            SizeOrDeviceId::Size(size) => (size.len(), 0usize, 0usize),
+        };
         (
             display_symlink_count(md).len(),
             display_uname(md, config).len(),
             display_group(md, config).len(),
-            display_size_or_rdev(md, config).len(),
+            size_len,
+            major_len,
+            minor_len,
             display_inode(md).len(),
         )
     } else {
-        (0, 0, 0, 0, 0)
+        (0, 0, 0, 0, 0, 0, 0)
     }
 }
 
@@ -1608,7 +1622,9 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
             mut longest_group_len,
             mut longest_context_len,
             mut longest_size_len,
-        ) = (1, 1, 1, 1, 1, 1);
+            mut longest_major_len,
+            mut longest_minor_len,
+        ) = (1, 1, 1, 1, 1, 1, 1, 1);
 
         #[cfg(not(unix))]
         let (
@@ -1622,26 +1638,41 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
         #[cfg(unix)]
         for item in items {
             let context_len = item.security_context.len();
-            let (link_count_len, uname_len, group_len, size_len, inode_len) =
+            let (link_count_len, uname_len, group_len, size_len, major_len, minor_len, inode_len) =
                 display_dir_entry_size(item, config, out);
             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);
+            if items.len() == 1usize {
+                longest_size_len = 0usize;
+                longest_major_len = 0usize;
+                longest_minor_len = 0usize;
+            } else {
+                longest_major_len = major_len.max(longest_major_len);
+                longest_minor_len = minor_len.max(longest_minor_len);
+                longest_size_len = size_len
+                    .max(longest_size_len)
+                    .max(longest_major_len + longest_minor_len + 2usize);
+            }
         }
 
         #[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) =
+            let (
-                display_dir_entry_size(item, config, out);
+                link_count_len,
+                uname_len,
+                group_len,
+                size_len,
+                _major_len,
+                _minor_len,
+                _inode_len,
+            ) = display_dir_entry_size(item, config, out);
             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 {
@@ -1661,6 +1692,10 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
                     longest_group_len,
                     longest_context_len,
                     longest_size_len,
+                    #[cfg(unix)]
+                    longest_major_len,
+                    #[cfg(unix)]
+                    longest_minor_len,
                 },
                 config,
                 out,
@@ -1894,15 +1929,39 @@ fn display_item_long(
             );
         }
 
+        match display_size_or_rdev(md, config) {
+            SizeOrDeviceId::Size(size) => {
+                let _ = write!(out, " {}", pad_left(&size, padding.longest_size_len),);
+            }
+            SizeOrDeviceId::Device(major, minor) => {
+                let _ = write!(
+                    out,
+                    " {}, {}",
+                    pad_left(
+                        &major,
+                        #[cfg(not(unix))]
+                        0usize,
+                        #[cfg(unix)]
+                        padding.longest_major_len.max(
+                            padding
+                                .longest_size_len
+                                .saturating_sub(padding.longest_minor_len.saturating_add(2usize))
+                        )
+                    ),
+                    pad_left(
+                        &minor,
+                        #[cfg(not(unix))]
+                        0usize,
+                        #[cfg(unix)]
+                        padding.longest_minor_len,
+                    ),
+                );
+            }
+        };
+
         let dfn = display_file_name(item, config, None, 0, out).contents;
 
-        let _ = writeln!(
+        let _ = writeln!(out, " {} {}", display_date(md, config), dfn);
-            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/restricted file
         #[cfg(unix)]
@@ -2112,19 +2171,35 @@ fn format_prefixed(prefixed: NumberPrefix<f64>) -> String {
     }
 }
 
-fn display_size_or_rdev(metadata: &Metadata, config: &Config) -> String {
+#[allow(dead_code)]
-    #[cfg(unix)]
+enum SizeOrDeviceId {
+    Size(String),
+    Device(String, String),
+}
+
+fn display_size_or_rdev(metadata: &Metadata, config: &Config) -> SizeOrDeviceId {
+    #[cfg(any(target_os = "macos", target_os = "ios"))]
+    {
+        let ft = metadata.file_type();
+        if ft.is_char_device() || ft.is_block_device() {
+            let dev: u64 = metadata.rdev();
+            let major = (dev >> 24) as u8;
+            let minor = (dev & 0xff) as u8;
+            return SizeOrDeviceId::Device(major.to_string(), minor.to_string());
+        }
+    }
+    #[cfg(target_os = "linux")]
     {
         let ft = metadata.file_type();
         if ft.is_char_device() || ft.is_block_device() {
             let dev: u64 = metadata.rdev();
             let major = (dev >> 8) as u8;
-            let minor = dev as u8;
+            let minor = (dev & 0xff) as u8;
-            return format!("{}, {}", major, minor,);
+            return SizeOrDeviceId::Device(major.to_string(), minor.to_string());
         }
     }
 
-    display_size(metadata.len(), config)
+    SizeOrDeviceId::Size(display_size(metadata.len(), config))
 }
 
 fn display_size(size: u64, config: &Config) -> String {

tests/by-util/test_ls.rs

@@ -56,8 +56,70 @@ fn test_ls_ordering() {
         .stdout_matches(&Regex::new("some-dir1:\\ntotal 0").unwrap());
 }
 
+//#[cfg(all(feature = "mknod"))]
+#[test]
+fn test_ls_devices() {
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
+    at.mkdir("some-dir1");
+
+    // Regex tests correct device ID and correct (no pad) spacing for a single file
+    #[cfg(any(target_os = "macos", target_os = "ios"))]
+    {
+        scene
+            .ucmd()
+            .arg("-al")
+            .arg("/dev/null")
+            .succeeds()
+            .stdout_matches(&Regex::new("[^ ] 3, 2 [^ ]").unwrap());
+    }
+
+    #[cfg(target_os = "linux")]
+    {
+        scene
+            .ucmd()
+            .arg("-al")
+            .arg("/dev/null")
+            .succeeds()
+            .stdout_matches(&Regex::new("[^ ] 1, 3 [^ ]").unwrap());
+    }
+
+    // Regex tests alignment against a file (stdout is a link to a tty)
+    #[cfg(unix)]
+    {
+        let res = scene
+            .ucmd()
+            .arg("-alL")
+            .arg("/dev/null")
+            .arg("/dev/stdout")
+            .succeeds();
+
+        let null_len = String::from_utf8(res.stdout().to_owned())
+            .ok()
+            .unwrap()
+            .lines()
+            .next()
+            .unwrap()
+            .strip_suffix("/dev/null")
+            .unwrap()
+            .len();
+
+        let stdout_len = String::from_utf8(res.stdout().to_owned())
+            .ok()
+            .unwrap()
+            .lines()
+            .nth(1)
+            .unwrap()
+            .strip_suffix("/dev/stdout")
+            .unwrap()
+            .len();
+
+        assert_eq!(stdout_len, null_len);
+    }
+}
+
+#[cfg(all(feature = "chmod"))]
 #[test]
-#[cfg(feature = "chmod")]
 fn test_ls_io_errors() {
     let scene = TestScenario::new(util_name!());
     let at = &scene.fixtures;