Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Apr 17, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0c766c4
add -T option parsing
emar-kar Mar 30, 2025
4a233a7
add usage of tab_size in display_grid
emar-kar Mar 30, 2025
1c19599
fix test_ls_columns with \t
emar-kar Mar 30, 2025
2a202e7
update test_tabsize_formatting
emar-kar Mar 30, 2025
35eb82b
use grid DEFAULT_SEPARATOR_SIZE
emar-kar Mar 30, 2025
48ca8ba
update Tabs
emar-kar Apr 8, 2025
e6e76e2
fix test with column width
emar-kar Apr 8, 2025
8b9af95
fix cspell
emar-kar Apr 10, 2025
c222dc4
fix linter warning on match bool
emar-kar Apr 10, 2025
b4fe319
add comment for 0 tab_size
emar-kar Apr 10, 2025
548c4ea
update tabsize test with -C
emar-kar Apr 10, 2025
17ab4cd
update one of the tabs tests to use -x
emar-kar Apr 11, 2025
471144c
remove comment and split tests for both x/C args
emar-kar Apr 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 39 additions & 14 deletions src/uu/ls/src/ls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -1123,6 +1134,7 @@ impl Config {
line_ending: LineEnding::from_zero_flag(options.get_flag(options::ZERO)),
dired,
hyperlink,
tab_size,
})
}
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you left this in as an optimization?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, since -T0 literally means no \t I thought it's gonna be a good idea to add this small check here to omit additional work in grid

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add that as a comment to the code?

_ => Filling::Tabs {
spaces: DEFAULT_SEPARATOR_SIZE,
tab_size,
},
};

let grid = Grid::new(
Expand Down
48 changes: 36 additions & 12 deletions tests/by-util/test_ls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should test both -x and -C? The logic is the same in uutils-term-grid I guess, but we shouldn't assume too much about that here.

.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(
Expand Down
Loading