Fix missing file names from rm errors

[michaeljohnalbers]

Description

Fixes a small bug with rm where names of files which couldn't be deleted due to error were not printed.

Fixes #9004

User-Facing Changes

Slightly different error message than previously. Nothing significant, though.

The new error message looks like this

~/Projects/rust/nushell> rm /proc/1/mem                                                                                                                                                            05/06/2023 01:13:23 PM
Error: nu::shell::remove_not_possible

  × Remove not possible
   ╭─[entry #3:1:1]
 1 │ rm /proc/1/mem
   ·    ─────┬─────
   ·         ╰── Could not delete /proc/1/mem: Operation not permitted (os error 1)
   ╰────

or when using a glob (only showing a single entry for brevity)

Error: nu::shell::remove_not_possible

  × Remove not possible
   ╭─[entry #2:1:1]
 1 │ rm --recursive --force --verbose /proc/1/*
   ·                                  ────┬────
   ·                                      ╰── Could not delete /proc/1/comm: Operation not permitted (os error 1)
   ╰────

Tests + Formatting

No new unit tests were added for this change as it is pretty difficult to test this particular case. However, manual testing was run with the following commands

rm /proc/1/mem
rm --recursive --force --verbose /proc/1/*

After Submitting

N/A

[amtoine]

i propose safer tests that do not require touching /

sudo mkdir -p /tmp/nushell/nushell#9120/foo
sudo touch /tmp/nushell/nushell#9120/foo/bar.txt

then

rm /tmp/nushell/nushell#9120/foo/bar.txt
rm --recursive --force --verbose /tmp/nushell/nushell#9120/foo/*

😌

[amtoine]

that looks good to me 💪

i could reproduce the same kind of errors with the /tmp/ tests above 😌

[codecov]

Codecov Report

Merging #9120 (79b6fd4) into main (39e51f1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9120   +/-   ##
=======================================
  Coverage   68.91%   68.91%           
=======================================
  Files         641      641           
  Lines      102337   102331    -6     
=======================================
- Hits        70522    70518    -4     
+ Misses      31815    31813    -2     

Impacted Files	Coverage Δ
crates/nu-protocol/src/shell_error.rs	30.76% <ø> (ø)
crates/nu-command/src/filesystem/rm.rs	79.87% <100.00%> (+2.59%)	⬆️

... and 2 files with indirect coverage changes

[juanPabloMiceli]

Can't we test this using something like this?

[amtoine]

Can't we test this using something like this?

oooh maybe!

@michaeljohnalbers
could you give this a try, that'd be great ✨

just to have your simple test in the source base 😌

[michaeljohnalbers]

@amtoine Added a unit test for this.

[amtoine]

@michaeljohnalbers
that looks promising 😇

you might have to run cargo fmt --all or

use toolkit.nu
toolkit fmt

to fix the CI 😌

also you can directly run

use toolkit.nu
toolkit check pr

to make sure everything is ok at once 😏
(be careful the tests require some RAM 👀)

[michaeljohnalbers]

@amtoine Sorry about that. I thought I had my git hooks set up, but it seems I didn't. I ran the formatter as well as toolkit check pr everything appeared to pass.

[amtoine]

amazing, that looks better 👌

let's run the CI and see if it passes 😇

[amtoine]

@michaeljohnalbers
so why do we have to skip the tests on windows? 😮

[michaeljohnalbers]

@amtoine We don't have to skip them, it just doesn't gain anything to run them on Windows. The code being tested is OS independent. However, triggering that specific error does start getting into OS specific territory. As I'm not a Windows dev and have nothing set up to develop and test the unit test on Windows it was easier to just exclude it from running on Windows.

[amtoine]

could we still try them in the CI, to see if it works on all OSes?
or do you mean it can not run on windows at all? 😮

i do not have windows available too, so i dunnot either 👀

[michaeljohnalbers]

One of the earlier CI runs did attempt to run the new test on Windows and it did fail. The assertion verifying the files were not deleted failed. This was despite making the containing folder/directory read-only.

Test which failed: https://github.com/nushell/nushell/actions/runs/4931264176/jobs/8830252909?pr=9120

[amtoine]

@michaeljohnalbers
oh yeah my bad 👍

i'm not a windows chad so i'd rather leave the final word to a more familiar member of the core-team 😌

[amtoine]

as this is quite a minor change, i.e. it only changes the error being sent to the user, no other behaviour change, i think we can land this and see how it goes 😌

thanks @michaeljohnalbers for the fix ✨