ls: align --ignore behavior with that of GNU ls

[ackerleytng]

Fixes #3753

[sylvestre]

I guess you saw

failures:

---- test_ls::test_ls_ignore_explicit_period stdout ----
current_directory_resolved: 
touch: C:\Users\RUNNER~1\AppData\Local\Temp\.tmpr8LvIE\.hidden.yml
touch: C:\Users\RUNNER~1\AppData\Local\Temp\.tmpr8LvIE\regular.yml
run: D:\a\coreutils\coreutils\target\i686-pc-windows-msvc\debug\coreutils.exe ls -a --ignore ?hidden.yml
thread 'test_ls::test_ls_ignore_explicit_period' panicked at ''.
..
regular.yml
' does not contain '.hidden.yml'', tests\common\util.rs:410:9

---- test_ls::test_ls_ignore_negation stdout ----
current_directory_resolved: 
touch: C:\Users\RUNNER~1\AppData\Local\Temp\.tmpxNeZbk\apple
touch: C:\Users\RUNNER~1\AppData\Local\Temp\.tmpxNeZbk\boy
run: D:\a\coreutils\coreutils\target\i686-pc-windows-msvc\debug\coreutils.exe ls --ignore [!a]*
run: D:\a\coreutils\coreutils\target\i686-pc-windows-msvc\debug\coreutils.exe ls --ignore [^a]*
thread 'test_ls::test_ls_ignore_negation' panicked at ''boy
' does not contain 'apple'', tests\common\util.rs:410:9


failures:
    test_ls::test_ls_ignore_explicit_period
    test_ls::test_ls_ignore_negation

[ackerleytng]

Interesting! When I was working on it I didn't get these failures, then when I saw this I tried again and saw the failures, but after checking out the same commit again the failures fixed themselves. I rebased on the upstream, here's another try.

[sylvestre]

fails again :)

[sylvestre]

Needs tests fixes

[ackerleytng]

Yup I'll work on it!

[sylvestre]

thanks!

[tertsdiepraam]

These tests are tricky for windows, because we do the argument expansion usually done by the unix shell ourselves for Windows. Take this test for example:

scene
    .ucmd()
    .arg("--ignore")
    .arg("[^a]*")
    .succeeds()
    .stdout_contains("apple")
    .stdout_does_not_contain("boy");

Here's what I think happens: the [^a] is interpreted as the character class containing ^ and a, so all files starting with a are ignored and the stdout does not contain apple. In the docs from wild they state that quoted arguments are not expanded, so that might be a way around this.

[sylvestre]

Could you please fix the conflicts? thanks

[ackerleytng]

I'm a little busy these free weeks but I'll definitely get to this

[ackerleytng]

Just rebased, letting tests run to see how it goes now.

[ackerleytng]

@tertsdiepraam is right.

Was digging into this a little - in the tests, taking ls -a --ignore *.yml of test_ls_ignore_explicit_period as an example,

• in Linux, *.yml is interpreted as an argument to --ignore, so the tests pass as expected.
• in Windows, *.yml is expanded first, so regular.yml is seen as a file argument and is listed out.

I can dig a little into why this expansion happens. Any tips?

[ackerleytng]

@tertsdiepraam Could you please point me to the part of the code that does shell expansion on Windows?

[tertsdiepraam]

Sure! There's a uucore::bin (defined in lib.rs) macro that we use inside of the main.rs files of the utils. In this macro, uucore::args_os (also defined in lib.rs) is called. This uses some lazy constant ARGV which has the value of wild::args_os. wild is a crate that does the expansion. The same uucore::args_os function is called in the main function of the multicall binary.

[ackerleytng]

Would you suggest removing that expansion?

[tertsdiepraam]

I'm not sure to he honest. It's quite convenient to have on Windows and gives the "unsurprising behaviour" in many cases. At least within the context of this PR, we should just work around it, but feel free to open an issue to discuss the expansion.

[ackerleytng]

Indeed, removing the glob expansion with this patch allows all ls tests to pass on windows.

diff --git a/src/uucore/src/lib/lib.rs b/src/uucore/src/lib/lib.rs
index e216b8fe3..f08d2d3c9 100644
--- a/src/uucore/src/lib/lib.rs
+++ b/src/uucore/src/lib/lib.rs
@@ -114,7 +114,7 @@ pub fn set_utility_is_second_arg() {

 // args_os() can be expensive to call, it copies all of argv before iterating.
 // So if we want only the first arg or so it's overkill. We cache it.
-static ARGV: Lazy<Vec<OsString>> = Lazy::new(|| wild::args_os().collect());
+static ARGV: Lazy<Vec<OsString>> = Lazy::new(|| std::env::args_os().collect());

 static UTIL_NAME: Lazy<String> = Lazy::new(|| {
     if get_utility_is_second_arg() {

I'll disable these tests on windows and create a new issue to discuss the expansion.

[sylvestre]

Nice:
Warning: Congrats! The gnu test tests/ls/readdir-mountpoint-inode is no longer failing!