rm: when error during recursive, stops prematurely

[niyaznigmatullin]

uutils doesn't remove b/d in the following example

$ mkdir -p b/a/p
$ mkdir b/c b/d
$ chmod -w b/a
$ rm -rf b
rm: cannot remove 'b': Permission denied
$ ls b
a d

but GNU removes b/d

That's probably because of fs::remove_dir_all behavior

[tommady]

I noticed that this issue with the test added above can be passed at the current main branch (615b562).

❯ cargo test test_rm
   Compiling coreutils v0.0.22 (/home/arch/code/tommady/coreutils)
    Finished test [unoptimized + debuginfo] target(s) in 2.67s
     Running unittests src/bin/coreutils.rs (target/debug/deps/coreutils-057d678475d87ea4)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/test_util_name.rs (target/debug/deps/test_util_name-befa9301860f5b3e)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 61 filtered out; finished in 0.00s

     Running tests/tests.rs (target/debug/deps/tests-49e604e72fb6a35c)

running 44 tests
test test_rm::test_rm_descend_directory ... ok
test test_rm::test_fifo_removal ... ok
test test_rm::test_rm_directory_without_flag ... ok
test test_rm::test_rm_force_multiple ... ok
test test_rm::test_rm_force ... ok
test test_rm::test_rm_empty_directory_verbose ... ok
test test_rm::test_invalid_arg ... ok
test test_rm::test_rm_interactive_never ... ok
test test_rm::test_rm_empty_directory ... ok
test test_rm::test_rm_force_no_operand ... ok
test test_rm::test_non_utf8 ... ok
test test_rm::test_rm_directory_rights_rm1 ... ok ---> here
test test_rm::test_rm_interactive_missing_value ... ok
test test_rm::test_rm_failed ... ok
test test_rm::test_rm_force_prompts_order ... ok
test test_rm::test_rm_invalid_symlink ... ok
test test_rm::test_rm_no_operand ... ok
test test_rm::test_rm_interactive_once_recursive_prompt ... ok
test test_rm::test_rm_interactive_once_prompt ... ok
test test_rm::test_rm_multiple_files ... ok
test test_rm::test_rm_recursive ... ok
test test_rm::test_rm_symlink_dir ... ok
test test_rm::test_rm_non_empty_directory ... ok
test test_rm::test_rm_silently_accepts_presume_input_tty2 ... ok
test test_rm::test_rm_verbose_slash ... ok
test test_rmdir::test_invalid_arg ... ok
test test_rm::test_rm_recursive_multiple ... ok
test test_rm::test_rm_verbose ... ok
test test_rm::test_rm_one_file ... ok
test test_rmdir::test_rmdir_remove_symlink_dangling ... ok
test test_rmdir::test_rmdir_empty_directory_with_parents ... ok
test test_rmdir::test_rmdir_ignore_nonempty_directory_no_parents ... ok
test test_rmdir::test_rmdir_remove_symlink_file ... ok
test test_rmdir::test_rmdir_empty_directory_no_parents ... ok
test test_rmdir::test_rmdir_ignore_nonempty_no_permissions ... ok
test test_rmdir::test_rmdir_not_a_directory ... ok
test test_rmdir::test_rmdir_ignore_nonempty_directory_with_parents ... ok
test test_rm::test_rm_interactive ... ok
test test_rmdir::test_rmdir_nonempty_directory_no_parents ... ok
test test_rmdir::test_rmdir_nonempty_directory_with_parents ... ok
test test_rmdir::test_verbose_multi ... ok
test test_rmdir::test_rmdir_remove_symlink_dir ... ok
test test_rmdir::test_verbose_single ... ok
test test_rmdir::test_verbose_nested_failure ... ok

test result: ok. 44 passed; 0 failed; 0 ignored; 0 measured; 2504 filtered out; finished in 0.06s

so could we consider removing the ignore flag on the test case and closing this PR?

[tertsdiepraam]

I presume that was due to this change? #5403 If that's the case let's add the test you made and then close this!

[cakebaker]

Hm, while the test passes when removing the ignore flag, doing the steps manually doesn't remove the folders here on my machine:

$ mkdir -p b/a/p
$ mkdir b/c b/d
$ chmod -w b/a
$ cargo run rm -rf b
rm: cannot remove 'b': Permission denied
$ ls b
a  c  d

[tommady]

ya interesting the CI also failed...
I need some time to dig it out...
change the PR into Draft.

[tommady]

Hi, I have a question at rm.rs:331

331: if options.interactive != InteractiveMode::Always && !options.verbose {
            if let Err(e) = fs::remove_dir_all(path) {
                // GNU compatibility (rm/empty-inacc.sh)
                // remove_dir_all failed. maybe it is because of the permissions
                // but if the directory is empty, remove_dir might work.
                // So, let's try that before failing for real
                if let Err(_e) = fs::remove_dir(path) {
                    had_err = true;
                    if e.kind() == std::io::ErrorKind::PermissionDenied {
                        // GNU compatibility (rm/fail-eacces.sh)
                        // here, GNU doesn't use some kind of remove_dir_all
                        // It will show directory+file
                        show_error!("cannot remove {}: {}", path.quote(), "Permission denied");
                    } else {
                        show_error!("cannot remove {}: {}", path.quote(), e);
                    }
                }
            }
349: } else {
       ... 
     }

why does the code need to be separated into two parts,
I mean at the rm.rs:349 in the else condition it already does a looping to do the deletion why do we need to do a detection at rm.rs:331 first and then go to use the std fs::remove_dir_all?

is it because of the performance?
thanks.

[sylvestre]

nope, the comment tries to explain it

because of the permissions
but if the directory is empty, remove_dir might work.

mostly because remove_dir_all is doing a better job than remove_dir on files with incorrect permissions

[tommady]

nope, the comment tries to explain it

because of the permissions
but if the directory is empty, remove_dir might work.

mostly because remove_dir_all is doing a better job than remove_dir on files with incorrect permissions

Sorry, my presentation was not clear enough.
I am confused about the LOGIC

the way and the thoughts when I see those codes,
they are doing the same things but separated by minor conditions.

so what I tried to ask is why separate them?

A = line 331~348 = using the standard lib fs to delete without a loop.
B = line 349~395 = using the standard lib fs to delete with a loop.

Those minor conditions separate A and B:

• InteractiveMode::Always &&
• !options.verbose

please guide me, thanks!