Patchwork [11,of,12] rollback: invalidate dirstate through the filecache

login
register
mail settings
Submitter Idan Kamara
Date Dec. 17, 2012, 3:35 p.m.
Message ID <dfe35839906aaf57d18e.1355758536@idan>
Download mbox | patch
Permalink /patch/165/
State Changes Requested, archived
Headers show

Comments

Idan Kamara - Dec. 17, 2012, 3:35 p.m.
# HG changeset patch
# User Idan Kamara <idankk86 at gmail.com>
# Date 1355568420 -7200
# Branch stable
# Node ID dfe35839906aaf57d18eeebfc529425de7c0ac85
# Parent  9157ffe3d66d41bb7ea3cfa062d775a87731206a
rollback: invalidate dirstate through the filecache

There's no reason to use invalidate on the dirstate instance since .hg/dirstate
was just replaced and the filecache will spot this and reload it.
Pierre-Yves David - Jan. 9, 2013, 6:52 p.m.
On 8 janv. 2013, at 20:34, Idan Kamara wrote:

> On Tue, Jan 8, 2013 at 9:23 PM, Pierre-Yves David <pierre-yves.david@ens-lyon.org> wrote:
> >
> >
> > On 8 janv. 2013, at 20:10, Idan Kamara wrote:
> >
> > > On Tue, Jan 8, 2013 at 8:00 PM, Pierre-Yves David
> > > <pierre-yves.david@logilab.fr> wrote:
> > > >
> > > > On Mon, Dec 17, 2012 at 05:35:36PM +0200, Idan Kamara wrote:
> > > > > # HG changeset patch
> > > > > # User Idan Kamara <idankk86@gmail.com>
> > > > > # Date 1355568420 -7200
> > > > > # Branch stable
> > > > > # Node ID dfe35839906aaf57d18eeebfc529425de7c0ac85
> > > > > # Parent  9157ffe3d66d41bb7ea3cfa062d775a87731206a
> > > > > rollback: invalidate dirstate through the filecache
> > > > >
> > > > > There's no reason to use invalidate on the dirstate instance since
> > > > > .hg/dirstate
> > > > > was just replaced and the filecache will spot this and reload it.
> > > > >
> > > > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > > > > --- a/mercurial/localrepo.py
> > > > > +++ b/mercurial/localrepo.py
> > > > > @@ -1009,6 +1009,7 @@
> > > > >                        parents[1] not in self.changelog.nodemap)
> > > > >          if parentgone:
> > > > >              util.rename(self.join('undo.dirstate'),
> > > > > self.join('dirstate'))
> > > > > +            self.invalidatedirstate()
> > > > >              try:
> > > > >                  branch = self.opener.read('undo.branch')
> > > > >                  self.dirstate.setbranch(encoding.tolocal(branch))
> > > >
> > > > What is this this extra call for ? You description only talk about the
> > > > removed call bellow.
> > >
> > > Which call? To dirstate.setbranch?
> >
> > The:
> > +            self.invalidatedirstate()
> 
> it replaces the old invalidate call.
> 
> But on second thought I might drop this patch altogether
> since it causes a stat on .hg/dirstate when it's not
> really needed.

My point is:

Your description say:
- call to XXX is not necessary, dropping it

Your patch do
- add call to YYY	
- drop call to XXX

I think you should either:
- explain why you add YYY
- split adding of YYY from the dropping of XXX
- not add call to YY
Idan Kamara - Jan. 9, 2013, 6:58 p.m.
On Wed, Jan 9, 2013 at 8:52 PM, Pierre-Yves David
<pierre-yves.david@ens-lyon.org> wrote:
>
>
> On 8 janv. 2013, at 20:34, Idan Kamara wrote:
>
> > On Tue, Jan 8, 2013 at 9:23 PM, Pierre-Yves David
> > <pierre-yves.david@ens-lyon.org> wrote:
> > >
> > >
> > > On 8 janv. 2013, at 20:10, Idan Kamara wrote:
> > >
> > > > On Tue, Jan 8, 2013 at 8:00 PM, Pierre-Yves David
> > > > <pierre-yves.david@logilab.fr> wrote:
> > > > >
> > > > > On Mon, Dec 17, 2012 at 05:35:36PM +0200, Idan Kamara wrote:
> > > > > > # HG changeset patch
> > > > > > # User Idan Kamara <idankk86@gmail.com>
> > > > > > # Date 1355568420 -7200
> > > > > > # Branch stable
> > > > > > # Node ID dfe35839906aaf57d18eeebfc529425de7c0ac85
> > > > > > # Parent  9157ffe3d66d41bb7ea3cfa062d775a87731206a
> > > > > > rollback: invalidate dirstate through the filecache
> > > > > >
> > > > > > There's no reason to use invalidate on the dirstate instance
> > > > > > since
> > > > > > .hg/dirstate
> > > > > > was just replaced and the filecache will spot this and reload
> > > > > > it.
> > > > > >
> > > > > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > > > > > --- a/mercurial/localrepo.py
> > > > > > +++ b/mercurial/localrepo.py
> > > > > > @@ -1009,6 +1009,7 @@
> > > > > >                        parents[1] not in self.changelog.nodemap)
> > > > > >          if parentgone:
> > > > > >              util.rename(self.join('undo.dirstate'),
> > > > > > self.join('dirstate'))
> > > > > > +            self.invalidatedirstate()
> > > > > >              try:
> > > > > >                  branch = self.opener.read('undo.branch')
> > > > > >
> > > > > > self.dirstate.setbranch(encoding.tolocal(branch))
> > > > >
> > > > > What is this this extra call for ? You description only talk about
> > > > > the
> > > > > removed call bellow.
> > > >
> > > > Which call? To dirstate.setbranch?
> > >
> > > The:
> > > +            self.invalidatedirstate()
> >
> > it replaces the old invalidate call.
> >
> > But on second thought I might drop this patch altogether
> > since it causes a stat on .hg/dirstate when it's not
> > really needed.
>
> My point is:
>
> Your description say:
> - call to XXX is not necessary, dropping it
>
> Your patch do
> - add call to YYY
> - drop call to XXX
>
> I think you should either:
> - explain why you add YYY
> - split adding of YYY from the dropping of XXX
> - not add call to YY

The first line says:

rollback: invalidate dirstate through the filecache

So I'm not sure what you're complaining about. Anyway
like I said, I dropped this patch on second thought.

Patch

diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
--- a/mercurial/localrepo.py
+++ b/mercurial/localrepo.py
@@ -1009,6 +1009,7 @@ 
                       parents[1] not in self.changelog.nodemap)
         if parentgone:
             util.rename(self.join('undo.dirstate'), self.join('dirstate'))
+            self.invalidatedirstate()
             try:
                 branch = self.opener.read('undo.branch')
                 self.dirstate.setbranch(encoding.tolocal(branch))
@@ -1017,7 +1018,6 @@ 
                           'current branch is still \'%s\'\n')
                         % self.dirstate.branch())
 
-            self.dirstate.invalidate()
             parents = tuple([p.rev() for p in self.parents()])
             if len(parents) > 1:
                 ui.status(_('working directory now based on '