[PATCH] update: fix edge-case with update.atomic-file and read-only files
Boris FELD
boris.feld at octobus.net
Sun Jan 13 04:26:32 EST 2019
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 at octobus.net
> <mailto:boris.feld at octobus.net>> wrote:
>
> # HG changeset patch
> # User Boris Feld <boris.feld at octobus.net
> <mailto: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.
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 at mercurial-scm.org
> <mailto:Mercurial-devel at mercurial-scm.org>
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>
>
> _______________________________________________
> 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/20190113/aa894d6e/attachment.html>
More information about the Mercurial-devel
mailing list