Skip to content

Conversation

@karanabe
Copy link
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
Contributor Author

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

@github-actions
Copy link

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

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

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!

- factor ANSI escape literals into named constants
- document why we bypass LsColors fallbacks when applying raw SGR codes
@github-actions
Copy link

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
Copy link

codspeed-hq bot commented Nov 14, 2025

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

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
Copy link
Contributor Author

karanabe commented Nov 14, 2025

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
Contributor

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

@karanabe
Copy link
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
Contributor

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

@github-actions
Copy link

GNU testsuite comparison:

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

@karanabe
Copy link
Contributor Author

@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) {
Copy link
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
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 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
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
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.

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

force_suffix_reset = true;

if !applied_raw_code {
if let Some(new_style) = new_style {
Copy link
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
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
Contributor Author

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

@sylvestre
Copy link
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
Copy link
Contributor Author

karanabe commented Jan 7, 2026

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
Copy link

github-actions bot commented Jan 7, 2026

GNU testsuite comparison:

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

validate_ls_colors(&ls_colors)
}

fn validate_ls_colors(ls_colors: &str) -> Result<(), LsColorsParseError> {
Copy link
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
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.

}

let mut state = State::Ground;
loop {
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
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.

if raw.is_empty() {
Some(RawIndicatorStyle::Empty)
} else {
Some(RawIndicatorStyle::Code(raw.clone()))
Copy link
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
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
Copy link

github-actions bot commented Jan 7, 2026

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
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