From 97a0c06ff46b8bf05039830f6e4ff2411cfbbe44 Mon Sep 17 00:00:00 2001 From: Mahmoud Soltan Date: Mon, 30 Aug 2021 23:09:16 +0200 Subject: [PATCH] Proper columns for `ls -l` (#2623) * Used .as_path() and .as_str() when required: when the argument required is a Path and not a PathBuf, or an str and not a Path, respectively. * Changed display_items to take Vec, which is passed, instead of [PathData] * Added a pad_right function. * Implemented column-formating to mimic the behavior of GNU coreutils's ls Added returns in display_dir_entry_size that keep track of uname and group lengths. Renamed variables to make more sense. Changed display_item_long to take all the lengths it needs to render correctly. Implemented owner, group, and author padding right to mimic GNU ls. * Added a todo for future quality-of-life cache addition. * Documented display_item_long, as a first step in documenting all functions. * Revert "Used .as_path() and .as_str() when required:" This reverts commit b88db6a8170f827a7adc58de14acb59f19be2db1. * Revert "Changed display_items to take Vec, which is passed, instead of [PathData]" This reverts commit 0c690dda8d5eb1b257afb4c74d9c2dfc1f7cc97b. * Ran cargo fmt to get rid of Style/format `fmt` testing error. * Added a test for `ls -l` and `ls -lan` line formats. * Changed uname -> username for cspell. Removed extra blank line for rustfmt. --- src/uu/ls/src/ls.rs | 102 +++++++++++++++++++++++++++++++++------ tests/by-util/test_ls.rs | 55 +++++++++++++++++++++ 2 files changed, 143 insertions(+), 14 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 3d957474c..bf449d96b 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1394,14 +1394,17 @@ fn get_metadata(entry: &Path, dereference: bool) -> std::io::Result { } } -fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize) { +fn display_dir_entry_size(entry: &PathData, config: &Config) -> (usize, usize, usize, usize) { + // TODO: Cache/memoize the display_* results so we don't have to recalculate them. if let Some(md) = entry.md() { ( display_symlink_count(md).len(), + display_uname(md, config).len(), + display_group(md, config).len(), display_size_or_rdev(md, config).len(), ) } else { - (0, 0) + (0, 0, 0, 0) } } @@ -1409,15 +1412,28 @@ fn pad_left(string: String, count: usize) -> String { format!("{:>width$}", string, width = count) } +fn pad_right(string: String, count: usize) -> String { + format!("{:) { if config.format == Format::Long { - let (mut max_links, mut max_width) = (1, 1); + let ( + mut longest_link_count_len, + mut longest_uname_len, + mut longest_group_len, + mut longest_size_len, + ) = (1, 1, 1, 1); let mut total_size = 0; for item in items { - let (links, width) = display_dir_entry_size(item, config); - max_links = links.max(max_links); - max_width = width.max(max_width); + let (link_count_len, uname_len, group_len, size_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); + longest_uname_len = uname_len.max(longest_uname_len); + longest_group_len = group_len.max(longest_group_len); + longest_size_len = size_len.max(longest_size_len); total_size += item.md().map_or(0, |md| get_block_size(md, config)); } @@ -1426,7 +1442,15 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter, ) { @@ -1557,27 +1619,39 @@ fn display_item_long( out, "{} {}", display_permissions(md, true), - pad_left(display_symlink_count(md), max_links), + pad_left(display_symlink_count(md), longest_link_count_len), ); if config.long.owner { - let _ = write!(out, " {}", display_uname(md, config)); + let _ = write!( + out, + " {}", + pad_right(display_uname(md, config), longest_uname_len) + ); } if config.long.group { - let _ = write!(out, " {}", display_group(md, config)); + let _ = write!( + out, + " {}", + pad_right(display_group(md, config), longest_group_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, " {}", display_uname(md, config)); + let _ = write!( + out, + " {}", + pad_right(display_uname(md, config), longest_uname_len) + ); } let _ = writeln!( out, " {} {} {}", - pad_left(display_size_or_rdev(md, config), max_size), + pad_left(display_size_or_rdev(md, config), 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 diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index a3372050a..546fb86a8 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -333,6 +333,61 @@ fn test_ls_long() { } } +#[test] +fn test_ls_long_format() { + #[cfg(not(windows))] + let last; + #[cfg(not(windows))] + { + let _guard = UMASK_MUTEX.lock(); + last = unsafe { umask(0) }; + + unsafe { + umask(0o002); + } + } + + let scene = TestScenario::new(util_name!()); + let at = &scene.fixtures; + at.mkdir(&at.plus_as_string("test-long-dir")); + at.touch(&at.plus_as_string("test-long-dir/test-long-file")); + at.mkdir(&at.plus_as_string("test-long-dir/test-long-dir")); + + for arg in &["-l", "--long", "--format=long", "--format=verbose"] { + let result = scene.ucmd().arg(arg).arg("test-long-dir").succeeds(); + // Assuming sane username do not have spaces within them. + // A line of the output should be: + // One of the characters -bcCdDlMnpPsStTx? + // rwx, with - for missing permissions, thrice. + // A number, preceded by column whitespace, and followed by a single space. + // A username, currently [^ ], followed by column whitespace, twice (or thrice for Hurd). + // A number, followed by a single space. + // A month, followed by a single space. + // A day, preceded by column whitespace, and followed by a single space. + // Either a year or a time, currently [0-9:]+, preceded by column whitespace, + // and followed by a single space. + // Whatever comes after is irrelevant to this specific test. + #[cfg(not(windows))] + result.stdout_matches(&Regex::new( + r"\n[-bcCdDlMnpPsStTx?]([r-][w-][xt-]){3} +\d+ [^ ]+ +[^ ]+( +[^ ]+)? +\d+ [A-Z][a-z]{2} {0,2}\d{0,2} {0,2}[0-9:]+ " + ).unwrap()); + } + + let result = scene.ucmd().arg("-lan").arg("test-long-dir").succeeds(); + // This checks for the line with the .. entry. The uname and group should be digits. + #[cfg(not(windows))] + result.stdout_matches(&Regex::new( + r"\nd([r-][w-][xt-]){3} +\d+ \d+ +\d+( +\d+)? +\d+ [A-Z][a-z]{2} {0,2}\d{0,2} {0,2}[0-9:]+ \.\." + ).unwrap()); + + #[cfg(not(windows))] + { + unsafe { + umask(last); + } + } +} + #[test] fn test_ls_long_total_size() { let scene = TestScenario::new(util_name!());