Patchwork posix: give the cached symlink a real target

login
register
mail settings
Submitter Martijn Pieters
Date Nov. 30, 2016, 4:51 p.m.
Message ID <4053f60f77657f8f54af.1480524701@mjpieters-mbp>
Download mbox | patch
Permalink /patch/17801/
State Accepted
Headers show

Comments

Martijn Pieters - Nov. 30, 2016, 4:51 p.m.
# HG changeset patch
# User Martijn Pieters <mjpieters@fb.com>
# Date 1480523976 0
#      Wed Nov 30 16:39:36 2016 +0000
# Node ID 4053f60f77657f8f54afc72d00bf629b75d0b4b9
# Parent  9e29d4e4e08b5996adda49cdd0b497d89e2b16ee
posix: give the cached symlink a real target

The NamedTemporaryFile file is cleared up so checklink ends up as a dangling
symlink, causing cp -r in tests to complain on both Solaris and OS X. Use
a permanent file instead when there is a .hg/cache directory.
Jun Wu - Nov. 30, 2016, 4:58 p.m.
Looks good to me. Thanks for the fix!
A small nit inline - probably could be fixed on flight.

Excerpts from Martijn Pieters's message of 2016-11-30 16:51:41 +0000:
> # HG changeset patch
> # User Martijn Pieters <mjpieters@fb.com>
> # Date 1480523976 0
> #      Wed Nov 30 16:39:36 2016 +0000
> # Node ID 4053f60f77657f8f54afc72d00bf629b75d0b4b9
> # Parent  9e29d4e4e08b5996adda49cdd0b497d89e2b16ee
> posix: give the cached symlink a real target
> 
> The NamedTemporaryFile file is cleared up so checklink ends up as a dangling
> symlink, causing cp -r in tests to complain on both Solaris and OS X. Use
> a permanent file instead when there is a .hg/cache directory.
> 
> diff --git a/mercurial/posix.py b/mercurial/posix.py
> --- a/mercurial/posix.py
> +++ b/mercurial/posix.py
> @@ -231,10 +231,18 @@
>              cachedir = None
>          name = tempfile.mktemp(dir=checkdir, prefix='checklink-')
>          try:
> -            fd = tempfile.NamedTemporaryFile(dir=checkdir,
> -                                             prefix='hg-checklink-')
> +            fd = None
> +            if cachedir is None:
> +                fd = tempfile.NamedTemporaryFile(dir=checkdir,
> +                                                 prefix='hg-checklink-')
> +                target = os.path.basename(fd.name)
> +            else:
> +                # create a fixed file to link to; doesn't matter if it
> +                # already exists.
> +                target = 'checklink-target'
> +                open(os.path.join(cachedir, target), 'w')

Should we append ".close()" here to just make things explicit?

>              try:
> -                os.symlink(os.path.basename(fd.name), name)
> +                os.symlink(target, name)
>                  if cachedir is None:
>                      os.unlink(name)
>                  else:
> @@ -249,7 +257,8 @@
>                      continue
>                  raise
>              finally:
> -                fd.close()
> +                if fd is not None:
> +                    fd.close()
>          except AttributeError:
>              return False
>          except OSError as inst:
> diff --git a/tests/test-clone.t b/tests/test-clone.t
> --- a/tests/test-clone.t
> +++ b/tests/test-clone.t
> @@ -33,6 +33,7 @@
>    branch2-served
>    checkisexec
>    checklink
> +  checklink-target
>    checknoexec
>    rbc-names-v1
>    rbc-revs-v1
> @@ -50,6 +51,7 @@
>    branch2-served
>    checkisexec
>    checklink
> +  checklink-target
>  
>    $ cat a
>    a
> diff --git a/tests/test-hardlinks.t b/tests/test-hardlinks.t
> --- a/tests/test-hardlinks.t
> +++ b/tests/test-hardlinks.t
> @@ -212,6 +212,8 @@
>    2 r4/.hg/branch
>    2 r4/.hg/cache/branch2-served
>    2 r4/.hg/cache/checkisexec
> +  3 r4/.hg/cache/checklink (?)
> +  ? r4/.hg/cache/checklink-target (glob)
>    2 r4/.hg/cache/checknoexec
>    2 r4/.hg/cache/rbc-names-v1
>    2 r4/.hg/cache/rbc-revs-v1
> @@ -250,6 +252,7 @@
>    1 r4/.hg/branch
>    2 r4/.hg/cache/branch2-served
>    2 r4/.hg/cache/checkisexec
> +  2 r4/.hg/cache/checklink-target
>    2 r4/.hg/cache/checknoexec
>    2 r4/.hg/cache/rbc-names-v1
>    2 r4/.hg/cache/rbc-revs-v1
> diff --git a/tests/test-tags.t b/tests/test-tags.t
> --- a/tests/test-tags.t
> +++ b/tests/test-tags.t
> @@ -674,6 +674,7 @@
>    branch2-served
>    checkisexec
>    checklink
> +  checklink-target
>    hgtagsfnodes1
>    rbc-names-v1
>    rbc-revs-v1
> @@ -700,6 +701,7 @@
>    branch2-served
>    checkisexec
>    checklink
> +  checklink-target
>    hgtagsfnodes1
>    rbc-names-v1
>    rbc-revs-v1
Mateusz Kwapich - Dec. 1, 2016, 4:09 p.m.
What Jun said. All the rest looks good to me.

Excerpts from Martijn Pieters's message of 2016-11-30 16:51:41 +0000:
> # HG changeset patch
> # User Martijn Pieters <mjpieters@fb.com>
> # Date 1480523976 0
> #      Wed Nov 30 16:39:36 2016 +0000
> # Node ID 4053f60f77657f8f54afc72d00bf629b75d0b4b9
> # Parent  9e29d4e4e08b5996adda49cdd0b497d89e2b16ee
> posix: give the cached symlink a real target
> 
> The NamedTemporaryFile file is cleared up so checklink ends up as a dangling
> symlink, causing cp -r in tests to complain on both Solaris and OS X. Use
> a permanent file instead when there is a .hg/cache directory.
> 
> diff --git a/mercurial/posix.py b/mercurial/posix.py
> --- a/mercurial/posix.py
> +++ b/mercurial/posix.py
> @@ -231,10 +231,18 @@
>              cachedir = None
>          name = tempfile.mktemp(dir=checkdir, prefix='checklink-')
>          try:
> -            fd = tempfile.NamedTemporaryFile(dir=checkdir,
> -                                             prefix='hg-checklink-')
> +            fd = None
> +            if cachedir is None:
> +                fd = tempfile.NamedTemporaryFile(dir=checkdir,
> +                                                 prefix='hg-checklink-')
> +                target = os.path.basename(fd.name)
> +            else:
> +                # create a fixed file to link to; doesn't matter if it
> +                # already exists.
> +                target = 'checklink-target'
> +                open(os.path.join(cachedir, target), 'w')
>              try:
> -                os.symlink(os.path.basename(fd.name), name)
> +                os.symlink(target, name)
>                  if cachedir is None:
>                      os.unlink(name)
>                  else:
> @@ -249,7 +257,8 @@
>                      continue
>                  raise
>              finally:
> -                fd.close()
> +                if fd is not None:
> +                    fd.close()
>          except AttributeError:
>              return False
>          except OSError as inst:
> diff --git a/tests/test-clone.t b/tests/test-clone.t
> --- a/tests/test-clone.t
> +++ b/tests/test-clone.t
> @@ -33,6 +33,7 @@
>    branch2-served
>    checkisexec
>    checklink
> +  checklink-target
>    checknoexec
>    rbc-names-v1
>    rbc-revs-v1
> @@ -50,6 +51,7 @@
>    branch2-served
>    checkisexec
>    checklink
> +  checklink-target
>  
>    $ cat a
>    a
> diff --git a/tests/test-hardlinks.t b/tests/test-hardlinks.t
> --- a/tests/test-hardlinks.t
> +++ b/tests/test-hardlinks.t
> @@ -212,6 +212,8 @@
>    2 r4/.hg/branch
>    2 r4/.hg/cache/branch2-served
>    2 r4/.hg/cache/checkisexec
> +  3 r4/.hg/cache/checklink (?)
> +  ? r4/.hg/cache/checklink-target (glob)
>    2 r4/.hg/cache/checknoexec
>    2 r4/.hg/cache/rbc-names-v1
>    2 r4/.hg/cache/rbc-revs-v1
> @@ -250,6 +252,7 @@
>    1 r4/.hg/branch
>    2 r4/.hg/cache/branch2-served
>    2 r4/.hg/cache/checkisexec
> +  2 r4/.hg/cache/checklink-target
>    2 r4/.hg/cache/checknoexec
>    2 r4/.hg/cache/rbc-names-v1
>    2 r4/.hg/cache/rbc-revs-v1
> diff --git a/tests/test-tags.t b/tests/test-tags.t
> --- a/tests/test-tags.t
> +++ b/tests/test-tags.t
> @@ -674,6 +674,7 @@
>    branch2-served
>    checkisexec
>    checklink
> +  checklink-target
>    hgtagsfnodes1
>    rbc-names-v1
>    rbc-revs-v1
> @@ -700,6 +701,7 @@
>    branch2-served
>    checkisexec
>    checklink
> +  checklink-target
>    hgtagsfnodes1
>    rbc-names-v1
>    rbc-revs-v1
Augie Fackler - Dec. 2, 2016, 8:50 p.m.
On Wed, Nov 30, 2016 at 04:58:28PM +0000, Jun Wu wrote:
> Looks good to me. Thanks for the fix!
> A small nit inline - probably could be fixed on flight.
>
> Excerpts from Martijn Pieters's message of 2016-11-30 16:51:41 +0000:
> > # HG changeset patch
> > # User Martijn Pieters <mjpieters@fb.com>
> > # Date 1480523976 0
> > #      Wed Nov 30 16:39:36 2016 +0000
> > # Node ID 4053f60f77657f8f54afc72d00bf629b75d0b4b9
> > # Parent  9e29d4e4e08b5996adda49cdd0b497d89e2b16ee
> > posix: give the cached symlink a real target
> >
> > The NamedTemporaryFile file is cleared up so checklink ends up as a dangling
> > symlink, causing cp -r in tests to complain on both Solaris and OS X. Use
> > a permanent file instead when there is a .hg/cache directory.
> >
> > diff --git a/mercurial/posix.py b/mercurial/posix.py
> > --- a/mercurial/posix.py
> > +++ b/mercurial/posix.py
> > @@ -231,10 +231,18 @@
> >              cachedir = None
> >          name = tempfile.mktemp(dir=checkdir, prefix='checklink-')
> >          try:
> > -            fd = tempfile.NamedTemporaryFile(dir=checkdir,
> > -                                             prefix='hg-checklink-')
> > +            fd = None
> > +            if cachedir is None:
> > +                fd = tempfile.NamedTemporaryFile(dir=checkdir,
> > +                                                 prefix='hg-checklink-')
> > +                target = os.path.basename(fd.name)
> > +            else:
> > +                # create a fixed file to link to; doesn't matter if it
> > +                # already exists.
> > +                target = 'checklink-target'
> > +                open(os.path.join(cachedir, target), 'w')
>
> Should we append ".close()" here to just make things explicit?

Indeed we should, this risks file handle leaks on pypy et al. Queued
with .close() added in flight.

>
> >              try:
> > -                os.symlink(os.path.basename(fd.name), name)
> > +                os.symlink(target, name)
> >                  if cachedir is None:
> >                      os.unlink(name)
> >                  else:
> > @@ -249,7 +257,8 @@
> >                      continue
> >                  raise
> >              finally:
> > -                fd.close()
> > +                if fd is not None:
> > +                    fd.close()
> >          except AttributeError:
> >              return False
> >          except OSError as inst:
> > diff --git a/tests/test-clone.t b/tests/test-clone.t
> > --- a/tests/test-clone.t
> > +++ b/tests/test-clone.t
> > @@ -33,6 +33,7 @@
> >    branch2-served
> >    checkisexec
> >    checklink
> > +  checklink-target
> >    checknoexec
> >    rbc-names-v1
> >    rbc-revs-v1
> > @@ -50,6 +51,7 @@
> >    branch2-served
> >    checkisexec
> >    checklink
> > +  checklink-target
> >
> >    $ cat a
> >    a
> > diff --git a/tests/test-hardlinks.t b/tests/test-hardlinks.t
> > --- a/tests/test-hardlinks.t
> > +++ b/tests/test-hardlinks.t
> > @@ -212,6 +212,8 @@
> >    2 r4/.hg/branch
> >    2 r4/.hg/cache/branch2-served
> >    2 r4/.hg/cache/checkisexec
> > +  3 r4/.hg/cache/checklink (?)
> > +  ? r4/.hg/cache/checklink-target (glob)
> >    2 r4/.hg/cache/checknoexec
> >    2 r4/.hg/cache/rbc-names-v1
> >    2 r4/.hg/cache/rbc-revs-v1
> > @@ -250,6 +252,7 @@
> >    1 r4/.hg/branch
> >    2 r4/.hg/cache/branch2-served
> >    2 r4/.hg/cache/checkisexec
> > +  2 r4/.hg/cache/checklink-target
> >    2 r4/.hg/cache/checknoexec
> >    2 r4/.hg/cache/rbc-names-v1
> >    2 r4/.hg/cache/rbc-revs-v1
> > diff --git a/tests/test-tags.t b/tests/test-tags.t
> > --- a/tests/test-tags.t
> > +++ b/tests/test-tags.t
> > @@ -674,6 +674,7 @@
> >    branch2-served
> >    checkisexec
> >    checklink
> > +  checklink-target
> >    hgtagsfnodes1
> >    rbc-names-v1
> >    rbc-revs-v1
> > @@ -700,6 +701,7 @@
> >    branch2-served
> >    checkisexec
> >    checklink
> > +  checklink-target
> >    hgtagsfnodes1
> >    rbc-names-v1
> >    rbc-revs-v1
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/posix.py b/mercurial/posix.py
--- a/mercurial/posix.py
+++ b/mercurial/posix.py
@@ -231,10 +231,18 @@ 
             cachedir = None
         name = tempfile.mktemp(dir=checkdir, prefix='checklink-')
         try:
-            fd = tempfile.NamedTemporaryFile(dir=checkdir,
-                                             prefix='hg-checklink-')
+            fd = None
+            if cachedir is None:
+                fd = tempfile.NamedTemporaryFile(dir=checkdir,
+                                                 prefix='hg-checklink-')
+                target = os.path.basename(fd.name)
+            else:
+                # create a fixed file to link to; doesn't matter if it
+                # already exists.
+                target = 'checklink-target'
+                open(os.path.join(cachedir, target), 'w')
             try:
-                os.symlink(os.path.basename(fd.name), name)
+                os.symlink(target, name)
                 if cachedir is None:
                     os.unlink(name)
                 else:
@@ -249,7 +257,8 @@ 
                     continue
                 raise
             finally:
-                fd.close()
+                if fd is not None:
+                    fd.close()
         except AttributeError:
             return False
         except OSError as inst:
diff --git a/tests/test-clone.t b/tests/test-clone.t
--- a/tests/test-clone.t
+++ b/tests/test-clone.t
@@ -33,6 +33,7 @@ 
   branch2-served
   checkisexec
   checklink
+  checklink-target
   checknoexec
   rbc-names-v1
   rbc-revs-v1
@@ -50,6 +51,7 @@ 
   branch2-served
   checkisexec
   checklink
+  checklink-target
 
   $ cat a
   a
diff --git a/tests/test-hardlinks.t b/tests/test-hardlinks.t
--- a/tests/test-hardlinks.t
+++ b/tests/test-hardlinks.t
@@ -212,6 +212,8 @@ 
   2 r4/.hg/branch
   2 r4/.hg/cache/branch2-served
   2 r4/.hg/cache/checkisexec
+  3 r4/.hg/cache/checklink (?)
+  ? r4/.hg/cache/checklink-target (glob)
   2 r4/.hg/cache/checknoexec
   2 r4/.hg/cache/rbc-names-v1
   2 r4/.hg/cache/rbc-revs-v1
@@ -250,6 +252,7 @@ 
   1 r4/.hg/branch
   2 r4/.hg/cache/branch2-served
   2 r4/.hg/cache/checkisexec
+  2 r4/.hg/cache/checklink-target
   2 r4/.hg/cache/checknoexec
   2 r4/.hg/cache/rbc-names-v1
   2 r4/.hg/cache/rbc-revs-v1
diff --git a/tests/test-tags.t b/tests/test-tags.t
--- a/tests/test-tags.t
+++ b/tests/test-tags.t
@@ -674,6 +674,7 @@ 
   branch2-served
   checkisexec
   checklink
+  checklink-target
   hgtagsfnodes1
   rbc-names-v1
   rbc-revs-v1
@@ -700,6 +701,7 @@ 
   branch2-served
   checkisexec
   checklink
+  checklink-target
   hgtagsfnodes1
   rbc-names-v1
   rbc-revs-v1