Opened 10 years ago

Closed 10 years ago

#3097 enhancement closed fixed (fixed)

filepath.FilePath.remove breaks on symlinks

Reported by: Ying Li Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: therve Branch: branches/filepath-remove-symlink-3097
branch-diff, diff-cov, branch-cov, buildbot
Author:

Description

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.

Attachments (2)

filepath.remove.symlink.patch (1.1 KB) - added by Ying Li 10 years ago.
patch
filepath.remove.symlink.2.patch (1.7 KB) - added by Ying Li 10 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 10 years ago by Ying Li

Keywords: review added
Owner: Ying Li deleted
Priority: normalhighest

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.

Changed 10 years ago by Ying Li

patch

comment:2 Changed 10 years ago by 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.)

comment:3 Changed 10 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to Ying Li
Priority: highesthigh

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.

Changed 10 years ago by Ying Li

comment:4 Changed 10 years ago by Ying Li

Keywords: review added
Owner: Ying Li deleted
Priority: highhighest
Summary: FilePath.remove breaks on symlinksfilepath.FilePath.remove breaks on symlinks

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.

comment:5 Changed 10 years ago by Jean-Paul Calderone

author: exarkun
Branch: branches/filepath-remove-symlink-3097

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

comment:6 Changed 10 years ago by Jean-Paul Calderone

(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

comment:7 Changed 10 years ago by Jean-Paul Calderone

author: exarkun
Keywords: review removed

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

comment:8 Changed 10 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(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.

comment:9 Changed 7 years ago by <automation>

Note: See TracTickets for help on using tickets.