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

Previous Next

Full log

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);

Previous Next

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