Skip to content
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

ls: Match the gnu behavior for colors #5603

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?

@sylvestre sylvestre force-pushed the gnu-legacy branch 2 times, most recently from cf3f1a6 to 26ad2ff Compare November 30, 2023 17:00
@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 force-pushed the gnu-legacy branch 2 times, most recently from 6e2b976 to 07e755c Compare December 5, 2023 23:15
@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
Cargo.toml Outdated Show resolved Hide resolved
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
tests/by-util/test_ls.rs Show resolved Hide resolved
tests/by-util/test_ls.rs Show resolved Hide resolved
tests/by-util/test_ls.rs Outdated Show resolved Hide resolved
@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
54 of 55 checks passed
@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