fix(color): multiple arguments #346
fix(color): multiple arguments #346KSXGitHub wants to merge 4 commits intocopilot/add-color-flag-and-mappingfrom
Conversation
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
… 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
There was a problem hiding this comment.
Pull request overview
Adds a Unix-only regression test to ensure --color=always produces correct ANSI-colored output when the CLI is invoked with multiple file/directory arguments (multi-root mode).
Changes:
- Added
color_always_multiple_args()test covering multiple argument inputs (dirs, files, symlinks, empty dirs) under a synthetic “(total)” root. - Validates output coloring against
LS_COLORS-driven expectations using the existingColoring+Visualizerformatting.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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::<OsStr>::as_ref) | ||
| .collect::<Vec<_>>(), | ||
| color, | ||
| ) | ||
| })); | ||
| let coloring = Coloring::new(ls_colors, leaf_colors); |
There was a problem hiding this comment.
This new test expects multi-arg output to correctly color symlinks and empty directories under the synthetic "(total)" root. In the current CLI implementation, leaf colors are computed by build_coloring_map via file_color(PathBuf::from(path_components)); when multiple args are used, that PathBuf includes the fake "(total)" component, so is_symlink()/is_dir() will return false and leaves will be misclassified as Normal (breaking the behavior this test asserts). To make this test pass, the production code needs to compute the filesystem stat path without the synthetic root component (e.g., maintain a separate real-path stack, or special-case the fake root when building the PathBuf for file_color) while keeping the display-path key used for coloring lookups.
Performance Regression Reportscommit: 308ac76 There are no regressions. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
KSXGitHub
force-pushed
the
claude/add-total-leaf-coloring-eC5Yq
branch
from
974bd91 to
ebfc595
Compare
Summary
This PR adds a new test case to verify that the
--color=alwaysflag works correctly when multiple file and directory arguments are provided to the CLI.Key Changes
color_always_multiple_args()test function that validates colored output when processing multiple argumentsdir-a,dir-b)file-root.txt)link-dir,link-file.txt)empty-dir-1,empty-dir-2)LS_COLORSenvironment variable#[cfg(unix)]) due to symlink handlingImplementation Details
ColoringandVisualizerinfrastructure to generate expected outputhttps://claude.ai/code/session_01YKNARZMxcXeYyGZhHMurHA