From 13a62489c5db23a1687272a5d59f80cc4556fa92 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Wed, 11 Aug 2021 14:32:46 +0200 Subject: [PATCH 1/3] ls: correct fallbacks for terminal width If options::WIDTH is not given, we should try to use the terminal width. If that is unavailable, we should fall back to the 'COLUMNS' environment variable. If that is unavailable (or invalid), we should fall back to a default of 80. --- src/uu/ls/src/ls.rs | 38 ++++++++++++------ tests/by-util/test_ls.rs | 87 +++++++++++----------------------------- 2 files changed, 49 insertions(+), 76 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index ef314dfa8..44a374dbe 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -127,6 +127,8 @@ pub mod options { pub static IGNORE: &str = "ignore"; } +const DEFAULT_TERM_WIDTH: u16 = 80; + #[derive(Debug)] enum LsError { InvalidLineWidth(String), @@ -229,7 +231,7 @@ struct Config { inode: bool, color: Option, long: LongFormat, - width: Option, + width: u16, quoting_style: QuotingStyle, indicator_style: IndicatorStyle, time_style: TimeStyle, @@ -399,10 +401,25 @@ impl Config { let width = match options.value_of(options::WIDTH) { Some(x) => match x.parse::() { - Ok(u) => Some(u), + Ok(u) => u, Err(_) => return Err(LsError::InvalidLineWidth(x.into()).into()), }, - None => termsize::get().map(|s| s.cols), + None => match termsize::get() { + Some(size) => size.cols, + None => match std::env::var("COLUMNS") { + Ok(columns) => match columns.parse() { + Ok(columns) => columns, + Err(_) => { + show_error!( + "ignoring invalid width in environment variable COLUMNS: '{}'", + columns + ); + DEFAULT_TERM_WIDTH + } + }, + Err(_) => DEFAULT_TERM_WIDTH, + }, + }, }; #[allow(clippy::needless_bool)] @@ -1411,15 +1428,10 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter { - display_grid(names, width, Direction::TopToBottom, out) - } - (Format::Across, Some(width)) => { - display_grid(names, width, Direction::LeftToRight, out) - } - (Format::Commas, width_opt) => { - let term_width = width_opt.unwrap_or(1); + match config.format { + Format::Columns => display_grid(names, config.width, Direction::TopToBottom, out), + Format::Across => display_grid(names, config.width, Direction::LeftToRight, out), + Format::Commas => { let mut current_col = 0; let mut names = names; if let Some(name) = names.next() { @@ -1428,7 +1440,7 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter term_width { + if current_col + name_width + 1 > config.width { current_col = name_width + 2; let _ = write!(out, ",\n{}", name.contents); } else { diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 44d14c304..0a19a44fa 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -184,16 +184,10 @@ fn test_ls_columns() { // Columns is the default let result = scene.ucmd().succeeds(); - #[cfg(not(windows))] - result.stdout_only("test-columns-1\ntest-columns-2\ntest-columns-3\ntest-columns-4\n"); - #[cfg(windows)] result.stdout_only("test-columns-1 test-columns-2 test-columns-3 test-columns-4\n"); for option in &["-C", "--format=columns"] { let result = scene.ucmd().arg(option).succeeds(); - #[cfg(not(windows))] - result.stdout_only("test-columns-1\ntest-columns-2\ntest-columns-3\ntest-columns-4\n"); - #[cfg(windows)] result.stdout_only("test-columns-1 test-columns-2 test-columns-3 test-columns-4\n"); } @@ -205,6 +199,22 @@ fn test_ls_columns() { .succeeds() .stdout_only("test-columns-1 test-columns-3\ntest-columns-2 test-columns-4\n"); } + + for option in &["-C", "--format=columns"] { + scene + .ucmd() + .env("COLUMNS", "40") + .arg(option) + .succeeds() + .stdout_only("test-columns-1 test-columns-3\ntest-columns-2 test-columns-4\n"); + } + + scene + .ucmd() + .env("COLUMNS", "garbage") + .succeeds() + .stdout_is("test-columns-1 test-columns-2 test-columns-3 test-columns-4\n") + .stderr_is("ls: ignoring invalid width in environment variable COLUMNS: 'garbage'"); } #[test] @@ -220,11 +230,7 @@ fn test_ls_across() { let result = scene.ucmd().arg(option).succeeds(); // Because the test terminal has width 0, this is the same output as // the columns option. - if cfg!(unix) { - result.stdout_only("test-across-1\ntest-across-2\ntest-across-3\ntest-across-4\n"); - } else { - result.stdout_only("test-across-1 test-across-2 test-across-3 test-across-4\n"); - } + result.stdout_only("test-across-1 test-across-2 test-across-3 test-across-4\n"); } for option in &["-x", "--format=across"] { @@ -250,11 +256,7 @@ fn test_ls_commas() { for option in &["-m", "--format=commas"] { let result = scene.ucmd().arg(option).succeeds(); - if cfg!(unix) { - result.stdout_only("test-commas-1,\ntest-commas-2,\ntest-commas-3,\ntest-commas-4\n"); - } else { - result.stdout_only("test-commas-1, test-commas-2, test-commas-3, test-commas-4\n"); - } + result.stdout_only("test-commas-1, test-commas-2, test-commas-3, test-commas-4\n"); } for option in &["-m", "--format=commas"] { @@ -571,13 +573,11 @@ fn test_ls_sort_name() { at.touch("test-1"); at.touch("test-2"); - let sep = if cfg!(unix) { "\n" } else { " " }; - scene .ucmd() .arg("--sort=name") .succeeds() - .stdout_is(["test-1", "test-2", "test-3\n"].join(sep)); + .stdout_is("test-1 test-2 test-3\n"); let scene_dot = TestScenario::new(util_name!()); let at = &scene_dot.fixtures; @@ -591,7 +591,7 @@ fn test_ls_sort_name() { .arg("--sort=name") .arg("-A") .succeeds() - .stdout_is([".a", ".b", "a", "b\n"].join(sep)); + .stdout_is(".a .b a b\n"); } #[test] @@ -612,27 +612,15 @@ fn test_ls_order_size() { scene.ucmd().arg("-al").succeeds(); let result = scene.ucmd().arg("-S").succeeds(); - #[cfg(not(windows))] - result.stdout_only("test-4\ntest-3\ntest-2\ntest-1\n"); - #[cfg(windows)] result.stdout_only("test-4 test-3 test-2 test-1\n"); let result = scene.ucmd().arg("-S").arg("-r").succeeds(); - #[cfg(not(windows))] - result.stdout_only("test-1\ntest-2\ntest-3\ntest-4\n"); - #[cfg(windows)] result.stdout_only("test-1 test-2 test-3 test-4\n"); let result = scene.ucmd().arg("--sort=size").succeeds(); - #[cfg(not(windows))] - result.stdout_only("test-4\ntest-3\ntest-2\ntest-1\n"); - #[cfg(windows)] result.stdout_only("test-4 test-3 test-2 test-1\n"); let result = scene.ucmd().arg("--sort=size").arg("-r").succeeds(); - #[cfg(not(windows))] - result.stdout_only("test-1\ntest-2\ntest-3\ntest-4\n"); - #[cfg(windows)] result.stdout_only("test-1 test-2 test-3 test-4\n"); } @@ -755,9 +743,6 @@ fn test_ls_styles() { at.touch("test2"); let result = scene.ucmd().arg("--full-time").arg("-x").succeeds(); - #[cfg(not(windows))] - assert_eq!(result.stdout_str(), "test\ntest2\n"); - #[cfg(windows)] assert_eq!(result.stdout_str(), "test test2\n"); } @@ -794,27 +779,15 @@ fn test_ls_order_time() { // ctime was changed at write, so the order is 4 3 2 1 let result = scene.ucmd().arg("-t").succeeds(); - #[cfg(not(windows))] - result.stdout_only("test-4\ntest-3\ntest-2\ntest-1\n"); - #[cfg(windows)] result.stdout_only("test-4 test-3 test-2 test-1\n"); let result = scene.ucmd().arg("--sort=time").succeeds(); - #[cfg(not(windows))] - result.stdout_only("test-4\ntest-3\ntest-2\ntest-1\n"); - #[cfg(windows)] result.stdout_only("test-4 test-3 test-2 test-1\n"); let result = scene.ucmd().arg("-tr").succeeds(); - #[cfg(not(windows))] - result.stdout_only("test-1\ntest-2\ntest-3\ntest-4\n"); - #[cfg(windows)] result.stdout_only("test-1 test-2 test-3 test-4\n"); let result = scene.ucmd().arg("--sort=time").arg("-r").succeeds(); - #[cfg(not(windows))] - result.stdout_only("test-1\ntest-2\ntest-3\ntest-4\n"); - #[cfg(windows)] result.stdout_only("test-1 test-2 test-3 test-4\n"); // 3 was accessed last in the read @@ -826,19 +799,11 @@ fn test_ls_order_time() { // It seems to be dependent on the platform whether the access time is actually set if file3_access > file4_access { - if cfg!(not(windows)) { - result.stdout_only("test-3\ntest-4\ntest-2\ntest-1\n"); - } else { - result.stdout_only("test-3 test-4 test-2 test-1\n"); - } + result.stdout_only("test-3 test-4 test-2 test-1\n"); } else { // Access time does not seem to be set on Windows and some other // systems so the order is 4 3 2 1 - if cfg!(not(windows)) { - result.stdout_only("test-4\ntest-3\ntest-2\ntest-1\n"); - } else { - result.stdout_only("test-4 test-3 test-2 test-1\n"); - } + result.stdout_only("test-4 test-3 test-2 test-1\n"); } } @@ -847,7 +812,7 @@ fn test_ls_order_time() { #[cfg(unix)] { let result = scene.ucmd().arg("-tc").succeeds(); - result.stdout_only("test-2\ntest-4\ntest-3\ntest-1\n"); + result.stdout_only("test-2 test-4 test-3 test-1\n"); } } @@ -2009,11 +1974,7 @@ fn test_ls_path() { }; scene.ucmd().arg(&abs_path).run().stdout_is(expected_stdout); - let expected_stdout = if cfg!(windows) { - format!("{} {}\n", path, file1) - } else { - format!("{}\n{}\n", path, file1) - }; + let expected_stdout = format!("{} {}\n", path, file1); scene .ucmd() .arg(file1) From 988cc49d4a171e76c46e9ea25a4daf37c32196b1 Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Wed, 11 Aug 2021 15:53:39 +0200 Subject: [PATCH 2/3] ls: print a single line when width is set to 0 This means that we treat a width=0 as infinite width. --- src/uu/ls/src/ls.rs | 48 +++++++++++++++++++++++++------------- tests/by-util/test_ls.rs | 50 +++++++++++++++++++++++++--------------- 2 files changed, 64 insertions(+), 34 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 44a374dbe..8183f3bb8 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1440,7 +1440,8 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter config.width { + // If the width is 0 we print one single line + if config.width != 0 && current_col + name_width + 1 > config.width { current_col = name_width + 2; let _ = write!(out, ",\n{}", name.contents); } else { @@ -1492,22 +1493,37 @@ fn display_grid( direction: Direction, out: &mut BufWriter, ) { - let mut grid = Grid::new(GridOptions { - filling: Filling::Spaces(2), - direction, - }); - - for name in names { - grid.add(name); - } - - match grid.fit_into_width(width as usize) { - Some(output) => { - let _ = write!(out, "{}", output); + if width == 0 { + // If the width is 0 we print one single line + let mut printed_something = false; + for name in names { + if printed_something { + let _ = write!(out, " "); + } + printed_something = true; + let _ = write!(out, "{}", name.contents); } - // Width is too small for the grid, so we fit it in one column - None => { - let _ = write!(out, "{}", grid.fit_into_columns(1)); + if printed_something { + let _ = writeln!(out); + } + } else { + let mut grid = Grid::new(GridOptions { + filling: Filling::Spaces(2), + direction, + }); + + for name in names { + grid.add(name); + } + + match grid.fit_into_width(width as usize) { + Some(output) => { + let _ = write!(out, "{}", output); + } + // Width is too small for the grid, so we fit it in one column + None => { + let _ = write!(out, "{}", grid.fit_into_columns(1)); + } } } } diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 0a19a44fa..bc3d89c23 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -140,16 +140,7 @@ fn test_ls_width() { .stdout_only("test-width-1 test-width-3\ntest-width-2 test-width-4\n"); } - for option in &[ - "-w 25", - "-w=25", - "--width=25", - "--width 25", - "-w 0", - "-w=0", - "--width=0", - "--width 0", - ] { + for option in &["-w 25", "-w=25", "--width=25", "--width 25"] { scene .ucmd() .args(&option.split(' ').collect::>()) @@ -157,6 +148,14 @@ fn test_ls_width() { .stdout_only("test-width-1\ntest-width-2\ntest-width-3\ntest-width-4\n"); } + for option in &["-w 0", "-w=0", "--width=0", "--width 0"] { + scene + .ucmd() + .args(&option.split(' ').collect::>()) + .succeeds() + .stdout_only("test-width-1 test-width-2 test-width-3 test-width-4\n"); + } + scene .ucmd() .arg("-w=bad") @@ -200,21 +199,36 @@ fn test_ls_columns() { .stdout_only("test-columns-1 test-columns-3\ntest-columns-2 test-columns-4\n"); } - for option in &["-C", "--format=columns"] { + // On windows we are always able to get the terminal size, so we can't simulate falling back to the + // environment variable. + #[cfg(not(windows))] + { + for option in &["-C", "--format=columns"] { + scene + .ucmd() + .env("COLUMNS", "40") + .arg(option) + .succeeds() + .stdout_only("test-columns-1 test-columns-3\ntest-columns-2 test-columns-4\n"); + } + scene .ucmd() - .env("COLUMNS", "40") - .arg(option) + .env("COLUMNS", "garbage") .succeeds() - .stdout_only("test-columns-1 test-columns-3\ntest-columns-2 test-columns-4\n"); + .stdout_is("test-columns-1 test-columns-2 test-columns-3 test-columns-4\n") + .stderr_is("ls: ignoring invalid width in environment variable COLUMNS: 'garbage'"); } - scene .ucmd() - .env("COLUMNS", "garbage") + .arg("-w0") .succeeds() - .stdout_is("test-columns-1 test-columns-2 test-columns-3 test-columns-4\n") - .stderr_is("ls: ignoring invalid width in environment variable COLUMNS: 'garbage'"); + .stdout_only("test-columns-1 test-columns-2 test-columns-3 test-columns-4\n"); + scene + .ucmd() + .arg("-mw0") + .succeeds() + .stdout_only("test-columns-1, test-columns-2, test-columns-3, test-columns-4\n"); } #[test] From 0af244ac426f4cb85ef9cab7f722b0c734694c5a Mon Sep 17 00:00:00 2001 From: Michael Debertol Date: Wed, 11 Aug 2021 22:03:41 +0200 Subject: [PATCH 3/3] ls: default to one-line output if stdout is not a tty --- src/uu/ls/src/ls.rs | 19 +++++++++++-------- tests/by-util/test_ls.rs | 40 ++++++++++++++++++++++++---------------- 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 8183f3bb8..5c58bac7c 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -260,16 +260,20 @@ impl Config { // below should never happen as clap already restricts the values. _ => unreachable!("Invalid field for --format"), }, - options::FORMAT, + Some(options::FORMAT), ) } else if options.is_present(options::format::LONG) { - (Format::Long, options::format::LONG) + (Format::Long, Some(options::format::LONG)) } else if options.is_present(options::format::ACROSS) { - (Format::Across, options::format::ACROSS) + (Format::Across, Some(options::format::ACROSS)) } else if options.is_present(options::format::COMMAS) { - (Format::Commas, options::format::COMMAS) + (Format::Commas, Some(options::format::COMMAS)) + } else if options.is_present(options::format::COLUMNS) { + (Format::Columns, Some(options::format::COLUMNS)) + } else if atty::is(atty::Stream::Stdout) { + (Format::Columns, None) } else { - (Format::Columns, options::format::COLUMNS) + (Format::OneLine, None) }; // The -o, -n and -g options are tricky. They cannot override with each @@ -288,9 +292,8 @@ impl Config { // options, but manually whether they have an index that's greater than // the other format options. If so, we set the appropriate format. if format != Format::Long { - let idx = options - .indices_of(opt) - .map(|x| x.max().unwrap()) + let idx = opt + .and_then(|opt| options.indices_of(opt).map(|x| x.max().unwrap())) .unwrap_or(0); if [ options::format::LONG_NO_OWNER, diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index bc3d89c23..a3372050a 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -128,6 +128,7 @@ fn test_ls_width() { scene .ucmd() .args(&option.split(' ').collect::>()) + .arg("-C") .succeeds() .stdout_only("test-width-1 test-width-2 test-width-3 test-width-4\n"); } @@ -136,6 +137,7 @@ fn test_ls_width() { scene .ucmd() .args(&option.split(' ').collect::>()) + .arg("-C") .succeeds() .stdout_only("test-width-1 test-width-3\ntest-width-2 test-width-4\n"); } @@ -144,6 +146,7 @@ fn test_ls_width() { scene .ucmd() .args(&option.split(' ').collect::>()) + .arg("-C") .succeeds() .stdout_only("test-width-1\ntest-width-2\ntest-width-3\ntest-width-4\n"); } @@ -152,6 +155,7 @@ fn test_ls_width() { scene .ucmd() .args(&option.split(' ').collect::>()) + .arg("-C") .succeeds() .stdout_only("test-width-1 test-width-2 test-width-3 test-width-4\n"); } @@ -159,6 +163,7 @@ fn test_ls_width() { scene .ucmd() .arg("-w=bad") + .arg("-C") .fails() .stderr_contains("invalid line width"); @@ -166,6 +171,7 @@ fn test_ls_width() { scene .ucmd() .args(&option.split(' ').collect::>()) + .arg("-C") .fails() .stderr_only("ls: invalid line width: '1a'"); } @@ -183,7 +189,7 @@ fn test_ls_columns() { // Columns is the default let result = scene.ucmd().succeeds(); - result.stdout_only("test-columns-1 test-columns-2 test-columns-3 test-columns-4\n"); + result.stdout_only("test-columns-1\ntest-columns-2\ntest-columns-3\ntest-columns-4\n"); for option in &["-C", "--format=columns"] { let result = scene.ucmd().arg(option).succeeds(); @@ -215,13 +221,14 @@ fn test_ls_columns() { scene .ucmd() .env("COLUMNS", "garbage") + .arg("-C") .succeeds() .stdout_is("test-columns-1 test-columns-2 test-columns-3 test-columns-4\n") .stderr_is("ls: ignoring invalid width in environment variable COLUMNS: 'garbage'"); } scene .ucmd() - .arg("-w0") + .arg("-Cw0") .succeeds() .stdout_only("test-columns-1 test-columns-2 test-columns-3 test-columns-4\n"); scene @@ -591,7 +598,7 @@ fn test_ls_sort_name() { .ucmd() .arg("--sort=name") .succeeds() - .stdout_is("test-1 test-2 test-3\n"); + .stdout_is("test-1\ntest-2\ntest-3\n"); let scene_dot = TestScenario::new(util_name!()); let at = &scene_dot.fixtures; @@ -605,7 +612,7 @@ fn test_ls_sort_name() { .arg("--sort=name") .arg("-A") .succeeds() - .stdout_is(".a .b a b\n"); + .stdout_is(".a\n.b\na\nb\n"); } #[test] @@ -626,16 +633,16 @@ fn test_ls_order_size() { scene.ucmd().arg("-al").succeeds(); let result = scene.ucmd().arg("-S").succeeds(); - result.stdout_only("test-4 test-3 test-2 test-1\n"); + result.stdout_only("test-4\ntest-3\ntest-2\ntest-1\n"); let result = scene.ucmd().arg("-S").arg("-r").succeeds(); - result.stdout_only("test-1 test-2 test-3 test-4\n"); + result.stdout_only("test-1\ntest-2\ntest-3\ntest-4\n"); let result = scene.ucmd().arg("--sort=size").succeeds(); - result.stdout_only("test-4 test-3 test-2 test-1\n"); + result.stdout_only("test-4\ntest-3\ntest-2\ntest-1\n"); let result = scene.ucmd().arg("--sort=size").arg("-r").succeeds(); - result.stdout_only("test-1 test-2 test-3 test-4\n"); + result.stdout_only("test-1\ntest-2\ntest-3\ntest-4\n"); } #[test] @@ -793,16 +800,16 @@ fn test_ls_order_time() { // ctime was changed at write, so the order is 4 3 2 1 let result = scene.ucmd().arg("-t").succeeds(); - result.stdout_only("test-4 test-3 test-2 test-1\n"); + result.stdout_only("test-4\ntest-3\ntest-2\ntest-1\n"); let result = scene.ucmd().arg("--sort=time").succeeds(); - result.stdout_only("test-4 test-3 test-2 test-1\n"); + result.stdout_only("test-4\ntest-3\ntest-2\ntest-1\n"); let result = scene.ucmd().arg("-tr").succeeds(); - result.stdout_only("test-1 test-2 test-3 test-4\n"); + result.stdout_only("test-1\ntest-2\ntest-3\ntest-4\n"); let result = scene.ucmd().arg("--sort=time").arg("-r").succeeds(); - result.stdout_only("test-1 test-2 test-3 test-4\n"); + result.stdout_only("test-1\ntest-2\ntest-3\ntest-4\n"); // 3 was accessed last in the read // So the order should be 2 3 4 1 @@ -813,11 +820,11 @@ fn test_ls_order_time() { // It seems to be dependent on the platform whether the access time is actually set if file3_access > file4_access { - result.stdout_only("test-3 test-4 test-2 test-1\n"); + result.stdout_only("test-3\ntest-4\ntest-2\ntest-1\n"); } else { // Access time does not seem to be set on Windows and some other // systems so the order is 4 3 2 1 - result.stdout_only("test-4 test-3 test-2 test-1\n"); + result.stdout_only("test-4\ntest-3\ntest-2\ntest-1\n"); } } @@ -826,7 +833,7 @@ fn test_ls_order_time() { #[cfg(unix)] { let result = scene.ucmd().arg("-tc").succeeds(); - result.stdout_only("test-2 test-4 test-3 test-1\n"); + result.stdout_only("test-2\ntest-4\ntest-3\ntest-1\n"); } } @@ -970,6 +977,7 @@ fn test_ls_color() { .ucmd() .arg("--color") .arg("-w=15") + .arg("-C") .succeeds() .stdout_only(format!( "{} test-color\nb {}\n", @@ -1988,7 +1996,7 @@ fn test_ls_path() { }; scene.ucmd().arg(&abs_path).run().stdout_is(expected_stdout); - let expected_stdout = format!("{} {}\n", path, file1); + let expected_stdout = format!("{}\n{}\n", path, file1); scene .ucmd() .arg(file1)