Skip to content

ls: Match the gnu behavior for colors #5603

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Dec 9, 2023
Merged

ls: Match the gnu behavior for colors #5603

merged 4 commits into from
Dec 9, 2023

Conversation

sylvestre
Copy link
Contributor

No description provided.

@sylvestre
Copy link
Contributor Author

it is failing but I would like to get the results

@sylvestre sylvestre changed the title Gnu legacy ls: Match the gnu behavior for colors Nov 30, 2023
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!
GNU test failed: tests/tail/inotify-dir-recreate. tests/tail/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

1 similar comment
@github-actions GitHub Actions
Copy link

GNU testsuite comparison:

Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!
GNU test failed: tests/tail/inotify-dir-recreate. tests/tail/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

@sylvestre sylvestre marked this pull request as ready for review December 5, 2023 23:15
@sylvestre sylvestre requested review from tertsdiepraam and cakebaker and removed request for tertsdiepraam December 5, 2023 23:15
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!

cakebaker
cakebaker reviewed Dec 6, 2023
View reviewed changes
cakebaker
cakebaker reviewed Dec 6, 2023
View reviewed changes
src/uu/ls/Cargo.toml Outdated
@@ -22,7 +22,7 @@ number_prefix = { workspace = true }
uutils_term_grid = { workspace = true }
terminal_size = { workspace = true }
glob = { workspace = true }
lscolors = { workspace = true }
lscolors = { workspace = true, features = ["gnu_legacy"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? We didn't have the nu-ansi-term feature mentioned here.

cakebaker
cakebaker reviewed Dec 6, 2023
View reviewed changes
Copy link
Contributor

@cakebaker cakebaker left a comment

Choose a reason for hiding this comment

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

I'm not sure whether this is the intended behavior or a bug. When running ls --color=always and redirecting the output to a file, the output of uutils ls and GNU ls differs. All but the first escape code are different, with the ones from uutils ls being prefixed with ^[[0m.

@sylvestre
Copy link
Contributor Author

how do you compare the output ?
i did

ls > out
cat -v out

@cakebaker
Copy link
Contributor

Yes, that's what I'm using.

@cakebaker
Copy link
Contributor

Here the steps I did:

$ mkdir example example/a example/b
$ cargo run ls --color=always example/ > color.txt
$ ls --color=always example/ > colorgnu.txt
$ cat -v color.txt colorgnu.txt 
^[[0m^[[01;34ma^[[0m
^[[0m^[[01;34mb^[[0m
^[[0m^[[01;34ma^[[0m
^[[01;34mb^[[0m

@sylvestre
Copy link
Contributor Author

The difference is that GNU ls does not immediately reset the text style after each directory name. It only resets (^[[0m) before starting a new colored text, or at the end of the output.

tertsdiepraam
tertsdiepraam reviewed Dec 6, 2023
View reviewed changes
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 6, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!
GNU test failed: tests/tail/inotify-dir-recreate. tests/tail/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 9, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/ls/multihardlink is no longer failing!
Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!
GNU test failed: tests/tail/inotify-dir-recreate. tests/tail/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

tertsdiepraam
tertsdiepraam approved these changes Dec 9, 2023
View reviewed changes
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice! The spell-check is failing but once that's fixed LGTM!

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 9, 2023

GNU testsuite comparison:

Congrats! The gnu test tests/ls/multihardlink is no longer failing!
Congrats! The gnu test tests/ls/root-rel-symlink-color is no longer failing!

@sylvestre sylvestre merged commit 0308e01 into uutils:main Dec 9, 2023
@sylvestre sylvestre deleted the gnu-legacy branch December 9, 2023 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cakebaker cakebaker cakebaker left review comments

@tertsdiepraam tertsdiepraam tertsdiepraam approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sylvestre @cakebaker @tertsdiepraam