Re: [PATCH] rm: new option (-d) to remove empty directories

[Jim Meyering]

Krzysztof Goj wrote:
> This January I've submitted a patch adding a -d flag to rm which
> allowed it to remove empty directories, same as BSD rm does. I lacked
> a FSF assignment back then, which I have for some time now (RT:
> 721463; I can send the pdf). Here's the original thread in archive for
> those who want more context:
>
> http://lists.gnu.org/archive/html/coreutils/2012-01/msg00111.html
>
> I've rebased my patch against current master and fixed the tests as
> Jim Meyering have suggested. New patch is attached, it can also be
> viewed at GitHub:
>
> https://github.com/goj/coreutils/commit/rm-d

Thank you for the reminder.
This is a good time for it.

Please add a NEWS entry, a description of the new option
in coreutils.texi and adjust for the issues below
and then I'll push it.

> Subject: [PATCH] rm: new option (-d) to remove empty directories
>
> Adds new option to rm (-d/--dir), which allows removal of

s/Adds/Add/

> empty directories, while still safely disallowing removal
> of non-empty ones.

Mention that this provides compatibility with BSD's rm.

> * src/remove.c (rm_fts): allow removal of empty dir if the option is set
> * src/remove.h (rm_options): new option - remove_empty_directories
> * src/rm.c (long_opts, usage, main): usage && option parsing
> * tests/Makefile.am: added new test cases (d-1, d-2)

s/added/Add/

> * tests/rm/d-1: new test case - successfully delete empty dir
> * tests/rm/d-2: new test case - refuse to delete nonempty dir
...
> diff --git a/src/remove.c b/src/remove.c
> index 5ebd2ce..58a9b38 100644
> --- a/src/remove.c
> +++ b/src/remove.c
> @@ -414,11 +414,15 @@ rm_fts (FTS *fts, FTSENT *ent, struct rm_options const 
> *x)
>    switch (ent->fts_info)
>      {
>      case FTS_D:                      /* preorder directory */
> -      if (! x->recursive)
> +      if (! x->recursive
> +          && !(x->remove_empty_directories
> +               && is_empty_dir (fts->fts_cwd_fd, ent->fts_accpath)))
>          {
> -          /* This is the first (pre-order) encounter with a directory.
> +          /* This is the first (pre-order) encounter with a directory
> +           * which we can not delete.

Please retain the style of comment (no leading '*') and adjust wording:

                that we cannot delete.

>               Not recursive, so arrange to skip contents.  */
> -          error (0, EISDIR, _("cannot remove %s"), quote (ent->fts_path));
> +          int err = x->remove_empty_directories ? ENOTEMPTY : EISDIR;
> +          error (0, err, _("cannot remove %s"), quote (ent->fts_path));
>            mark_ancestor_dirs (ent);
>            fts_skip_tree (fts, ent);
>            return RM_ERROR;
> diff --git a/src/remove.h b/src/remove.h
> index 4eab282..cd3cb29 100644
> --- a/src/remove.h
> +++ b/src/remove.h
> @@ -49,6 +49,9 @@ struct rm_options
>    /* If true, recursively remove directories.  */
>    bool recursive;
>
> +  /* If true, remove empty directories. */

Two spaces after a period:
s/\./. /

> +  bool remove_empty_directories;

When adding a new member like this, one must also initialize it
at the other point of use: mv.c's rm_option_init.  Otherwise,
the new member would be used uninitialized when removing a
hierarchy upon a cross-device "mv".

>    /* Pointer to the device and inode numbers of '/', when --recursive
>       and preserving '/'.  Otherwise NULL.  */
>    struct dev_ino *root_dev_ino;
> diff --git a/src/rm.c b/src/rm.c
> index 02809f2..a45594e 100644
> --- a/src/rm.c
> +++ b/src/rm.c
> @@ -77,6 +77,7 @@ static struct option const long_opts[] =
>    {"-presume-input-tty", no_argument, NULL, PRESUME_INPUT_TTY_OPTION},
>
>    {"recursive", no_argument, NULL, 'r'},
> +  {"dir", no_argument, NULL, 'd'},
>    {"verbose", no_argument, NULL, 'v'},
>    {GETOPT_HELP_OPTION_DECL},
>    {GETOPT_VERSION_OPTION_DECL},
> @@ -154,6 +155,7 @@ Remove (unlink) the FILE(s).\n\
>        --no-preserve-root  do not treat '/' specially\n\
>        --preserve-root   do not remove '/' (default)\n\
>    -r, -R, --recursive   remove directories and their contents recursively\n\
> +  -d, --dir             remove empty directories\n\
>    -v, --verbose         explain what is being done\n\
>  "), stdout);
>        fputs (HELP_OPTION_DESCRIPTION, stdout);
> @@ -189,6 +191,7 @@ rm_option_init (struct rm_options *x)
>    x->ignore_missing_files = false;
>    x->interactive = RMI_SOMETIMES;
>    x->one_file_system = false;
> +  x->remove_empty_directories = false;
>    x->recursive = false;
>    x->root_dev_ino = NULL;
>    x->stdin_tty = isatty (STDIN_FILENO);
> @@ -220,10 +223,14 @@ main (int argc, char **argv)
>    /* Try to disable the ability to unlink a directory.  */
>    priv_set_remove_linkdir ();
>
> -  while ((c = getopt_long (argc, argv, "firvIR", long_opts, NULL)) != -1)
> +  while ((c = getopt_long (argc, argv, "dfirvIR", long_opts, NULL)) != -1)
>      {
>        switch (c)
>          {
> +        case 'd':
> +          x.remove_empty_directories = true;
> +          break;
> +
>          case 'f':
>            x.interactive = RMI_NEVER;
>            x.ignore_missing_files = true;
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index edc04b4..8caffda 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -125,6 +125,8 @@ TESTS =                                           \
>    rm/r-2                                     \
>    rm/r-3                                     \
>    rm/r-4                                     \
> +  rm/d-1                                     \
> +  rm/d-2                                     \

Please insert the new entries in sorted order.

>    rm/readdir-bug                             \
>    rm/rm1                                     \
>    touch/empty-file                           \
> diff --git a/tests/rm/d-1 b/tests/rm/d-1
...
> diff --git a/tests/rm/d-2 b/tests/rm/d-2
...
> +. "${srcdir=.}/init.sh"; path_prepend_ ../src
> +print_ver_ rm
> +
> +mkdir d || framework_failure_
> +> d/a

Detect failure:

   > d/a || framework_failure_

> +

Spurious blank line.

> +
> +rm -d d 2> out && fail=1
> +printf "%s\n" \
> +    "rm: cannot remove 'd': Directory not empty" \
> +    > exp || framework_failure_
> +
> +compare exp out || fail=1
> +
> +Exit $fail