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

login
register
mail settings
Submitter Tony Tung
Date Aug. 23, 2016, 5:39 p.m.
Message ID <800bdaf50847ed85c913.1471973969@andromeda.local>
Download mbox | patch
Permalink /patch/16395/
State Changes Requested
Headers show

Comments

Tony Tung - Aug. 23, 2016, 5:39 p.m.
# HG changeset patch
# User Tony Tung <tonytung@merly.org>
# Date 1471973931 25200
#      Tue Aug 23 10:38:51 2016 -0700
# Node ID 800bdaf50847ed85c913d8d8701d7c6738ee082c
# 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. 24, 2016, 12:14 p.m.
On Tue, 23 Aug 2016 10:39:29 -0700, ttung@fb.com wrote:
> # HG changeset patch
> # User Tony Tung <tonytung@merly.org>
> # Date 1471973931 25200
> #      Tue Aug 23 10:38:51 2016 -0700
> # Node ID 800bdaf50847ed85c913d8d8701d7c6738ee082c
> # 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.
> 
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -1305,34 +1305,46 @@
>  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
> +    f1path, f2path, f1handle, f2handle = None, None, None, None

Nit: this could be f1path = f2path = ... = None.

>      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)
> +        f1handle, f1path = tempfile.mkstemp(prefix=filename, dir=dirpath)
> +        os.close(f1handle)
> +        f1handle = None
> +
> +        f2handle, f2path = tempfile.mkstemp(prefix=filename, dir=dirpath)
> +        os.close(f2handle)
> +        f2handle = None
> +
> +        # 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(f2path)
> +            oslink(f1path, f2path)
> +            # nlinks() may behave differently for files on Windows shares if
> +            # the file is open.
> +            f2handle = posixfile(f2path)
> +            return nlinks(f2path) > 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):
> +        for f in (f1handle, f2handle):
> +            try:
> +                if f is None:
> +                    continue
> +                elif isinstance(f, int):
> +                    os.close(f)
> +                else:
> +                    f.close()

I don't think there's a benefit to store file descriptor or file object
to the same variable. And IIRC, f.close() would raise IOError on failure,
whereas os.close() would OSError.

> +            except OSError:
> +                pass
> +        for f in (f1path, f2path):

f1path and f2path could be None.

>              try:
>                  os.unlink(f)

Patch

diff --git a/mercurial/util.py b/mercurial/util.py
--- a/mercurial/util.py
+++ b/mercurial/util.py
@@ -1305,34 +1305,46 @@ 
 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
+    f1path, f2path, f1handle, f2handle = None, None, None, None
+
     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)
+        f1handle, f1path = tempfile.mkstemp(prefix=filename, dir=dirpath)
+        os.close(f1handle)
+        f1handle = None
+
+        f2handle, f2path = tempfile.mkstemp(prefix=filename, dir=dirpath)
+        os.close(f2handle)
+        f2handle = None
+
+        # 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(f2path)
+            oslink(f1path, f2path)
+            # nlinks() may behave differently for files on Windows shares if
+            # the file is open.
+            f2handle = posixfile(f2path)
+            return nlinks(f2path) > 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):
+        for f in (f1handle, f2handle):
+            try:
+                if f is None:
+                    continue
+                elif isinstance(f, int):
+                    os.close(f)
+                else:
+                    f.close()
+            except OSError:
+                pass
+        for f in (f1path, f2path):
             try:
                 os.unlink(f)
             except OSError: