Patchwork util: make ensuredirs safer against races

login
register
mail settings
Submitter Bryan O'Sullivan
Date Feb. 13, 2013, 7:04 p.m.
Message ID <e6216ef880db51568b06.1360782269@australite.local>
Download mbox | patch
Permalink /patch/978/
State Accepted
Commit 423eee0b0b14086173f3adf423e512b1f9f75053
Headers show

Comments

Bryan O'Sullivan - Feb. 13, 2013, 7:04 p.m.
# HG changeset patch
# User Bryan O'Sullivan <bryano@fb.com>
# Date 1360782158 28800
# Node ID e6216ef880db51568b067ab4fc88c2bdf50beca2
# Parent  1506eb487dddbc377398096ce797fa5ccd712f10
util: make ensuredirs safer against races
Bryan O'Sullivan - Feb. 13, 2013, 7:34 p.m.
On Wed, Feb 13, 2013 at 11:32 AM, Kevin Bullock <
kbullock+mercurial@ringworld.org> wrote:

> Is stat actually needed? I don't see it used below.
>

Good catch.
Matt Mackall - Feb. 13, 2013, 8:03 p.m.
On Wed, 2013-02-13 at 11:34 -0800, Bryan O'Sullivan wrote:
> On Wed, Feb 13, 2013 at 11:32 AM, Kevin Bullock <
> kbullock+mercurial@ringworld.org> wrote:
> 
> > Is stat actually needed? I don't see it used below.
> >
> 
> Good catch.

The test suite will catch unused imports if you have pyflakes installed.

Patch

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -306,8 +306,7 @@  class vfs(abstractvfs):
             # to a directory. Let the posixfile() call below raise IOError.
             if basename:
                 if atomictemp:
-                    if not os.path.isdir(dirname):
-                        util.ensuredirs(dirname, self.createmode)
+                    util.ensuredirs(dirname, self.createmode)
                     return util.atomictempfile(f, mode, self.createmode)
                 try:
                     if 'w' in mode:
@@ -325,8 +324,7 @@  class vfs(abstractvfs):
                     if e.errno != errno.ENOENT:
                         raise
                     nlink = 0
-                    if not os.path.isdir(dirname):
-                        util.ensuredirs(dirname, self.createmode)
+                    util.ensuredirs(dirname, self.createmode)
                 if nlink > 0:
                     if self._trustnlink is None:
                         self._trustnlink = nlink > 1 or util.checknlink(f)
@@ -345,9 +343,7 @@  class vfs(abstractvfs):
         except OSError:
             pass
 
-        dirname = os.path.dirname(linkname)
-        if not os.path.exists(dirname):
-            util.ensuredirs(dirname, self.createmode)
+        util.ensuredirs(os.path.dirname(linkname), self.createmode)
 
         if self._cansymlink:
             try:
diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -16,7 +16,7 @@  hide platform-specific details from the 
 from i18n import _
 import error, osutil, encoding, collections
 import errno, re, shutil, sys, tempfile, traceback
-import os, time, datetime, calendar, textwrap, signal
+import os, stat, time, datetime, calendar, textwrap, signal
 import imp, socket, urllib
 
 if os.name == 'nt':
@@ -882,13 +882,20 @@  def makedirs(name, mode=None):
 
 def ensuredirs(name, mode=None):
     """race-safe recursive directory creation"""
+    if os.path.isdir(name):
+        return
+    parent = os.path.dirname(os.path.abspath(name))
+    if parent != name:
+        ensuredirs(parent, mode)
     try:
-        makedirs(name, mode)
+        os.mkdir(name)
     except OSError, err:
         if err.errno == errno.EEXIST and os.path.isdir(name):
             # someone else seems to have won a directory creation race
             return
         raise
+    if mode is not None:
+        os.chmod(name, mode)
 
 def readfile(path):
     fp = open(path, 'rb')