[PATCH 12 of 14] vfs: add the possibility to have a "ward" to check vfs usage

[Yuya Nishihara]

On Tue, 11 Jul 2017 16:32:46 +0200, Boris Feld wrote:
> On Sat, 2017-07-08 at 16:36 +0900, Yuya Nishihara wrote:
> > On Fri, 07 Jul 2017 18:48:44 +0200, Boris Feld wrote:
> > > On Fri, 2017-07-07 at 21:48 +0900, Yuya Nishihara wrote:
> > > > On Thu, 06 Jul 2017 20:53:50 +0200, Boris Feld wrote:
> > > > > On Wed, 2017-07-05 at 23:55 +0900, Yuya Nishihara wrote:
> > > > > > On Sun, 02 Jul 2017 04:56:37 +0200, Pierre-Yves David wrote:
> > > > > > > # HG changeset patch
> > > > > > > # User Pierre-Yves David <pierre-yves.david at ens-lyon.org>
> > > > > > > # Date 1470323266 -7200
> > > > > > > #      Thu Aug 04 17:07:46 2016 +0200
> > > > > > > # Node ID ebf81efdd6d7ff15c64683933635616c00475688
> > > > > > > # Parent  34b8be7f0420b07d0f7b71379c6055e5b26223d5
> > > > > > > # EXP-Topic vfs.ward
> > > > > > > # Available At https://www.mercurial-scm.org/repo/users/mar
> > > > > > > mout
> > > > > > > e/me
> > > > > > > rcurial/
> > > > > > > #              hg pull https://www.mercurial-scm.org/repo/u
> > > > > > > sers
> > > > > > > /mar
> > > > > > > moute/mercurial/ -r ebf81efdd6d7
> > > > > > > vfs: add the possibility to have a "ward" to check vfs
> > > > > > > usage
> > > > > > > The feature has some similarity with the 'pathauditor', but
> > > > > > > further
> > > > > > > digging
> > > > > > > show it make more sense to keep them separated. The path
> > > > > > > auditor is
> > > > > > > fairly
> > > > > > > autonomous (create from vfs.__init__ without any extra
> > > > > > > information), and check
> > > > > > > very generic details. On the other hand, the wards will
> > > > > > > have
> > > > > > > deeper
> > > > > > > knownledge
> > > > > > > of the Mercurial logic and is created at the local
> > > > > > > repository
> > > > > > > level. Mixing the
> > > > > > > two would add much complexity for no real gains.
> > > > > > 
> > > > > > My gut feeling is that vfs isn't the right place if they
> > > > > > require
> > > > > > deeper
> > > > > > knowledge of repository/dirstate logic. And the whole weakref
> > > > > > business
> > > > > > floating around wards, hooks and transaction seems like just
> > > > > > getting
> > > > > > around
> > > > > > layering violation.
> > > > > 
> > > > > We pondered on this choice a long time and couldn't find any
> > > > > other
> > > > > layer that is used by both Mercurial core and the extensions to
> > > > > access
> > > > > the file system. It seems to be the highest abstraction layer
> > > > > we
> > > > > could
> > > > > hook into without introducing a new one and updating all
> > > > > callers.
> > > > > 
> > > > > Our reflexion is, as the vfs classes comes from the scmutil
> > > > > file,
> > > > > it
> > > > > seems okay to have scm related logic in that layer.
> > > > 
> > > > IIRC, one possible alternative was to move lock itself to a vfs
> > > > as
> > > > the
> > > > whole vfs is theoretically covered by the lock.
> > > 
> > > The practice divert a bit from the theory. For example, the
> > > ".hg/bookmark" file is in theory covered by the 'wlock'. In
> > > practice it
> > > is now covered by the transaction and therefor the 'lock'. On the
> > > other
> > > hand there are multiple files handled by the vfs ('hgrc',
> > > 'dirstate',
> > > 'store/', etc) that are not covered by the wlock.
> > > 
> > > Right now, all these exceptions are stored on the repository, and
> > > extension can update them. To us, the repository layers seems the
> > > right
> > > layer for logic regarding individual file semantic. We also want to
> > > add
> > > some logic regarding open transaction to the ward, something
> > > unrelated
> > > to the vfs.
> > > 
> > > The initial motivation for this series (beside catching bug) is to
> > > make
> > > it safer to introduce new vfs. For example, the share extensions
> > > suffers from lack of cache sharing, each share has its own
> > > '.hg/cache/'
> > > directory. Introducing a 'repo.cvfs' dedicated to cache could fix
> > > that,
> > > but we wants to warn people still accessing 'cache/' through
> > > 'repo.vfs'.
> > > 
> > > To push that logic further, "share" actually needs a better
> > > distinction
> > > between the working copy specific piece and the more generic
> > > pieces. An
> > > extra split of "repo.vfs" could take care of that. However, some of
> > > the
> > > file, like bookmark, can be shared or not depending on some config
> > > and
> > > share option. So the ward needs access to that logic (already
> > > living on
> > > the repository) too.
> > 
> > It's probably okay to handle simple repo-related business by hooking
> > vfs
> > calls (via auditor/ward), but the future story sounds pretty
> > complicated.
> > I don't think it's great idea to introduce many-consumers <-> many-
> > vfs
> > relation mediated by a low-level hook, "ward".
> > 
> > > We need to eventually rework the repository format to a more race
> > > free
> > > data structure. This will likely involve more vfs to access data
> > > dispatched in various directory (eg: append-only vs full update).
> > > The
> > > ward should help a lot here but it need to be aware of many high
> > > level
> > > logic regarding file semantic.
> > 
> > At this point, I think we'll need a proper storage layer which
> > shadows
> > low-level vfs operation.

[...]

> We are not sure to follow you here. Introducing a proper storage layer
> seems complicated and out-of-scope of this series. I cannot see how we
> could make sure that both core and extensions are forced to use this
> new storage layer instead of vfs directly. Could you clarify what do
> you have in mind?

No idea how it will look like. Sometimes I heard read-write order of
bookmarks, changelog, etc. was wrong for example, so I don't think
inspecting vfs operations will prevent us from doing common bad things.
That's one reason why I had a gut feeling that we'll need a higher-level
object to manage repository data.