Skip to content

simplify the ls code #11391

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

Open
sylvestre wants to merge 10 commits into
base: main
Choose a base branch
from
+30 −50
Open

simplify the ls code #11391

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
06cf84a
ls: reduce redundant env var reads in is_color_compatible_term()
sylvestre Mar 18, 2026
ef9a39b
ls: use is_some_and for opt_block_size checks
sylvestre Mar 18, 2026
f8b6970
ls: cache color option lookup in extract_color()
sylvestre Mar 18, 2026
8bb83dd
ls: remove redundant display_inode wrapper
sylvestre Mar 18, 2026
834f2ae
ls: avoid allocation in calculate_line_len
sylvestre Mar 18, 2026
f7fbabd
ls: avoid string allocation for line ending bytes
sylvestre Mar 18, 2026
c8b0a2f
ls: cache file_type() call in get_block_size
sylvestre Mar 18, 2026
9889c05
ls: avoid string allocation for indicator char
sylvestre Mar 18, 2026
feec448
ls: simplify dired helpers
sylvestre Mar 18, 2026
abb52b2
ls: simplify canonicalize_indicator_value
sylvestre Mar 18, 2026
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
7 changes: 2 additions & 5 deletions src/uu/ls/src/colors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,11 +786,8 @@ fn parse_indicator_codes() -> (FxHashMap<Indicator, String>, bool) {
}

fn canonicalize_indicator_value(value: &str) -> Cow<'_, str> {
if value.len() == 1 && value.chars().all(|c| c.is_ascii_digit()) {
let mut canonical = String::with_capacity(2);
canonical.push('0');
canonical.push_str(value);
Cow::Owned(canonical)
if value.len() == 1 && value.as_bytes()[0].is_ascii_digit() {
Cow::Owned(format!("0{value}"))
Copy link
Collaborator

@drinkcat drinkcat Mar 20, 2026

Choose a reason for hiding this comment

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

My hunch is that the manual string construction is faster than format -- but I haven't benchmarked stuff for a while now, maybe format improved...

Not sure how much you care about performance in this case (didn't look at callers)

} else {
Cow::Borrowed(value)
}
Expand Down
17 changes: 5 additions & 12 deletions src/uu/ls/src/dired.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ pub fn print_dired_output(
}

/// Helper function to print positions with a given prefix.
fn print_positions(prefix: &str, positions: &Vec<BytePosition>) {
fn print_positions(prefix: &str, positions: &[BytePosition]) {
print!("{prefix}");
for c in positions {
print!(" {c}");
Expand All @@ -117,16 +117,9 @@ fn print_positions(prefix: &str, positions: &Vec<BytePosition>) {
}

pub fn add_total(dired: &mut DiredOutput, total_len: usize) {
if dired.padding == 0 {
// when dealing with " total: xx", it isn't part of the //DIRED//
// so, we just keep the size line to add it to the position of the next file
dired.padding = total_len + DIRED_TRAILING_OFFSET;
} else {
// += because if we are in -R, we have " dir:\n total X". So, we need to take the
// previous padding too.
// and we already have the previous position in mind
dired.padding += total_len + DIRED_TRAILING_OFFSET;
}
// += because if we are in -R, we have " dir:\n total X". So, we need to take the
// previous padding too.
dired.padding += total_len + DIRED_TRAILING_OFFSET;
}

// when using -R, we have the dirname. we need to add it to the padding
Expand Down Expand Up @@ -156,7 +149,7 @@ pub fn update_positions(dired: &mut DiredOutput, start: usize, end: usize, line_
start: start + padding,
end: end + padding,
});
dired.line_offset = dired.line_offset + padding + line_len;
dired.line_offset += padding + line_len;
// Remove the previous padding
dired.padding = 0;
}
Expand Down
56 changes: 23 additions & 33 deletions src/uu/ls/src/ls.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -571,11 +571,8 @@ fn extract_time(options: &clap::ArgMatches) -> MetadataTimeField {
/// Some env variables can be passed
/// For now, we are only verifying if empty or not and known for `TERM`
fn is_color_compatible_term() -> bool {
let is_term_set = std::env::var("TERM").is_ok();
let is_colorterm_set = std::env::var("COLORTERM").is_ok();

let term = std::env::var("TERM").unwrap_or_default();
let colorterm = std::env::var("COLORTERM").unwrap_or_default();
let term = std::env::var_os("TERM");
let colorterm = std::env::var_os("COLORTERM");

// Search function in the TERM struct to manage the wildcards
let term_matches = |term: &str| -> bool {
Expand All @@ -585,11 +582,14 @@ fn is_color_compatible_term() -> bool {
})
};

if is_term_set && term.is_empty() && is_colorterm_set && colorterm.is_empty() {
let term_str = term.as_deref().unwrap_or_default().to_string_lossy();
let colorterm_str = colorterm.as_deref().unwrap_or_default().to_string_lossy();

if term.is_some() && term_str.is_empty() && colorterm.is_some() && colorterm_str.is_empty() {
Comment on lines +585 to +588
Copy link
Contributor

@cakebaker cakebaker Mar 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

While the changes to this function are an improvement, I think using a match would be cleaner. Something like:

    match (term, colorterm) {
        (Some(t), Some(c)) if t.is_empty() && c.is_empty() => false,
        (Some(t), _) if !t.is_empty() => term_matches(&t.to_string_lossy()),
        _ => true,
    }

Please note that the semantics of the third line are slightly different than those of the original code. I don't know if that's an issue.

return false;
}

if !term.is_empty() && !term_matches(&term) {
if !term_str.is_empty() && !term_matches(&term_str) {
return false;
}
true
Expand All @@ -613,13 +613,13 @@ fn extract_color(options: &clap::ArgMatches) -> bool {
}
};

let color_index = options
.get_one::<String>(options::COLOR)
let color_val = options.get_one::<String>(options::COLOR);
let color_index = color_val
.and_then(|_| options.indices_of(options::COLOR))
.map_or(0, |mut indices| indices.next_back().unwrap_or(0));
let unsorted_all_index = get_last_index(options::files::UNSORTED_ALL);

let color_enabled = match options.get_one::<String>(options::COLOR) {
let color_enabled = match color_val {
None => options.contains_id(options::COLOR),
Some(val) => match val.as_str() {
"" | "always" | "yes" | "force" => true,
Expand Down Expand Up @@ -922,17 +922,9 @@ impl Config {
let hyperlink = extract_hyperlink(options);

let opt_block_size = options.get_one::<String>(options::size::BLOCK_SIZE);
let opt_si = opt_block_size.is_some()
&& options
.get_one::<String>(options::size::BLOCK_SIZE)
.unwrap()
.eq("si")
|| options.get_flag(options::size::SI);
let opt_hr = (opt_block_size.is_some()
&& options
.get_one::<String>(options::size::BLOCK_SIZE)
.unwrap()
.eq("human-readable"))
let opt_si =
opt_block_size.is_some_and(|v| v == "si") || options.get_flag(options::size::SI);
Copy link
Collaborator

@drinkcat drinkcat Mar 20, 2026

Choose a reason for hiding this comment

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

Does this work? opt_block_size == Some("si") (I think so)

let opt_hr = opt_block_size.is_some_and(|v| v == "human-readable")
|| options.get_flag(options::size::HUMAN_READABLE);
let opt_kb = options.get_flag(options::size::KIBIBYTES);

Expand Down Expand Up @@ -2857,7 +2849,8 @@ fn get_block_size(md: &Metadata, config: &Config) -> u64 {
*/
#[cfg(unix)]
{
let raw_blocks = if md.file_type().is_char_device() || md.file_type().is_block_device() {
let ft = md.file_type();
let raw_blocks = if ft.is_char_device() || ft.is_block_device() {
0u64
} else {
md.blocks() * 512
Expand Down Expand Up @@ -2952,8 +2945,9 @@ fn display_grid(
Ok(())
}

fn calculate_line_len(output_len: usize, item_len: usize, line_ending: LineEnding) -> usize {
output_len + item_len + line_ending.to_string().len()
fn calculate_line_len(output_len: usize, item_len: usize, _line_ending: LineEnding) -> usize {
Copy link
Contributor

@cakebaker cakebaker Mar 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I would remove either the _line_ending param to simplify the function signature. Or the entire function. The function is called in a single place and so the (trivial) calculation could be done there.

Both approaches also allow you to simplify the signature of update_dired_for_item.

// LineEnding is always a single byte ('\n' or '\0')
output_len + item_len + 1
}

fn update_dired_for_item(
Expand Down Expand Up @@ -3125,7 +3119,7 @@ fn display_item_long(
};

write_os_str(&mut output_display, &displayed_item)?;
output_display.extend(config.line_ending.to_string().as_bytes());
output_display.push(u8::from(config.line_ending));
} else {
#[cfg(unix)]
let leading_char = {
Expand Down Expand Up @@ -3225,7 +3219,7 @@ fn display_item_long(
}
let displayed_item = displayed_item.displayed;
write_os_str(&mut output_display, &displayed_item)?;
output_display.extend(config.line_ending.to_string().as_bytes());
output_display.push(u8::from(config.line_ending));
}
state.out.write_all(&output_display)?;

Expand Down Expand Up @@ -3443,7 +3437,8 @@ fn display_item_name(
};

if let Some(c) = char_opt {
name.push(OsStr::new(&c.to_string()));
let mut buf = [0u8; 4];
name.push(OsStr::new(c.encode_utf8(&mut buf)));
}
}

Expand Down Expand Up @@ -3600,11 +3595,6 @@ fn display_symlink_count(metadata: &Metadata) -> String {
metadata.nlink().to_string()
}

#[cfg(unix)]
fn display_inode(metadata: &Metadata) -> String {
get_inode(metadata)
}

/// This returns the `SELinux` security context as UTF8 `String`.
/// In the long term this should be changed to [`OsStr`], see discussions at #2621/#2656
fn get_security_context<'a>(
Expand Down Expand Up @@ -3706,7 +3696,7 @@ fn calculate_padding_collection(
#[cfg(unix)]
if config.inode {
let inode_len = if let Some(md) = item.metadata() {
display_inode(md).len()
get_inode(md).len()
} else {
continue;
};
Expand Down
Loading