[PATCH 4 of 7] manifest: use specific opener to avoid file stat ambiguity around truncation
FUJIWARA Katsunori
foozy at lares.dti.ne.jp
Wed Sep 21 02:57:56 EDT 2016
At Tue, 20 Sep 2016 18:14:32 +0200,
Pierre-Yves David wrote:
>
> On 09/16/2016 10:51 PM, FUJIWARA Katsunori wrote:
> > # HG changeset patch
> > # User FUJIWARA Katsunori <foozy at lares.dti.ne.jp>
> > # Date 1474057496 -32400
> > # Sat Sep 17 05:24:56 2016 +0900
> > # Node ID 55bd90d63ad3a76fd4d27d5eabb3fe9a7fa0642b
> > # Parent 71b6b49f8a7ab6c894028b9153290f4bbf0f54f6
> > manifest: use specific opener to avoid file stat ambiguity around truncation
> >
> > If steps below occurs at "the same time in sec", all of mtime, ctime
> > and size are same between (1) and (3).
> >
> > 1. append data to 00manifest.i (and close transaction)
> > 2. discard appended data by truncation (strip or rollback)
> > 3. append same size but different data to 00manifest.i again
> >
> > Therefore, cache validation doesn't work after (3) as expected.
> >
> > To avoid such file stat ambiguity around truncation, this patch uses
> > specific opener, which forces checkambig=True at writing 00manifest.i
> > changes out.
> >
> > Hooking vfs.__call__() is the only way to centralize changes into
> > manifest.py, because all logic to actually write in-memory changes out
> > is implemented in revlog layer.
> >
> > In fact, reusing already wrapped self.opener for dirlogcache entries
> > like as below is a little redundant, because storecache-ed properties
> > of localrepository doesn't refer file stat of non-root 00manifest.i
> > files (= ambiguity of such files doesn't cause any issue).
> >
> > if dir not in self._dirlogcache:
> > self._dirlogcache[dir] = manifestrevlog(self.opener, dir,
> > self._dirlogcache)
> >
> > But wrapped opener never checks ambiguity of file stat for non-root
> > 00manifest.i files, because "path == '00manifest.i'" condition in
> > wrapper.__call__() is always False for such files. Therefore, this
> > patch simply reuses wrapped self.opener for non-root 00manifest.i
> > files.
> >
> > Even after this patch, avoiding file stat ambiguity of 00manifest.i
> > around truncation isn't yet completed, because truncation side isn't
> > aware of this issue.
> >
> > This is a part of ExactCacheValidationPlan.
> >
> > https://www.mercurial-scm.org/wiki/ExactCacheValidationPlan
> >
> > diff --git a/mercurial/manifest.py b/mercurial/manifest.py
> > --- a/mercurial/manifest.py
> > +++ b/mercurial/manifest.py
> > @@ -892,6 +892,28 @@ class treemanifest(object):
> > subp1, subp2 = subp2, subp1
> > writesubtree(subm, subp1, subp2)
> >
> > +def _checkambigopener(opener):
> > + """build an opener to add checkambig=True at changing 00manifest.i
> > + """
> > + class wrapper(opener.__class__):
> > + def __call__(self, path, mode="r", text=False, atomictemp=False,
> > + notindexed=False, backgroundclose=False, checkambig=False,
> > + **kwargs):
> > + if path == '00manifest.i' and mode not in ('r', 'rb'):
> > + # check file stat ambiguity at closing forcibly
> > + checkambig = True
> > + supercall = super(wrapper, self).__call__
> > + return supercall(path, mode,
> > + text=text,
> > + atomictemp=atomictemp,
> > + notindexed=notindexed,
> > + backgroundclose=backgroundclose,
> > + checkambig=checkambig,
> > + **kwargs)
> > + opener.__class__ = wrapper
>
> This wrapper logic is a bit scary here. Could we had the necessary
> minimal hooks in revlog to allow manifest and changelog to hooks in ?
> That would seems more robust/clean than wrapping core from core.
How about adding optional argument "checkambig" to revlog.__init__(),
of which default value is False ?
> > +
> > + return opener
> > +
> > class manifestrevlog(revlog.revlog):
> > '''A revlog that stores manifest texts. This is responsible for caching the
> > full-text manifest contents.
> > @@ -927,6 +949,13 @@ class manifestrevlog(revlog.revlog):
> > else:
> > self._dirlogcache = {'': self}
> >
> > + # If "dir" isn't empty, this "opener" is reused one via
> > + # "self.opener" of another manifestrevlog. Therefore,
> > + # wrapping opener is needed only at once for root
> > + # 00manifest.i file, to avoid multiple (= redundant)
> > + # wrapping.
> > + opener = _checkambigopener(opener)
> > +
> > super(manifestrevlog, self).__init__(opener, indexfile)
> >
> > @property
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >
>
> --
> Pierre-Yves David
>
----------------------------------------------------------------------
[FUJIWARA Katsunori] foozy at lares.dti.ne.jp
More information about the Mercurial-devel
mailing list