Merge lp:~mbp/bzr/427773-empty-limbo into lp:bzr/2.4

[Martin Pool]

tolerate empty limbo and pending-deletion directories (bug 427773)

fix up niggling bug 427773 by tolerating empty limbo dirs.

also, I saw we were actually squashing any exception raised while creating them, so this fixes that.

[Jelmer Vernooij (jelmer)]

I guess FAT is another filesystem on Unix where this occurs?

102 +class _PosixPermissionsFeature(Feature):
103 +
104 + def _probe(self):
105 + def has_perms():
106 + # create temporary file and check if specified perms are maintained.
107 + import tempfile
108 +
109 + write_perms = stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR
110 + f = tempfile.mkstemp(prefix='bzr_perms_chk_')
Should this be using self.test_dir ? I'm not sure how to get at that variable from within a Feature though. I'm not sure how many people have /tmp mounted over SMB though. :-)

review: Approve

[Martin Pool (mbp)]

ah, this is orignially off 2.4 so the diff is a bit weird...

[Martin Pool (mbp)]

+class _PosixPermissionsFeature(Feature):

That's in 2.4 and I guess not yet merged up to trunk.

I think it is probably harmless enough not to be worth chasing: it's possible that tempfile creates things in a different place to what our TestCase does but not all that likely.

It would be kind of nice to move the tests's own tempdir into a feature that could somehow be depended upon by this. It seems like you want almost a concept of a dynamic scope environment for the tests, or perhaps just a pointer back to the test object is enough.

Anyhow, I will merge the limbo related bits, thanks for the review.

[Martin Pool (mbp)]

sent to pqm by email

[Martin Pool (mbp)]

sent to pqm by email

=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2011-11-18 00:32:50 +0000
+++ bzrlib/osutils.py	2011-12-01 01:06:24 +0000
@@ -2532,3 +2532,18 @@
     fn = getattr(os, 'fdatasync', getattr(os, 'fsync', None))
     if fn is not None:
         fn(fileno)
+
+
+def ensure_empty_directory_exists(path, exception_class):
+    """Make sure a local directory exists and is empty.
+    
+    If it does not exist, it is created.  If it exists and is not empty, an
+    instance of exception_class is raised.
+    """
+    try:
+        os.mkdir(path)
+    except OSError, e:
+        if e.errno != errno.EEXIST:
+            raise
+        if os.listdir(path) != []:
+            raise exception_class(path)
 
=== modified file 'bzrlib/tests/per_merger.py'
--- bzrlib/tests/per_merger.py	2011-01-12 01:01:53 +0000
+++ bzrlib/tests/per_merger.py	2011-12-01 01:06:24 +0000
@@ -175,17 +175,33 @@
         transform.finalize()
         return (limbodir, deletiondir)
 
-    def test_merge_with_existing_limbo(self):
-        wt = self.make_branch_and_tree('this')
-        (limbodir, deletiondir) =  self.get_limbodir_deletiondir(wt)
-        os.mkdir(limbodir)
+    def test_merge_with_existing_limbo_empty(self):
+        """Empty limbo dir is just cleaned up - see bug 427773"""
+        wt = self.make_branch_and_tree('this')
+        (limbodir, deletiondir) =  self.get_limbodir_deletiondir(wt)
+        os.mkdir(limbodir)
+        self.do_merge(wt, wt)
+
+    def test_merge_with_existing_limbo_non_empty(self):
+        wt = self.make_branch_and_tree('this')
+        (limbodir, deletiondir) =  self.get_limbodir_deletiondir(wt)
+        os.mkdir(limbodir)
+        os.mkdir(os.path.join(limbodir, 'something'))
         self.assertRaises(errors.ExistingLimbo, self.do_merge, wt, wt)
         self.assertRaises(errors.LockError, wt.unlock)
 
-    def test_merge_with_pending_deletion(self):
-        wt = self.make_branch_and_tree('this')
-        (limbodir, deletiondir) =  self.get_limbodir_deletiondir(wt)
-        os.mkdir(deletiondir)
+    def test_merge_with_pending_deletion_empty(self):
+        wt = self.make_branch_and_tree('this')
+        (limbodir, deletiondir) =  self.get_limbodir_deletiondir(wt)
+        os.mkdir(deletiondir)
+        self.do_merge(wt, wt)
+
+    def test_merge_with_pending_deletion_non_empty(self):
+        """Also see bug 427773"""
+        wt = self.make_branch_and_tree('this')
+        (limbodir, deletiondir) =  self.get_limbodir_deletiondir(wt)
+        os.mkdir(deletiondir)
+        os.mkdir(os.path.join(deletiondir, 'something'))
         self.assertRaises(errors.ExistingPendingDeletion, self.do_merge, wt, wt)
         self.assertRaises(errors.LockError, wt.unlock)
 
 
=== modified file 'bzrlib/transform.py'
--- bzrlib/transform.py	2011-11-14 09:37:40 +0000
+++ bzrlib/transform.py	2011-12-01 01:06:24 +0000
@@ -1562,18 +1562,14 @@
         try:
             limbodir = urlutils.local_path_from_url(
                 tree._transport.abspath('limbo'))
-            try:
-                os.mkdir(limbodir)
-            except OSError, e:
-                if e.errno == errno.EEXIST:
-                    raise ExistingLimbo(limbodir)
+            osutils.ensure_empty_directory_exists(
+                limbodir,
+                errors.ExistingLimbo)
             deletiondir = urlutils.local_path_from_url(
                 tree._transport.abspath('pending-deletion'))
-            try:
-                os.mkdir(deletiondir)
-            except OSError, e:
-                if e.errno == errno.EEXIST:
-                    raise errors.ExistingPendingDeletion(deletiondir)
+            osutils.ensure_empty_directory_exists(
+                deletiondir,
+                errors.ExistingPendingDeletion)
         except:
             tree.unlock()
             raise
 
=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-11-14 09:37:40 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-12-01 01:06:24 +0000
@@ -35,6 +35,12 @@
 * Cope with Unix filesystems, such as smbfs, where chmod gives 'permission
   denied'.  (Martin Pool, #606537)
 
+* When the ``limbo`` or ``pending-deletion`` directories exist, typically
+  because of an interrupted tree update, but are empty, bzr no longer
+  errors out, because there is nothing for the user to clean up.  Also,
+  errors in creation of these directories are no longer squelched.
+  (Martin Pool, #427773)
+
 * During merges, when two entries end up using the same path for two
   different file-ids (the same file being 'bzr added' in two different
   branches) , 'duplicate' conflicts are created instead of 'content'