Patchwork update: fix edge-case with update.atomic-file and read-only files

# HG changeset patch
# User Boris Feld <boris.feld@octobus.net>
# Date 1547128621 -3600
#      Thu Jan 10 14:57:01 2019 +0100
# Node ID 9d5ed4b088da0ece901b80c0fa40177cc6eaf676
# Parent  963462786f6e028563bcedc9008622e0f3b59c86
# EXP-Topic atomic-update-read-only
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 9d5ed4b088da
update: fix edge-case with update.atomic-file and read-only files

We used to create the tempfile with the original file mode. That means
creating a read-only tempfile when the original file is read-only, which crash
if we need to write on the tempfile.

The file in the working directory ends up being writable with and without the
atomic update config, so the behavior is the same.

On Fri, Jan 11, 2019 at 7:26 AM Boris Feld <boris.feld@octobus.net> wrote:

> # HG changeset patch
> # User Boris Feld <boris.feld@octobus.net>
> # Date 1547128621 -3600
> #      Thu Jan 10 14:57:01 2019 +0100
> # Node ID 9d5ed4b088da0ece901b80c0fa40177cc6eaf676
> # Parent  963462786f6e028563bcedc9008622e0f3b59c86
> # EXP-Topic atomic-update-read-only
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r
> 9d5ed4b088da
> update: fix edge-case with update.atomic-file and read-only files
>
> We used to create the tempfile with the original file mode. That means
> creating a read-only tempfile when the original file is read-only, which
> crash
> if we need to write on the tempfile.
>
> The file in the working directory ends up being writable with and without
> the
> atomic update config, so the behavior is the same.
>
> diff --git a/mercurial/util.py b/mercurial/util.py
> --- a/mercurial/util.py
> +++ b/mercurial/util.py
> @@ -48,6 +48,7 @@ from . import (
>      policy,
>      pycompat,
>      urllibcompat,
> +    util
>

Delete.


>  )
>  from .utils import (
>      procutil,
> @@ -2205,6 +2206,14 @@ class atomictempfile(object):
>          self.__name = name      # permanent name
>          self._tempname = mktempcopy(name, emptyok=('w' in mode),
>                                      createmode=createmode)
> +
> +        # If we are gonna write on the tempfile, we need to make sure we
> have
> +        # the permission to do so
> +        if 'w' in mode and os.path.isfile(name):
> +            oldstat = filestat.frompath(name)
> +            newstat = oldstat.stat.st_mode | stat.S_IWUSR
> +            os.chmod(self._tempname, newstat)
> +
>          self._fp = posixfile(self._tempname, mode)
>          self._checkambig = checkambig
>
> diff --git a/tests/test-update-atomic.t b/tests/test-update-atomic.t
> new file mode 100644
> --- /dev/null
> +++ b/tests/test-update-atomic.t
> @@ -0,0 +1,124 @@
> +Checking that experimental.atomic-file works.
> +
> +  $ hg init .
> +  $ cat > .hg/showwrites.py <<EOF
> +  > def uisetup(ui):
> +  >   from mercurial import vfs
> +  >   class newvfs(vfs.vfs):
> +  >     def __call__(self, *args, **kwargs):
> +  >       print('vfs open', args, sorted(list(kwargs.items())))
> +  >       return super(newvfs, self).__call__(*args, **kwargs)
> +  >   vfs.vfs = newvfs
> +  > EOF
> +  $ for v in a1 a2 b1 b2 c ro; do echo $v > $v; done
> +  $ chmod +x b*
>

I think you need "#require execbit" for this.


> +  $ hg commit -Aqm _
> +
> +# We check that
> +# - the changes are actually atomic
> +# - that permissions are correct (all 4 cases of (executable before) *
> (executable after))
> +# - that renames work, though they should be atomic anyway
> +# - that it works when source files are read-only (but directories are
> read-write still)
> +
> +  $ for v in a1 a2 b1 b2 ro; do echo changed-$v > $v; done
> +  $ chmod -x *1; chmod +x *2
> +  $ hg rename c d
> +  $ hg commit -qm _
> +
> +Check behavior without update.atomic-file
> +
> +  $ hg update -r 0 -q
> +  $ hg update -r 1 --config extensions.showwrites=.hg/showwrites.py |&
> grep "a1'.*wb"
>

test-check-code.t fails:

@@ -15,6 +15,34 @@
   Skipping i18n/polib.py it has no-che?k-code (glob)
   Skipping mercurial/statprof.py it has no-che?k-code (glob)
   Skipping tests/badserverext.py it has no-che?k-code (glob)
+  tests/test-update-atomic.t:31:
+   >   $ hg update -r 1 --config extensions.showwrites=.hg/showwrites.py
|& grep "a1'.*wb"
+   don't use |&, use 2>&1
+  tests/test-update-atomic.t:34:
+   >   $ find * -printf "%f:%p:%m\n"
+   don't use 'find -printf', it doesn't exist on BSD find(1)
+  tests/test-update-atomic.t:56:
+   >   $ find * -printf "%f:%p:%m\n"
+   don't use 'find -printf', it doesn't exist on BSD find(1)
+  tests/test-update-atomic.t:68:
+   >   $ find ro -printf "%f:%p:%m\n"
+   don't use 'find -printf', it doesn't exist on BSD find(1)
+  tests/test-update-atomic.t:76:
+   >   $ find ro -printf "%f:%p:%m\n"
+   don't use 'find -printf', it doesn't exist on BSD find(1)
+  tests/test-update-atomic.t:90:
+   >   $ hg update -r 1 --config extensions.showwrites=.hg/showwrites.py
|& grep "a1'.*wb"
+   don't use |&, use 2>&1
+  tests/test-update-atomic.t:101:
+   >   $ find * -printf "%f:%p:%m\n"
+   don't use 'find -printf', it doesn't exist on BSD find(1)
+  tests/test-update-atomic.t:113:
+   >   $ find ro -printf "%f:%p:%m\n"
+   don't use 'find -printf', it doesn't exist on BSD find(1)
+  tests/test-update-atomic.t:121:
+   >   $ find ro -printf "%f:%p:%m\n"
+   don't use 'find -printf', it doesn't exist on BSD find(1)
+  [1]


> +  ('vfs open', ('a1', 'wb'), [('atomictemp', False), ('backgroundclose',
> True)])
> +
> +  $ find * -printf "%f:%p:%m\n"
> +  a1:a1:644
> +  a2:a2:755
> +  b1:b1:644
> +  b2:b2:755
> +  d:d:644
> +  ro:ro:644
> +
> +Add a second revision for the ro file so we can test update when the file
> is
> +present or not
> +
> +  $ echo "ro" > ro
> +
> +  $ hg commit -qm _
> +
> +Check behavior without update.atomic-file first
> +
> +  $ hg update -C -r 0 -q
> +
> +  $ hg update -r 1
> +  6 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +
> +  $ find * -printf "%f:%p:%m\n"
> +  a1:a1:644
> +  a2:a2:755
> +  b1:b1:644
> +  b2:b2:755
> +  d:d:644
> +  ro:ro:644
> +
> +Manually reset the mode of the read-only file
>

Can we also have a test case where a read-only file is committed (so
updating to it should create it in read-only mode)?


> +
> +  $ chmod a-w ro
> +
> +  $ find ro -printf "%f:%p:%m\n"
> +  ro:ro:444
> +
> +Now the file is present and read-only
>

Unlike most other comments, this one seems to be about the state *above*
the comment, not about what it's testing below. Could you make the comments
consistently say what the next few lines do instead of what the previous
few lines did?

+
> +  $ hg up -r 2
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +
> +  $ find ro -printf "%f:%p:%m\n"
> +  ro:ro:644
> +
> +# The file which was read-only is now writable in the default behavior
> +
> +Check behavior with update.atomic-files
> +
> +
> +  $ cat >> .hg/hgrc <<EOF
> +  > [experimental]
> +  > update.atomic-file = true
> +  > EOF
> +
> +  $ hg update -C -r 0 -q
> +  $ hg update -r 1 --config extensions.showwrites=.hg/showwrites.py |&
> grep "a1'.*wb"
> +  ('vfs open', ('a1', 'wb'), [('atomictemp', True), ('backgroundclose',
> True)])
> +  $ hg st -A --rev 1
> +  C a1
> +  C a2
> +  C b1
> +  C b2
> +  C d
> +  C ro
> +
> +Check the file permission after update
> +  $ find * -printf "%f:%p:%m\n"
> +  a1:a1:644
> +  a2:a2:755
> +  b1:b1:644
> +  b2:b2:755
> +  d:d:644
> +  ro:ro:644
> +
> +Manually reset the mode of the read-only file
> +
> +  $ chmod a-w ro
> +
> +  $ find ro -printf "%f:%p:%m\n"
> +  ro:ro:444
> +
> +Now the file is present and read-only
> +
> +  $ hg update -r 2
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +
> +  $ find ro -printf "%f:%p:%m\n"
> +  ro:ro:644
> +
> +# The behavior is the same as without atomic update
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

>          self._tempname = mktempcopy(name, emptyok=('w' in mode),
>                                      createmode=createmode)
> +
> +        # If we are gonna write on the tempfile, we need to make sure we have
> +        # the permission to do so
> +        if 'w' in mode and os.path.isfile(name):
> +            oldstat = filestat.frompath(name)
> +            newstat = oldstat.stat.st_mode | stat.S_IWUSR
> +            os.chmod(self._tempname, newstat)

Isn't it better to add an option for mktempcopy() to forcibly flag S_IWUSR
bit on?

Thank you for the thorough review

On 11/01/2019 18:24, Martin von Zweigbergk via Mercurial-devel wrote:
>
>
> On Fri, Jan 11, 2019 at 7:26 AM Boris Feld <boris.feld@octobus.net
> <mailto:boris.feld@octobus.net>> wrote:
>
>     # HG changeset patch
>     # User Boris Feld <boris.feld@octobus.net
>     <mailto:boris.feld@octobus.net>>
>     # Date 1547128621 -3600
>     #      Thu Jan 10 14:57:01 2019 +0100
>     # Node ID 9d5ed4b088da0ece901b80c0fa40177cc6eaf676
>     # Parent  963462786f6e028563bcedc9008622e0f3b59c86
>     # EXP-Topic atomic-update-read-only
>     # Available At https://bitbucket.org/octobus/mercurial-devel/
>     #              hg pull
>     https://bitbucket.org/octobus/mercurial-devel/ -r 9d5ed4b088da
>     update: fix edge-case with update.atomic-file and read-only files
>
>     We used to create the tempfile with the original file mode. That means
>     creating a read-only tempfile when the original file is read-only,
>     which crash
>     if we need to write on the tempfile.
>
>     The file in the working directory ends up being writable with and
>     without the
>     atomic update config, so the behavior is the same.
>
>     diff --git a/mercurial/util.py b/mercurial/util.py
>     --- a/mercurial/util.py
>     +++ b/mercurial/util.py
>     @@ -48,6 +48,7 @@ from . import (
>          policy,
>          pycompat,
>          urllibcompat,
>     +    util
>
>
> Delete.
>  
>
>      )
>      from .utils import (
>          procutil,
>     @@ -2205,6 +2206,14 @@ class atomictempfile(object):
>              self.__name = name      # permanent name
>              self._tempname = mktempcopy(name, emptyok=('w' in mode),
>                                          createmode=createmode)
>     +
>     +        # If we are gonna write on the tempfile, we need to make
>     sure we have
>     +        # the permission to do so
>     +        if 'w' in mode and os.path.isfile(name):
>     +            oldstat = filestat.frompath(name)
>     +            newstat = oldstat.stat.st_mode | stat.S_IWUSR
>     +            os.chmod(self._tempname, newstat)
>     +
>              self._fp = posixfile(self._tempname, mode)
>              self._checkambig = checkambig
>
>     diff --git a/tests/test-update-atomic.t b/tests/test-update-atomic.t
>     new file mode 100644
>     --- /dev/null
>     +++ b/tests/test-update-atomic.t
>     @@ -0,0 +1,124 @@
>     +Checking that experimental.atomic-file works.
>     +
>     +  $ hg init .
>     +  $ cat > .hg/showwrites.py <<EOF
>     +  > def uisetup(ui):
>     +  >   from mercurial import vfs
>     +  >   class newvfs(vfs.vfs):
>     +  >     def __call__(self, *args, **kwargs):
>     +  >       print('vfs open', args, sorted(list(kwargs.items())))
>     +  >       return super(newvfs, self).__call__(*args, **kwargs)
>     +  >   vfs.vfs = newvfs
>     +  > EOF
>     +  $ for v in a1 a2 b1 b2 c ro; do echo $v > $v; done
>     +  $ chmod +x b*
>
>
> I think you need "#require execbit" for this.
Indeed, nice catch
>  
>
>     +  $ hg commit -Aqm _
>     +
>     +# We check that
>     +# - the changes are actually atomic
>     +# - that permissions are correct (all 4 cases of (executable
>     before) * (executable after))
>     +# - that renames work, though they should be atomic anyway
>     +# - that it works when source files are read-only (but
>     directories are read-write still)
>     +
>     +  $ for v in a1 a2 b1 b2 ro; do echo changed-$v > $v; done
>     +  $ chmod -x *1; chmod +x *2
>     +  $ hg rename c d
>     +  $ hg commit -qm _
>     +
>     +Check behavior without update.atomic-file
>     +
>     +  $ hg update -r 0 -q
>     +  $ hg update -r 1 --config
>     extensions.showwrites=.hg/showwrites.py |& grep "a1'.*wb"
>
>
> test-check-code.t fails:
>
> @@ -15,6 +15,34 @@
>    Skipping i18n/polib.py it has no-che?k-code (glob)
>    Skipping mercurial/statprof.py it has no-che?k-code (glob)
>    Skipping tests/badserverext.py it has no-che?k-code (glob)
> +  tests/test-update-atomic.t:31:
> +   >   $ hg update -r 1 --config
> extensions.showwrites=.hg/showwrites.py |& grep "a1'.*wb"
> +   don't use |&, use 2>&1
> +  tests/test-update-atomic.t:34:
> +   >   $ find * -printf "%f:%p:%m\n"
> +   don't use 'find -printf', it doesn't exist on BSD find(1)
> +  tests/test-update-atomic.t:56:
> +   >   $ find * -printf "%f:%p:%m\n"
> +   don't use 'find -printf', it doesn't exist on BSD find(1)
> +  tests/test-update-atomic.t:68:
> +   >   $ find ro -printf "%f:%p:%m\n"
> +   don't use 'find -printf', it doesn't exist on BSD find(1)
> +  tests/test-update-atomic.t:76:
> +   >   $ find ro -printf "%f:%p:%m\n"
> +   don't use 'find -printf', it doesn't exist on BSD find(1)
> +  tests/test-update-atomic.t:90:
> +   >   $ hg update -r 1 --config
> extensions.showwrites=.hg/showwrites.py |& grep "a1'.*wb"
> +   don't use |&, use 2>&1
> +  tests/test-update-atomic.t:101:
> +   >   $ find * -printf "%f:%p:%m\n"
> +   don't use 'find -printf', it doesn't exist on BSD find(1)
> +  tests/test-update-atomic.t:113:
> +   >   $ find ro -printf "%f:%p:%m\n"
> +   don't use 'find -printf', it doesn't exist on BSD find(1)
> +  tests/test-update-atomic.t:121:
> +   >   $ find ro -printf "%f:%p:%m\n"
> +   don't use 'find -printf', it doesn't exist on BSD find(1)
> +  [1]

That's fixed now, thanks

>  
>
>     +  ('vfs open', ('a1', 'wb'), [('atomictemp', False),
>     ('backgroundclose', True)])
>     +
>     +  $ find * -printf "%f:%p:%m\n"
>     +  a1:a1:644
>     +  a2:a2:755
>     +  b1:b1:644
>     +  b2:b2:755
>     +  d:d:644
>     +  ro:ro:644
>     +
>     +Add a second revision for the ro file so we can test update when
>     the file is
>     +present or not
>     +
>     +  $ echo "ro" > ro
>     +
>     +  $ hg commit -qm _
>     +
>     +Check behavior without update.atomic-file first
>     +
>     +  $ hg update -C -r 0 -q
>     +
>     +  $ hg update -r 1
>     +  6 files updated, 0 files merged, 1 files removed, 0 files
>     unresolved
>     +
>     +  $ find * -printf "%f:%p:%m\n"
>     +  a1:a1:644
>     +  a2:a2:755
>     +  b1:b1:644
>     +  b2:b2:755
>     +  d:d:644
>     +  ro:ro:644
>     +
>     +Manually reset the mode of the read-only file
>
>
> Can we also have a test case where a read-only file is committed (so
> updating to it should create it in read-only mode)?

I tried modifying the test case to commit `ro` in read-only for
changeset #2 but it didn't changed a things. I think that Mercurial is
only tracking the exec bit on files. I'm not surprised that the files
ends up writable after an update (with or without atomic temp option).

Do you think it's a bug?

>  
>
>     +
>     +  $ chmod a-w ro
>     +
>     +  $ find ro -printf "%f:%p:%m\n"
>     +  ro:ro:444
>     +
>     +Now the file is present and read-only
>
>
> Unlike most other comments, this one seems to be about the state
> *above* the comment, not about what it's testing below. Could you make
> the comments consistently say what the next few lines do instead of
> what the previous few lines did?
I've clarified the comments
>
>     +
>     +  $ hg up -r 2
>     +  1 files updated, 0 files merged, 0 files removed, 0 files
>     unresolved
>     +
>     +  $ find ro -printf "%f:%p:%m\n"
>     +  ro:ro:644
>     +
>     +# The file which was read-only is now writable in the default
>     behavior
>     +
>     +Check behavior with update.atomic-files
>     +
>     +
>     +  $ cat >> .hg/hgrc <<EOF
>     +  > [experimental]
>     +  > update.atomic-file = true
>     +  > EOF
>     +
>     +  $ hg update -C -r 0 -q
>     +  $ hg update -r 1 --config
>     extensions.showwrites=.hg/showwrites.py |& grep "a1'.*wb"
>     +  ('vfs open', ('a1', 'wb'), [('atomictemp', True),
>     ('backgroundclose', True)])
>     +  $ hg st -A --rev 1
>     +  C a1
>     +  C a2
>     +  C b1
>     +  C b2
>     +  C d
>     +  C ro
>     +
>     +Check the file permission after update
>     +  $ find * -printf "%f:%p:%m\n"
>     +  a1:a1:644
>     +  a2:a2:755
>     +  b1:b1:644
>     +  b2:b2:755
>     +  d:d:644
>     +  ro:ro:644
>     +
>     +Manually reset the mode of the read-only file
>     +
>     +  $ chmod a-w ro
>     +
>     +  $ find ro -printf "%f:%p:%m\n"
>     +  ro:ro:444
>     +
>     +Now the file is present and read-only
>     +
>     +  $ hg update -r 2
>     +  1 files updated, 0 files merged, 0 files removed, 0 files
>     unresolved
>     +
>     +  $ find ro -printf "%f:%p:%m\n"
>     +  ro:ro:644
>     +
>     +# The behavior is the same as without atomic update
>     _______________________________________________
>     Mercurial-devel mailing list
>     Mercurial-devel@mercurial-scm.org
>     <mailto: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

On 12/01/2019 04:32, Yuya Nishihara wrote:
>>          self._tempname = mktempcopy(name, emptyok=('w' in mode),
>>                                      createmode=createmode)
>> +
>> +        # If we are gonna write on the tempfile, we need to make sure we have
>> +        # the permission to do so
>> +        if 'w' in mode and os.path.isfile(name):
>> +            oldstat = filestat.frompath(name)
>> +            newstat = oldstat.stat.st_mode | stat.S_IWUSR
>> +            os.chmod(self._tempname, newstat)
> Isn't it better to add an option for mktempcopy() to forcibly flag S_IWUSR
> bit on?

I was hesitant between the two designs. I give a try to the mktempcopy
modification design thanks to your comment and it seems better.

I'm not familiar with how Windows handle file permissions but I see that
copymode for windows does nothing. Should we also avoid enforcing the
writable bit on Windows platform?

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

On Sun, Jan 13, 2019, 01:26 Boris FELD <boris.feld@octobus.net wrote:

> Thank you for the thorough review
> On 11/01/2019 18:24, Martin von Zweigbergk via Mercurial-devel wrote:
>
>
>
> On Fri, Jan 11, 2019 at 7:26 AM Boris Feld <boris.feld@octobus.net>
> wrote:
>
> +
>> +  $ find * -printf "%f:%p:%m\n"
>> +  a1:a1:644
>> +  a2:a2:755
>> +  b1:b1:644
>> +  b2:b2:755
>> +  d:d:644
>> +  ro:ro:644
>> +
>> +Manually reset the mode of the read-only file
>>
>
> Can we also have a test case where a read-only file is committed (so
> updating to it should create it in read-only mode)?
>
> I tried modifying the test case to commit `ro` in read-only for changeset
> #2 but it didn't changed a things. I think that Mercurial is only tracking
> the exec bit on files. I'm not surprised that the files ends up writable
> after an update (with or without atomic temp option).
>
> Do you think it's a bug?
>
No, just not supported. I should have known that. Sorry to make you waste
time trying that.

Perhaps we should add a test for updating a read-only file when *not* using
atomic-temp. I assume we don't since nothing failed when you ran with
--extra-config-opt. I don't even know how it behaves.

On Sun, 13 Jan 2019 10:28:33 +0100, Boris FELD wrote:
> On 12/01/2019 04:32, Yuya Nishihara wrote:
> >>          self._tempname = mktempcopy(name, emptyok=('w' in mode),
> >>                                      createmode=createmode)
> >> +
> >> +        # If we are gonna write on the tempfile, we need to make sure we have
> >> +        # the permission to do so
> >> +        if 'w' in mode and os.path.isfile(name):
> >> +            oldstat = filestat.frompath(name)
> >> +            newstat = oldstat.stat.st_mode | stat.S_IWUSR
> >> +            os.chmod(self._tempname, newstat)
> > Isn't it better to add an option for mktempcopy() to forcibly flag S_IWUSR
> > bit on?
> 
> I was hesitant between the two designs. I give a try to the mktempcopy
> modification design thanks to your comment and it seems better.
> 
> I'm not familiar with how Windows handle file permissions but I see that
> copymode for windows does nothing. Should we also avoid enforcing the
> writable bit on Windows platform?

Maybe no. If we didn't copy the file mode, the temporary file should be
writable.