| 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
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: