Skip to content

Make ls pass the GNU ls-misc.pl suite#9262

Merged
sylvestre merged 11 commits into
uutils:mainfrom
karanabe:test/fix-ls-mics
Jan 8, 2026
Merged

Make ls pass the GNU ls-misc.pl suite#9262
sylvestre merged 11 commits into
uutils:mainfrom
karanabe:test/fix-ls-mics

Conversation

@karanabe

Copy link
Copy Markdown
Contributor

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

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.
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.
@karanabe

Copy link
Copy Markdown
Contributor Author

Existing Rust color tests were updated so their expected output now matches GNU ls.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

GNU test failed: tests/ls/color-dtype-dir. tests/ls/color-dtype-dir is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/ls/color-norm. tests/ls/color-norm is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/ls/multihardlink. tests/ls/multihardlink is passing on 'main'. Maybe you have to rebase?
GNU test failed: tests/ls/stat-free-color. tests/ls/stat-free-color is passing on 'main'. Maybe you have to rebase?
Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/ls/ls-misc is no longer failing!

@karanabe
karanabe marked this pull request as draft November 13, 2025 16:09
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/misc/tee (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/ls/ls-misc is no longer failing!

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.
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/ls/ls-misc is no longer failing!

Comment thread src/uu/ls/src/colors.rs Outdated
Comment thread src/uu/ls/src/colors.rs Outdated
Comment thread src/uu/ls/src/ls.rs Outdated
- factor ANSI escape literals into named constants
- document why we bypass LsColors fallbacks when applying raw SGR codes
@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/ls/ls-misc is no longer failing!

@karanabe
karanabe force-pushed the test/fix-ls-mics branch 3 times, most recently from 6164a04 to af8afe0 Compare November 14, 2025 16:19
@codspeed-hq

codspeed-hq Bot commented Nov 14, 2025

Copy link
Copy Markdown

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing karanabe:test/fix-ls-mics (a9b5ab5) with main (b804ca4)

Summary

✅ 140 untouched benchmarks
⏩ 37 skipped benchmarks1

Footnotes

  1. 37 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/ls/ls-misc is no longer failing!

@karanabe

karanabe commented Nov 14, 2025

Copy link
Copy Markdown
Contributor Author

The CI failure is caused by a different issue (refer to #9276), not by this change.
CICD / Test all features separately (ubuntu-latest, feat_os_unix) (pull_request)

@karanabe
karanabe marked this pull request as ready for review November 14, 2025 17:38
@karanabe
karanabe requested a review from sylvestre November 14, 2025 17:38
@sylvestre

Copy link
Copy Markdown
Contributor

I was wondering if some of these changes should not in the crate lscolor. Wdyt? Thanks

@karanabe

Copy link
Copy Markdown
Contributor Author

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.

@sylvestre

Copy link
Copy Markdown
Contributor

@karanabe ok, thanks!
@sharkdp in case there is anything you would like to cherrypick for lscolors :)

@github-actions

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/ls/ls-misc is no longer failing!

@karanabe

Copy link
Copy Markdown
Contributor Author

@sylvestre Thanks for the review! If everything looks good, could you proceed with the merge?

Comment thread src/uu/ls/src/colors.rs
style_manager.apply_style_based_on_metadata(path, md_option.as_ref(), name, wrap)
}

fn parse_indicator_codes() -> (HashMap<Indicator, String>, bool) {

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/uu/ls/src/ls.rs Outdated
Comment on lines +2113 to +2127
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)
}
}

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/uu/ls/src/colors.rs
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,

Copy link
Copy Markdown
Contributor

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 ?

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/uu/ls/src/colors.rs Outdated
force_suffix_reset = true;

if !applied_raw_code {
if let Some(new_style) = new_style {

Copy link
Copy Markdown
Contributor

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 ?

Copy link
Copy Markdown
Contributor Author

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.

@karanabe

Copy link
Copy Markdown
Contributor Author

Thanks for the review! I’ll address the review comments.

@sylvestre

Copy link
Copy Markdown
Contributor

@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.
@karanabe

karanabe commented Jan 7, 2026

Copy link
Copy Markdown
Contributor Author

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.
Locale escaping was de-duplicated.

@github-actions

github-actions Bot commented Jan 7, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/ls/ls-misc is no longer failing!

Comment thread src/uu/ls/src/colors.rs
validate_ls_colors(&ls_colors)
}

fn validate_ls_colors(ls_colors: &str) -> Result<(), LsColorsParseError> {

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/uu/ls/src/colors.rs
}

let mut state = State::Ground;
loop {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Copy Markdown
Contributor Author

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.

Comment thread src/uu/ls/src/colors.rs Outdated
if raw.is_empty() {
Some(RawIndicatorStyle::Empty)
} else {
Some(RawIndicatorStyle::Code(raw.clone()))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove the clone()

Copy link
Copy Markdown
Contributor Author

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.

@github-actions

github-actions Bot commented Jan 7, 2026

Copy link
Copy Markdown

GNU testsuite comparison:

Congrats! The gnu test tests/ls/ls-misc is no longer failing!

@sylvestre
sylvestre merged commit 0b0968a into uutils:main Jan 8, 2026
131 checks passed
@karanabe
karanabe deleted the test/fix-ls-mics branch January 17, 2026 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants