Patchwork [STABLE,v2] chmod: create a new file when flags are set on a hardlinked file

login
register
mail settings
Submitter Koen Van Hoof
Date May 3, 2017, 9:36 a.m.
Message ID <04b33b356260d9d4c27f.1493804183@devws223>
Download mbox | patch
Permalink /patch/20403/
State Changes Requested
Headers show

Comments

Koen Van Hoof - May 3, 2017, 9:36 a.m.
# HG changeset patch
# User Koen Van Hoof <koen.van_hoof@nokia.com>
# Date 1493215522 -7200
#      Wed Apr 26 16:05:22 2017 +0200
# Branch stable
# Node ID 04b33b356260d9d4c27f8cd223c7e5446a51ac48
# Parent  2de67783dd31c66be1677aa443b95eabb0129a75
chmod: create a new file when flags are set on a hardlinked file

For performance reasons we have several repositories where the files in the working
directory of 1 repo are hardlinks to the files of the other repo
When an update in one repo results in a chmod of a such a file, the hardlink
has to be deleted and replaced by a regular file to make sure that the change
does not happen in the other repo
Gregory Szorc - May 4, 2017, 6:58 a.m.
On Wed, May 3, 2017 at 2:36 AM, Koen Van Hoof <koen.van_hoof@nokia.com>
wrote:

> # HG changeset patch
> # User Koen Van Hoof <koen.van_hoof@nokia.com>
> # Date 1493215522 -7200
> #      Wed Apr 26 16:05:22 2017 +0200
> # Branch stable
> # Node ID 04b33b356260d9d4c27f8cd223c7e5446a51ac48
> # Parent  2de67783dd31c66be1677aa443b95eabb0129a75
> chmod: create a new file when flags are set on a hardlinked file
>
> For performance reasons we have several repositories where the files in
> the working
> directory of 1 repo are hardlinks to the files of the other repo
> When an update in one repo results in a chmod of a such a file, the
> hardlink
> has to be deleted and replaced by a regular file to make sure that the
> change
> does not happen in the other repo
>
> diff --git a/mercurial/posix.py b/mercurial/posix.py
> --- a/mercurial/posix.py
> +++ b/mercurial/posix.py
> @@ -98,7 +98,8 @@ def isexec(f):
>      return (os.lstat(f).st_mode & 0o100 != 0)
>
>  def setflags(f, l, x):
> -    s = os.lstat(f).st_mode
> +    st = os.lstat(f)
> +    s = st.st_mode
>      if l:
>          if not stat.S_ISLNK(s):
>              # switch file to link
> @@ -124,6 +125,14 @@ def setflags(f, l, x):
>          fp.close()
>          s = 0o666 & ~umask # avoid restatting for chmod
>
> +    if st.st_nlink > 1: # the file is a hardlink, break the hardlink
> +        fpin = open(f,"r")
> +        unlink(f)
> +        fpout = open(f, "w")
> +        fpout.write(fpin.read())
> +        fpin.close()
> +        fpout.close()
> +
>

I'll let someone else comment on the semantics of the change. But this
should use context managers so file descriptors aren't leaked [until next
garbage collection] in case of an exception.


>      sx = s & 0o100
>      if x and not sx:
>          # Turn on +x for every +r bit when making a file executable
> @@ -132,6 +141,8 @@ def setflags(f, l, x):
>      elif not x and sx:
>          # Turn off all +x bits
>          os.chmod(f, s & 0o666)
> +    elif st.st_nlink > 1: # new file still needs its stat to be copied
> +        os.chmod(f, s)
>
>  def copymode(src, dst, mode=None):
>      '''Copy the file mode from the file at path src to dst.
> diff --git a/tests/test-hardlinks.t b/tests/test-hardlinks.t
> --- a/tests/test-hardlinks.t
> +++ b/tests/test-hardlinks.t
> @@ -193,11 +193,18 @@ Committing a change to f1 in r1 must bre
>    1 r2/.hg/store/data/f1.i
>    [12] r2/\.hg/store/fncache (re)
>
> +Create a file which exec permissions we will change
> +  $ cd r3
> +  $ echo "echo hello world" > f3
> +  $ hg add f3
> +  $ hg ci -mf3
> +  $ cd ..
>
>    $ cd r3
>    $ hg tip --template '{rev}:{node|short}\n'
> -  11:a6451b6bc41f
> +  12:d3b77733a28a
>    $ echo bla > f1
> +  $ chmod +x f3
>    $ hg ci -m1
>    $ cd ..
>
> @@ -226,6 +233,7 @@ r4 has hardlinks in the working dir (not
>    2 r4/.hg/store/data/d1/f2.d
>    2 r4/.hg/store/data/d1/f2.i
>    2 r4/.hg/store/data/f1.i
> +  2 r4/.hg/store/data/f3.i
>    2 r4/.hg/store/fncache
>    2 r4/.hg/store/phaseroots
>    2 r4/.hg/store/undo
> @@ -241,11 +249,12 @@ r4 has hardlinks in the working dir (not
>    2 r4/d1/data1
>    2 r4/d1/f2
>    2 r4/f1
> +  2 r4/f3
>
> -Update back to revision 11 in r4 should break hardlink of file f1:
> +Update back to revision 12 in r4 should break hardlink of file f1 and f3:
>
> -  $ hg -R r4 up 11
> -  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  $ hg -R r4 up 12
> +  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
>
>    $ nlinksdir r4
>    2 r4/.hg/00changelog.i
> @@ -265,6 +274,7 @@ Update back to revision 11 in r4 should
>    2 r4/.hg/store/data/d1/f2.d
>    2 r4/.hg/store/data/d1/f2.i
>    2 r4/.hg/store/data/f1.i
> +  2 r4/.hg/store/data/f3.i
>    2 r4/.hg/store/fncache
>    2 r4/.hg/store/phaseroots
>    2 r4/.hg/store/undo
> @@ -280,6 +290,7 @@ Update back to revision 11 in r4 should
>    2 r4/d1/data1
>    2 r4/d1/f2
>    1 r4/f1
> +  1 r4/f3
>
>
>  Test hardlinking outside hg:
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
Augie Fackler - May 6, 2017, 4:54 p.m.
On Wed, May 03, 2017 at 11:58:17PM -0700, Gregory Szorc wrote:
> On Wed, May 3, 2017 at 2:36 AM, Koen Van Hoof <koen.van_hoof@nokia.com>
> wrote:
>
> > # HG changeset patch
> > # User Koen Van Hoof <koen.van_hoof@nokia.com>
> > # Date 1493215522 -7200
> > #      Wed Apr 26 16:05:22 2017 +0200
> > # Branch stable
> > # Node ID 04b33b356260d9d4c27f8cd223c7e5446a51ac48
> > # Parent  2de67783dd31c66be1677aa443b95eabb0129a75
> > chmod: create a new file when flags are set on a hardlinked file
> >
> > For performance reasons we have several repositories where the files in
> > the working
> > directory of 1 repo are hardlinks to the files of the other repo
> > When an update in one repo results in a chmod of a such a file, the
> > hardlink
> > has to be deleted and replaced by a regular file to make sure that the
> > change
> > does not happen in the other repo
> >
> > diff --git a/mercurial/posix.py b/mercurial/posix.py
> > --- a/mercurial/posix.py
> > +++ b/mercurial/posix.py
> > @@ -98,7 +98,8 @@ def isexec(f):
> >      return (os.lstat(f).st_mode & 0o100 != 0)
> >
> >  def setflags(f, l, x):
> > -    s = os.lstat(f).st_mode
> > +    st = os.lstat(f)
> > +    s = st.st_mode
> >      if l:
> >          if not stat.S_ISLNK(s):
> >              # switch file to link
> > @@ -124,6 +125,14 @@ def setflags(f, l, x):
> >          fp.close()
> >          s = 0o666 & ~umask # avoid restatting for chmod
> >
> > +    if st.st_nlink > 1: # the file is a hardlink, break the hardlink
> > +        fpin = open(f,"r")
> > +        unlink(f)
> > +        fpout = open(f, "w")
> > +        fpout.write(fpin.read())
> > +        fpin.close()
> > +        fpout.close()
> > +
> >
>
> I'll let someone else comment on the semantics of the change. But this
> should use context managers so file descriptors aren't leaked [until next
> garbage collection] in case of an exception.

I'm generally okay with the semantics of the patch - it was surprising
to me as a long-standing source control hacker that this
misbehaves. Sorry I didn't notice the context manager bit in the last
cycle.

(If anyone has objections to these semantics, please speak up.)

>
>
> >      sx = s & 0o100
> >      if x and not sx:
> >          # Turn on +x for every +r bit when making a file executable
> > @@ -132,6 +141,8 @@ def setflags(f, l, x):
> >      elif not x and sx:
> >          # Turn off all +x bits
> >          os.chmod(f, s & 0o666)
> > +    elif st.st_nlink > 1: # new file still needs its stat to be copied
> > +        os.chmod(f, s)
> >
> >  def copymode(src, dst, mode=None):
> >      '''Copy the file mode from the file at path src to dst.
> > diff --git a/tests/test-hardlinks.t b/tests/test-hardlinks.t
> > --- a/tests/test-hardlinks.t
> > +++ b/tests/test-hardlinks.t
> > @@ -193,11 +193,18 @@ Committing a change to f1 in r1 must bre
> >    1 r2/.hg/store/data/f1.i
> >    [12] r2/\.hg/store/fncache (re)
> >
> > +Create a file which exec permissions we will change
> > +  $ cd r3
> > +  $ echo "echo hello world" > f3
> > +  $ hg add f3
> > +  $ hg ci -mf3
> > +  $ cd ..
> >
> >    $ cd r3
> >    $ hg tip --template '{rev}:{node|short}\n'
> > -  11:a6451b6bc41f
> > +  12:d3b77733a28a
> >    $ echo bla > f1
> > +  $ chmod +x f3
> >    $ hg ci -m1
> >    $ cd ..
> >
> > @@ -226,6 +233,7 @@ r4 has hardlinks in the working dir (not
> >    2 r4/.hg/store/data/d1/f2.d
> >    2 r4/.hg/store/data/d1/f2.i
> >    2 r4/.hg/store/data/f1.i
> > +  2 r4/.hg/store/data/f3.i
> >    2 r4/.hg/store/fncache
> >    2 r4/.hg/store/phaseroots
> >    2 r4/.hg/store/undo
> > @@ -241,11 +249,12 @@ r4 has hardlinks in the working dir (not
> >    2 r4/d1/data1
> >    2 r4/d1/f2
> >    2 r4/f1
> > +  2 r4/f3
> >
> > -Update back to revision 11 in r4 should break hardlink of file f1:
> > +Update back to revision 12 in r4 should break hardlink of file f1 and f3:
> >
> > -  $ hg -R r4 up 11
> > -  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> > +  $ hg -R r4 up 12
> > +  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
> >
> >    $ nlinksdir r4
> >    2 r4/.hg/00changelog.i
> > @@ -265,6 +274,7 @@ Update back to revision 11 in r4 should
> >    2 r4/.hg/store/data/d1/f2.d
> >    2 r4/.hg/store/data/d1/f2.i
> >    2 r4/.hg/store/data/f1.i
> > +  2 r4/.hg/store/data/f3.i
> >    2 r4/.hg/store/fncache
> >    2 r4/.hg/store/phaseroots
> >    2 r4/.hg/store/undo
> > @@ -280,6 +290,7 @@ Update back to revision 11 in r4 should
> >    2 r4/d1/data1
> >    2 r4/d1/f2
> >    1 r4/f1
> > +  1 r4/f3
> >
> >
> >  Test hardlinking outside hg:
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Yuya Nishihara - May 7, 2017, 3:41 a.m.
On Sat, 6 May 2017 12:54:41 -0400, Augie Fackler wrote:
> On Wed, May 03, 2017 at 11:58:17PM -0700, Gregory Szorc wrote:
> > On Wed, May 3, 2017 at 2:36 AM, Koen Van Hoof <koen.van_hoof@nokia.com>
> > wrote:
> >
> > > # HG changeset patch
> > > # User Koen Van Hoof <koen.van_hoof@nokia.com>
> > > # Date 1493215522 -7200
> > > #      Wed Apr 26 16:05:22 2017 +0200
> > > # Branch stable
> > > # Node ID 04b33b356260d9d4c27f8cd223c7e5446a51ac48
> > > # Parent  2de67783dd31c66be1677aa443b95eabb0129a75
> > > chmod: create a new file when flags are set on a hardlinked file
> > >
> > > For performance reasons we have several repositories where the files in
> > > the working
> > > directory of 1 repo are hardlinks to the files of the other repo
> > > When an update in one repo results in a chmod of a such a file, the
> > > hardlink
> > > has to be deleted and replaced by a regular file to make sure that the
> > > change
> > > does not happen in the other repo
> > >
> > > diff --git a/mercurial/posix.py b/mercurial/posix.py
> > > --- a/mercurial/posix.py
> > > +++ b/mercurial/posix.py
> > > @@ -98,7 +98,8 @@ def isexec(f):
> > >      return (os.lstat(f).st_mode & 0o100 != 0)
> > >
> > >  def setflags(f, l, x):
> > > -    s = os.lstat(f).st_mode
> > > +    st = os.lstat(f)
> > > +    s = st.st_mode
> > >      if l:
> > >          if not stat.S_ISLNK(s):
> > >              # switch file to link
> > > @@ -124,6 +125,14 @@ def setflags(f, l, x):
> > >          fp.close()
> > >          s = 0o666 & ~umask # avoid restatting for chmod
> > >
> > > +    if st.st_nlink > 1: # the file is a hardlink, break the hardlink

Perhaps we should break the link only if bool(x) != bool(sx).

> > > +        fpin = open(f,"r")
> > > +        unlink(f)
> > > +        fpout = open(f, "w")
> > > +        fpout.write(fpin.read())
> > > +        fpin.close()
> > > +        fpout.close()
> > > +
> > >
> >
> > I'll let someone else comment on the semantics of the change. But this
> > should use context managers so file descriptors aren't leaked [until next
> > garbage collection] in case of an exception.
> 
> I'm generally okay with the semantics of the patch - it was surprising
> to me as a long-standing source control hacker that this
> misbehaves. Sorry I didn't notice the context manager bit in the last
> cycle.
> 
> (If anyone has objections to these semantics, please speak up.)

I think both old/new semantics are okay. The core Mercurial shouldn't be
affected by this change. We don't do atomic write on working-copy files,
so hardlinks in wdir would be a problem anyway.

Patch

diff --git a/mercurial/posix.py b/mercurial/posix.py
--- a/mercurial/posix.py
+++ b/mercurial/posix.py
@@ -98,7 +98,8 @@  def isexec(f):
     return (os.lstat(f).st_mode & 0o100 != 0)
 
 def setflags(f, l, x):
-    s = os.lstat(f).st_mode
+    st = os.lstat(f)
+    s = st.st_mode
     if l:
         if not stat.S_ISLNK(s):
             # switch file to link
@@ -124,6 +125,14 @@  def setflags(f, l, x):
         fp.close()
         s = 0o666 & ~umask # avoid restatting for chmod
 
+    if st.st_nlink > 1: # the file is a hardlink, break the hardlink
+        fpin = open(f,"r")
+        unlink(f)
+        fpout = open(f, "w")
+        fpout.write(fpin.read())
+        fpin.close()
+        fpout.close()
+
     sx = s & 0o100
     if x and not sx:
         # Turn on +x for every +r bit when making a file executable
@@ -132,6 +141,8 @@  def setflags(f, l, x):
     elif not x and sx:
         # Turn off all +x bits
         os.chmod(f, s & 0o666)
+    elif st.st_nlink > 1: # new file still needs its stat to be copied
+        os.chmod(f, s)
 
 def copymode(src, dst, mode=None):
     '''Copy the file mode from the file at path src to dst.
diff --git a/tests/test-hardlinks.t b/tests/test-hardlinks.t
--- a/tests/test-hardlinks.t
+++ b/tests/test-hardlinks.t
@@ -193,11 +193,18 @@  Committing a change to f1 in r1 must bre
   1 r2/.hg/store/data/f1.i
   [12] r2/\.hg/store/fncache (re)
 
+Create a file which exec permissions we will change
+  $ cd r3
+  $ echo "echo hello world" > f3
+  $ hg add f3
+  $ hg ci -mf3
+  $ cd ..
 
   $ cd r3
   $ hg tip --template '{rev}:{node|short}\n'
-  11:a6451b6bc41f
+  12:d3b77733a28a
   $ echo bla > f1
+  $ chmod +x f3
   $ hg ci -m1
   $ cd ..
 
@@ -226,6 +233,7 @@  r4 has hardlinks in the working dir (not
   2 r4/.hg/store/data/d1/f2.d
   2 r4/.hg/store/data/d1/f2.i
   2 r4/.hg/store/data/f1.i
+  2 r4/.hg/store/data/f3.i
   2 r4/.hg/store/fncache
   2 r4/.hg/store/phaseroots
   2 r4/.hg/store/undo
@@ -241,11 +249,12 @@  r4 has hardlinks in the working dir (not
   2 r4/d1/data1
   2 r4/d1/f2
   2 r4/f1
+  2 r4/f3
 
-Update back to revision 11 in r4 should break hardlink of file f1:
+Update back to revision 12 in r4 should break hardlink of file f1 and f3:
 
-  $ hg -R r4 up 11
-  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  $ hg -R r4 up 12
+  2 files updated, 0 files merged, 0 files removed, 0 files unresolved
 
   $ nlinksdir r4
   2 r4/.hg/00changelog.i
@@ -265,6 +274,7 @@  Update back to revision 11 in r4 should 
   2 r4/.hg/store/data/d1/f2.d
   2 r4/.hg/store/data/d1/f2.i
   2 r4/.hg/store/data/f1.i
+  2 r4/.hg/store/data/f3.i
   2 r4/.hg/store/fncache
   2 r4/.hg/store/phaseroots
   2 r4/.hg/store/undo
@@ -280,6 +290,7 @@  Update back to revision 11 in r4 should 
   2 r4/d1/data1
   2 r4/d1/f2
   1 r4/f1
+  1 r4/f3
 
 
 Test hardlinking outside hg: