Patchwork [V2] util: move checknlink away from the dependency of a hard-coded filename

login
register
mail settings
Submitter Tony Tung
Date Aug. 24, 2016, 6:18 p.m.
Message ID <e062df885ace82e0aeea.1472062682@andromeda.local>
Download mbox | patch
Permalink /patch/16402/
State Superseded
Headers show

Comments

Tony Tung - Aug. 24, 2016, 6:18 p.m.
# HG changeset patch
# User Tony Tung <tonytung@merly.org>
# Date 1472062675 25200
#      Wed Aug 24 11:17:55 2016 -0700
# Node ID e062df885ace82e0aeea19d93c80dd3911078c41
# Parent  2efc5a05d80a6d4253767f2ce0c2fb062ba83cb6
util: move checknlink away from the dependency of a hard-coded filename

This somewhat insulates us against bugs in checknlink when a stale file
left behind could result in checknlink always returning False.
Yuya Nishihara - Aug. 25, 2016, 2 p.m.
On Wed, 24 Aug 2016 11:18:02 -0700, ttung@fb.com wrote:
> # HG changeset patch
> # User Tony Tung <tonytung@merly.org>
> # Date 1472062675 25200
> #      Wed Aug 24 11:17:55 2016 -0700
> # Node ID e062df885ace82e0aeea19d93c80dd3911078c41
> # Parent  2efc5a05d80a6d4253767f2ce0c2fb062ba83cb6
> util: move checknlink away from the dependency of a hard-coded filename

>      finally:
> -        if fd is not None:
> -            fd.close()
> -        for f in (f1, f2):
> -            try:
> -                os.unlink(f)
> -            except OSError:
> -                pass
> +        for fi in (f1, f2):
> +            if 'path' in fi:
> +                try:
> +                    os.unlink(fi['path'])
> +                except OSError:
> +                    pass

Perhaps we'd better to close files first to make Windows happy. posixfile()
should have no problem, but I've no idea if files open by Windows' mkstemp()
can be deleted before close().

> +
> +            if 'fd' in fi:
> +                try:
> +                    os.close(fi['fd'])
> +                except OSError:
> +                    pass
> +
> +            if 'handle' in fi:
> +                try:
> +                    fi['handle'].close()
> +                except OSError:
> +                    pass

I think file.close() would raise IOError on failure. Am I wrong?
Adrian Buehlmann - Aug. 25, 2016, 4:46 p.m.
On 2016-08-25 16:00, Yuya Nishihara wrote:
> Perhaps we'd better to close files first to make Windows happy. posixfile()
> should have no problem, but I've no idea if files open by Windows' mkstemp()
> can be deleted before close().
> 

See https://www.mercurial-scm.org/wiki/UnlinkingFilesOnWindows for
posixfile behavior on Windows.

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1305,38 +1305,54 @@ 
 def checknlink(testfile):
     '''check whether hardlink count reporting works properly'''
 
-    # testfile may be open, so we need a separate file for checking to
-    # work around issue2543 (or testfile may get lost on Samba shares)
-    f1 = testfile + ".hgtmp1"
-    if os.path.lexists(f1):
-        return False
+    f1 = {}
+    f2 = {}
+
     try:
-        posixfile(f1, 'w').close()
-    except IOError:
+        dirpath, filename = os.path.split(testfile)
+
+        # testfile may be open, so we need a separate file for checking to
+        # work around issue2543 (or testfile may get lost on Samba shares)
+        f1['fd'], f1['path'] = tempfile.mkstemp(prefix=filename, dir=dirpath)
+        os.close(f1['fd'])
+        del f1['fd']
+
+        f2['fd'], f2['path'] = tempfile.mkstemp(prefix=filename, dir=dirpath)
+        os.close(f2['fd'])
+        del f2['fd']
+
+        # there's a small race condition that another file can jam itself in
+        # between the time we remove f2 and the time we create the hard link.
+        # in the unlikely scenario that happens, we'll treat it as nlink being
+        # unreliable.
         try:
-            os.unlink(f1)
+            os.unlink(f2['path'])
+            oslink(f1['path'], f2['path'])
+            # nlinks() may behave differently for files on Windows shares if
+            # the file is open.
+            f2['handle'] = posixfile(f2['path'])
+            return nlinks(f2['path']) > 1
         except OSError:
-            pass
-        return False
-
-    f2 = testfile + ".hgtmp2"
-    fd = None
-    try:
-        oslink(f1, f2)
-        # nlinks() may behave differently for files on Windows shares if
-        # the file is open.
-        fd = posixfile(f2)
-        return nlinks(f2) > 1
-    except OSError:
-        return False
+            return False
     finally:
-        if fd is not None:
-            fd.close()
-        for f in (f1, f2):
-            try:
-                os.unlink(f)
-            except OSError:
-                pass
+        for fi in (f1, f2):
+            if 'path' in fi:
+                try:
+                    os.unlink(fi['path'])
+                except OSError:
+                    pass
+
+            if 'fd' in fi:
+                try:
+                    os.close(fi['fd'])
+                except OSError:
+                    pass
+
+            if 'handle' in fi:
+                try:
+                    fi['handle'].close()
+                except OSError:
+                    pass
 
 def endswithsep(path):
     '''Check path ends with os.sep or os.altsep.'''