[PATCH] osutils.rmtree + test

[John A Meinel]

Alexander Belchenko wrote:
> Aaron Bentley пишет:
>> Alexander Belchenko wrote:
>> | Attached patch helps to remove read-only files and directories on
>> win32.
>> | Pleae, review this patch.
>> +1
>>
>> This is a good start, but I think we can go further; I think we should
>> provide an "always-works" version of rmtree in osutils, and start using
>> it everywhere.

I'm not 100% that we want 'always-works'. I know we switched from having
branch entries being readonly so we could let people do 'rm -r' and not
have to use '-f'. I also don't want us to suppress other errors because
we always delete everything.

I *am* 100% that we should be able to delete the testing tree. We know
we put everything in there, and it is okay to delete it all.

I also can say that we are just trying to provide the expected
functionality to 'rmtree'. On Linux it does delete everything, we expect
that, so it is only used where it is safe. So we are just bringing that
into win32.

I just want this discussed a little bit before I give full approval.

> 
> Here is implementation of osutils.rmtree and test for this function.
> 
> First part of patch (rmtree-test.diff) provide rmtree stub and test.
> Second part of patch (rmtree-osutils.diff) provide rmtree implementation
> for windows that can delete readonly files and dirs. Also this patch
> change all internal usage of shutil.rmtree to osutils.rmtree.
> 
> -- 
> Alexander
> 

Because I'm very close to full approval, I'll still review the patch, so
that it can be pulled in easily.
And thanks for doing this, Alexander.

> 
> ------------------------------------------------------------------------
> 
> === modified file 'bzrlib/osutils.py'
> --- bzrlib/osutils.py	
> +++ bzrlib/osutils.py	
> @@ -24,6 +24,7 @@
>  import os
>  import re
>  import sha
> +import shutil
>  import string
>  import sys
>  import time
> @@ -180,6 +181,7 @@
>  rename = os.rename
>  dirname = os.path.dirname
>  basename = os.path.basename
> +rmtree = shutil.rmtree
>  
>  if os.name == "posix":
>      # In Python 2.4.2 and older, os.path.abspath and os.path.realpath
> 
> === modified file 'bzrlib/tests/test_osutils.py'
> --- bzrlib/tests/test_osutils.py	
> +++ bzrlib/tests/test_osutils.py	
> @@ -63,6 +63,21 @@
>      # TODO: test fancy_rename using a MemoryTransport
>  
>  
> +    def test_rmtree(self):
> +        # Check to remove tree with read-only files/dirs
> +        os.mkdir('dir')
> +        osutils.make_readonly('dir')
> +        f = file('dir/file', 'w')
> +        f.write('spam')
> +        f.close()
> +        osutils.make_readonly('dir/file')
> +
> +        osutils.rmtree('dir')
> +
> +        self.failIfExists('dir/file')
> +        self.failIfExists('dir')
> +
> +
>  class TestSafeUnicode(TestCase):
>  
>      def test_from_ascii_string(self):
> 
> 

Looks good so far.

...

> === modified file 'bzrlib/osutils.py'
> --- bzrlib/osutils.py	
> +++ bzrlib/osutils.py	
> @@ -220,6 +220,24 @@
>          fancy_rename(old, new, rename_func=os.rename, unlink_func=os.unlink)
>  
>  
> +    def _win32_delete_readonly(function, path, excinfo):
> +        """Error handler for shutil.rmtree function [for win32]
> +        Helps to remove files and dirs marked as read-only.
> +        """
> +        type_, value = excinfo[:2]
> +        if function in (os.remove, os.rmdir) \
> +            and type_ == OSError \
> +            and value.errno == errno.EACCES:
> +            bzrlib.osutils.make_writable(path)
> +            function(path)
> +        else:
> +            raise
> +
> +    def rmtree(path, ignore_errors=False, onerror=_win32_delete_readonly):
> +        """Replacer for shutil.rmtree: could remove readonly dirs/files"""
> +        return shutil.rmtree(path, ignore_errors, onerror)
> +
> +

Looks correct.

>  def normalizepath(f):
>      if hasattr(os.path, 'realpath'):
>          F = realpath
> 
> === modified file 'bzrlib/tests/__init__.py'
> --- bzrlib/tests/__init__.py	
> +++ bzrlib/tests/__init__.py	
> @@ -27,7 +27,6 @@
>  import logging
>  import os
>  import re
> -import shutil
>  import sys
>  import tempfile
>  import unittest
> @@ -632,7 +631,7 @@
>      # have to delete their temp directories themselves.
>      if result.wasSuccessful() or not keep_output:
>          if TestCaseInTempDir.TEST_ROOT is not None:
> -            shutil.rmtree(TestCaseInTempDir.TEST_ROOT) 
> +            osutils.rmtree(TestCaseInTempDir.TEST_ROOT)
>      else:
>          print "Failed tests working directories are in '%s'\n" % TestCaseInTempDir.TEST_ROOT
>      return result.wasSuccessful()
> 
> === modified file 'bzrlib/tests/blackbox/test_too_much.py'
> --- bzrlib/tests/blackbox/test_too_much.py	
> +++ bzrlib/tests/blackbox/test_too_much.py	
> @@ -37,13 +37,12 @@
>  from cStringIO import StringIO
>  import os
>  import re
> -import shutil
>  import sys
>  
>  from bzrlib.branch import Branch
>  from bzrlib.clone import copy_branch
>  from bzrlib.errors import BzrCommandError
> -from bzrlib.osutils import has_symlinks, pathjoin
> +from bzrlib.osutils import has_symlinks, pathjoin, rmtree
>  from bzrlib.tests.HTTPTestUtil import TestCaseWithWebserver
>  from bzrlib.tests.blackbox import ExternalBase
>  
> @@ -403,9 +402,9 @@
>          os.chdir('..')
>          # naughty - abstraction violations RBC 20050928  
>          print "test_branch used to delete the stores, how is this meant to work ?"
> -        #shutil.rmtree('a/.bzr/revision-store')
> -        #shutil.rmtree('a/.bzr/inventory-store', ignore_errors=True)
> -        #shutil.rmtree('a/.bzr/text-store', ignore_errors=True)
> +        #rmtree('a/.bzr/revision-store')
> +        #rmtree('a/.bzr/inventory-store', ignore_errors=True)
> +        #rmtree('a/.bzr/text-store', ignore_errors=True)
>          self.runbzr('branch a d --basis b')

I think we need to figure out what this test should be doing, rather
than putting up with the print statement, and commented out code. I
don't expect Alexander to do this, but what is really going on with this
test?

Other than the couple of points I raised, it all looks good.

John
=:->


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 249 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060122/043d546e/attachment.pgp