[PATCH 0/2] rm: Strip is_empty_dir call when possible

[Ben]

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 <ben@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 <ben@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