mkdir: fixed not respecting set umask

[pyoky]

Resolves #3119.

Removed the permissions mode argument's hardcoded default_value() of 755, instead using uutils::mode::get_umask() when argument isn't specified.

Before:

$ umask 000 && ./coreutils mkdir new_dir && ls -l | grep new_dir
drwxr-xr-x  2 pyoky pyoky     4096 Feb 17 08:30 new_dir

After:

$ umask 000 && ./coreutils mkdir new_dir && ls -l | grep new_dir
d---------  2 pyoky pyoky     4096 Feb 17 08:29 new_dir

This mirrors the behavior of GNU mkdir.

[sylvestre]

Looks good but could you please add a test?

[pyoky]

@sylvestre I'm having issues writing a test that would modify the umask, create a new directory, and test if the permissions are expected. umask is a shell builtin, meaning that TestScenario can't execute it as a command. This is what I'm trying instead:

#[test]
#[cfg(not(windows))]
fn test_umask_compliance() {
    let scene = TestScenario::new(util_name!());
    let at = scene.fixtures.clone();
    let umask_out = scene.cmd("sh")
         .arg("-c")
         .arg("umask")
         .arg("000")
         .succeeds();
    let mkdir_out = scene.ucmd().arg(TEST_DIR8).succeeds();
    let perms = at.metadata(TEST_DIR8).permissions().mode();
    assert_eq!(perms, 0o40000);
}

but this has the issue that umask is dependent on the shell, which means as sh exits the modified umask disappears along with it.

I can write one that checks if it respects the already set umask but it won't be effective. What do you suggest?

[sylvestre]

@pyoky maybe try to do the two operations at the same time?

[tertsdiepraam]

I think that's a good solution. On my machine, this seems to give the correct umask:

❯ sh -c "umask 000 && mkdir a"
❯ mkdir b
❯ ls -l
total 0
drwxrwxrwx 2 terts terts 40 19 feb 01:48 a
drwxr-xr-x 2 terts terts 40 19 feb 01:48 b

[pyoky]

Thanks @sylvestre @tertsdiepraam, though in the end I wrote it to call libc::umask(), since it was similar to test_chmod.rs's permissions testing. Grateful for the kind response - this was my first oss contribution.

[sylvestre]

This fails:

error[E0308]: mismatched types
   --> /Users/runner/work/coreutils/coreutils/tests/by-util/test_mkdir.rs:113:45
    |
113 |         let original_umask = unsafe { umask(umask_set) };
    |                                             ^^^^^^^^^ expected `u16`, found `u32`
    |
help: you can convert a `u32` to a `u16` and panic if the converted value doesn't fit
    |
113 |         let original_umask = unsafe { umask(umask_set.try_into().unwrap()) };
    |                                                      ++++++++++++++++++++

could you please have a look? thanks

[pyoky]

@sylvestre I changed the umask_set to mode_t. Test should work now.

[sylvestre]

A few tests are failing, could you please have a look? Thanks

[sylvestre]

ping?

[pyoky]

@sylvestre Yup I'm working on it. I'll push a commit in a day or two.

[tertsdiepraam]

There is a small linting error. 746 tests are also suddenly failing, due to permission denied errors. Let's see if they're still around once the linting issue is fixed.

[pyoky]

@tertsdiepraam It seems like my use of libc::umask() messes with permissions of test directories with messes up permissions for tests that come after. The test now only test umasks with a reasonable number of permission combinations. (Testing all 0o0~0o777 was unnecessary anyways.)

While testing I also found a slight bug in test_pwd.rs where the directory name in a test command was not escaped properly. It was a minor bug so I included in the commit with descriptions.

Lints should also be fixed. Sorry for my mistakes - I'm still new to contributions.

[tertsdiepraam]

No worries! Mistakes are basically expected :) Great work on the fix!

[tertsdiepraam]

I think this should fix the last clippy error

[pyoky]

@tertsdiepraam Is there an issue still with checks? I'm seeing the tests are canceled. If there's still issues with merging let me know.

[tertsdiepraam]

I'm rerunning those checks. If they pass, I think this can get merged.

[jfinkels]

Cool

[jfinkels]

These are not all the possible permission settings. That's fine, let's just make sure the comment matches the code.