Skip to content

Fix missing file names from rm errors#9120

Merged
amtoine merged 7 commits intonushell:mainfrom
michaeljohnalbers:9004-rm-filename-missing
Jun 18, 2023
Merged

Fix missing file names from rm errors#9120
amtoine merged 7 commits intonushell:mainfrom
michaeljohnalbers:9004-rm-filename-missing

Conversation

@michaeljohnalbers
Copy link
Copy Markdown
Contributor

@michaeljohnalbers michaeljohnalbers commented May 6, 2023

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

@michaeljohnalbers michaeljohnalbers mentioned this pull request May 6, 2023
Closed
@amtoine
Copy link
Copy Markdown
Member

amtoine commented May 7, 2023

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
amtoine approved these changes May 7, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that looks good to me 💪

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

@codecov
Copy link
Copy Markdown

codecov bot commented May 7, 2023 •
edited
Loading

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

Impacted file tree graph

@@           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
Copy link
Copy Markdown
Contributor

juanPabloMiceli commented May 7, 2023

Can't we test this using something like this?

@amtoine

This comment was marked as outdated.

Sign in to view
amtoine
amtoine requested changes May 8, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Copy Markdown
Contributor Author

michaeljohnalbers commented May 9, 2023

@amtoine Added a unit test for this.

@amtoine
Copy link
Copy Markdown
Member

amtoine commented May 9, 2023

@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
Copy link
Copy Markdown
Contributor Author

michaeljohnalbers commented May 9, 2023

@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
amtoine reviewed May 10, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazing, that looks better 👌

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

@michaeljohnalbers michaeljohnalbers requested a review from amtoine May 11, 2023 01:04
@amtoine
Copy link
Copy Markdown
Member

amtoine commented May 12, 2023

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

@michaeljohnalbers
Copy link
Copy Markdown
Contributor Author

michaeljohnalbers commented May 15, 2023

@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
Copy link
Copy Markdown
Member

amtoine commented May 15, 2023

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
Copy link
Copy Markdown
Contributor Author

michaeljohnalbers commented May 15, 2023 •
edited
Loading

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
Copy link
Copy Markdown
Member

amtoine commented May 16, 2023

@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
amtoine approved these changes Jun 18, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@amtoine amtoine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ✨

@amtoine amtoine merged commit c12b211 into nushell:main Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amtoine amtoine amtoine approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Filename is missing from rm errors

3 participants

@michaeljohnalbers @amtoine @juanPabloMiceli