Skip to content

Commit ebfc595

Browse files
committed
fix: build coloring map before renaming synthetic root to "(total)"
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<OsString> 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
1 parent 458b7c6 commit ebfc595

4 files changed

Lines changed: 62 additions & 53 deletions

File tree

src/app/sub.rs

Lines changed: 43 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use pipe_trait::Pipe;
1717
use serde::Serialize;
1818
use std::{
1919
collections::HashMap,
20-
ffi::OsStr,
20+
ffi::{OsStr, OsString},
2121
io::stdout,
2222
iter::once,
2323
path::{Path, PathBuf},
@@ -133,7 +133,7 @@ where
133133
}
134134

135135
let min_ratio: f32 = min_ratio.into();
136-
let (data_tree, deduplication_record) = {
136+
let (mut data_tree, deduplication_record) = {
137137
let mut data_tree = data_tree;
138138
if min_ratio > 0.0 {
139139
data_tree.par_cull_insignificant_data(min_ratio);
@@ -144,11 +144,41 @@ where
144144
let deduplication_record = hardlinks_handler.deduplicate(&mut data_tree);
145145
if !only_one_arg {
146146
assert_eq!(data_tree.name().as_os_str().to_str(), Some(""));
147-
*data_tree.name_mut() = OsStringDisplay::os_string_from("(total)");
148147
}
149148
(data_tree, deduplication_record)
150149
};
151150

151+
// Build the coloring map while the multi-arg root is still "" (a valid path prefix)
152+
// so that file_color receives real filesystem paths.
153+
let mut leaf_color_map: Option<HashMap<Vec<OsString>, Color>> = color.as_ref().map(|_| {
154+
let mut map = HashMap::new();
155+
build_coloring_map(&data_tree, &mut Vec::new(), &mut map);
156+
map
157+
});
158+
159+
// Rename the synthetic root and rekey the coloring map to match.
160+
if !only_one_arg {
161+
*data_tree.name_mut() = OsStringDisplay::os_string_from("(total)");
162+
if let Some(map) = &mut leaf_color_map {
163+
let total = OsString::from("(total)");
164+
let empty = OsString::from("");
165+
*map = map
166+
.drain()
167+
.map(|(mut key, color)| {
168+
if key.first() == Some(&empty) {
169+
key[0] = total.clone();
170+
}
171+
(key, color)
172+
})
173+
.collect();
174+
}
175+
}
176+
177+
let coloring: Option<Coloring> = color.map(|ls_colors| {
178+
let map = leaf_color_map.take().unwrap();
179+
Coloring::new(ls_colors, map)
180+
});
181+
152182
GLOBAL_STATUS_BOARD.clear_line(0);
153183

154184
if let Some(json_output) = json_output {
@@ -198,24 +228,6 @@ where
198228
.or(deduplication_result);
199229
}
200230

201-
let coloring: Option<Coloring> = color.map(|ls_colors| {
202-
let mut map = HashMap::new();
203-
if only_one_arg {
204-
build_coloring_map(&data_tree, &mut Vec::new(), &mut Vec::new(), &mut map);
205-
} else {
206-
// For multi-arg invocations the root is the synthetic "(total)" node.
207-
// Include it in the map key (the visualizer's ancestor chain contains it)
208-
// but skip it in the filesystem path (it doesn't exist on disk).
209-
let root_name = data_tree.name().as_os_str();
210-
for child in data_tree.children() {
211-
let mut key_stack = vec![root_name];
212-
let mut fs_stack = Vec::new();
213-
build_coloring_map(child, &mut key_stack, &mut fs_stack, &mut map);
214-
}
215-
}
216-
Coloring::new(ls_colors, map)
217-
});
218-
219231
let visualizer = Visualizer {
220232
data_tree: &data_tree,
221233
bytes_format,
@@ -294,33 +306,27 @@ where
294306

295307
/// Recursively walk a pruned [`DataTree`] and build a map of path-component vectors to [`Color`] values.
296308
///
297-
/// `key_stack` tracks the ancestor chain used as the HashMap key (must match what the
298-
/// [`Visualizer`] constructs). `fs_path_stack` tracks the real filesystem path used for
299-
/// file-type detection. These two stacks diverge when the root is a synthetic node like
300-
/// `(total)` that has no corresponding directory on disk.
301-
///
309+
/// The `path_stack` argument is a reusable buffer of path components representing the current
310+
/// ancestor chain. Each recursive call pushes the node's name and pops it on return, so no
311+
/// cloning occurs during traversal — only at leaf insertions.
302312
/// Leaf nodes (files or childless directories after pruning) are added to the map.
303313
/// Nodes with children are skipped because the [`Visualizer`] uses the children count to
304314
/// determine their color at render time.
305315
fn build_coloring_map<'a>(
306316
node: &'a DataTree<OsStringDisplay, impl size::Size>,
307-
key_stack: &mut Vec<&'a OsStr>,
308-
fs_path_stack: &mut Vec<&'a OsStr>,
309-
map: &mut HashMap<Vec<&'a OsStr>, Color>,
317+
path_stack: &mut Vec<&'a OsStr>,
318+
map: &mut HashMap<Vec<OsString>, Color>,
310319
) {
311-
let name = node.name().as_os_str();
312-
key_stack.push(name);
313-
fs_path_stack.push(name);
320+
path_stack.push(node.name().as_os_str());
314321
if node.children().is_empty() {
315-
let color = file_color(&fs_path_stack.iter().collect::<PathBuf>());
316-
map.insert(key_stack.clone(), color);
322+
let color = file_color(&path_stack.iter().collect::<PathBuf>());
323+
map.insert(path_stack.iter().map(|s| s.to_os_string()).collect(), color);
317324
} else {
318325
for child in node.children() {
319-
build_coloring_map(child, key_stack, fs_path_stack, map);
326+
build_coloring_map(child, path_stack, map);
320327
}
321328
}
322-
key_stack.pop();
323-
fs_path_stack.pop();
329+
path_stack.pop();
324330
}
325331

326332
fn file_color(path: &Path) -> Color {

src/visualizer.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ where
6262
/// Distribution and total number of characters/blocks can be placed in a line.
6363
pub column_width_distribution: ColumnWidthDistribution,
6464
/// Optional coloring configuration for colorful output, mapping full node paths to colors.
65-
pub coloring: Option<&'a Coloring<'a>>,
65+
pub coloring: Option<&'a Coloring>,
6666
}
6767

6868
mod copy;

src/visualizer/coloring.rs

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,23 @@
11
use super::{ChildPosition, TreeHorizontalSlice};
22
use crate::ls_colors::LsColors;
33
use derive_more::Display;
4-
use std::{collections::HashMap, ffi::OsStr, fmt};
4+
use std::{
5+
collections::HashMap,
6+
ffi::{OsStr, OsString},
7+
fmt,
8+
};
59
use zero_copy_pads::Width;
610

711
/// Coloring configuration: ANSI prefix strings from the environment and a full-path-to-color map.
812
#[derive(Debug)]
9-
pub struct Coloring<'a> {
13+
pub struct Coloring {
1014
ls_colors: LsColors,
11-
map: HashMap<Vec<&'a OsStr>, Color>,
15+
map: HashMap<Vec<OsString>, Color>,
1216
}
1317

14-
impl<'a> Coloring<'a> {
18+
impl Coloring {
1519
/// Create a new [`Coloring`] from LS_COLORS prefixes and a path-components-to-color map.
16-
pub fn new(ls_colors: LsColors, map: HashMap<Vec<&'a OsStr>, Color>) -> Self {
20+
pub fn new(ls_colors: LsColors, map: HashMap<Vec<OsString>, Color>) -> Self {
1721
Coloring { ls_colors, map }
1822
}
1923
}
@@ -91,7 +95,7 @@ impl Width for ColoredTreeHorizontalSlice<'_> {
9195
/// Path components are only constructed when coloring is enabled, avoiding
9296
/// unnecessary allocation in the common no-color case.
9397
pub(super) fn maybe_colored_slice<'a, 'b>(
94-
coloring: Option<&'b Coloring<'a>>,
98+
coloring: Option<&'b Coloring>,
9599
ancestors: impl Iterator<Item = &'a OsStr>,
96100
name: &'a OsStr,
97101
has_children: bool,
@@ -101,7 +105,10 @@ pub(super) fn maybe_colored_slice<'a, 'b>(
101105
Some(coloring) => coloring,
102106
None => return MaybeColoredTreeHorizontalSlice::Colorless(slice),
103107
};
104-
let path_components: Vec<&OsStr> = ancestors.chain(std::iter::once(name)).collect();
108+
let path_components: Vec<OsString> = ancestors
109+
.chain(std::iter::once(name))
110+
.map(OsString::from)
111+
.collect();
105112
let color = if has_children {
106113
Some(Color::Directory)
107114
} else {

tests/usual_cli.rs

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ use parallel_disk_usage::{
2828
visualizer::{Color, Coloring},
2929
};
3030
#[cfg(unix)]
31-
use std::{collections::HashMap, ffi::OsStr};
31+
use std::{collections::HashMap, ffi::OsString};
3232

3333
fn stdio(command: Command) -> Command {
3434
command
@@ -857,9 +857,7 @@ fn color_always() {
857857
];
858858
let leaf_colors = HashMap::from(leaf_colors.map(|(path, color)| {
859859
(
860-
path.split('/')
861-
.map(AsRef::<OsStr>::as_ref)
862-
.collect::<Vec<_>>(),
860+
path.split('/').map(OsString::from).collect::<Vec<_>>(),
863861
color,
864862
)
865863
}));
@@ -913,7 +911,7 @@ fn color_always_multiple_args() {
913911
};
914912
eprintln!("ACTUAL:\n{actual}\n");
915913

916-
let mut data_tree = args
914+
let data_tree = args
917915
.iter()
918916
.map(|name| {
919917
let builder = FsTreeBuilder {
@@ -950,9 +948,7 @@ fn color_always_multiple_args() {
950948
];
951949
let leaf_colors = HashMap::from(leaf_colors.map(|(path, color)| {
952950
(
953-
path.split('/')
954-
.map(AsRef::<OsStr>::as_ref)
955-
.collect::<Vec<_>>(),
951+
path.split('/').map(OsString::from).collect::<Vec<_>>(),
956952
color,
957953
)
958954
}));

0 commit comments

Comments
 (0)