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

[Jim Meyering]

Krzysztof Goj wrote:
> I created a patch adding new option to rm (-d/--dir) which allows to
> remove empty directories.
> You may have seen this issue being discussed in comments on Rob Pike's Google+
> (https://plus.google.com/101960720994009339267/posts/3WDmR2RTTrv).
>
> Removing empty directories by rm was default behaviour in the Research
> Unix and Plan 9.
> I think it's safer to add a flag and let people opt in with alias rm='rm 
> --dir'.
>
> It was raised in the Google+ discussion that -d flag has been added to
> BSD code base in 1990.
> I checked the man pages of FreeBSD, NetBSD, OpenBSD and Mac OS X and
> the -d option is there.
>
> I attach output of `git format-patch --stdout -1`, you can also
> get the changes from my github (https://github.com/goj/coreutils,
> rm-d branch).
...

I like the idea.

Thank you for contributing.
This change is large enough that we'll require paperwork.
Can you assign copyright?

    http://git.sv.gnu.org/cgit/coreutils.git/tree/HACKING#n444

To make this a complete patch, you may want to update the documentation
in coreutils.texi and to mention the new feature in the NEWS file.

...
> diff --git a/tests/rm/d-1 b/tests/rm/d-1
> new file mode 100755
> index 0000000..69c5860
> --- /dev/null
> +++ b/tests/rm/d-1
> @@ -0,0 +1,42 @@
> +#!/bin/sh
> +# Test "rm -r --verbose".
> +
> +# Copyright (C) 1997-1998, 2000, 2002, 2004, 2006-2012 Free Software
> +# Foundation, Inc.

For new tests, please use only the current year, even
when copying from a nearby file, or to be safe, just use
the tests/sample-test script, which does that.

  # Copyright (C) 2012 Free Software Foundation, Inc.

...
> +test=d-1

No need to worry about constructing unique output file names.
Each init.sh-using test is run in a temporary-just-created directory
which is removed after the test is run.

I see you copied this anachronistic technique from another tests/rm/* script.
I've just fixed those bad examples:

   http://thread.gmane.org/gmane.comp.gnu.coreutils.general/2152

> +. "${srcdir=.}/init.sh"; path_prepend_ ../src
> +print_ver_ rm
> +
> +mkdir a || framework_failure_
> +> b || framework_failure_
> +
> +cat <<\EOF > $test.E || framework_failure_
> +removed directory: 'a'
> +removed 'b'
> +EOF

Please use "exp" or similar as the expected output file name.
I have a slight preference these days for using printf to generate such files:

  printf '%s\n' \
    "removed directory: 'a'"
    "removed 'b'" > exp || framework_failure_

> +rm --verbose -d a b > $test.O || fail=1

Similarly, simply using "out" here is fine.

> +if test -e a -o -e b; then

That should trigger a "make syntax-check" failure,
to inform you about portability problems with test's -o option.

> +fail=1
> +fi

Rewrite like e.g.,

  test -e a || fail=1
  test -e b || fail=1

> +# Compare expected and actual output.
> +compare $test.E $test.O || fail=1
> +
> +Exit $fail
> 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

Might as well be consistent and detect failure here, too:

    > d/a || framework_failure_

> +
> +rm -d d 2> out && fail=1
> +cat <<\EOF > exp || fail=1
> +rm: cannot remove 'd': Directory not empty
> +EOF

Especially for a one-liner, use printf rather than a here-doc:

  printf "rm: cannot remove 'd': Directory not empty" >  exp || fail=1

> +compare exp out || fail=1
> +
> +Exit $fail