filepath.FilePath.remove breaks on symlinks

[cyli]

FilePath.remove on a symlink whose target is a directory will result in an OSError. FilePath.remove currently checks to see if the path is a directory, and if so, removes all children and then calls os.rmdir on itself. Otherwise, it calls os.remove on itself. The problem is that a link that points to a directory returns true for both islink() and isdir(). Therefore, when you call FilePath.remove on a symlink whose target is a directory, it will both empty that target directory (which probably is not a desirable outcome) and call rmdir on the link, which raises an error.

[cyli]

This solves the problem. I think this is the desired behavior for filepath.FilePath.remove

Another behavior that may be desirable (I didn't think so, so it's not in the patch) is that when filepath.FilePath.remove is called on a symlink that points to a directory: 1) everything inside the target directory is removed, 2) the target directory is removed, and 3) the symlink to that directory is removed. If this is the desired behavior, filepath.FilePath.remove can take an optional argument (ex. followLink) that defaults to False (since I don't think it's usually the desired behavior) but can be True when this behavior is desired.

[cyli]

patch

[spiv]

It's a shame that remove is implemented using look-before-you-leap, which is inherently susceptible to race conditions. Although I guess any attempt to implement "delete this path, whatever sort of thing it is" is going to be racy on POSIX (because there's no atomic "unlink or rmdir" operation, let alone one that recurses), so this perhaps doesn't matter much in practice...

Getting more off-topic, having remove always mean recursively delete seems dubious to me. There certainly ought to be a convenience method with those semantics, but I'd also like there to be a more primitive "remove a file or error" method, like os.unlink. Maybe I'm just too used to thinking in terms of POSIX semantics :)

(These remarks should not be taken as criticism of the proposed fix, btw.)

[therve]

That looks sensible. My comment on the patch:

• the new behavior should be tested in a new (different) test, not in testRemove. The lack of os.symlink should result to an explicit skip
• it would be great if remove could gain a doctring in the process. I won't press you to do it though :).

Thanks.

[cyli]

Docstring put in, and tests moved to a new test that explicitly skips if symlinks are not supported. I won't change it here, since it's not part of the bug, but I could make remove take an optional argument as to whether or not to recursively remove directories.

[exarkun]

(In [23048]) Branching to 'filepath-remove-symlink-3097'

[exarkun]

(In [23050]) simple non-behavioral changes

use getattr instead of hasattr - hasattr can mask exceptions (probably not on a module, but who knows) write docstring in a newer style than is prevalant in test_paths.py currently use assertTrue and assertFalse instead of failIf and failUnless fix a bit of a grammatical thinko in the all-new remove docstring

refs #3097

[exarkun]

Okay I like this now, I think I'll merge it.

[exarkun]

(In [23062]) Merge filepath-remove-symlink-3097

Author: cyli Reviewer: therve, exarkun Fixes #3097

Change FilePath.remove so that symlinks are only deleted by it, not the contents of the directories they point to, and so that when a symlink which points to a directory is encountered, an exception is not raised.