[PATCH 3 of 3] commit: avoid a dirstate race with multiple commits in the same process

[Matt Mackall]

On Wed, 2011-03-16 at 09:01 -0400, Greg Ward wrote:
> On 15 March 2011, Matt Mackall said:
> > On Tue, 2011-03-15 at 21:39 -0400, Greg Ward wrote:
> > > diff --git a/mercurial/localrepo.py b/mercurial/localrepo.py
> > > --- a/mercurial/localrepo.py
> > > +++ b/mercurial/localrepo.py
> > > @@ -113,6 +113,11 @@
> > >          self._datafilters = {}
> > >          self._transref = self._lockref = self._wlockref = None
> > >  
> > > +        # List of files added/modified by the previous commit.  Needed
> > > +        # to avoid a dirstate race when we do two commits in the same
> > > +        # second in the same process while holding the repo lock.
> > > +        self._lastcommitfiles = None
> > > +
> > >      def _applyrequirements(self, requirements):
> > >          self.requirements = requirements
> > >          self.sopener.options = {}
> > > @@ -922,6 +927,14 @@
> > >                  raise util.Abort(_('cannot partially commit a merge '
> > >                                     '(do not specify files or patterns)'))
> > >  
> > > +            if self._lastcommitfiles:
> > > +                files = [f for f in self._lastcommitfiles
> > > +                         if f in self.dirstate]
> > > +                if files:
> > > +                    now = self.dirstate.getfstime(self.path)
> > > +                    for f in files:
> > > +                        self.dirstate.maybelookup(f, now)
> > > +
> > 
> > Yes, this is all much easier to read when presented this way.
> > 
> > So what I don't like here is that some of the magic has leaked out of
> > dirstate and into localrepo. I think we should be able to prevent that.
> 
> Sounds good.
> 
> > In other words, we build a _lastnormalfiles in dirstate, and check it
> > internally in status/walk. Then status/walk can always be right,
> > regardless of what code is calling it, without that code having to know
> > about the timing magic.
> 
> One advantage of my current patch: it's clear what point in time
> repo._lastcommitfiles refers to: the last commit in this process.
> 
> But I'm a little confused about when dirstate._lastnormalfiles would
> be updated: in dirstate.write()? status()? normal()?  I guess write()
> makes the most sense: is that what you were thinking?

I was thinking there might not be a write?

-- 
Mathematics is the supreme nostalgia of our time.