Merge pull request #2566 from miDeb/ls/width

ls: correct output width

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<LsColors>,
     long: LongFormat,
-    width: Option<u16>,
+    width: u16,
     quoting_style: QuotingStyle,
     indicator_style: IndicatorStyle,
     time_style: TimeStyle,
@@ -258,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
@@ -286,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
+            let idx = opt
-                .indices_of(opt)
+                .and_then(|opt| options.indices_of(opt).map(|x| x.max().unwrap()))
-                .map(|x| x.max().unwrap())
                 .unwrap_or(0);
             if [
                 options::format::LONG_NO_OWNER,
@@ -399,10 +404,25 @@ impl Config {
 
         let width = match options.value_of(options::WIDTH) {
             Some(x) => match x.parse::<u16>() {
-                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 +1431,10 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
     } else {
         let names = items.iter().filter_map(|i| display_file_name(i, config));
 
-        match (&config.format, config.width) {
+        match config.format {
-            (Format::Columns, Some(width)) => {
+            Format::Columns => display_grid(names, config.width, Direction::TopToBottom, out),
-                display_grid(names, width, Direction::TopToBottom, out)
+            Format::Across => display_grid(names, config.width, Direction::LeftToRight, out),
-            }
+            Format::Commas => {
-            (Format::Across, Some(width)) => {
-                display_grid(names, width, Direction::LeftToRight, out)
-            }
-            (Format::Commas, width_opt) => {
-                let term_width = width_opt.unwrap_or(1);
                 let mut current_col = 0;
                 let mut names = names;
                 if let Some(name) = names.next() {
@@ -1428,7 +1443,8 @@ fn display_items(items: &[PathData], config: &Config, out: &mut BufWriter<Stdout
                 }
                 for name in names {
                     let name_width = name.width as u16;
-                    if current_col + name_width + 1 > term_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 {
@@ -1480,22 +1496,37 @@ fn display_grid(
     direction: Direction,
     out: &mut BufWriter<Stdout>,
 ) {
-    let mut grid = Grid::new(GridOptions {
+    if width == 0 {
-        filling: Filling::Spaces(2),
+        // If the width is 0 we print one single line
-        direction,
+        let mut printed_something = false;
-    });
+        for name in names {
-
+            if printed_something {
-    for name in names {
+                let _ = write!(out, "  ");
-        grid.add(name);
+            }
-    }
+            printed_something = true;
-
+            let _ = write!(out, "{}", name.contents);
-    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
+        if printed_something {
-        None => {
+            let _ = writeln!(out);
-            let _ = write!(out, "{}", grid.fit_into_columns(1));
+        }
+    } 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));
+            }
         }
     }
 }

tests/by-util/test_ls.rs

@@ -128,6 +128,7 @@ fn test_ls_width() {
         scene
             .ucmd()
             .args(&option.split(' ').collect::<Vec<_>>())
+            .arg("-C")
             .succeeds()
             .stdout_only("test-width-1  test-width-2  test-width-3  test-width-4\n");
     }
@@ -136,30 +137,33 @@ fn test_ls_width() {
         scene
             .ucmd()
             .args(&option.split(' ').collect::<Vec<_>>())
+            .arg("-C")
             .succeeds()
             .stdout_only("test-width-1  test-width-3\ntest-width-2  test-width-4\n");
     }
 
-    for option in &[
+    for option in &["-w 25", "-w=25", "--width=25", "--width 25"] {
-        "-w 25",
-        "-w=25",
-        "--width=25",
-        "--width 25",
-        "-w 0",
-        "-w=0",
-        "--width=0",
-        "--width 0",
-    ] {
         scene
             .ucmd()
             .args(&option.split(' ').collect::<Vec<_>>())
+            .arg("-C")
             .succeeds()
             .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::<Vec<_>>())
+            .arg("-C")
+            .succeeds()
+            .stdout_only("test-width-1  test-width-2  test-width-3  test-width-4\n");
+    }
+
     scene
         .ucmd()
         .arg("-w=bad")
+        .arg("-C")
         .fails()
         .stderr_contains("invalid line width");
 
@@ -167,6 +171,7 @@ fn test_ls_width() {
         scene
             .ucmd()
             .args(&option.split(' ').collect::<Vec<_>>())
+            .arg("-C")
             .fails()
             .stderr_only("ls: invalid line width: '1a'");
     }
@@ -184,16 +189,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 +204,38 @@ fn test_ls_columns() {
             .succeeds()
             .stdout_only("test-columns-1  test-columns-3\ntest-columns-2  test-columns-4\n");
     }
+
+    // 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", "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("-Cw0")
+        .succeeds()
+        .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]
@@ -220,11 +251,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  test-across-2  test-across-3  test-across-4\n");
-            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");
-        }
     }
 
     for option in &["-x", "--format=across"] {
@@ -250,11 +277,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, test-commas-2, test-commas-3, test-commas-4\n");
-            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");
-        }
     }
 
     for option in &["-m", "--format=commas"] {
@@ -571,13 +594,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\ntest-2\ntest-3\n");
 
     let scene_dot = TestScenario::new(util_name!());
     let at = &scene_dot.fixtures;
@@ -591,7 +612,7 @@ fn test_ls_sort_name() {
         .arg("--sort=name")
         .arg("-A")
         .succeeds()
-        .stdout_is([".a", ".b", "a", "b\n"].join(sep));
+        .stdout_is(".a\n.b\na\nb\n");
 }
 
 #[test]
@@ -612,28 +633,16 @@ 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");
 }
 
 #[test]
@@ -755,9 +764,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,28 +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();
-    #[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
     // So the order should be 2 3 4 1
@@ -826,19 +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 {
-            if cfg!(not(windows)) {
+            result.stdout_only("test-3\ntest-4\ntest-2\ntest-1\n");
-                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");
-            }
         } 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");
-                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");
-            }
         }
     }
 
@@ -991,6 +977,7 @@ fn test_ls_color() {
         .ucmd()
         .arg("--color")
         .arg("-w=15")
+        .arg("-C")
         .succeeds()
         .stdout_only(format!(
             "{}  test-color\nb  {}\n",
@@ -2009,11 +1996,7 @@ fn test_ls_path() {
     };
     scene.ucmd().arg(&abs_path).run().stdout_is(expected_stdout);
 
-    let expected_stdout = if cfg!(windows) {
+    let expected_stdout = format!("{}\n{}\n", path, file1);
-        format!("{}  {}\n", path, file1)
-    } else {
-        format!("{}\n{}\n", path, file1)
-    };
     scene
         .ucmd()
         .arg(file1)