[PATCH 1 of 3 RFC] scmutil: introduce the flagstate class to suppliment missing flag support

[Matt Mackall]

On Wed, 2016-07-06 at 22:58 -0400, Matt Harbison wrote:
> On Wed, 06 Jul 2016 13:35:44 -0400, Matt Mackall <mpm at selenic.com> wrote:
> 
> > 
> > On Sun, 2016-07-03 at 17:44 -0400, Matt Harbison wrote:
> > > 
> > > # HG changeset patch
> > > # User Matt Harbison <matt_harbison at yahoo.com>
> > > # Date 1467571904 14400
> > > #      Sun Jul 03 14:51:44 2016 -0400
> > > # Node ID e9fce4275ce6b26b941f47044744015e90367e7b
> > > # Parent  de4a80a2b45c6fcae0948ac12872dd8a61ced26a
> > > scmutil: introduce the flagstate class to suppliment missing flag  
> > > support
> > > 
> > > This will allow manipulation of the executable bit when filesystems  
> > > don't
> > > track
> > > it natively.  This file isn't tracked in any form, and will likely be  
> > > small
> > > since it can be purged completely when wdir is cleaned- the manifest  
> > > tracks
> > > the
> > > flags in permanent history.  It is intended only for platforms that  
> > > don't have
> > > native flag support, and will noop on platforms that do for simplicity.
> > > Obviously a tree copy in either direction between filesystems with and  
> > > without
> > > flag support will lose the flag state, but that seems like an edge case  
> > > that
> > > isn't worth having two sources of data that can get out of sync on Unix.
> > > 
> > > We could in theory also support designating a file as a symlink, so I  
> > > kept
> > > this
> > > generic.  Since the file isn't tracked, and the flags are stored  
> > > symbolically,
> > > it should be extensible without changing the file format.  So I didn't  
> > > bother
> > > with a version identifier.  I have no idea if there would be any  
> > > encoding
> > > concerns (I saw references to encode/decode in the fncache class when I  
> > > was
> > > trying to find a suitable bit of code to model), but I wouldn't think  
> > > so.
> > No, filenames are not (re-)encoded.
> > 
> > > 
> > > 
> > > I couldn't find an existing suitable file format, and I really wanted  
> > > to just
> > > munge the flags on dirstate to get `commit`, `update -C` and `revert`  
> > > handling
> > > for free.  But dirstate doesn't know about the flag building function  
> > > used in
> > > context.py,
> > That's intentional, because context is at a higher abstraction level.  
> > Dirstate
> > doesn't even know that Mercurial only recognizes two mode bits and  
> > stores 32.
> > 
> > > 
> > >  and seems to be populated by what the OS reports, though I couldn't
> > > figure out where exactly.
> > Here: https://www.selenic.com/hg/file/tip/mercurial/dirstate.py#l493
> > 
> > Note the call to stat and how the results are passed on to _addpath.
> Thanks.  I saw the former, but missed that _addpath was setting _dirty, so  
> the changes weren't being written.  That made me think something somewhere  
> else was also setting this mode.
> 
> > 
> > Note also the trick we do with the _rangemask to deal with weird sizes  
> > and
> > times. We could use a similar trick on the mode field to get some  
> > protected bits
> > for storing our virtual mode bits.
> > 
> > > 
> > > I wasn't looking to do a dirstate transaction with this, but the tests  
> > > were
> > > totally unstable from run to run without the normallookup() call.  I'm  
> > > not
> > > sure
> > > if we need to backup the file, but that's what the fncache code was  
> > > doing.
> > Whether or not this is part of the dirstate file, it's logically a  
> > dirstate
> > transaction. Getting it into the dirstate proper will avoid a number of
> > headaches.
> Agreed.  One of the headaches with dirstate (or any tracking file), is how  
> to handle {util,vfs}.setflags().  It seems wrong that either would know  
> about dirstate (or another file).  In my functioning dirstate based WIP, I  
> changed localrepo.wwrite() so that `update -C` and `revert` end up with  
> the proper bits:
> 
> diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> --- a/mercurial/localrepo.py
> +++ b/mercurial/localrepo.py
> @@ -988,6 +988,7 @@
>               self.wvfs.write(filename, data,  
> backgroundclose=backgroundclose)
>               if 'x' in flags:
>                   self.wvfs.setflags(filename, False, True)
> +            self.dirstate.setflags(filename, False, 'x' in flags)
>           return len(data)
> 
>       def wwritedata(self, filename, data):
> 
> But it also seems wrong that setflags has to be called on two different  
> things side by side.

This isn't so horrible. This is a pretty good bottleneck for callers of setflags
and it's at the right level.

It also wouldn't be horrible to conditionally use a derived class of the vfs
that knew about dirstate for wvfs. This would reduce overhead for non-Windows
machines.

We're going to eventually need such a class for https://www.mercurial-scm.org/wi
ki/WindowsUTF8Plan anyway.

>   OTOH, there are places where dirstate will never be  
> available (like convert's svn sink).  Thoughts?



> > 
> > --Mathematics is the supreme nostalgia of our time.
-- 
Mathematics is the supreme nostalgia of our time.