ls: Fix quoting for dirnames with a colon `:` in recursive mode, and patch the `quote-align` GNU test #6559

RenjiSann · 2024-07-10T14:53:44Z

Fixes #6554 and adds a test for it.

github-actions · 2024-07-10T15:20:05Z

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/inotify-dir-recreate (passes in this run but fails in the 'main' branch)

RenjiSann · 2024-07-18T15:22:52Z

@BenWiederhake @sylvestre @tertsdiepraam ping, if one of you has the time to take a look :)

sylvestre · 2024-07-18T15:28:17Z

Sorry, i didn't look because your test fails a bunch of jobs:

--- TRY 3 STDERR:        coreutils::tests test_ls::test_ls_recursive_escape_dirname ---
thread 'test_ls::test_ls_recursive_escape_dirname' panicked at tests\common\util.rs:956:40:
called `Result::unwrap()` on an `Err` value: Os { code: 267, kind: NotADirectory, message: "The directory name is invalid." }
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library\std\src\panicking.rs:652
   1: core::panicking::panic_fmt
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library\core\src\panicking.rs:72
   2: core::result::unwrap_failed
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library\core\src\result.rs:1654
   3: enum2$<core::result::Result<tuple$<>,std::io::error::Error> >::unwrap
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081\library\core\src\result.rs:1077
   4: tests::common::util::AtPath::mkdir<ref$<str$> >
             at .\tests\common\util.rs:956
   5: tests::test_ls::test_ls_recursive_escape_dirname
             at .\tests\by-util\test_ls.rs:2204
   6: tests::test_ls::test_ls_recursive_escape_dirname::closure$0
             at .\tests\by-util\test_ls.rs:2201
   7: core::ops::function::FnOnce::call_once<tests::test_ls::test_ls_recursive_escape_dirname::closure_env$0,tuple$<> >
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081\library\core\src\ops\function.rs:250
   8: core::ops::function::FnOnce::call_once
             at /rustc/129f3b9964af4d4a709d1383930ade12dfe7c081/library\core\src\ops\function.rs:250
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

github-actions · 2024-07-18T15:53:25Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)

src/uu/ls/src/ls.rs

src/uucore/src/lib/features/quoting_style.rs

RenjiSann · 2024-07-22T09:24:56Z

I have guessed that when using the shell quoting style, ~~with show-control-chars display enabled, ls should quote the name if it does encounter a control character.~~

~~Commit 83c1447 handle this case.~~

EDIT: It seems that in shell mode, control characters induce simple quoting, no matter the status of show_control.

RenjiSann · 2024-07-22T14:22:04Z

I have made a substantial change in the quoting_style file, regarding the quoting behavior of control characters in shell-* modes.

Even though I am pretty sure this change is sound for ls, it might have repercussions on other utils that use quoting_style

RenjiSann · 2024-07-25T21:51:42Z

I realize this PR does duplicate the work in #6555.

Let's compare the changes and keep the best of both.

github-actions · 2024-07-25T22:41:51Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/ls/quote-align is no longer failing!
Congrats! The gnu test tests/rm/rm2 is no longer failing!

RenjiSann · 2024-07-25T22:43:56Z

Style/Lint jobs are failing due to the clippy bump, but I will rebase the PR once #6592 has landed.

github-actions · 2024-07-25T23:43:27Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/ls/quote-align is no longer failing!
Congrats! The gnu test tests/rm/rm2 is no longer failing!

RenjiSann · 2024-07-25T23:47:47Z

I keep finding edge cases : \a is a control character, yet it shall not be escaped in shell mode. I need to rework my solution.

So far:

$ LC_ALL=C /usr/bin/ls --quoting=shell test
 xx?bell   xx?backspace  'xx?tab'  'xx?linefeed'   xx?vtab   xx?formfeed  'xx?carriage'  'xx space'

$ LC_ALL=C ./target/debug/ls --quoting=shell test
'xx?bell'  'xx?backspace'  'xx?tab'  'xx?linefeed'  'xx?vtab'  'xx?formfeed'  'xx?carriage'  'xx space'

I don't see a specific pattern, so I guess we will have to do it by hand.

github-actions · 2024-08-12T14:32:07Z

GNU testsuite comparison:

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

src/uucore/src/lib/features/quoting_style.rs

github-actions · 2024-09-16T08:43:53Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/inotify-dir-recreate (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/ls/quote-align is no longer failing!

src/uucore/src/lib/features/quoting_style.rs

tests/by-util/test_ls.rs

github-actions · 2024-09-18T13:56:27Z

GNU testsuite comparison:

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

github-actions · 2024-09-18T14:52:09Z

GNU testsuite comparison:

Congrats! The gnu test tests/ls/quote-align is no longer failing!
Skipping an intermittent issue tests/rm/rm1 (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/timeout/timeout is no longer failing!

BenWiederhake · 2024-10-06T11:47:18Z

Current set of problems with this PR:

git conflict, cannot merge as-is
= should be quoted in --quoting-style=shell
= should be quoted in --quoting-style=shell-escape
Non-unicode filenames are lost, everything is replaced by \xEF\xBF\xBD ("replacement character"). If you wish we can define this as out-of-scope for this PR.
Filenames with high bit set (including valid unicode) in c, escape, shell-escape, and shell-escape-always. This is known issue ls: printing of 8-bit filenames is incorrect in most cases in LC_ALL=C #6639, and therefore out-of-scope.

github-actions · 2024-10-10T16:13:18Z

GNU testsuite comparison:

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

RenjiSann · 2024-10-10T16:17:02Z

Current set of problems with this PR:

* `=` should be quoted in `--quoting-style=shell`

* `=` should be quoted in `--quoting-style=shell-escape`

* Non-unicode filenames are lost, everything is replaced by `\xEF\xBF\xBD` ("replacement character"). If you wish we can define this as out-of-scope for this PR.

* Filenames with high bit set (including valid unicode) in `c`, `escape`, `shell-escape`, and `shell-escape-always`. This is known issue [ls: printing of 8-bit filenames is incorrect in most cases in `LC_ALL=C` #6639](https://github.com/uutils/coreutils/issues/6639), and therefore out-of-scope.

Thanks for the review.

The conflict and the behavior for = are fixed.

Regarding the two other points, we can leave them for future PRs, I think it's out of the scope of this PR.

github-actions · 2024-10-15T09:38:39Z

GNU testsuite comparison:

GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?
Congrats! The gnu test tests/ls/quote-align is no longer failing!

sylvestre · 2024-10-15T09:44:21Z

thanks :)

sylvestre force-pushed the renji/fix-6554 branch from 88127cc to b3b8504 Compare July 18, 2024 15:27

BenWiederhake reviewed Jul 18, 2024

View reviewed changes

src/uu/ls/src/ls.rs Outdated Show resolved Hide resolved

RenjiSann force-pushed the renji/fix-6554 branch from b3b8504 to f121149 Compare July 19, 2024 10:30

BenWiederhake reviewed Jul 19, 2024

View reviewed changes

src/uucore/src/lib/features/quoting_style.rs Outdated Show resolved Hide resolved

RenjiSann force-pushed the renji/fix-6554 branch 5 times, most recently from ff73f42 to a754178 Compare July 22, 2024 08:34

RenjiSann force-pushed the renji/fix-6554 branch 3 times, most recently from 9c5b073 to c709722 Compare July 22, 2024 14:18

RenjiSann mentioned this pull request Jul 25, 2024

ls: fix for gnu test case quote-align compatibility #6555

Closed

RenjiSann changed the title ~~ls: Fix quoting for dirnames with a colon : in recursive mode~~ ls: Fix quoting for dirnames with a colon : in recursive mode, and patch the quote-align GNU test Jul 25, 2024

sylvestre force-pushed the renji/fix-6554 branch from 6fce655 to c9d665f Compare July 25, 2024 22:45

RenjiSann force-pushed the renji/fix-6554 branch 3 times, most recently from 5d96927 to fc0f388 Compare July 25, 2024 22:48

RenjiSann force-pushed the renji/fix-6554 branch from fc0f388 to 95d8c96 Compare July 26, 2024 00:50

RenjiSann force-pushed the renji/fix-6554 branch from 95d8c96 to bb716e2 Compare August 12, 2024 14:05

sylvestre reviewed Aug 13, 2024

View reviewed changes

src/uucore/src/lib/features/quoting_style.rs Show resolved Hide resolved

RenjiSann force-pushed the renji/fix-6554 branch from bb716e2 to 72619bf Compare August 17, 2024 19:41

sylvestre reviewed Sep 18, 2024

View reviewed changes

src/uucore/src/lib/features/quoting_style.rs Outdated Show resolved Hide resolved

sylvestre reviewed Sep 18, 2024

View reviewed changes

tests/by-util/test_ls.rs Show resolved Hide resolved

RenjiSann force-pushed the renji/fix-6554 branch from b5eae0f to 9498f54 Compare September 18, 2024 13:29

RenjiSann force-pushed the renji/fix-6554 branch 2 times, most recently from 014fbc0 to 66b0f12 Compare September 18, 2024 14:24

RenjiSann force-pushed the renji/fix-6554 branch from 66b0f12 to 300bf10 Compare October 10, 2024 15:47

sylvestre requested a review from BenWiederhake October 10, 2024 20:59

RenjiSann and others added 5 commits October 15, 2024 11:11

ls: Escape dirname in recursive mode when it has a colon

3c5a419

ls: Fix quoting for control characters in shell mode

e292e05

test(quoting_style): Fix tests

2a1558f

test(ls): Add test for uutils#6554

758b103

patch build-gnu.sh for quote-align

68e724f

RenjiSann force-pushed the renji/fix-6554 branch from 300bf10 to 68e724f Compare October 15, 2024 09:11

sylvestre merged commit 6935a0d into uutils:main Oct 15, 2024

BrewTestBot mentioned this pull request Nov 16, 2024

uutils-coreutils 0.0.28 Homebrew/homebrew-core#197947

Merged

moonfruit mentioned this pull request Nov 22, 2024

uutils-selected 0.0.28 moonfruit/homebrew-tap#178

Closed

RenjiSann mentioned this pull request Dec 17, 2024

quoting_style: Add support for non-UTF-8 bytes #6882

Merged

RenjiSann deleted the renji/fix-6554 branch January 29, 2025 15:21

Uh oh!

ls: Fix quoting for dirnames with a colon : in recursive mode, and patch the quote-align GNU test #6559

ls: Fix quoting for dirnames with a colon : in recursive mode, and patch the quote-align GNU test #6559

Uh oh!

Conversation

RenjiSann commented Jul 10, 2024

Uh oh!

github-actions bot commented Jul 10, 2024

Uh oh!

RenjiSann commented Jul 18, 2024

Uh oh!

sylvestre commented Jul 18, 2024

Uh oh!

github-actions bot commented Jul 18, 2024

Uh oh!

Uh oh!

Uh oh!

RenjiSann commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RenjiSann commented Jul 22, 2024

Uh oh!

RenjiSann commented Jul 25, 2024

Uh oh!

github-actions bot commented Jul 25, 2024

Uh oh!

RenjiSann commented Jul 25, 2024

Uh oh!

github-actions bot commented Jul 25, 2024

Uh oh!

RenjiSann commented Jul 25, 2024

Uh oh!

github-actions bot commented Aug 12, 2024

Uh oh!

Uh oh!

github-actions bot commented Sep 16, 2024

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 18, 2024

Uh oh!

github-actions bot commented Sep 18, 2024

Uh oh!

BenWiederhake commented Oct 6, 2024

Uh oh!

github-actions bot commented Oct 10, 2024

Uh oh!

RenjiSann commented Oct 10, 2024

Uh oh!

github-actions bot commented Oct 15, 2024

Uh oh!

sylvestre commented Oct 15, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ls: Fix quoting for dirnames with a colon `:` in recursive mode, and patch the `quote-align` GNU test #6559

ls: Fix quoting for dirnames with a colon `:` in recursive mode, and patch the `quote-align` GNU test #6559

RenjiSann commented Jul 22, 2024 •

edited

Loading