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

[Matt Mackall]

On Thu, 2011-03-17 at 09:10 -0400, Greg Ward wrote:
> On 15 March 2011, Matt Mackall said:
> > 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.
> > 
> > >              changes = self.status(match=match, clean=force)
> > 
> > For instance, in here.
> > 
> > > -            for f in changes[0] + changes[1]:
> > > +            self._lastcommitfiles = changes[0] + changes[1]
> > > +            for f in self._lastcommitfiles:
> > >                  self.dirstate.normal(f)
> > 
> > And here.
> > 
> > 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.
> 
> I tried to implement something like this, and I'm stuck.  I started in
> dirstate.write(), since that's one of the few places where we process
> the entire dirstate.  The patch to write() looks like this:
> 
> """
> @@ -436,16 +438,20 @@
>          pack = struct.pack
>          write = cs.write
>          write("".join(self._pl))
> +        normal = set()
>          for f in self._map.iterkeys():
>              e = self.checkmtime(f, now)
>              if f in copymap:
>                  f = "%s\0%s" % (f, copymap[f])
> +            if e[0] == 'n':
> +                normal.add(f)
>              e = pack(_format, e[0], e[1], e[2], e[3], len(f))
>              write(e)
>              write(f)
>          st.write(cs.getvalue())
>          st.rename()
>          self._dirty = self._dirtypl = False
> +        self._lastnormal = normal
> """
> 
> Then I modified status() to pay attention to _lastnormal:
> 
> """
> @@ -659,6 +665,9 @@
>          radd = removed.append
>          dadd = deleted.append
>          cadd = clean.append
> +        lastnormal = self._lastnormal
> +        if lastnormal:
> +            now = self.getfstime()
>  
>          lnkkind = stat.S_IFLNK
>  
> @@ -672,7 +681,10 @@
>                      uadd(fn)
>                  continue
>  
> -            state, mode, size, time = dmap[fn]
> +            if lastnormal and fn in lastnormal:
> +                state, mode, size, time = self.checkmtime(fn, now)
> +            else:
> +                state, mode, size, time = dmap[fn]
>  
>              if not st and state in "nma":
>                  dadd(fn)
> """
> 
> (Note: I'm ignoring performance for the moment.  Correctness first.)
> 
> But this doesn't fix the "commit twice" bug because repo.commit() does
> not call dirstate.write().  And that makes sense; we don't want to
> write the dirstate more than once per process.  That should be done
> late in the game.  
> 
> So then I thought about putting _lastnormal entirely under the control
> of status().  But status() only looks at a subset of the dirstate, so
> it cannot create a definitive _lastnormal list.  I'm not sure how much
> this matters.
> 
> Maybe I'm barking up the wrong method.  Perhaps normal() should be
> responsible for building up the list of "files last asserted to be in
> state normal", which then affects the behaviour of status() on those
> files.  Hmmmmm.

You're in danger of becoming an expert. This is more or less what I
expected:

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

I think normal() is the right place, which is why I picked _lastnormal.
I should have been more clear, sorry.


> Will try again this evening.  Hints welcome in the meantime.  ;-)
> 
>         Greg


-- 
Mathematics is the supreme nostalgia of our time.