-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Make ls pass the GNU ls-misc.pl suite #9262
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.
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
Conversation
Ensure StyleManager honors `or=`, `mi=` and `ln=target` the same as GNU ls when coloring dangling symlinks. Reset sequences are still printed for blank `or=` entries, and targets fall back to missing-file colors.
81fd4d3 to
2dc62c3
Compare
Add seven ls tests mirroring GNU tests/ls/ls-misc.pl sl-dangle3..9 to validate combinations of `ln=`, `or=` and `mi=` settings. The tests verify both file-level output and directory listings, including the `\x1b[m` reset requirement when `or=:` is set. Adjust existing Rust tests to GNU output. Existing color tests were updated to match GNU output.
2dc62c3 to
90e54fb
Compare
|
Existing Rust color tests were updated so their expected output now matches GNU ls. |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
c0088ea to
480b198
Compare
Colors: make StyleManager rely on lscolors’ explicit-style flags, avoid unnecessary metadata/stat probes, and ensure ln=target, or=, mi= honor GNU coloring semantics. Tests: update test_ls_color_norm expectations so the fi= and fi=0 cases now match GNU ls output, ensuring the new color handling is verified.
480b198 to
692c1a1
Compare
|
GNU testsuite comparison: |
- factor ANSI escape literals into named constants - document why we bypass LsColors fallbacks when applying raw SGR codes
|
GNU testsuite comparison: |
6164a04 to
af8afe0
Compare
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
Footnotes
|
|
GNU testsuite comparison: |
|
The CI failure is caused by a different issue (refer to #9276), not by this change. |
|
I was wondering if some of these changes should not in the crate lscolor. Wdyt? Thanks |
|
We already lean on lscolors for the generic bits (parsing LS_COLORS, providing Indicator styles, etc.). The code here lives on top because it’s tightly coupled to StyleManager state (indicator_codes, ln=target handling, PathData-based stat avoidance) and to the GNU-specific behavior we emulate. lscolors’ public API doesn’t expose those knobs, so moving this logic there would either require a significant API expansion or reintroduce the same context we already have on our side. For now it’s cleaner to keep the GNU-specific tailoring in ls.rs/colors.rs and continue reusing lscolors for the common functionality. In the future, if more consumers of lscolors run into the same GNU-specific needs (e.g., ln=target, empty or= semantics, stat avoidance hooks), we can revisit the API with the maintainers and see whether it makes sense to expose those knobs upstream. Until then, keeping this logic local lets us iterate quickly while still benefitting from lscolors for the generic parts. |
|
GNU testsuite comparison: |
|
@sylvestre Thanks for the review! If everything looks good, could you proceed with the merge? |
| style_manager.apply_style_based_on_metadata(path, md_option.as_ref(), name, wrap) | ||
| } | ||
|
|
||
| fn parse_indicator_codes() -> (HashMap<Indicator, String>, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe it could benefit from better error handling for malformed LS_COLOR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added GNU-like LS_COLORS validation with warnings and color disabled on parse errors.
src/uu/ls/src/ls.rs
Outdated
| fn escape_dir_name_with_locale(name: &OsStr, config: &Config) -> OsString { | ||
| if let Some(locale) = config.locale_quoting { | ||
| locale_quote(name, locale) | ||
| } else { | ||
| locale_aware_escape_dir_name(name, config.quoting_style) | ||
| } | ||
| } | ||
|
|
||
| fn escape_name_with_locale(name: &OsStr, config: &Config) -> OsString { | ||
| if let Some(locale) = config.locale_quoting { | ||
| locale_quote(name, locale) | ||
| } else { | ||
| locale_aware_escape_name(name, config.quoting_style) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it could be dedup a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deduplicated locale escaping by introducing a shared helper.
| style_code.push_str(self.reset(!self.initial_reset_is_done)); | ||
| style_code.push_str(&self.get_style_code(new_style)); | ||
| if let Some(path) = path { | ||
| // Fast-path: apply LS_COLORS raw SGR codes verbatim, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move this block into a function to decrease complexity ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted raw LS_COLORS application into helpers to reduce complexity.
src/uu/ls/src/colors.rs
Outdated
| force_suffix_reset = true; | ||
|
|
||
| if !applied_raw_code { | ||
| if let Some(new_style) = new_style { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe move this block into a function to decrease complexity ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved fallback style application into a helper to simplify the flow.
|
Thanks for the review! I’ll address the review comments. |
|
@karanabe ping ? |
Add GNU-like LS_COLORS validation and disable color output on parse errors with warnings. Refactor raw style application and fallback handling to reduce complexity and document empty entries. De-duplicate locale escaping and add warning strings for LS_COLORS errors.
|
Thanks for the ping. I’ve made a small update to this PR. This update adds GNU-like LS_COLORS validation and refactors style application to reduce complexity. |
|
GNU testsuite comparison: |
| validate_ls_colors(&ls_colors) | ||
| } | ||
|
|
||
| fn validate_ls_colors(ls_colors: &str) -> Result<(), LsColorsParseError> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this function could benefit from some comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a short comment describing the GNU-like LS_COLORS validation intent.
| } | ||
|
|
||
| let mut state = State::Ground; | ||
| loop { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a short comment explaining that the parser handles GNU-compatible escapes.
src/uu/ls/src/colors.rs
Outdated
| if raw.is_empty() { | ||
| Some(RawIndicatorStyle::Empty) | ||
| } else { | ||
| Some(RawIndicatorStyle::Code(raw.clone())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the clone()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the clone by switching to an Indicator-based path and re-fetching the raw code.
|
GNU testsuite comparison: |
Align ls’s symlink coloring with GNU semantics—or=, mi= and ln=target now behave exactly like /bin/ls, including emitting resets for blank or= entries—and add Rust integration tests that mirror the failing sl-dangle3..9 cases so regressions are caught locally.
#9127