[PATCH] update: fix edge-case with update.atomic-file and read-only files

Martin von Zweigbergk martinvonz at google.com
Fri Jan 11 12:24:41 EST 2019


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

> # HG changeset patch
> # User Boris Feld <boris.feld at 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 at mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.mercurial-scm.org/pipermail/mercurial-devel/attachments/20190111/5c72ef4c/attachment.html>


More information about the Mercurial-devel mailing list