When using rmtree with a symlink to a directory as path, it will first
follow the symlink, (try to) remove all the contents of the source
directory and then raise the exception "OSError: [Errno 20] Not a
directory".

Expected behaviour: The function should only raise an exception.

If the current behaviour is indeed wanted, it should a least be documented.

Please try to include stack trace in bug reports. I reproduced the error
on my Linux (SuSE 10). 

marvin:tmp$ ls -l dirlink testdir
lrwxrwxrwx 1 raghu users    7 2007-12-20 10:10 dirlink -> testdir/

testdir:
total 0
-rw-r--r-- 1 raghu users 0 2007-12-20 10:36 testfile

shutil.rmtree('dirlink') produces:

----------
Traceback (most recent call last):
  File "t.py", line 4, in <module>
    shutil.rmtree('dirlink')
  File "/localhome/raghu/localwork/cpython/python/Lib/shutil.py", line
194, in rmtree
    onerror(os.rmdir, path, sys.exc_info())
  File "/localhome/raghu/localwork/cpython/python/Lib/shutil.py", line
192, in rmtree
    os.rmdir(path)
OSError: [Errno 20] Not a directory: 'dirlink'
----------

While we are removing the contents of the target directory as expected,
the final 'rmdir' fails on removing the symbolic link. For the sake of
consistency, we can explicitly remove the target directory and leave the
symbolic link intact. Is this what you are expecting?

> While we are removing the contents of the target directory as expected,

This is not what I expected at all. I expected the function to fail,
because the target was not a directory, just a symlink to a directory.
That, or behavior similar to the command "rm -rf" which only removes the
symlink, but not the contents.

The current behavior is in my opinion inconsistent; if the symlink is
treated as a normal directory, it should also get deleted.

As I said, if the current behavior is apparently expected, it should be
documented, because I can think of no program that follow symlinks while
deleting unless specifically instructed to.

I agree with Tesiph, more useful behavior would be to raise an error
immediately because the argument is not a directory. If you wanted to
remove the think linked to, you could use rmtree("foo/.",
ignore_errors=True).

I am ok with disallowing symlinks in rmtree() because if it were to be
allowed, the semantics are not clear. In addition, neither 'rmdir' nor
'rm -rf' delete the target directory. 

The attached patch would raise error if symbolic link is passed to
rmtree. Even though there is a parameter 'ignore_errors", I don't think
it should be used in suppressing symbolic link error. Comments?

Thanks for the patch. I think it should raise IOError, not ValueError,
and it should use the onerror() handling used for all other errors.
Also, can you include an update for the docs in the Doc tree?

Index: Lib/shutil.py
===================================================================
--- Lib/shutil.py       (revision 59581)
+++ Lib/shutil.py       (working copy)
@@ -156,6 +156,16 @@
     elif onerror is None:
         def onerror(*args):
             raise
+
+    try:
+        if os.path.islink(path):
+            if ignore_errors:
+                return
+            else:
+                raise IOError('path can not be symbolic link')
+    except IOError, err:
+        onerror(os.path.islink, path, sys.exc_info())
+
     names = []
     try:
         names = os.listdir(path)
-------------------

How does this look? The error handling is slightly different for this
case because it can not continue if 'ignore_errors' is True. I will
update the doc if the code change is ok.

On Dec 20, 2007 12:43 PM, Guido van Rossum <report@bugs.python.org> wrote:
>
> Guido van Rossum added the comment:
>
> Thanks for the patch. I think it should raise IOError, not ValueError,
> and it should use the onerror() handling used for all other errors.
> Also, can you include an update for the docs in the Doc tree?
>
>
> __________________________________
> Tracker <report@bugs.python.org>
> <http://bugs.python.org/issue1669>
> __________________________________
>

Committed a variant of the last patch in r60139; it now raises OSError
like all other functions that are used in rmtree(). Added docs and tests.

If rmtree() always returns in case of symbolic links (as it is
checked-in), wouldn't it be much simpler to totally do away with
'onerror' handling? I thought 'onerror' gives the user the option how to
proceed (which is true in other usages).

Guido explicitly said "it should raise IOError, not ValueError,
and it should use the onerror() handling used for all other errors"
which makes sense for me too.

> and it should use the onerror() handling used for all other errors"
> which makes sense for me too.

Sure. I am ok with using 'onerror'. The point I am trying to make is
that there is slight difference between 'onerror' in this case and in
the three other places where it is called. In the case of symlink,
rmtree() returns even when 'onerror' doesn't raise exception. This is
not same in the other cases where rmtree() continues. I am just
wondering if this inconsistency is worth it and if it is, an explicit
mention in the doc will be nice.