coreutils icon indicating copy to clipboard operation
coreutils copied to clipboard

rm: Added descend messages for interactive mode Fixes #3817

Open palaster opened this issue 3 years ago • 6 comments

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

palaster avatar Sep 13 '22 22:09 palaster

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

sylvestre avatar Sep 14 '22 08:09 sylvestre

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 avatar Sep 14 '22 23:09 palaster

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

palaster avatar Sep 15 '22 00:09 palaster

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.

tertsdiepraam avatar Sep 15 '22 05:09 tertsdiepraam

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 avatar Sep 15 '22 20:09 palaster

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

palaster avatar Sep 15 '22 21:09 palaster

There seems to be some trouble on macos and windows:

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

tertsdiepraam avatar Oct 01 '22 12:10 tertsdiepraam

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

palaster avatar Oct 01 '22 17:10 palaster

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

sylvestre avatar Oct 05 '22 11:10 sylvestre