rm: Added descend messages for interactive mode Fixes #3817 #3931

palaster · 2022-09-13T22:39:55Z

Added descend functionality for rm's interactive mode

Setup

mkdir -p a/b/
touch a/at.txt a/b/bt.txt
rm -ri a

Current

rm: remove file 'test/a/b/bt.txt'? y
rm: remove file 'test/a/at.txt'? y
rm: remove directory 'test/a/b'? y
rm: remove directory 'test/a'? y

GNU

rm: descend into directory 'a'? y
rm: descend into directory 'a/b'? y
rm: remove regular empty file 'a/b/bt.txt'? y
rm: remove directory 'a/b'? y
rm: remove regular empty file 'a/at.txt'? y
rm: remove directory 'a'? y

After Commit (Using the same answers as before)

rm: descend into directory 'test/a'? y
rm: descend into directory 'test/a/b'? y
rm: remove file 'test/a/b/bt.txt'? y
rm: remove file 'test/a/at.txt'? y
rm: remove directory 'test/a/b'? y
rm: remove directory 'test/a'? y

After Commit (Don't descend into a/b)

rm: descend into directory 'test/a'? y
rm: descend into directory 'test/a/b'? n
rm: remove file 'test/a/at.txt'? y
rm: remove directory 'test/a'? y
./uutils/target/debug/rm: cannot remove 'test/a': Directory not empty

tertsdiepraam

Cool! Looks like WalkDir is becoming a bit of a hassle to work around, but this looks like a good way to do this for now. Could you add a test to check that we don't regress?

sylvestre · 2022-09-14T08:27:15Z

it would be nice if you could make GNU's test rm3.sh & rm5.sh work as they are also testing the
"descend into directory" thing.

bash util/run-gnu-test.sh tests/rm/rm3.sh
and
bash util/run-gnu-test.sh tests/rm/rm5.sh

palaster · 2022-09-14T23:28:41Z

For making a regression test do you guys know/have an example test where you interact with the child process stdout and stdin?
Progress report for the test:
I looked at rm3.sh and will have to look deeper and see what it is doing
For rm5.sh I should be able to fix it. It is currently failing because GNU doesn't ask to descend if the directory is empty

palaster · 2022-09-15T00:16:33Z

Just looked at rm3.sh and it seems to only be falling because our prompts aren't as descriptive as gnu.
For example GNU for an empty file with prompt:
rm: remove regular empty file 'z/empty'
While ours is just:
rm: remove file 'z/empty'
I might make that into a separate PR to give our prompts better descriptions

tertsdiepraam · 2022-09-15T05:12:18Z

For making a regression test do you guys know/have an example test where you interact with the child process stdout and stdin?

Would the pipe_in method work? Here's an example:

https://github.com/uutils/coreutils/blob/main/tests/by-util/test_cp.rs#L199-L208

I might make that into a separate PR to give our prompts better descriptions.

Yeah a separate PR makes sense I think.

palaster · 2022-09-15T20:40:12Z

Unfortunately, that won't work for this use case where the command waits for responses and does something different depending on it. I am currently looking at just using a bare Child and then reading and writing to its stdin and stdout... but that is proving challenging.

palaster · 2022-09-15T21:59:18Z

Okay, I have created a very basic test for checking directory descending.

tertsdiepraam

I like it! We can merge this once the CI succeeds and my nits below are addressed.

tests/by-util/test_rm.rs

src/uu/rm/src/rm.rs

tertsdiepraam · 2022-10-01T12:24:07Z

There seems to be some trouble on macos and windows:

test test_rm::test_rm_descend_directory has been running for over 60 seconds

palaster · 2022-10-01T17:23:57Z

So, the reason it was hanging up is because of CRLF vs LF when I was inserting it into stdin. I also had to remake the test because of something I noticed with WalkDir that depending on the system the order in which it traverses can differ. Everything seems to be working now

sylvestre · 2022-10-03T11:34:38Z

tests/by-util/test_rm.rs

+
+    let mut child: Child = scene.ucmd().arg("-ri").arg("a").run_no_wait();
+
+    let mut child_stdin = child.stdin.take().unwrap();


please document why it is needed

sylvestre · 2022-10-03T11:35:05Z

src/uu/rm/src/rm.rs

+                        if options.interactive == InteractiveMode::Always {
+                            for not_descend in &not_descended {
+                                if entry.path().starts_with(not_descend) {
+                                    continue 'outer;


please add a comment explaining why we do that here

sylvestre · 2022-10-03T11:35:51Z

src/uu/rm/src/rm.rs

+                                } else {
+                                    not_descended.push(entry.path().to_path_buf());
+                                }
+                            } else {


please add a comment here. it is starting to be quite complex with the various if and else

sylvestre

misses some comments

…cend

Co-authored-by: Terts Diepraam <[email protected]>

…omplains

sylvestre · 2022-10-05T11:35:03Z

Nice
Congrats! The gnu test tests/rm/rm5 is no longer failing!

tertsdiepraam linked an issue Sep 14, 2022 that may be closed by this pull request

rm descend into directory at interactive mode missing #3817

Closed

tertsdiepraam reviewed Sep 14, 2022

View reviewed changes

palaster requested a review from tertsdiepraam September 29, 2022 01:43

tertsdiepraam reviewed Sep 29, 2022

View reviewed changes

tests/by-util/test_rm.rs Outdated Show resolved Hide resolved

src/uu/rm/src/rm.rs Show resolved Hide resolved

sylvestre reviewed Oct 3, 2022

View reviewed changes

sylvestre requested changes Oct 3, 2022

View reviewed changes

palaster requested a review from sylvestre October 4, 2022 20:10

palaster and others added 10 commits October 4, 2022 15:15

rm: Added descend messages for interactive mode Fixes #3817

a837202

Forgot to run cargo fmt

de3826a

Fix for gnu rm5 test by checking if dir is empty before asking to des…

698c5d0

…cend

Added rm: descend directory test

ae1943b

Forgot to run cargo fmt

70254f0

Changed scene.ccmd to scene.ucmd

e7bfd83

Co-authored-by: Terts Diepraam <[email protected]>

Use CRLF EOL on windows

9be74f9

Cargo fmt... I am never going to remeber to do this until the thing c…

57a0f7a

…omplains

Changed rm_descend_directory test

fcb974a

Added comments

d9252ac

sylvestre merged commit 493a262 into uutils:main Oct 5, 2022

palaster deleted the r-i-descend branch October 5, 2022 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rm: Added descend messages for interactive mode Fixes #3817 #3931

rm: Added descend messages for interactive mode Fixes #3817 #3931

palaster commented Sep 13, 2022

tertsdiepraam left a comment

sylvestre commented Sep 14, 2022

palaster commented Sep 14, 2022

palaster commented Sep 15, 2022

tertsdiepraam commented Sep 15, 2022 •

edited

Loading

palaster commented Sep 15, 2022 •

edited

Loading

palaster commented Sep 15, 2022

tertsdiepraam left a comment

tertsdiepraam commented Oct 1, 2022

palaster commented Oct 1, 2022 •

edited

Loading

sylvestre Oct 3, 2022

sylvestre Oct 3, 2022

sylvestre Oct 3, 2022

sylvestre left a comment

sylvestre commented Oct 5, 2022


		let mut child: Child = scene.ucmd().arg("-ri").arg("a").run_no_wait();

		let mut child_stdin = child.stdin.take().unwrap();

rm: Added descend messages for interactive mode Fixes #3817 #3931

rm: Added descend messages for interactive mode Fixes #3817 #3931

Conversation

palaster commented Sep 13, 2022

tertsdiepraam left a comment

Choose a reason for hiding this comment

sylvestre commented Sep 14, 2022

palaster commented Sep 14, 2022

palaster commented Sep 15, 2022

tertsdiepraam commented Sep 15, 2022 • edited Loading

palaster commented Sep 15, 2022 • edited Loading

palaster commented Sep 15, 2022

tertsdiepraam left a comment

Choose a reason for hiding this comment

tertsdiepraam commented Oct 1, 2022

palaster commented Oct 1, 2022 • edited Loading

sylvestre Oct 3, 2022

Choose a reason for hiding this comment

sylvestre Oct 3, 2022

Choose a reason for hiding this comment

sylvestre Oct 3, 2022

Choose a reason for hiding this comment

sylvestre left a comment

Choose a reason for hiding this comment

sylvestre commented Oct 5, 2022

tertsdiepraam commented Sep 15, 2022 •

edited

Loading

palaster commented Sep 15, 2022 •

edited

Loading

palaster commented Oct 1, 2022 •

edited

Loading