From 17f5dbe1b814c95cbe8e4b0888f4988fbf3c066a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Mar 2026 06:01:08 +0000 Subject: [PATCH 1/4] test: add failing test for --color=always with multiple args When pdu is invoked with multiple path arguments, a synthetic "(total)" root node is created. The build_coloring_map function includes this synthetic name in the filesystem path used for file type detection, producing paths like "(total)/link-dir" that don't exist. This causes all leaf nodes to be colored as Normal instead of their actual types (Symlink, Directory, etc.). This test demonstrates the bug by verifying colored output with multiple args against expected output with correct leaf colors. https://claude.ai/code/session_01YKNARZMxcXeYyGZhHMurHA --- tests/usual_cli.rs | 94 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/tests/usual_cli.rs b/tests/usual_cli.rs index 9f0c3e06..d1e42307 100644 --- a/tests/usual_cli.rs +++ b/tests/usual_cli.rs @@ -880,6 +880,100 @@ fn color_always() { assert_eq!(actual, expected); } +#[cfg(unix)] +#[test] +fn color_always_multiple_args() { + let workspace = SampleWorkspace::simple_tree_with_diverse_kinds(); + + let args = [ + "dir-a", + "dir-b", + "file-root.txt", + "link-dir", + "link-file.txt", + "empty-dir-1", + "empty-dir-2", + ]; + + let actual = { + let mut cmd = Command::new(PDU); + cmd = cmd + .with_current_dir(&workspace) + .with_arg("--color=always") + .with_arg("--total-width=100") + .with_arg("--min-ratio=0") + .with_env("LS_COLORS", LS_COLORS); + for arg in &args { + cmd = cmd.with_arg(arg); + } + cmd.pipe(stdio) + .output() + .expect("spawn command with --color=always and multiple args") + .pipe(stdout_text) + }; + eprintln!("ACTUAL:\n{actual}\n"); + + let mut data_tree = args + .iter() + .map(|name| { + let builder = FsTreeBuilder { + root: workspace.to_path_buf().join(name), + size_getter: DEFAULT_GET_SIZE, + hardlinks_recorder: &HardlinkIgnorant, + reporter: &ErrorOnlyReporter::new(ErrorReport::SILENT), + max_depth: 10, + }; + let mut data_tree: DataTree = builder.into(); + *data_tree.name_mut() = OsStringDisplay::os_string_from(name); + data_tree + }) + .pipe(|children| { + DataTree::dir( + OsStringDisplay::os_string_from("(total)"), + 0.into(), + children.collect(), + ) + }) + .into_par_sorted(|left, right| left.size().cmp(&right.size()).reverse()); + data_tree.par_cull_insignificant_data(0.0); + + let ls_colors = LsColors::from_str(LS_COLORS); + let leaf_colors = [ + ("(total)/dir-a/file-a1.txt", Color::Normal), + ("(total)/dir-a/file-a2.txt", Color::Normal), + ("(total)/dir-a/subdir-a/file-a3.txt", Color::Normal), + ("(total)/dir-b/file-b1.txt", Color::Normal), + ("(total)/file-root.txt", Color::Normal), + ("(total)/link-dir", Color::Symlink), + ("(total)/link-file.txt", Color::Symlink), + ("(total)/empty-dir-1", Color::Directory), + ("(total)/empty-dir-2", Color::Directory), + ]; + let leaf_colors = HashMap::from(leaf_colors.map(|(path, color)| { + ( + path.split('/') + .map(AsRef::::as_ref) + .collect::>(), + color, + ) + })); + let coloring = Coloring::new(ls_colors, leaf_colors); + + let visualizer = Visualizer:: { + data_tree: &data_tree, + bytes_format: BytesFormat::MetricUnits, + direction: Direction::BottomUp, + bar_alignment: BarAlignment::Left, + column_width_distribution: ColumnWidthDistribution::total(100), + coloring: Some(&coloring), + }; + let expected = format!("{visualizer}"); + let expected = expected.trim_end(); + eprintln!("EXPECTED:\n{expected}\n"); + + assert_eq!(actual, expected); +} + #[test] fn colorful_equals_colorless() { let workspace = SampleWorkspace::default(); From 382bd8c4bfcc0a36922196994bf698458f989540 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Mar 2026 06:08:11 +0000 Subject: [PATCH 2/4] fix: use real filesystem paths for leaf color detection with multiple args When multiple paths are given, the synthetic "(total)" root was included in the filesystem path passed to file_color(), producing nonexistent paths like "(total)/link-dir". This caused all leaf nodes to be colored as Normal. Split build_coloring_map into two stacks: key_stack (for HashMap keys matching the visualizer's ancestor chain) and fs_path_stack (for actual filesystem type detection). For multi-arg invocations, the caller seeds key_stack with the "(total)" root name but starts fs_path_stack empty, so children resolve to real paths on disk. https://claude.ai/code/session_01YKNARZMxcXeYyGZhHMurHA --- src/app/sub.rs | 38 ++++++++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/src/app/sub.rs b/src/app/sub.rs index 4ee419a6..0b75ec5f 100644 --- a/src/app/sub.rs +++ b/src/app/sub.rs @@ -200,7 +200,19 @@ where let coloring: Option = color.map(|ls_colors| { let mut map = HashMap::new(); - build_coloring_map(&data_tree, &mut Vec::new(), &mut map); + if only_one_arg { + build_coloring_map(&data_tree, &mut Vec::new(), &mut Vec::new(), &mut map); + } else { + // For multi-arg invocations the root is the synthetic "(total)" node. + // Include it in the map key (the visualizer's ancestor chain contains it) + // but skip it in the filesystem path (it doesn't exist on disk). + let root_name = data_tree.name().as_os_str(); + for child in data_tree.children() { + let mut key_stack = vec![root_name]; + let mut fs_stack = Vec::new(); + build_coloring_map(child, &mut key_stack, &mut fs_stack, &mut map); + } + } Coloring::new(ls_colors, map) }); @@ -282,27 +294,33 @@ where /// Recursively walk a pruned [`DataTree`] and build a map of path-component vectors to [`Color`] values. /// -/// The `path_stack` argument is a reusable buffer of path components representing the current -/// ancestor chain. Each recursive call pushes the node's name and pops it on return, so no -/// cloning occurs during traversal — only at leaf insertions. +/// `key_stack` tracks the ancestor chain used as the HashMap key (must match what the +/// [`Visualizer`] constructs). `fs_path_stack` tracks the real filesystem path used for +/// file-type detection. These two stacks diverge when the root is a synthetic node like +/// `(total)` that has no corresponding directory on disk. +/// /// Leaf nodes (files or childless directories after pruning) are added to the map. /// Nodes with children are skipped because the [`Visualizer`] uses the children count to /// determine their color at render time. fn build_coloring_map<'a>( node: &'a DataTree, - path_stack: &mut Vec<&'a OsStr>, + key_stack: &mut Vec<&'a OsStr>, + fs_path_stack: &mut Vec<&'a OsStr>, map: &mut HashMap, Color>, ) { - path_stack.push(node.name().as_os_str()); + let name = node.name().as_os_str(); + key_stack.push(name); + fs_path_stack.push(name); if node.children().is_empty() { - let color = file_color(&path_stack.iter().collect::()); - map.insert(path_stack.clone(), color); + let color = file_color(&fs_path_stack.iter().collect::()); + map.insert(key_stack.clone(), color); } else { for child in node.children() { - build_coloring_map(child, path_stack, map); + build_coloring_map(child, key_stack, fs_path_stack, map); } } - path_stack.pop(); + key_stack.pop(); + fs_path_stack.pop(); } fn file_color(path: &Path) -> Color { From 458b7c6ec12f1c68fdbf70b84c620a3c1adc1387 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Mar 2026 08:14:38 +0000 Subject: [PATCH 3/4] chore: remove no-op par_cull_insignificant_data(0.0) in test https://claude.ai/code/session_01YKNARZMxcXeYyGZhHMurHA --- tests/usual_cli.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/usual_cli.rs b/tests/usual_cli.rs index d1e42307..6e3b6ec7 100644 --- a/tests/usual_cli.rs +++ b/tests/usual_cli.rs @@ -935,7 +935,6 @@ fn color_always_multiple_args() { ) }) .into_par_sorted(|left, right| left.size().cmp(&right.size()).reverse()); - data_tree.par_cull_insignificant_data(0.0); let ls_colors = LsColors::from_str(LS_COLORS); let leaf_colors = [ From ebfc595cb36d3087b7f26e7707a67278047ee38e Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 8 Mar 2026 09:00:55 +0000 Subject: [PATCH 4/4] fix: build coloring map before renaming synthetic root to "(total)" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move the coloring map construction to before the "" → "(total)" rename so that file_color() receives real filesystem paths. After renaming, rekey the map to replace "" with "(total)" so keys match the visualizer's ancestor chain. Switch coloring map keys from borrowed Vec<&OsStr> to owned Vec to avoid lifetime conflicts when mutating the data tree root name after the map is built. Also fix unused-mut warning in color_always_multiple_args test. https://claude.ai/code/session_01YKNARZMxcXeYyGZhHMurHA --- src/app/sub.rs | 80 ++++++++++++++++++++------------------ src/visualizer.rs | 2 +- src/visualizer/coloring.rs | 21 ++++++---- tests/usual_cli.rs | 12 ++---- 4 files changed, 62 insertions(+), 53 deletions(-) diff --git a/src/app/sub.rs b/src/app/sub.rs index 0b75ec5f..8648fdcc 100644 --- a/src/app/sub.rs +++ b/src/app/sub.rs @@ -17,7 +17,7 @@ use pipe_trait::Pipe; use serde::Serialize; use std::{ collections::HashMap, - ffi::OsStr, + ffi::{OsStr, OsString}, io::stdout, iter::once, path::{Path, PathBuf}, @@ -133,7 +133,7 @@ where } let min_ratio: f32 = min_ratio.into(); - let (data_tree, deduplication_record) = { + let (mut data_tree, deduplication_record) = { let mut data_tree = data_tree; if min_ratio > 0.0 { data_tree.par_cull_insignificant_data(min_ratio); @@ -144,11 +144,41 @@ where let deduplication_record = hardlinks_handler.deduplicate(&mut data_tree); if !only_one_arg { assert_eq!(data_tree.name().as_os_str().to_str(), Some("")); - *data_tree.name_mut() = OsStringDisplay::os_string_from("(total)"); } (data_tree, deduplication_record) }; + // Build the coloring map while the multi-arg root is still "" (a valid path prefix) + // so that file_color receives real filesystem paths. + let mut leaf_color_map: Option, Color>> = color.as_ref().map(|_| { + let mut map = HashMap::new(); + build_coloring_map(&data_tree, &mut Vec::new(), &mut map); + map + }); + + // Rename the synthetic root and rekey the coloring map to match. + if !only_one_arg { + *data_tree.name_mut() = OsStringDisplay::os_string_from("(total)"); + if let Some(map) = &mut leaf_color_map { + let total = OsString::from("(total)"); + let empty = OsString::from(""); + *map = map + .drain() + .map(|(mut key, color)| { + if key.first() == Some(&empty) { + key[0] = total.clone(); + } + (key, color) + }) + .collect(); + } + } + + let coloring: Option = color.map(|ls_colors| { + let map = leaf_color_map.take().unwrap(); + Coloring::new(ls_colors, map) + }); + GLOBAL_STATUS_BOARD.clear_line(0); if let Some(json_output) = json_output { @@ -198,24 +228,6 @@ where .or(deduplication_result); } - let coloring: Option = color.map(|ls_colors| { - let mut map = HashMap::new(); - if only_one_arg { - build_coloring_map(&data_tree, &mut Vec::new(), &mut Vec::new(), &mut map); - } else { - // For multi-arg invocations the root is the synthetic "(total)" node. - // Include it in the map key (the visualizer's ancestor chain contains it) - // but skip it in the filesystem path (it doesn't exist on disk). - let root_name = data_tree.name().as_os_str(); - for child in data_tree.children() { - let mut key_stack = vec![root_name]; - let mut fs_stack = Vec::new(); - build_coloring_map(child, &mut key_stack, &mut fs_stack, &mut map); - } - } - Coloring::new(ls_colors, map) - }); - let visualizer = Visualizer { data_tree: &data_tree, bytes_format, @@ -294,33 +306,27 @@ where /// Recursively walk a pruned [`DataTree`] and build a map of path-component vectors to [`Color`] values. /// -/// `key_stack` tracks the ancestor chain used as the HashMap key (must match what the -/// [`Visualizer`] constructs). `fs_path_stack` tracks the real filesystem path used for -/// file-type detection. These two stacks diverge when the root is a synthetic node like -/// `(total)` that has no corresponding directory on disk. -/// +/// The `path_stack` argument is a reusable buffer of path components representing the current +/// ancestor chain. Each recursive call pushes the node's name and pops it on return, so no +/// cloning occurs during traversal — only at leaf insertions. /// Leaf nodes (files or childless directories after pruning) are added to the map. /// Nodes with children are skipped because the [`Visualizer`] uses the children count to /// determine their color at render time. fn build_coloring_map<'a>( node: &'a DataTree, - key_stack: &mut Vec<&'a OsStr>, - fs_path_stack: &mut Vec<&'a OsStr>, - map: &mut HashMap, Color>, + path_stack: &mut Vec<&'a OsStr>, + map: &mut HashMap, Color>, ) { - let name = node.name().as_os_str(); - key_stack.push(name); - fs_path_stack.push(name); + path_stack.push(node.name().as_os_str()); if node.children().is_empty() { - let color = file_color(&fs_path_stack.iter().collect::()); - map.insert(key_stack.clone(), color); + let color = file_color(&path_stack.iter().collect::()); + map.insert(path_stack.iter().map(|s| s.to_os_string()).collect(), color); } else { for child in node.children() { - build_coloring_map(child, key_stack, fs_path_stack, map); + build_coloring_map(child, path_stack, map); } } - key_stack.pop(); - fs_path_stack.pop(); + path_stack.pop(); } fn file_color(path: &Path) -> Color { diff --git a/src/visualizer.rs b/src/visualizer.rs index 2498d5b3..608d3885 100644 --- a/src/visualizer.rs +++ b/src/visualizer.rs @@ -62,7 +62,7 @@ where /// Distribution and total number of characters/blocks can be placed in a line. pub column_width_distribution: ColumnWidthDistribution, /// Optional coloring configuration for colorful output, mapping full node paths to colors. - pub coloring: Option<&'a Coloring<'a>>, + pub coloring: Option<&'a Coloring>, } mod copy; diff --git a/src/visualizer/coloring.rs b/src/visualizer/coloring.rs index a5c2a392..84674e97 100644 --- a/src/visualizer/coloring.rs +++ b/src/visualizer/coloring.rs @@ -1,19 +1,23 @@ use super::{ChildPosition, TreeHorizontalSlice}; use crate::ls_colors::LsColors; use derive_more::Display; -use std::{collections::HashMap, ffi::OsStr, fmt}; +use std::{ + collections::HashMap, + ffi::{OsStr, OsString}, + fmt, +}; use zero_copy_pads::Width; /// Coloring configuration: ANSI prefix strings from the environment and a full-path-to-color map. #[derive(Debug)] -pub struct Coloring<'a> { +pub struct Coloring { ls_colors: LsColors, - map: HashMap, Color>, + map: HashMap, Color>, } -impl<'a> Coloring<'a> { +impl Coloring { /// Create a new [`Coloring`] from LS_COLORS prefixes and a path-components-to-color map. - pub fn new(ls_colors: LsColors, map: HashMap, Color>) -> Self { + pub fn new(ls_colors: LsColors, map: HashMap, Color>) -> Self { Coloring { ls_colors, map } } } @@ -91,7 +95,7 @@ impl Width for ColoredTreeHorizontalSlice<'_> { /// Path components are only constructed when coloring is enabled, avoiding /// unnecessary allocation in the common no-color case. pub(super) fn maybe_colored_slice<'a, 'b>( - coloring: Option<&'b Coloring<'a>>, + coloring: Option<&'b Coloring>, ancestors: impl Iterator, name: &'a OsStr, has_children: bool, @@ -101,7 +105,10 @@ pub(super) fn maybe_colored_slice<'a, 'b>( Some(coloring) => coloring, None => return MaybeColoredTreeHorizontalSlice::Colorless(slice), }; - let path_components: Vec<&OsStr> = ancestors.chain(std::iter::once(name)).collect(); + let path_components: Vec = ancestors + .chain(std::iter::once(name)) + .map(OsString::from) + .collect(); let color = if has_children { Some(Color::Directory) } else { diff --git a/tests/usual_cli.rs b/tests/usual_cli.rs index 6e3b6ec7..f3ca4093 100644 --- a/tests/usual_cli.rs +++ b/tests/usual_cli.rs @@ -28,7 +28,7 @@ use parallel_disk_usage::{ visualizer::{Color, Coloring}, }; #[cfg(unix)] -use std::{collections::HashMap, ffi::OsStr}; +use std::{collections::HashMap, ffi::OsString}; fn stdio(command: Command) -> Command { command @@ -857,9 +857,7 @@ fn color_always() { ]; let leaf_colors = HashMap::from(leaf_colors.map(|(path, color)| { ( - path.split('/') - .map(AsRef::::as_ref) - .collect::>(), + path.split('/').map(OsString::from).collect::>(), color, ) })); @@ -913,7 +911,7 @@ fn color_always_multiple_args() { }; eprintln!("ACTUAL:\n{actual}\n"); - let mut data_tree = args + let data_tree = args .iter() .map(|name| { let builder = FsTreeBuilder { @@ -950,9 +948,7 @@ fn color_always_multiple_args() { ]; let leaf_colors = HashMap::from(leaf_colors.map(|(path, color)| { ( - path.split('/') - .map(AsRef::::as_ref) - .collect::>(), + path.split('/').map(OsString::from).collect::>(), color, ) }));