[PATCH] dirstate: rebuild should update dirstate properly

[Mateusz Kwapich]

On 8/28/16, 3:01 PM, "Yuya Nishihara" <youjah at gmail.com on behalf of yuya at tcha.org> wrote:

    On Wed, 24 Aug 2016 12:09:18 -0700, Mateusz Kwapich wrote:
    > # HG changeset patch
    > # User Mateusz Kwapich <mitrandir at fb.com>
    > # Date 1472065656 25200
    > #      Wed Aug 24 12:07:36 2016 -0700
    > # Node ID a29e5e93a814268490afd617aebce2f79bd4a75b
    > # Parent  a19f569214c76d99b0df4c4844f2ab34c9f8cfc6
    > dirstate: rebuild should update dirstate properly
    
    This patch can't be applied to the current tip. Can you rebase?

Sure, I will send a rebased version.
    
    > Updating dirstate by simply adding and dropping files from self._map doesn't
    > keep the other maps updated (think: _dirs, _copymap, _foldmap, _nonormalset)
    > thus introducing cache inconsistency.
    
    I think that's why rebuild() calls self.clear(). IIRC, I've pointed out that it
    would be wrong to skip the whole clear() step when changedfiles is not None.

Clear doesn’t invalidate all the caches – for example it doesn’t touch filefoldmap or
Dirfoldmap. Also: why would we clear those caches instead of updating them?
    
    > This is also affecting the debugstate tests since now we don't even try to set
    > correct mode and mtime for the files because they are marked dirty anyway and
    > will be checked during next status call.
    > 
    > diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
    > --- a/mercurial/dirstate.py
    > +++ b/mercurial/dirstate.py
    > @@ -675,19 +675,13 @@ class dirstate(object):
    >              self.clear()
    >              self._lastnormaltime = lastnormaltime
    >  
    > +        self._pl = (parent, nullid)
    >          for f in changedfiles:
    > -            mode = 0o666
    > -            if f in allfiles and 'x' in allfiles.flags(f):
    > -                mode = 0o777
    > -
    >              if f in allfiles:
    > -                self._map[f] = dirstatetuple('n', mode, -1, 0)
    > +                self.normallookup(f)
    >              else:
    > -                self._map.pop(f, None)
    > -                if f in self._nonnormalset:
    > -                    self._nonnormalset.remove(f)
    > +                self.drop(f)
    >  
    > -        self._pl = (parent, nullid)
    >          self._dirty = True
    >  
    >      def write(self, tr):
    > diff --git a/tests/test-rebuildstate.t b/tests/test-rebuildstate.t
    > --- a/tests/test-rebuildstate.t
    > +++ b/tests/test-rebuildstate.t
    > @@ -48,8 +48,8 @@ basic test for hg debugrebuildstate
    >  state dump after
    >  
    >    $ hg debugstate --nodates | sort
    > -  n 644         -1 set                 bar
    > -  n 644         -1 set                 foo
    > +  n   0         -1 unset               bar
    > +  n   0         -1 unset               foo
    
    This seems wrong. debugrebuilddirstate is documented as "dirstate will be
    set to the files of the given revision. The actual working directory content
    [...] is not considered."

Yeah. It still doesn’t consider the working directory contents, still contains the same files
and all entries will still be checked at the next status call. Only things that are different in
this output are file permissions and mtime which are not stored in the commit itself.
  

Best,
Mateusz