Merge lp:~jderose/filestore/ensuredir into lp:filestore

[Jason Gerard DeRose]

This is a small tweak to the ensuredir() function so that only one sys call to stat() or lstat() is made.

This makes ensuredir() more atomic and correct. Also should improve FileStore.init_dirs() slightly, although that wasn't the motivation.

[Paul Hummer (rockstar)]

's' is not a very good name for a variable. Sure, the scope is limited, but it looks like you've also got a 'd' variable nearby. I would discourage that even in a language where the variable length might matter (like client-side javascript). You might want to re-think that.

...of course, I'm only a "community" vote.

review: Needs Fixing

[Jason Gerard DeRose (jderose)]

Paul,

What would you recommend instead of 's'?

Thanks for the review!

[lp:~jderose/filestore/ensuredir]

185. By Jason Gerard DeRose on 2011-09-07

Improved variable naming in ensuredir()

[Jason Gerard DeRose (jderose)]

Better?

review: Needs Resubmitting

[Paul Hummer (rockstar)]

That looks great to me.

review: Approve

=== modified file 'filestore.py'
--- filestore.py	2011-08-24 00:31:50 +0000
+++ filestore.py	2011-09-07 01:28:23 +0000
@@ -860,12 +860,13 @@
         os.mkdir(d)
         return True
     except OSError:
-        if not path.isdir(d):
+        mode = os.lstat(d).st_mode
+        if stat.S_ISLNK(mode):
+            raise ValueError(
+                '{!r} is symlink to {!r}'.format(d, os.readlink(d))
+            )
+        if not stat.S_ISDIR(mode):
             raise ValueError('not a directory: {!r}'.format(d))
-        if path.islink(d):
-            raise ValueError(
-                '{!r} is symlink to {!r}'.format(d, os.readlink(d))
-            )
         return False
 
 

=== modified file 'test_filestore.py'
--- test_filestore.py	2011-08-21 18:54:48 +0000
+++ test_filestore.py	2011-09-07 01:28:23 +0000
@@ -706,9 +706,12 @@
 
         # Test that os.makedirs() is not used:
         d = tmp.join('foo', 'bar')
-        with self.assertRaises(ValueError) as cm:
+        with self.assertRaises(OSError) as cm:
             f(d)
-        self.assertEqual(str(cm.exception), 'not a directory: {!r}'.format(d))
+        self.assertEqual(
+            str(cm.exception), 
+            '[Errno 2] No such file or directory: {!r}'.format(d)
+        )
         self.assertFalse(path.exists(d))
         self.assertFalse(path.exists(tmp.join('foo')))