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

[Greg Ward]

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.

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

        Greg
-- 
Greg Ward                                http://www.gerg.ca/
Shape shifting reptilians are about to achieve complete control over
this planet!