Hi,
I was looking into optimizing rm, I saw that it always
checked is_empty_dir in prompt, which can be skipped.
The first patch, fixes a minimal issue, the test relied
on the readdir in prompt, which it shouldn't.
Instead of fixing tests/rm/rm3.sh and tests/rm/rm5.sh,
I have thought about adding x->interactive as a flag to
the skip_check test, but I don't think the interactive
flag should interfere with behavior in that way.
But as the current change *does* change behavior,
I leave it up to you to decide.
Thank you,
Ben Wijen (2):
rm: Fix readdir test
rm: Skip is_empty_dir in prompt
src/remove.c | 7 +++++--
tests/rm/rm-readdir-fail.sh | 4 ++--
tests/rm/{rm3.sh => rm3a.sh} | 2 +-
tests/rm/{rm3.sh => rm3b.sh} | 4 ++++
tests/rm/{rm5.sh => rm5a.sh} | 2 +-
tests/rm/{rm5.sh => rm5b.sh} | 2 ++
6 files changed, 15 insertions(+), 6 deletions(-)
copy tests/rm/{rm3.sh => rm3a.sh} (98%)
rename tests/rm/{rm3.sh => rm3b.sh} (95%)
copy tests/rm/{rm5.sh => rm5a.sh} (97%)
rename tests/rm/{rm5.sh => rm5b.sh} (97%)
--
2.29.2
>From f5712fc8047b064a1fafadcf41c5e38d80a776ee Mon Sep 17 00:00:00 2001
From: Ben Wijen <b...@wijen.net>
Date: Wed, 13 Jan 2021 10:35:54 +0100
Subject: [PATCH 1/2] rm: Fix readdir test
Currently the test depends on whether real_readdir is
called multiple times, which it shouldn't
---
tests/rm/rm-readdir-fail.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/rm/rm-readdir-fail.sh b/tests/rm/rm-readdir-fail.sh
index 1da63c078..c07244ea2 100755
--- a/tests/rm/rm-readdir-fail.sh
+++ b/tests/rm/rm-readdir-fail.sh
@@ -56,8 +56,8 @@ struct dirent *readdir (DIR *dirp)
errno = ESRCH;
return NULL;
}
- struct dirent* d;
- if (! (d = real_readdir (dirp)))
+ static struct dirent* d;
+ if (! d && ! (d = real_readdir (dirp)))
{
fprintf (stderr, "Failed to get dirent\n");
errno = ENOENT;
--
2.29.2
>From 8aa303abc0c2bd10275299f20ffa505ac3a50bf6 Mon Sep 17 00:00:00 2001
From: Ben Wijen <b...@wijen.net>
Date: Mon, 11 Jan 2021 09:48:48 +0100
Subject: [PATCH 2/2] rm: Skip is_empty_dir in prompt
When prompt is called in with either recursive or without
remove_emtpy_directories flags, it's not necessary to call
is_empty_dir, because - even is the directory is empty -
it will be removed during postorder directory.
---
src/remove.c | 7 +++++--
tests/rm/{rm3.sh => rm3a.sh} | 2 +-
tests/rm/{rm3.sh => rm3b.sh} | 4 ++++
tests/rm/{rm5.sh => rm5a.sh} | 2 +-
tests/rm/{rm5.sh => rm5b.sh} | 2 ++
5 files changed, 13 insertions(+), 4 deletions(-)
copy tests/rm/{rm3.sh => rm3a.sh} (98%)
rename tests/rm/{rm3.sh => rm3b.sh} (95%)
copy tests/rm/{rm5.sh => rm5a.sh} (97%)
rename tests/rm/{rm5.sh => rm5b.sh} (97%)
diff --git a/src/remove.c b/src/remove.c
index da380e0a7..2cebc2ac4 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -498,10 +498,13 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
{
Ternary is_empty_directory;
+ bool skip_check = (x->recursive
+ && ! x->remove_empty_directories);
enum RM_status s = prompt (fts, ent, true /*is_dir*/, x,
- PA_DESCEND_INTO_DIR, &is_empty_directory);
+ PA_DESCEND_INTO_DIR,
+ skip_check ? NULL : &is_empty_directory);
- if (s == RM_OK && is_empty_directory == T_YES)
+ if (s == RM_OK && !skip_check && is_empty_directory == T_YES)
{
/* When we know (from prompt when in interactive mode)
that this is an empty directory, don't prompt twice. */
diff --git a/tests/rm/rm3.sh b/tests/rm/rm3a.sh
similarity index 98%
copy from tests/rm/rm3.sh
copy to tests/rm/rm3a.sh
index 44b57e753..ae72ca371 100755
--- a/tests/rm/rm3.sh
+++ b/tests/rm/rm3a.sh
@@ -44,7 +44,7 @@ y
EOF
# Both of these should fail.
-rm -ir z < in > out 2>&1 || fail=1
+rm -ird z < in > out 2>&1 || fail=1
# Given input like 'rm: ...? rm: ...? ' (no trailing newline),
# the 'head...' part of the pipeline below removes the trailing space, so
diff --git a/tests/rm/rm3.sh b/tests/rm/rm3b.sh
similarity index 95%
rename from tests/rm/rm3.sh
rename to tests/rm/rm3b.sh
index 44b57e753..644085526 100755
--- a/tests/rm/rm3.sh
+++ b/tests/rm/rm3b.sh
@@ -41,6 +41,8 @@ y
y
y
y
+y
+y
EOF
# Both of these should fail.
@@ -61,7 +63,9 @@ rm: remove write-protected regular file 'z/fu'
rm: remove write-protected regular empty file 'z/empty-u'
rm: remove symbolic link 'z/slink'
rm: remove symbolic link 'z/slinkdot'
+rm: descend into directory 'z/d'
rm: remove directory 'z/d'
+rm: descend into write-protected directory 'z/du'
rm: remove write-protected directory 'z/du'
rm: remove directory 'z'
EOF
diff --git a/tests/rm/rm5.sh b/tests/rm/rm5a.sh
similarity index 97%
copy from tests/rm/rm5.sh
copy to tests/rm/rm5a.sh
index dc3dff767..626147fcf 100755
--- a/tests/rm/rm5.sh
+++ b/tests/rm/rm5a.sh
@@ -34,7 +34,7 @@ rm: remove directory 'd'
EOF
-rm -ir d < in > out 2>&1 || fail=1
+rm -ird d < in > out 2>&1 || fail=1
# Given input like 'rm: ...? rm: ...? ' (no trailing newline),
# the 'head...' part of the pipeline below removes the trailing space, so
diff --git a/tests/rm/rm5.sh b/tests/rm/rm5b.sh
similarity index 97%
rename from tests/rm/rm5.sh
rename to tests/rm/rm5b.sh
index dc3dff767..71933e8ff 100755
--- a/tests/rm/rm5.sh
+++ b/tests/rm/rm5b.sh
@@ -25,10 +25,12 @@ cat <<EOF > in || framework_failure_
y
y
y
+y
EOF
cat <<\EOF > exp || framework_failure_
rm: descend into directory 'd'
+rm: descend into directory 'd/e'
rm: remove directory 'd/e'
rm: remove directory 'd'
EOF
--
2.29.2
