ls: add -T support and fix --classify output (#7616)

* add -T option parsing

* add usage of tab_size in display_grid

* fix test_ls_columns with \t

since this behavior is on by default

* update test_tabsize_formatting

* use grid DEFAULT_SEPARATOR_SIZE

* update Tabs

* fix test with column width

* fix cspell

* fix linter warning on match bool

* add comment for 0 tab_size

* update tabsize test with -C

* update one of the tabs tests to use -x

* remove comment and split tests for both x/C args

Author: emar-kar

src/uu/ls/src/ls.rs

@@ -33,7 +33,7 @@ use clap::{
 };
 use glob::{MatchOptions, Pattern};
 use lscolors::LsColors;
-use term_grid::{Direction, Filling, Grid, GridOptions};
+use term_grid::{DEFAULT_SEPARATOR_SIZE, Direction, Filling, Grid, GridOptions, SPACES_IN_TAB};
 use thiserror::Error;
 use uucore::error::USimpleError;
 use uucore::format::human::{SizeFormat, human_readable};
@@ -91,7 +91,7 @@ pub mod options {
         pub static LONG: &str = "long";
         pub static COLUMNS: &str = "C";
         pub static ACROSS: &str = "x";
-        pub static TAB_SIZE: &str = "tabsize"; // silently ignored (see #3624)
+        pub static TAB_SIZE: &str = "tabsize";
         pub static COMMAS: &str = "m";
         pub static LONG_NO_OWNER: &str = "g";
         pub static LONG_NO_GROUP: &str = "o";
@@ -385,6 +385,7 @@ pub struct Config {
     line_ending: LineEnding,
     dired: bool,
     hyperlink: bool,
+    tab_size: usize,
 }
 
 // Fields that can be removed or added to the long format
@@ -1086,6 +1087,16 @@ impl Config {
             Dereference::DirArgs
         };
 
+        let tab_size = if !needs_color {
+            options
+                .get_one::<String>(options::format::TAB_SIZE)
+                .and_then(|size| size.parse::<usize>().ok())
+                .or_else(|| std::env::var("TABSIZE").ok().and_then(|s| s.parse().ok()))
+        } else {
+            Some(0)
+        }
+        .unwrap_or(SPACES_IN_TAB);
+
         Ok(Self {
             format,
             files,
@@ -1123,6 +1134,7 @@ impl Config {
             line_ending: LineEnding::from_zero_flag(options.get_flag(options::ZERO)),
             dired,
             hyperlink,
+            tab_size,
         })
     }
 }
@@ -1239,13 +1251,12 @@ pub fn uu_app() -> Command {
                 .action(ArgAction::SetTrue),
         )
         .arg(
-            // silently ignored (see #3624)
             Arg::new(options::format::TAB_SIZE)
                 .short('T')
                 .long(options::format::TAB_SIZE)
                 .env("TABSIZE")
                 .value_name("COLS")
-                .help("Assume tab stops at each COLS instead of 8 (unimplemented)"),
+                .help("Assume tab stops at each COLS instead of 8"),
         )
         .arg(
             Arg::new(options::format::COMMAS)
@@ -2554,10 +2565,24 @@ fn display_items(
 
         match config.format {
             Format::Columns => {
-                display_grid(names, config.width, Direction::TopToBottom, out, quoted)?;
+                display_grid(
+                    names,
+                    config.width,
+                    Direction::TopToBottom,
+                    out,
+                    quoted,
+                    config.tab_size,
+                )?;
             }
             Format::Across => {
-                display_grid(names, config.width, Direction::LeftToRight, out, quoted)?;
+                display_grid(
+                    names,
+                    config.width,
+                    Direction::LeftToRight,
+                    out,
+                    quoted,
+                    config.tab_size,
+                )?;
             }
             Format::Commas => {
                 let mut current_col = 0;
@@ -2625,6 +2650,7 @@ fn display_grid(
     direction: Direction,
     out: &mut BufWriter<Stdout>,
     quoted: bool,
+    tab_size: usize,
 ) -> UResult<()> {
     if width == 0 {
         // If the width is 0 we print one single line
@@ -2674,14 +2700,13 @@ fn display_grid(
             .map(|s| s.to_string_lossy().into_owned())
             .collect();
 
-        // Determine whether to use tabs for separation based on whether any entry ends with '/'.
-        // If any entry ends with '/', it indicates that the -F flag is likely used to classify directories.
-        let use_tabs = names.iter().any(|name| name.ends_with('/'));
-
-        let filling = if use_tabs {
-            Filling::Text("\t".to_string())
-        } else {
-            Filling::Spaces(2)
+        // Since tab_size=0 means no \t, use Spaces separator for optimization.
+        let filling = match tab_size {
+            0 => Filling::Spaces(DEFAULT_SEPARATOR_SIZE),
+            _ => Filling::Tabs {
+                spaces: DEFAULT_SEPARATOR_SIZE,
+                tab_size,
+            },
         };
 
         let grid = Grid::new(

tests/by-util/test_ls.rs

@@ -837,7 +837,7 @@ fn test_ls_columns() {
 
     for option in COLUMN_ARGS {
         let result = scene.ucmd().arg(option).succeeds();
-        result.stdout_only("test-columns-1  test-columns-2  test-columns-3  test-columns-4\n");
+        result.stdout_only("test-columns-1\ttest-columns-2\ttest-columns-3\ttest-columns-4\n");
     }
 
     for option in COLUMN_ARGS {
@@ -846,7 +846,7 @@ fn test_ls_columns() {
             .arg("-w=40")
             .arg(option)
             .succeeds()
-            .stdout_only("test-columns-1  test-columns-3\ntest-columns-2  test-columns-4\n");
+            .stdout_only("test-columns-1\ttest-columns-3\ntest-columns-2\ttest-columns-4\n");
     }
 
     // On windows we are always able to get the terminal size, so we can't simulate falling back to the
@@ -859,15 +859,15 @@ fn test_ls_columns() {
                 .env("COLUMNS", "40")
                 .arg(option)
                 .succeeds()
-                .stdout_only("test-columns-1  test-columns-3\ntest-columns-2  test-columns-4\n");
+                .stdout_only("test-columns-1\ttest-columns-3\ntest-columns-2\ttest-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")
+            .stdout_is("test-columns-1\ttest-columns-2\ttest-columns-3\ttest-columns-4\n")
             .stderr_is("ls: ignoring invalid width in environment variable COLUMNS: 'garbage'\n");
     }
     scene
@@ -4366,28 +4366,52 @@ fn test_tabsize_option() {
     scene.ucmd().arg("-T").fails();
 }
 
-#[ignore = "issue #3624"]
 #[test]
 fn test_tabsize_formatting() {
-    let (at, mut ucmd) = at_and_ucmd!();
+    let scene = TestScenario::new(util_name!());
+    let at = &scene.fixtures;
 
     at.touch("aaaaaaaa");
     at.touch("bbbb");
     at.touch("cccc");
     at.touch("dddddddd");
 
-    ucmd.args(&["-T", "4"])
+    scene
+        .ucmd()
+        .args(&["-x", "-w18", "-T4"])
         .succeeds()
-        .stdout_is("aaaaaaaa bbbb\ncccc\t dddddddd");
+        .stdout_is("aaaaaaaa  bbbb\ncccc\t  dddddddd\n");
 
-    ucmd.args(&["-T", "2"])
+    scene
+        .ucmd()
+        .args(&["-C", "-w18", "-T4"])
+        .succeeds()
+        .stdout_is("aaaaaaaa  cccc\nbbbb\t  dddddddd\n");
+
+    scene
+        .ucmd()
+        .args(&["-x", "-w18", "-T2"])
         .succeeds()
-        .stdout_is("aaaaaaaa bbbb\ncccc\t\t dddddddd");
+        .stdout_is("aaaaaaaa\tbbbb\ncccc\t\t\tdddddddd\n");
+
+    scene
+        .ucmd()
+        .args(&["-C", "-w18", "-T2"])
+        .succeeds()
+        .stdout_is("aaaaaaaa\tcccc\nbbbb\t\t\tdddddddd\n");
+
+    scene
+        .ucmd()
+        .args(&["-x", "-w18", "-T0"])
+        .succeeds()
+        .stdout_is("aaaaaaaa  bbbb\ncccc      dddddddd\n");
 
     // use spaces
-    ucmd.args(&["-T", "0"])
+    scene
+        .ucmd()
+        .args(&["-C", "-w18", "-T0"])
         .succeeds()
-        .stdout_is("aaaaaaaa bbbb\ncccc     dddddddd");
+        .stdout_is("aaaaaaaa  cccc\nbbbb      dddddddd\n");
 }
 
 #[cfg(any(