Opened 11 years ago

Closed 10 years ago

#3911 defect closed fixed (fixed)

'descend' argument in 'walk' method in twisted.python.filepath._PathHelper class doesn't work well

Reported by: Markon Owned by:
Priority: high Milestone:
Component: core Keywords:
Cc: Branch: branches/filepath-symlink-cycle-3911
branch-diff, diff-cov, branch-cov, buildbot
Author: Markon

Description

The 'walk' method in the class _PathHelper, located in twisted/python/filepath.py doesn't work according to the 'descend' argument passed. This is an example:

>>> import twisted.python.filepath as FP
>>> filep = FP.FilePath("/home/marco/Desktop")
>>> for x in filep.walk(lambda path: not path.islink()):
...     print x
...
FilePath('/home/marco/Desktop')
FilePath('/home/marco/Desktop/amarok.desktop')
[...]
FilePath('/home/marco/Desktop/MyFolder')
FilePath('/home/marco/Desktop/MyFolder/myfile.txt')
[...]
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.5/site-packages/twisted/python/filepath.py", line 214, in walk
    raise LinkError("Cycle in file graph.")
twisted.python.filepath.LinkError: Cycle in file graph.

In the Desktop directory there's a symlink "Risorse", which points to "/home/marco". So it creates a cycle, ok. But I've specified the descend argument and it shouldn't follow the link.

However, I've also provided a patch, and if I use it, the argument 'descend' is read fine:

>>> import filepath as FP
>>> filep = FP.FilePath("/home/marco/Desktop")
>>> for x in filep.walk(lambda path: not path.islink()):
...     print x
...
FilePath('/home/marco/Desktop')
FilePath('/home/marco/Desktop/amarok.desktop')
[...]
FilePath('/home/marco/Desktop/MyFolder')
FilePath('/home/marco/Desktop/MyFolder/myfile.txt')
[...]
FilePath('/home/marco/Desktop/Risorse') <<<---- this is the cyclic symlink
[... other files ...]

I should write the tests, but in the meantime I attach the patch I've written.

Attachments (3)

filepath.patch (1.2 KB) - added by Markon 11 years ago.
This is the patch provided for '_PathHelper.walk'
filepath_test.patch (1.0 KB) - added by Markon 11 years ago.
This is the new test added to FilePathTestCase
filepath_test.2.patch (1.0 KB) - added by Markon 11 years ago.
This is the new test added to FilePathTestCase

Download all attachments as: .zip

Change History (11)

Changed 11 years ago by Markon

Attachment: filepath.patch added

This is the patch provided for '_PathHelper.walk'

comment:1 Changed 11 years ago by Markon

I've added a little test with some cyclical symlinks in FilePathTestCase (twisted/test/test_paths.py):

    def test_walkObeysDescendWithCyclicalSymlinks(self):
        """
        Verify that, after making a path with cyclical symlinks,
        when the supplied C{descend} predicate returns C{False},
        the target is not traversed, as if it was a simple symlink.
        """
        self.createLinks()
        # we create cyclical symlinks
        self.symlink(self.path.child("sub1").path,
                     self.path.child("sub1").child("sub1.loopylink").path)
        def noSymLinks(path):
            return not path.islink()
        def iterateOverPath():
            return [foo.path for foo in self.path.walk(descend=noSymLinks)]
        self.assertTrue(iterateOverPath())

I'm sorry if this test is not pretty good, but it's the first time I write unittests for "official" works :) However, if you use the "old" version of twisted.python.filepath.FilePath (without the patch), it Fail, since it would raise an exception: filepath.LinkErrror. Infact:

[...]
test_walkObeysDescendWithCyclicalSymlinks ...                       [ERROR]
[ERROR]: test_paths.FilePathTestCase.test_walkObeysDescendWithCyclicalSymlinks

Traceback (most recent call last):
  File "test_paths.py", line 366, in test_walkObeysDescendWithCyclicalSymlinks
    self.assertTrue(iterateOverPath())
  File "test_paths.py", line 365, in iterateOverPath
    return [foo.path for foo in self.path.walk(descend=noSymLinks)]
  File "/usr/lib/python2.5/site-packages/twisted/python/filepath.py", line 211, in walk
    for subc in c.walk(descend):
  File "/usr/lib/python2.5/site-packages/twisted/python/filepath.py", line 214, in walk
    raise LinkError("Cycle in file graph.")
twisted.python.filepath.LinkError: Cycle in file graph.

Instead, if you change the code to use the new "walk" method, it works well, without fails.

test_walkObeysDescendWithCyclicalSymlinks ...                          [OK]

I attach the "patch" to twisted/test/test_paths.py

Changed 11 years ago by Markon

Attachment: filepath_test.patch added

This is the new test added to FilePathTestCase

Changed 11 years ago by Markon

Attachment: filepath_test.2.patch added

This is the new test added to FilePathTestCase

comment:2 Changed 11 years ago by Markon

Keywords: review added
Owner: Glyph deleted

comment:3 Changed 11 years ago by Jean-Paul Calderone

Author: MarkonMarkon, exarkun
Branch: branches/filepath-symlink-cycle-3911

(In [27154]) Branching to 'filepath-symlink-cycle-3911'

comment:4 Changed 11 years ago by Jean-Paul Calderone

(In [27155]) Apply (manually) filepath.patch and filepath_test.2.patch from Markon

refs #3911

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

Author: Markon, exarkunMarkon

comment:6 Changed 11 years ago by therve

Keywords: review removed
Owner: set to Jean-Paul Calderone

It looks reasonable, and doesn't break any other tests. Please merge!

comment:7 Changed 10 years ago by therve

Resolution: fixed
Status: newclosed

(In [27362]) Merge filepath-symlink-cycle-3911

Author: Markon Reviewer: therve Fixes #3911

Fix a bug with the descend parameter of the walk method, calling it earlier to be able to catch cycles sooner.

comment:8 Changed 9 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.