uutils/coreutils
0
0
Fork 0
mirror of https://github.com/uutils/coreutils.git synced 2026-06-10 15:48:22 -07:00
Code Issues Packages Projects Releases Wiki Activity

ls:fix ls dired reports (#10527)

Browse Source 
* refactor: simplify DisplayItemName struct and improve dired position handling

This commit refactors the `display_item_name` function to return a `DisplayItemName` struct containing both the displayed name and its dired length, rather than just the OsString. This change simplifies the code by:

1. Eliminating the need to call `dired_name_len()` separately after getting the displayed name
2. Reducing redundant length calculations in the dired position handling
3. Making the code more maintainable by encapsulating related data together

The change also fixes an issue where dired positions were being calculated incorrectly for symlink names that required quoting, ensuring proper highlighting in dired mode.

Closes: #10248
This commit is contained in:
mattsu
2026-02-14 10:50:11 +01:00
committed by GitHub
parent e4b3b5ca72
commit eea061db56
4 changed files with 206 additions and 94 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/uu/ls/src/dired.rs
+43 -58
View File
@@ -51,6 +51,7 @@ pub struct DiredOutput {
pub dired_positions: Vec<BytePosition>,
pub subdired_positions: Vec<BytePosition>,
pub padding: usize,
pub line_offset: usize,
}
impl fmt::Display for BytePosition {
@@ -62,21 +63,13 @@ impl fmt::Display for BytePosition {
// When --dired is used, all lines starts with 2 spaces
static DIRED_TRAILING_OFFSET: usize = 2;
fn get_offset_from_previous_line(dired_positions: &[BytePosition]) -> usize {
if let Some(last_position) = dired_positions.last() {
last_position.end + 1
} else {
0
}
}
/// Calculates the byte positions for DIRED
pub fn calculate_dired(
dired_positions: &[BytePosition],
dired: &DiredOutput,
output_display_len: usize,
dfn_len: usize,
) -> (usize, usize) {
let offset_from_previous_line = get_offset_from_previous_line(dired_positions);
let offset_from_previous_line = dired.line_offset;
let start = output_display_len + offset_from_previous_line;
let end = start + dfn_len;
@@ -89,16 +82,8 @@ pub fn indent(out: &mut BufWriter<Stdout>) -> UResult<()> {
}
pub fn calculate_subdired(dired: &mut DiredOutput, path_len: usize) {
let offset_from_previous_line = get_offset_from_previous_line(&dired.dired_positions);
let additional_offset = if dired.subdired_positions.is_empty() {
0
} else {
// if we have several directories: \n\n
2
};
let start = offset_from_previous_line + DIRED_TRAILING_OFFSET + additional_offset;
let offset_from_previous_line = dired.line_offset + dired.padding;
let start = offset_from_previous_line + DIRED_TRAILING_OFFSET;
let end = start + path_len;
dired.subdired_positions.push(BytePosition { start, end });
}
@@ -113,7 +98,9 @@ pub fn print_dired_output(
if !dired.dired_positions.is_empty() {
print_positions("//DIRED//", &dired.dired_positions);
}
if config.recursive {
// SUBDIRED is needed whenever directory headings are printed (multiple args or -R),
// so don't gate it on config.recursive.
if !dired.subdired_positions.is_empty() {
print_positions("//SUBDIRED//", &dired.subdired_positions);
}
println!("//DIRED-OPTIONS// --quoting-style={}", config.quoting_style);
@@ -131,10 +118,9 @@ fn print_positions(prefix: &str, positions: &Vec<BytePosition>) {
pub fn add_total(dired: &mut DiredOutput, total_len: usize) {
if dired.padding == 0 {
let offset_from_previous_line = get_offset_from_previous_line(&dired.dired_positions);
// 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 + offset_from_previous_line + DIRED_TRAILING_OFFSET;
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.
@@ -145,8 +131,8 @@ pub fn add_total(dired: &mut DiredOutput, total_len: usize) {
// when using -R, we have the dirname. we need to add it to the padding
pub fn add_dir_name(dired: &mut DiredOutput, dir_len: usize) {
// 1 for the ":" in " dirname:"
dired.padding += dir_len + DIRED_TRAILING_OFFSET + 1;
// " dirname:\n"
dired.padding += dir_len + DIRED_TRAILING_OFFSET + 2;
}
/// Calculates byte positions and updates the dired structure.
@@ -154,27 +140,23 @@ pub fn calculate_and_update_positions(
dired: &mut DiredOutput,
output_display_len: usize,
dfn_len: usize,
line_len: usize,
) {
let offset = dired
.dired_positions
.last()
.map_or(DIRED_TRAILING_OFFSET, |last_position| {
last_position.start + DIRED_TRAILING_OFFSET
});
let start = output_display_len + offset + DIRED_TRAILING_OFFSET;
let end = start + dfn_len;
update_positions(dired, start, end);
let (start, end) = calculate_dired(dired, output_display_len, dfn_len);
update_positions(dired, start, end, line_len);
}
/// Updates the dired positions based on the given start and end positions.
/// update when it is the first element in the list (to manage "total X")
/// insert when it isn't the about total
pub fn update_positions(dired: &mut DiredOutput, start: usize, end: usize) {
pub fn update_positions(dired: &mut DiredOutput, start: usize, end: usize, line_len: usize) {
// padding can be 0 but as it doesn't matter
let padding = dired.padding;
dired.dired_positions.push(BytePosition {
start: start + dired.padding,
end: end + dired.padding,
start: start + padding,
end: end + padding,
});
dired.line_offset = dired.line_offset + padding + line_len;
// Remove the previous padding
dired.padding = 0;
}
@@ -194,22 +176,18 @@ mod tests {
fn test_calculate_dired() {
let output_display = "sample_output".to_string();
let dfn = "sample_file".to_string();
let dired_positions = vec![BytePosition { start: 5, end: 10 }];
let (start, end) = calculate_dired(&dired_positions, output_display.len(), dfn.len());
let dired = DiredOutput {
dired_positions: vec![BytePosition { start: 5, end: 10 }],
subdired_positions: vec![],
padding: 0,
line_offset: 11,
};
let (start, end) = calculate_dired(&dired, output_display.len(), dfn.len());
assert_eq!(start, 24);
assert_eq!(end, 35);
}
#[test]
fn test_get_offset_from_previous_line() {
let positions = vec![
BytePosition { start: 0, end: 3 },
BytePosition { start: 4, end: 7 },
BytePosition { start: 8, end: 11 },
];
assert_eq!(get_offset_from_previous_line(&positions), 12);
}
#[test]
fn test_calculate_subdired() {
let mut dired = DiredOutput {
@@ -220,6 +198,7 @@ mod tests {
],
subdired_positions: vec![],
padding: 0,
line_offset: 12,
};
let path_len = 5;
calculate_subdired(&mut dired, path_len);
@@ -239,6 +218,7 @@ mod tests {
],
subdired_positions: vec![],
padding: 0,
line_offset: 0,
};
let dir_len = 5;
add_dir_name(&mut dired, dir_len);
@@ -251,8 +231,9 @@ mod tests {
BytePosition { start: 8, end: 11 },
],
subdired_positions: vec![],
// 8 = 1 for the \n + 5 for dir_len + 2 for " " + 1 for :
padding: 8
// 9 = 5 for dir_len + 2 for " " + 1 for : + 1 for \n
padding: 9,
line_offset: 0,
}
);
}
@@ -267,12 +248,13 @@ mod tests {
],
subdired_positions: vec![],
padding: 0,
line_offset: 12,
};
// if we have "total: 2"
let total_len = 8;
add_total(&mut dired, total_len);
// 22 = 8 (len) + 2 (padding) + 11 (previous position) + 1 (\n)
assert_eq!(dired.padding, 22);
// 10 = 8 (len) + 2 (" ")
assert_eq!(dired.padding, 10);
}
#[test]
@@ -290,15 +272,16 @@ mod tests {
],
subdired_positions: vec![],
padding: 0,
line_offset: 12,
};
let dir_len = 5;
add_dir_name(&mut dired, dir_len);
// 8 = 2 (" ") + 1 (\n) + 5 + 1 (: of dirname)
assert_eq!(dired.padding, 8);
// 9 = 2 (" ") + 1 (\n) + 5 + 1 (: of dirname)
assert_eq!(dired.padding, 9);
let total_len = 8;
add_total(&mut dired, total_len);
assert_eq!(dired.padding, 18);
assert_eq!(dired.padding, 19);
}
#[test]
@@ -307,16 +290,17 @@ mod tests {
dired_positions: vec![BytePosition { start: 5, end: 10 }],
subdired_positions: vec![],
padding: 10,
line_offset: 0,
};
// Test with adjust = true
update_positions(&mut dired, 15, 20);
update_positions(&mut dired, 15, 20, 1);
let last_position = dired.dired_positions.last().unwrap();
assert_eq!(last_position.start, 25); // 15 + 10 (end of the previous position)
assert_eq!(last_position.end, 30); // 20 + 10 (end of the previous position)
// Test with adjust = false
update_positions(&mut dired, 30, 35);
update_positions(&mut dired, 30, 35, 1);
let last_position = dired.dired_positions.last().unwrap();
assert_eq!(last_position.start, 30);
assert_eq!(last_position.end, 35);
@@ -332,10 +316,11 @@ mod tests {
],
subdired_positions: vec![],
padding: 5,
line_offset: 12,
};
let output_display_len = 15;
let dfn_len = 5;
calculate_and_update_positions(&mut dired, output_display_len, dfn_len);
calculate_and_update_positions(&mut dired, output_display_len, dfn_len, 20);
assert_eq!(
dired.dired_positions,
vec![
src/uu/ls/src/ls.rs
+71 -33
View File
@@ -410,6 +410,11 @@ struct PaddingCollection {
block_size: usize,
}
struct DisplayItemName {
displayed: OsString,
dired_name_len: usize,
}
/// Extracts the format to display the information based on the options provided.
///
/// # Returns
@@ -2281,24 +2286,24 @@ pub fn list(locs: Vec<&Path>, config: &Config) -> UResult<()> {
// Print dir heading - name... 'total' comes after error display
if initial_locs_len > 1 || config.recursive {
if pos.eq(&0usize) && files.is_empty() {
if config.dired {
dired::indent(&mut state.out)?;
}
show_dir_name(path_data, &mut state.out, config)?;
let needs_blank_line = !(pos.eq(&0usize) && files.is_empty());
if needs_blank_line {
writeln!(state.out)?;
if config.dired {
// First directory displayed
let dir_len = path_data.display_name().len();
// add the //SUBDIRED// coordinates
dired::calculate_subdired(&mut dired, dir_len);
// Add the padding for the dir name
dired::add_dir_name(&mut dired, dir_len);
dired.padding += 1;
}
} else {
writeln!(state.out)?;
show_dir_name(path_data, &mut state.out, config)?;
writeln!(state.out)?;
}
if config.dired {
dired::indent(&mut state.out)?;
}
show_dir_name(path_data, &mut state.out, config)?;
writeln!(state.out)?;
if config.dired {
let dir_len = path_data.display_name().len();
// add the //SUBDIRED// coordinates
dired::calculate_subdired(&mut dired, dir_len);
// Add the padding for the dir name
dired::add_dir_name(&mut dired, dir_len);
}
}
let mut listed_ancestors = FxHashSet::default();
@@ -2525,8 +2530,8 @@ fn enter_directory(
if config.dired {
// We already injected the first dir
// Continue with the others
// 2 = \n + \n
dired.padding = 2;
// blank line between directory sections
dired.padding += 1;
dired::indent(&mut state.out)?;
let dir_name_size = e.path().as_os_str().len();
dired::calculate_subdired(dired, dir_name_size);
@@ -2749,7 +2754,7 @@ fn display_items(
LazyCell::new(Box::new(|| 0)),
);
names_vec.push(cell);
names_vec.push(cell.displayed);
}
let mut names = names_vec.into_iter();
@@ -2913,6 +2918,21 @@ 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 update_dired_for_item(
dired: &mut DiredOutput,
output_display_len: usize,
displayed_len: usize,
dired_name_len: usize,
line_ending: LineEnding,
) {
let line_len = calculate_line_len(output_display_len, displayed_len, line_ending);
dired::calculate_and_update_positions(dired, output_display_len, dired_name_len, line_len);
}
/// This writes to the [`BufWriter`] `state.out` a single string of the output of `ls -l`.
///
/// It writes the following keys, in order:
@@ -3033,7 +3053,7 @@ fn display_item_long(
display_date(md, config, state, &mut output_display)?;
output_display.extend(b" ");
let item_name = display_item_name(
let item_display = display_item_name(
item,
config,
None,
@@ -3044,22 +3064,32 @@ fn display_item_long(
})),
);
let displayed_item = if quoted && !os_str_starts_with(&item_name, b"'") {
let needs_space = quoted && !os_str_starts_with(&item_display.displayed, b"'");
if config.dired {
let mut dired_name_len = item_display.dired_name_len;
if needs_space {
dired_name_len += 1;
}
let displayed_len = item_display.displayed.len() + usize::from(needs_space);
update_dired_for_item(
dired,
output_display.len(),
displayed_len,
dired_name_len,
config.line_ending,
);
}
let item_name = item_display.displayed;
let displayed_item = if needs_space {
let mut ret: OsString = " ".into();
ret.push(item_name);
ret.push(&item_name);
ret
} else {
item_name
};
if config.dired {
let (start, end) = dired::calculate_dired(
&dired.dired_positions,
output_display.len(),
displayed_item.len(),
);
dired::update_positions(dired, start, end);
}
write_os_str(&mut output_display, &displayed_item)?;
output_display.extend(config.line_ending.to_string().as_bytes());
} else {
@@ -3151,12 +3181,15 @@ fn display_item_long(
output_display.extend(b" ");
if config.dired {
dired::calculate_and_update_positions(
update_dired_for_item(
dired,
output_display.len(),
displayed_item.to_string_lossy().trim().len(),
displayed_item.displayed.len(),
displayed_item.dired_name_len,
config.line_ending,
);
}
let displayed_item = displayed_item.displayed;
write_os_str(&mut output_display, &displayed_item)?;
output_display.extend(config.line_ending.to_string().as_bytes());
}
@@ -3329,7 +3362,7 @@ fn display_item_name(
more_info: Option<String>,
state: &mut ListState,
current_column: LazyCell<usize, Box<dyn FnOnce() -> usize + '_>>,
) -> OsString {
) -> DisplayItemName {
// This is our return value. We start by `&path.display_name` and modify it along the way.
let mut name = escape_name_with_locale(path.display_name(), config);
@@ -3380,6 +3413,8 @@ fn display_item_name(
}
}
let dired_name_len = if config.dired { name.len() } else { 0 };
if config.format == Format::Long
&& path.file_type().is_some_and(|ft| ft.is_symlink())
&& !path.must_dereference
@@ -3477,7 +3512,10 @@ fn display_item_name(
}
}
name
DisplayItemName {
displayed: name,
dired_name_len,
}
}
fn create_hyperlink(name: &OsStr, path: &PathData) -> OsString {
tests/by-util/test_ls.rs
+92
View File
@@ -5207,6 +5207,51 @@ fn test_ls_dired_recursive() {
.stdout_contains("//DIRED-OPTIONS// --quoting-style");
}
#[test]
fn test_ls_dired_subdired_multiple_dirs_non_recursive() {
let scene = TestScenario::new(util_name!());
let at = &scene.fixtures;
at.mkdir("dir1");
at.mkdir("dir2");
at.touch("dir1/a");
at.touch("dir2/b");
let result = scene
.ucmd()
.arg("--dired")
.arg("dir1")
.arg("dir2")
.succeeds();
let output = result.stdout_str().to_string();
let subdired_line = output
.lines()
.find(|&line| line.starts_with("//SUBDIRED//"))
.unwrap();
let positions: Vec<usize> = subdired_line
.split_whitespace()
.skip(1)
.map(|s| s.parse().unwrap())
.collect();
assert_eq!(positions.len() % 2, 0); // Ensure there's an even number of positions
let dirnames: Vec<String> = positions
.chunks(2)
.map(|chunk| {
let start_pos = chunk[0];
let end_pos = chunk[1];
String::from_utf8(output.as_bytes()[start_pos..end_pos].to_vec())
.unwrap()
.trim()
.to_string()
})
.collect();
assert_eq!(dirnames, vec!["dir1", "dir2"]);
}
#[test]
fn test_ls_dired_outputs_parent_offset() {
let scene = TestScenario::new(util_name!());
@@ -5336,6 +5381,53 @@ fn test_ls_dired_simple() {
assert_eq!(filename, "a1");
}
#[test]
fn test_ls_dired_symlink_name_only() {
let scene = TestScenario::new(util_name!());
let at = &scene.fixtures;
at.mkdir("d");
at.touch("d/target");
at.relative_symlink_file("target", "d/link");
let result = scene
.ucmd()
.arg("--dired")
.arg("-l")
.arg("--color=never")
.arg("d")
.succeeds();
let output = result.stdout_str().to_string();
assert!(output.contains("link -> target"));
let dired_line = output
.lines()
.find(|&line| line.starts_with("//DIRED//"))
.unwrap();
let positions: Vec<usize> = dired_line
.split_whitespace()
.skip(1)
.map(|s| s.parse().unwrap())
.collect();
assert_eq!(positions.len() % 2, 0);
let filenames: Vec<String> = positions
.chunks(2)
.map(|chunk| {
let start_pos = chunk[0];
let end_pos = chunk[1];
String::from_utf8(output.as_bytes()[start_pos..end_pos].to_vec())
.unwrap()
.trim()
.to_string()
})
.collect();
assert_eq!(filenames, vec!["link", "target"]);
}
#[test]
fn test_ls_dired_complex() {
let scene = TestScenario::new(util_name!());
util/build-gnu.sh
-3
View File
@@ -299,9 +299,6 @@ sed -i '1s/^/exit 0 # Skip test - uutils du uses safe traversal that prevents t
awk 'BEGIN {count=0} /compare exp out2/ && count < 6 {sub(/compare exp out2/, "grep -q \"cannot be used with\" out2"); count++} 1' tests/df/df-output.sh > tests/df/df-output.sh.tmp && mv tests/df/df-output.sh.tmp tests/df/df-output.sh
# with ls --dired, in case of error, we have a slightly different error position
sed -i -e "s|44 45|48 49|" tests/ls/stat-failed.sh
# small difference in the error message
sed -i -e "s/ls: invalid argument 'XX' for 'time style'/ls: invalid --time-style argument 'XX'/" \
-e "s/Valid arguments are:/Possible values are:/" \