`ls -lF` symlink target indicators by joknarf · Pull Request #11554 · uutils/coreutils

joknarf · 2026-03-30T21:32:46Z

Fixes #11542
gnu output incompatibility with --classify on symlink targets

before:

 • ls -lF mylink*
lrwxrwxrwx 1 user group 5 Nov 22 08:33 mylinktodir@ -> mydir
lrwxrwxrwx 1 user group 9 Nov 22 08:31 mylinktoexe@ -> myexec.sh
lrwxrwxrwx 1 user group 6 Nov 22 08:34 mylinktofile@ -> myfile
lrwxrwxrwx 1 user group 6 Nov 22 08:47 mylinktopipe@ -> mypipe
lrwxrwxrwx 1 user group 6 Mar 29 10:42 mylinktosock@ -> mysock

after (gnu identical output):

 • gnuls -lF mylink*
lrwxrwxrwx 1 user group 5 Nov 22 09:33 mylinktodir -> mydir/
lrwxrwxrwx 1 user group 9 Nov 22 09:31 mylinktoexe -> myexec.sh*
lrwxrwxrwx 1 user group 6 Nov 22 09:34 mylinktofile -> myfile
lrwxrwxrwx 1 user group 6 Nov 22 09:47 mylinktopipe -> mypipe|
lrwxrwxrwx 1 user group 6 Mar 29 10:42 mylinktosock -> mysock=

Alonely0

Looks fine, but there are a lot of removed comments (vibecode?) and unnecessary performance hits.

Alonely0 · 2026-03-30T21:54:44Z

src/uu/ls/src/display.rs

 }

+fn indicator_char(path: &PathData, style: IndicatorStyle) -> Option<char> {
+    let sym = classify_file(path);


The classify_files() function is now called unconditionally, but beforehand it was contingent on config.indicator_style != IndicatorStyle::None. I'm concerned this is going to negatively affect performance since it has to stat files. Maybe the best solution is to remove the IndicatorStyle::None branch and change all occurrences of IndicatorStyle to Option<IndicatorStyle>, then do a let-else.

PD: please keep the old comments on the code you refactored to this new function. Did you vibecode this? Don't take it the wrong way; this is just a usual telltale.

Alonely0 · 2026-03-30T22:11:27Z

src/uu/ls/src/display.rs

+                        None => target_path.clone(),
+                    }
+                } else {
+                    target_path.clone()


Please do not clone here; it is not necessary. Use references, like the previous code that you have replaced. This was fixed in edfe26c.

Alonely0 · 2026-03-30T22:16:33Z

src/uu/ls/src/display.rs

                                ),
                            );
+                        } else {
+                            name.push(escaped_target);


Please keep comments. Previously in L804-805.

Alonely0 · 2026-03-30T22:27:14Z

src/uu/ls/src/display.rs

+                            false,
+                        );
+
+                        let mut target_display = escaped_target.clone();


Please do not clone here. You should refactor the immediate assignment inside the if to be the expression of the let definition, like let var = if ... { ... } else { ... };. Alternatively, use references or a Cow if you have to.

Alonely0 · 2026-03-30T22:32:06Z

src/uu/ls/src/display.rs

-                                false,
-                            );
-
-                            // Check if the target actually needs coloring


Keep this comment if you can.

Alonely0 · 2026-03-30T22:32:13Z

src/uu/ls/src/display.rs

-                            );

-                            if style.is_some() {
-                                // Only apply coloring if there's actually a style


Keep this comment if you can.

Alonely0 · 2026-03-30T22:34:44Z

src/uu/ls/src/display.rs

                                    is_wrap(name.len()),
-                                ));
-                            } else {
-                                // For regular files with no coloring, just use plain text


Once/if you apply the other requested changes, consider keeping this comment.

github-actions · 2026-03-31T05:18:07Z

GNU testsuite comparison:

Skip an intermittent issue tests/date/date-locale-hour (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Skip an intermittent issue tests/tty/tty-eof (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/seq/seq-epipe is now passing!

codspeed-hq · 2026-03-31T05:29:27Z

Merging this PR will degrade performance by 4.83%

⚠️

Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
❌ 3 regressed benchmarks
✅ 300 untouched benchmarks
⏩ 46 skipped benchmarks¹

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Mode	Benchmark	`BASE`	`HEAD`	Efficiency
❌	Simulation	`ls_recursive_balanced_tree[(6, 4, 15)]`	51.9 ms	54.3 ms	-4.37%
❌	Simulation	`ls_recursive_deep_tree[(200, 2)]`	1.7 ms	1.8 ms	-4.83%
❌	Simulation	`ls_recursive_wide_tree[(10000, 1000)]`	36 ms	37.7 ms	-4.34%
⚡	Simulation	`cp_large_file[16]`	279.5 µs	264.9 µs	+5.51%

_{Comparing joknarf:classify_target (c9b81dc) with main (236e220)}

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

sylvestre · 2026-03-31T07:18:39Z

please fix the failing tasks

joknarf mentioned this pull request Mar 30, 2026

ls -lF (--classify) symlink target indicator missing #11542

Open

joknarf force-pushed the classify_target branch from 9f9989d to f11d4e6 Compare March 30, 2026 21:40

ls -lF symlink target indicator

c9b81dc

joknarf force-pushed the classify_target branch from 77108f2 to c9b81dc Compare March 30, 2026 22:20

Alonely0 suggested changes Mar 30, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

`ls -lF` symlink target indicators#11554

`ls -lF` symlink target indicators#11554
joknarf wants to merge 1 commit intouutils:mainfrom
joknarf:classify_target

joknarf commented Mar 30, 2026 •

edited by cakebaker

Loading

Uh oh!

Alonely0 left a comment

Uh oh!

Alonely0 Mar 30, 2026

Uh oh!

Alonely0 Mar 30, 2026

Uh oh!

Alonely0 Mar 30, 2026

Uh oh!

Alonely0 Mar 30, 2026

Uh oh!

Alonely0 Mar 30, 2026

Uh oh!

Alonely0 Mar 30, 2026

Uh oh!

Alonely0 Mar 30, 2026

Uh oh!

Alonely0 Mar 30, 2026

Uh oh!

github-actions bot commented Mar 31, 2026

Uh oh!

codspeed-hq bot commented Mar 31, 2026

Uh oh!

sylvestre commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

joknarf commented Mar 30, 2026 • edited by cakebaker Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alonely0 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 31, 2026

Uh oh!

codspeed-hq bot commented Mar 31, 2026

Merging this PR will degrade performance by 4.83%

Performance Changes

Footnotes

Uh oh!

sylvestre commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joknarf commented Mar 30, 2026 •

edited by cakebaker

Loading