GNU bug report logs - #12260
rm -d in coreutils 8.19

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 12260 in the body.
You can then email your comments to 12260 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Michael Price <mprice <at> atl.lmco.com>
To: bug-coreutils <at> gnu.org
Subject: rm -d in coreutils 8.19
Date: Wed, 22 Aug 2012 11:57:24 -0400

This seems wrong:

$ mkdir bar
$ rm -d bar
$ mkdir bar
$ rm -i -d bar
rm: cannot remove 'bar': Is a directory

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Robert Day <robertkday <at> gmail.com>
To: bug-coreutils <at> gnu.org
Subject: bug#12260: rm -d in coreutils 8.19
Date: Wed, 22 Aug 2012 23:23:49 +0100

[Message part 1 (text/plain, inline)]

It looks as though the bug is that the prompt() function does its own
checking, returning EISDIR unless x->recursive is set, and this code wasn't
updated with the addition of -d. I've attached a patch which fixes this bug
and adds a test of that code path. The fixes can also be retrieved from
https://github.com/rkd91/coreutils_rm_di_patch.

I haven't submitted patches for coreutils before; if there's somewhere else
I should post this patch to, I'm happy to be pointed there.

Best,
Rob

[Message part 2 (text/html, inline)]

[rm.patch (application/octet-stream, attachment)]

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Robert Day <robertkday <at> gmail.com>
To: bug-coreutils <at> gnu.org
Subject: bug#12260: [patch] rm -d in coreutils 8.19
Date: Thu, 23 Aug 2012 12:17:11 +0100

[Message part 1 (text/plain, inline)]

On 22 August 2012 23:23, Robert Day <robertkday <at> gmail.com> wrote:

> I've attached a patch which fixes this bug and adds a test of that code
> path. The fixes can also be retrieved from
> https://github.com/rkd91/coreutils_rm_di_patch.
>

I've made a couple of related fixes (comments/code niceness and adding a
NEWS item), so I've attached a new patch and updated my github.

Best,
Rob


-- 
Robert K. Day
robert.day <at> merton.oxon.org

>
>

[Message part 2 (text/html, inline)]

[rm.2.patch (application/octet-stream, attachment)]

Message #14 received at 12260 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Robert Day <robertkday <at> gmail.com>
Cc: 12260 <at> debbugs.gnu.org
Subject: Re: bug#12260: [patch] rm -d in coreutils 8.19
Date: Thu, 23 Aug 2012 12:44:13 +0100

On 08/23/2012 12:17 PM, Robert Day wrote:
> On 22 August 2012 23:23, Robert Day <robertkday <at> gmail.com> wrote:
> 
>> I've attached a patch which fixes this bug and adds a test of that code
>> path. The fixes can also be retrieved from
>> https://github.com/rkd91/coreutils_rm_di_patch.
>>
> 
> I've made a couple of related fixes (comments/code niceness and adding a
> NEWS item), so I've attached a new patch and updated my github.

Thanks for handling that Robert.
It's on the borderline for copyright assignment
(which I don't think you have?).
It's probably OK to waive in this case anyway.

I've not got time to fully squash/review your
patches just now, but we'll add them in soon.

cheers,
Pádraig.

Message #17 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Rob Day <robert.day <at> merton.oxon.org>
To: bug-coreutils <at> gnu.org
Subject: Re: bug#12260: rm -d in coreutils 8.19
Date: Thu, 23 Aug 2012 08:29:17 +0100

[Message part 1 (text/plain, inline)]

On 22 August 2012 23:23, Robert Day <robertkday <at> gmail.com> wrote:

> I've attached a patch which fixes this bug and adds a test of that code
> path. The fixes can also be retrieved from
> https://github.com/rkd91/coreutils_rm_di_patch.
>

I've made a couple of related fixes (comments/code niceness and adding a
NEWS item), so I've attached a new patch and updated my github.

Best,
Rob


-- 
Robert K. Day
robert.day <at> merton.oxon.org

[Message part 2 (text/html, inline)]

[rm.2.patch (application/octet-stream, attachment)]

Message #20 received at 12260 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 12260 <at> debbugs.gnu.org, Robert Day <robertkday <at> gmail.com>
Subject: Re: bug#12260: [patch] rm -d in coreutils 8.19
Date: Thu, 23 Aug 2012 19:28:31 +0200

Pádraig Brady wrote:
> On 08/23/2012 12:17 PM, Robert Day wrote:
>> On 22 August 2012 23:23, Robert Day <robertkday <at> gmail.com> wrote:
>>
>>> I've attached a patch which fixes this bug and adds a test of that code
>>> path. The fixes can also be retrieved from
>>> https://github.com/rkd91/coreutils_rm_di_patch.
>>>
>>
>> I've made a couple of related fixes (comments/code niceness and adding a
>> NEWS item), so I've attached a new patch and updated my github.
>
> Thanks for handling that Robert.
> It's on the borderline for copyright assignment
> (which I don't think you have?).
> It's probably OK to waive in this case anyway.

I agree that it's borderline, but also that it's ok.

> I've not got time to fully squash/review your
> patches just now, but we'll add them in soon.

I'm going through it now, constructing a proper commit log, attributing
the reporter, fixing NEWS to avoid the minor syntax-check failure,
adjusting comment formatting, e.g.,

diff --git a/src/remove.c b/src/remove.c
index 06bc926..2dbb441 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -199,7 +199,6 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
   else
     is_empty = false;

-
   /* When nonzero, this indicates that we failed to remove a child entry,
      either because the user declined an interactive prompt, or due to
      some other failure, like permissions.  */
@@ -249,8 +248,8 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,

           case DT_DIR:
              /* Unless we're either deleting directories or deleting
-              * recursively, we want to raise an EISDIR error rather than
-              * prompting the user  */
+                recursively, we want to raise an EISDIR error rather than
+                prompting the user  */
             if (!x->recursive
                 && !(x->remove_empty_directories && is_empty))
               {

I'll post for final review shortly.

Message #23 received at 12260 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 12260 <at> debbugs.gnu.org, Robert Day <robertkday <at> gmail.com>
Subject: Re: bug#12260: [patch] rm -d in coreutils 8.19
Date: Thu, 23 Aug 2012 19:38:21 +0200

Jim Meyering wrote:

> Pádraig Brady wrote:
>> On 08/23/2012 12:17 PM, Robert Day wrote:
>>> On 22 August 2012 23:23, Robert Day <robertkday <at> gmail.com> wrote:
>>>
>>>> I've attached a patch which fixes this bug and adds a test of that code
>>>> path. The fixes can also be retrieved from
>>>> https://github.com/rkd91/coreutils_rm_di_patch.
>>>>
>>>
>>> I've made a couple of related fixes (comments/code niceness and adding a
>>> NEWS item), so I've attached a new patch and updated my github.
>>
>> Thanks for handling that Robert.
>> It's on the borderline for copyright assignment
>> (which I don't think you have?).
>> It's probably OK to waive in this case anyway.
>
> I agree that it's borderline, but also that it's ok.
>
>> I've not got time to fully squash/review your
>> patches just now, but we'll add them in soon.
>
> I'm going through it now, constructing a proper commit log, attributing
> the reporter, fixing NEWS to avoid the minor syntax-check failure,
> adjusting comment formatting, e.g.,
>
> diff --git a/src/remove.c b/src/remove.c
> index 06bc926..2dbb441 100644
> --- a/src/remove.c
> +++ b/src/remove.c
> @@ -199,7 +199,6 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
>    else
>      is_empty = false;
>
> -
>    /* When nonzero, this indicates that we failed to remove a child entry,
>       either because the user declined an interactive prompt, or due to
>       some other failure, like permissions.  */
> @@ -249,8 +248,8 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
>
>            case DT_DIR:
>               /* Unless we're either deleting directories or deleting
> -              * recursively, we want to raise an EISDIR error rather than
> -              * prompting the user  */
> +                recursively, we want to raise an EISDIR error rather than
> +                prompting the user  */
>              if (!x->recursive
>                  && !(x->remove_empty_directories && is_empty))
>                {
>
> I'll post for final review shortly.

Here's another few code changes:

The first hunk is because I don't like dangling single-line brace-less
"else" bodies when the then block has braces, and beside that first hunk
saves one line.
The second factors out an "!", and makes it a little easier to read.
The third removes a stray doubled space.

diff --git a/src/remove.c b/src/remove.c
index 2dbb441..69faae6 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -190,14 +190,12 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
   int dirent_type = is_dir ? DT_DIR : DT_UNKNOWN;
   int write_protected = 0;

-  bool is_empty;
+  bool is_empty = false;
   if (is_empty_p)
     {
       is_empty = is_empty_dir (fd_cwd, filename);
       *is_empty_p = is_empty ? T_YES : T_NO;
     }
-  else
-    is_empty = false;

   /* When nonzero, this indicates that we failed to remove a child entry,
      either because the user declined an interactive prompt, or due to
@@ -250,8 +248,7 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
              /* Unless we're either deleting directories or deleting
                 recursively, we want to raise an EISDIR error rather than
                 prompting the user  */
-            if (!x->recursive
-                && !(x->remove_empty_directories && is_empty))
+            if ( ! (x->recursive || (x->remove_empty_directories && is_empty)))
               {
                 write_protected = -1;
                 wp_errno = EISDIR;
@@ -425,7 +422,7 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
           /* This is the first (pre-order) encounter with a directory
              that we cannot delete.
              Not recursive, and it's not an empty directory (if we're removing
-             them)  so arrange to skip contents.  */
+             them) so arrange to skip contents.  */
           int err = x->remove_empty_directories ? ENOTEMPTY : EISDIR;
           error (0, err, _("cannot remove %s"), quote (ent->fts_path));
           mark_ancestor_dirs (ent);

Message #26 received at 12260 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigBrady.com>
Cc: 12260 <at> debbugs.gnu.org, Robert Day <robertkday <at> gmail.com>
Subject: Re: bug#12260: [patch] rm -d in coreutils 8.19
Date: Thu, 23 Aug 2012 20:53:22 +0200

Jim Meyering wrote:
> Jim Meyering wrote:
>> Pádraig Brady wrote:
>>> On 08/23/2012 12:17 PM, Robert Day wrote:
>>>> On 22 August 2012 23:23, Robert Day <robertkday <at> gmail.com> wrote:
>>>>
>>>>> I've attached a patch which fixes this bug and adds a test of that code
>>>>> path. The fixes can also be retrieved from
>>>>> https://github.com/rkd91/coreutils_rm_di_patch.
>>>>>
>>>>
>>>> I've made a couple of related fixes (comments/code niceness and adding a
>>>> NEWS item), so I've attached a new patch and updated my github.
>>>
>>> Thanks for handling that Robert.
>>> It's on the borderline for copyright assignment
>>> (which I don't think you have?).
>>> It's probably OK to waive in this case anyway.
>>
>> I agree that it's borderline, but also that it's ok.
>>
>>> I've not got time to fully squash/review your
>>> patches just now, but we'll add them in soon.
>>
>> I'm going through it now, constructing a proper commit log, attributing
>> the reporter, fixing NEWS to avoid the minor syntax-check failure,
>> adjusting comment formatting, e.g.,

Thanks again for the patch, Robert.
I'll wait for your ACK before pushing this.

From dd22da8e9539cc88193987b6997769ae4ede2b15 Mon Sep 17 00:00:00 2001
From: Rob Day <robertkday <at> gmail.com>
Date: Wed, 22 Aug 2012 23:04:19 +0100
Subject: [PATCH] rm: fix the new --dir (-d) option to work with -i

* src/remove.c (prompt): Hoist the computation of is_empty, since we'll
need it slightly earlier.
Before, this function would arrange to fail with EISDIR when processing
a directory without --recursive (-r).  Adjust the condition to exempt
an empty directory when --dir has been specified.
Improve comments.
* tests/rm/d-3: New file, to ensure that rm -d -i dir works.
* tests/Makefile.am (TESTS): Add it.
* NEWS (Bug fixes): Mention it.
* THANKS.in: Update.
Reported by Michael Price in http://bugs.gnu.org/12260
---
 NEWS              |  4 ++++
 THANKS.in         |  1 +
 src/remove.c      | 24 +++++++++++++-----------
 tests/Makefile.am |  1 +
 tests/rm/d-3      | 37 +++++++++++++++++++++++++++++++++++++
 5 files changed, 56 insertions(+), 11 deletions(-)
 create mode 100755 tests/rm/d-3

diff --git a/NEWS b/NEWS
index d8a47ab..e6d79bf 100644
--- a/NEWS
+++ b/NEWS
@@ -9,6 +9,10 @@ GNU coreutils NEWS                                    -*- outline -*-
   it detects this precise type of cycle, diagnoses it as such and
   eventually exits nonzero.

+  rm -i -d now prompts the user then removes an empty directory, rather
+  than ignoring the -d option and failing with an 'Is a directory' error.
+  [bug introduced in coreutils-8.19, with the addition of --dir (-d)]
+

 * Noteworthy changes in release 8.19 (2012-08-20) [stable]

diff --git a/THANKS.in b/THANKS.in
index ca22e15..f288174 100644
--- a/THANKS.in
+++ b/THANKS.in
@@ -425,6 +425,7 @@ Michael McFarland                   sidlon <at> yahoo.com
 Michael McLagan                     mmclagan <at> invlogic.com
 Michael Mol                         mikemol <at> gmail.com
 Michael Piefel                      piefel <at> informatik.hu-berlin.de
+Michael Price                       mprice <at> atl.lmco.com
 Michael Steffens                    michael.steffens <at> s.netic.de
 Michael Stummvoll                   michael <at> stummi.org
 Michael Stutz                       stutz <at> dsl.org
diff --git a/src/remove.c b/src/remove.c
index c4972ac..69faae6 100644
--- a/src/remove.c
+++ b/src/remove.c
@@ -190,6 +190,13 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
   int dirent_type = is_dir ? DT_DIR : DT_UNKNOWN;
   int write_protected = 0;

+  bool is_empty = false;
+  if (is_empty_p)
+    {
+      is_empty = is_empty_dir (fd_cwd, filename);
+      *is_empty_p = is_empty ? T_YES : T_NO;
+    }
+
   /* When nonzero, this indicates that we failed to remove a child entry,
      either because the user declined an interactive prompt, or due to
      some other failure, like permissions.  */
@@ -238,7 +245,10 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
             break;

           case DT_DIR:
-            if (!x->recursive)
+             /* Unless we're either deleting directories or deleting
+                recursively, we want to raise an EISDIR error rather than
+                prompting the user  */
+            if ( ! (x->recursive || (x->remove_empty_directories && is_empty)))
               {
                 write_protected = -1;
                 wp_errno = EISDIR;
@@ -254,15 +264,6 @@ prompt (FTS const *fts, FTSENT const *ent, bool is_dir,
           return RM_ERROR;
         }

-      bool is_empty;
-      if (is_empty_p)
-        {
-          is_empty = is_empty_dir (fd_cwd, filename);
-          *is_empty_p = is_empty ? T_YES : T_NO;
-        }
-      else
-        is_empty = false;
-
       /* Issue the prompt.  */
       if (dirent_type == DT_DIR
           && mode == PA_DESCEND_INTO_DIR
@@ -420,7 +421,8 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const *x)
         {
           /* This is the first (pre-order) encounter with a directory
              that we cannot delete.
-             Not recursive, so arrange to skip contents.  */
+             Not recursive, and it's not an empty directory (if we're removing
+             them) so arrange to skip contents.  */
           int err = x->remove_empty_directories ? ENOTEMPTY : EISDIR;
           error (0, err, _("cannot remove %s"), quote (ent->fts_path));
           mark_ancestor_dirs (ent);
diff --git a/tests/Makefile.am b/tests/Makefile.am
index acd816d..87d6cad 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -101,6 +101,7 @@ TESTS =						\
   misc/ls-time					\
   rm/d-1					\
   rm/d-2					\
+  rm/d-3					\
   rm/deep-1					\
   rm/deep-2					\
   rm/dir-no-w					\
diff --git a/tests/rm/d-3 b/tests/rm/d-3
new file mode 100755
index 0000000..2f2cf74
--- /dev/null
+++ b/tests/rm/d-3
@@ -0,0 +1,37 @@
+#!/bin/sh
+# Ensure that 'rm -d -i dir' (i.e., without --recursive) gives a prompt and
+# then deletes the directory if it is empty
+
+# Copyright (C) 2012 Free Software Foundation, Inc.
+
+# This program is free software: you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+. "${srcdir=.}/init.sh"; path_prepend_ ../src
+print_ver_ rm
+
+mkdir d || framework_failure_
+
+echo "y" | rm -i -d --verbose d > out 2> out.err || fail=1
+printf "%s" \
+    "rm: remove directory 'd'? " \
+    > exp.err || framework_failure_
+
+printf "%s\n" \
+    "removed directory: 'd'" \
+    > exp || framework_failure_
+
+compare exp out || fail=1
+compare exp.err out.err || fail=1
+
+Exit $fail
--
1.7.12.70.g851f7e6

Message #29 received at 12260 <at> debbugs.gnu.org (full text, mbox):

From: Robert Day <robertkday <at> gmail.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 12260 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#12260: [patch] rm -d in coreutils 8.19
Date: Thu, 23 Aug 2012 20:06:27 +0100

[Message part 1 (text/plain, inline)]

Looks good to me; feel free to push it. I'll be mindful of your comments on
else-statement style, comment formatting etc. if and when I submit another
coreutils patch.

Cheers,
Rob

On 23 August 2012 19:53, Jim Meyering <jim <at> meyering.net> wrote:

> Jim Meyering wrote:
> > Jim Meyering wrote:
> >> Pádraig Brady wrote:
> >>> On 08/23/2012 12:17 PM, Robert Day wrote:
> >>>> On 22 August 2012 23:23, Robert Day <robertkday <at> gmail.com> wrote:
> >>>>
> >>>>> I've attached a patch which fixes this bug and adds a test of that
> code
> >>>>> path. The fixes can also be retrieved from
> >>>>> https://github.com/rkd91/coreutils_rm_di_patch.
> >>>>>
> >>>>
> >>>> I've made a couple of related fixes (comments/code niceness and
> adding a
> >>>> NEWS item), so I've attached a new patch and updated my github.
> >>>
> >>> Thanks for handling that Robert.
> >>> It's on the borderline for copyright assignment
> >>> (which I don't think you have?).
> >>> It's probably OK to waive in this case anyway.
> >>
> >> I agree that it's borderline, but also that it's ok.
> >>
> >>> I've not got time to fully squash/review your
> >>> patches just now, but we'll add them in soon.
> >>
> >> I'm going through it now, constructing a proper commit log, attributing
> >> the reporter, fixing NEWS to avoid the minor syntax-check failure,
> >> adjusting comment formatting, e.g.,
>
> Thanks again for the patch, Robert.
> I'll wait for your ACK before pushing this.
>
> From dd22da8e9539cc88193987b6997769ae4ede2b15 Mon Sep 17 00:00:00 2001
> From: Rob Day <robertkday <at> gmail.com>
> Date: Wed, 22 Aug 2012 23:04:19 +0100
> Subject: [PATCH] rm: fix the new --dir (-d) option to work with -i
>
> * src/remove.c (prompt): Hoist the computation of is_empty, since we'll
> need it slightly earlier.
> Before, this function would arrange to fail with EISDIR when processing
> a directory without --recursive (-r).  Adjust the condition to exempt
> an empty directory when --dir has been specified.
> Improve comments.
> * tests/rm/d-3: New file, to ensure that rm -d -i dir works.
> * tests/Makefile.am (TESTS): Add it.
> * NEWS (Bug fixes): Mention it.
> * THANKS.in: Update.
> Reported by Michael Price in http://bugs.gnu.org/12260
> ---
>  NEWS              |  4 ++++
>  THANKS.in         |  1 +
>  src/remove.c      | 24 +++++++++++++-----------
>  tests/Makefile.am |  1 +
>  tests/rm/d-3      | 37 +++++++++++++++++++++++++++++++++++++
>  5 files changed, 56 insertions(+), 11 deletions(-)
>  create mode 100755 tests/rm/d-3
>
> diff --git a/NEWS b/NEWS
> index d8a47ab..e6d79bf 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -9,6 +9,10 @@ GNU coreutils NEWS                                    -*-
> outline -*-
>    it detects this precise type of cycle, diagnoses it as such and
>    eventually exits nonzero.
>
> +  rm -i -d now prompts the user then removes an empty directory, rather
> +  than ignoring the -d option and failing with an 'Is a directory' error.
> +  [bug introduced in coreutils-8.19, with the addition of --dir (-d)]
> +
>
>  * Noteworthy changes in release 8.19 (2012-08-20) [stable]
>
> diff --git a/THANKS.in b/THANKS.in
> index ca22e15..f288174 100644
> --- a/THANKS.in
> +++ b/THANKS.in
> @@ -425,6 +425,7 @@ Michael McFarland                   sidlon <at> yahoo.com
>  Michael McLagan                     mmclagan <at> invlogic.com
>  Michael Mol                         mikemol <at> gmail.com
>  Michael Piefel                      piefel <at> informatik.hu-berlin.de
> +Michael Price                       mprice <at> atl.lmco.com
>  Michael Steffens                    michael.steffens <at> s.netic.de
>  Michael Stummvoll                   michael <at> stummi.org
>  Michael Stutz                       stutz <at> dsl.org
> diff --git a/src/remove.c b/src/remove.c
> index c4972ac..69faae6 100644
> --- a/src/remove.c
> +++ b/src/remove.c
> @@ -190,6 +190,13 @@ prompt (FTS const *fts, FTSENT const *ent, bool
> is_dir,
>    int dirent_type = is_dir ? DT_DIR : DT_UNKNOWN;
>    int write_protected = 0;
>
> +  bool is_empty = false;
> +  if (is_empty_p)
> +    {
> +      is_empty = is_empty_dir (fd_cwd, filename);
> +      *is_empty_p = is_empty ? T_YES : T_NO;
> +    }
> +
>    /* When nonzero, this indicates that we failed to remove a child entry,
>       either because the user declined an interactive prompt, or due to
>       some other failure, like permissions.  */
> @@ -238,7 +245,10 @@ prompt (FTS const *fts, FTSENT const *ent, bool
> is_dir,
>              break;
>
>            case DT_DIR:
> -            if (!x->recursive)
> +             /* Unless we're either deleting directories or deleting
> +                recursively, we want to raise an EISDIR error rather than
> +                prompting the user  */
> +            if ( ! (x->recursive || (x->remove_empty_directories &&
> is_empty)))
>                {
>                  write_protected = -1;
>                  wp_errno = EISDIR;
> @@ -254,15 +264,6 @@ prompt (FTS const *fts, FTSENT const *ent, bool
> is_dir,
>            return RM_ERROR;
>          }
>
> -      bool is_empty;
> -      if (is_empty_p)
> -        {
> -          is_empty = is_empty_dir (fd_cwd, filename);
> -          *is_empty_p = is_empty ? T_YES : T_NO;
> -        }
> -      else
> -        is_empty = false;
> -
>        /* Issue the prompt.  */
>        if (dirent_type == DT_DIR
>            && mode == PA_DESCEND_INTO_DIR
> @@ -420,7 +421,8 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const
> *x)
>          {
>            /* This is the first (pre-order) encounter with a directory
>               that we cannot delete.
> -             Not recursive, so arrange to skip contents.  */
> +             Not recursive, and it's not an empty directory (if we're
> removing
> +             them) so arrange to skip contents.  */
>            int err = x->remove_empty_directories ? ENOTEMPTY : EISDIR;
>            error (0, err, _("cannot remove %s"), quote (ent->fts_path));
>            mark_ancestor_dirs (ent);
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index acd816d..87d6cad 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -101,6 +101,7 @@ TESTS =                                             \
>    misc/ls-time                                 \
>    rm/d-1                                       \
>    rm/d-2                                       \
> +  rm/d-3                                       \
>    rm/deep-1                                    \
>    rm/deep-2                                    \
>    rm/dir-no-w                                  \
> diff --git a/tests/rm/d-3 b/tests/rm/d-3
> new file mode 100755
> index 0000000..2f2cf74
> --- /dev/null
> +++ b/tests/rm/d-3
> @@ -0,0 +1,37 @@
> +#!/bin/sh
> +# Ensure that 'rm -d -i dir' (i.e., without --recursive) gives a prompt
> and
> +# then deletes the directory if it is empty
> +
> +# Copyright (C) 2012 Free Software Foundation, Inc.
> +
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +
> +. "${srcdir=.}/init.sh"; path_prepend_ ../src
> +print_ver_ rm
> +
> +mkdir d || framework_failure_
> +
> +echo "y" | rm -i -d --verbose d > out 2> out.err || fail=1
> +printf "%s" \
> +    "rm: remove directory 'd'? " \
> +    > exp.err || framework_failure_
> +
> +printf "%s\n" \
> +    "removed directory: 'd'" \
> +    > exp || framework_failure_
> +
> +compare exp out || fail=1
> +compare exp.err out.err || fail=1
> +
> +Exit $fail
> --
> 1.7.12.70.g851f7e6
>

[Message part 2 (text/html, inline)]

Message #32 received at 12260 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Robert Day <robertkday <at> gmail.com>
Cc: 12260 <at> debbugs.gnu.org, Pádraig Brady <P <at> draigbrady.com>
Subject: Re: bug#12260: [patch] rm -d in coreutils 8.19
Date: Thu, 23 Aug 2012 21:42:35 +0200

Robert Day wrote:
> Looks good to me; feel free to push it. I'll be mindful of your comments on
> else-statement style, comment formatting etc. if and when I submit another
> coreutils patch.

Pushed.
Looking forward to more from you.


BTW, I noticed that your message was more than double the length
of the one to which you replied.  Oh... text *and* HTML.
Please tell your mail client to send only text (no HTML) to this list.
Some readers tend to ignore HTML-formatted messages.

Also, it's good netiquette to trim quoted text that is not relevant.
In this case, you could have elided almost everything that was quoted.

Message #37 received at 12260-done <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: 12260-done <at> debbugs.gnu.org
Subject: Re: bug#12260: [patch] rm -d in coreutils 8.19
Date: Sat, 15 Sep 2012 12:21:09 +0200

Jim Meyering wrote:

> Robert Day wrote:
>> Looks good to me; feel free to push it. I'll be mindful of your comments on
>> else-statement style, comment formatting etc. if and when I submit another
>> coreutils patch.
>
> Pushed.

and now (belatedly) marking this issue as "done"

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.