Patchwork [V2] dirstate: rebuild should update dirstate properly

login
register
mail settings
Submitter Mateusz Kwapich
Date Aug. 30, 2016, 10:17 p.m.
Message ID <aff2b9911d78a3d427e3.1472595456@dev10635.prn2.facebook.com>
Download mbox | patch
Permalink /patch/16496/
State Accepted
Headers show

Comments

Mateusz Kwapich - Aug. 30, 2016, 10:17 p.m.
# HG changeset patch
# User Mateusz Kwapich <mitrandir@fb.com>
# Date 1472595388 25200
#      Tue Aug 30 15:16:28 2016 -0700
# Node ID aff2b9911d78a3d427e3ba18a565e76215971948
# Parent  12f8bef59bfa2739d0c5d8425ab494fd2fe38a81
dirstate: rebuild should update dirstate properly

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.

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.
Matt Mackall - Aug. 30, 2016, 10:21 p.m.
On Tue, 2016-08-30 at 15:17 -0700, Mateusz Kwapich wrote:
> # HG changeset patch
> # User Mateusz Kwapich <mitrandir@fb.com>
> # Date 1472595388 25200
> #      Tue Aug 30 15:16:28 2016 -0700
> # Node ID aff2b9911d78a3d427e3ba18a565e76215971948
> # Parent  12f8bef59bfa2739d0c5d8425ab494fd2fe38a81
> dirstate: rebuild should update dirstate properly

Is this appropriate for stable?

> 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.
> 
> 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
> @@ -680,21 +680,15 @@ class dirstate(object):
>              self.clear()
>              self._lastnormaltime = lastnormaltime
>  
> -        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)
> -            else:
> -                self._map.pop(f, None)
> -                if f in self._nonnormalset:
> -                    self._nonnormalset.remove(f)
> -
>          if self._origpl is None:
>              self._origpl = self._pl
>          self._pl = (parent, nullid)
> +        for f in changedfiles:
> +            if f in allfiles:
> +                self.normallookup(f)
> +            else:
> +                self.drop(f)
> +
>          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
>  
>    $ hg debugadddrop --normal-lookup file1 file2
>    $ hg debugadddrop --drop bar
> @@ -57,7 +57,7 @@ state dump after
>    $ hg debugstate --nodates
>    n   0         -1 unset               file1
>    n   0         -1 unset               file2
> -  n 644         -1 set                 foo
> +  n   0         -1 unset               foo
>    $ hg debugrebuildstate
>  
>  status
> @@ -115,7 +115,7 @@ dirstate
>    $ hg debugrebuilddirstate --minimal
>    $ hg debugdirstate --nodates
>    r   0          0 * bar (glob)
> -  n 644         -1 * foo (glob)
> +  n   0         -1 * foo (glob)
>    a   0         -1 * qux (glob)
>    $ hg status -A
>    A qux
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Mateusz Kwapich - Aug. 31, 2016, 3:43 p.m.
I don’t think so. I’m not 100% sure that it doesn’t break any extension around
so I would prefer to release it with next major release. 

On 8/30/16, 11:21 PM, "Matt Mackall" <mpm@selenic.com> wrote:

    On Tue, 2016-08-30 at 15:17 -0700, Mateusz Kwapich wrote:
    > # HG changeset patch

    > # User Mateusz Kwapich <mitrandir@fb.com>

    > # Date 1472595388 25200

    > #      Tue Aug 30 15:16:28 2016 -0700

    > # Node ID aff2b9911d78a3d427e3ba18a565e76215971948

    > # Parent  12f8bef59bfa2739d0c5d8425ab494fd2fe38a81

    > dirstate: rebuild should update dirstate properly

    
    Is this appropriate for stable?
    
    > 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.

    > 

    > 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

    > @@ -680,21 +680,15 @@ class dirstate(object):

    >              self.clear()

    >              self._lastnormaltime = lastnormaltime

    >  

    > -        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)

    > -            else:

    > -                self._map.pop(f, None)

    > -                if f in self._nonnormalset:

    > -                    self._nonnormalset.remove(f)

    > -

    >          if self._origpl is None:

    >              self._origpl = self._pl

    >          self._pl = (parent, nullid)

    > +        for f in changedfiles:

    > +            if f in allfiles:

    > +                self.normallookup(f)

    > +            else:

    > +                self.drop(f)

    > +

    >          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

    >  

    >    $ hg debugadddrop --normal-lookup file1 file2

    >    $ hg debugadddrop --drop bar

    > @@ -57,7 +57,7 @@ state dump after

    >    $ hg debugstate --nodates

    >    n   0         -1 unset               file1

    >    n   0         -1 unset               file2

    > -  n 644         -1 set                 foo

    > +  n   0         -1 unset               foo

    >    $ hg debugrebuildstate

    >  

    >  status

    > @@ -115,7 +115,7 @@ dirstate

    >    $ hg debugrebuilddirstate --minimal

    >    $ hg debugdirstate --nodates

    >    r   0          0 * bar (glob)

    > -  n 644         -1 * foo (glob)

    > +  n   0         -1 * foo (glob)

    >    a   0         -1 * qux (glob)

    >    $ hg status -A

    >    A qux

    > _______________________________________________

    > Mercurial-devel mailing list

    > Mercurial-devel@mercurial-scm.org

    > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.mercurial-2Dscm.org_mailman_listinfo_mercurial-2Ddevel&d=DQIDaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=dK7q_6fOymlfdGMBe3wUaA&m=1S1KyZ73GzbGz2eXFYN6bfgMYY5w9rRfQztW_y3lVe0&s=qWhJdNoJNUc0sObYEpXAFWkY6YZzuQY2o53OKYNwcpA&e= 

    -- 
    Mathematics is the supreme nostalgia of our time.
Pierre-Yves David - Sept. 10, 2016, 11:27 a.m.
On 08/31/2016 12:17 AM, Mateusz Kwapich wrote:
> # HG changeset patch
> # User Mateusz Kwapich <mitrandir@fb.com>
> # Date 1472595388 25200
> #      Tue Aug 30 15:16:28 2016 -0700
> # Node ID aff2b9911d78a3d427e3ba18a565e76215971948
> # Parent  12f8bef59bfa2739d0c5d8425ab494fd2fe38a81
> dirstate: rebuild should update dirstate properly

What's the status on this?
Yuya Nishihara - Sept. 10, 2016, 3:10 p.m.
On Sat, 10 Sep 2016 13:27:57 +0200, Pierre-Yves David wrote:
> On 08/31/2016 12:17 AM, Mateusz Kwapich wrote:
> > # HG changeset patch
> > # User Mateusz Kwapich <mitrandir@fb.com>
> > # Date 1472595388 25200
> > #      Tue Aug 30 15:16:28 2016 -0700
> > # Node ID aff2b9911d78a3d427e3ba18a565e76215971948
> > # Parent  12f8bef59bfa2739d0c5d8425ab494fd2fe38a81
> > dirstate: rebuild should update dirstate properly
> 
> What's the status on this?

https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-August/087797.html

I can't be sure if the behavior change of "hg debugrebuilddirstate" is correct,
though the dirstate is updated by subsequent "hg status" so the BC is invisible
to users.

If that's okay, the code looks good to me.
Yuya Nishihara - Sept. 30, 2016, 1:23 p.m.
On Tue, 30 Aug 2016 15:17:36 -0700, Mateusz Kwapich wrote:
> # HG changeset patch
> # User Mateusz Kwapich <mitrandir@fb.com>
> # Date 1472595388 25200
> #      Tue Aug 30 15:16:28 2016 -0700
> # Node ID aff2b9911d78a3d427e3ba18a565e76215971948
> # Parent  12f8bef59bfa2739d0c5d8425ab494fd2fe38a81
> dirstate: rebuild should update dirstate properly

Queued this per the discussion on V1, thanks.

https://www.mercurial-scm.org/pipermail/mercurial-devel/2016-September/088556.html

Patch

diff --git a/mercurial/dirstate.py b/mercurial/dirstate.py
--- a/mercurial/dirstate.py
+++ b/mercurial/dirstate.py
@@ -680,21 +680,15 @@  class dirstate(object):
             self.clear()
             self._lastnormaltime = lastnormaltime
 
-        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)
-            else:
-                self._map.pop(f, None)
-                if f in self._nonnormalset:
-                    self._nonnormalset.remove(f)
-
         if self._origpl is None:
             self._origpl = self._pl
         self._pl = (parent, nullid)
+        for f in changedfiles:
+            if f in allfiles:
+                self.normallookup(f)
+            else:
+                self.drop(f)
+
         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
 
   $ hg debugadddrop --normal-lookup file1 file2
   $ hg debugadddrop --drop bar
@@ -57,7 +57,7 @@  state dump after
   $ hg debugstate --nodates
   n   0         -1 unset               file1
   n   0         -1 unset               file2
-  n 644         -1 set                 foo
+  n   0         -1 unset               foo
   $ hg debugrebuildstate
 
 status
@@ -115,7 +115,7 @@  dirstate
   $ hg debugrebuilddirstate --minimal
   $ hg debugdirstate --nodates
   r   0          0 * bar (glob)
-  n 644         -1 * foo (glob)
+  n   0         -1 * foo (glob)
   a   0         -1 * qux (glob)
   $ hg status -A
   A qux