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

Greg Ward greg-hg at gerg.ca
Tue Mar 15 20:39:41 CDT 2011


# HG changeset patch
# User Greg Ward <greg at gerg.ca>
# Date 1300238900 14400
# Node ID 26aade8f82d73e7943390ed874d984813f527404
# Parent  a32697a387d0ff0b7327cb993b700c1eb0310326
commit: avoid a dirstate race with multiple commits in the same process
(issue2264, issue2516)

The race happens when two commits in a row change the same file but do
not change its size, *if* those two commits happen in the same second
in the same process while holding the same repo lock.  For example:

  commit i:
    M a
    M b
  commit i+1:         # same process, same second, same repo lock
    M b               # modify b without changing its size
    M c

This first manifested in transplant, which is the most common way to
do multiple commits in the same process. But it can manifest in any
script or extension that does multiple commits under the same repo
lock. (Thus, the test script tests both transplant and a custom script.)

The problem was that dirstate failed to notice the changes to b when
localrepo is doing the second commit, meaning that change gets left in
the working directory. In the context of transplant, that means either
a crash ("RuntimeError: nothing committed after transplant") or a
silently inaccurate transplant, depending on whether any other files
were modified by the second transplanted changeset.

The fix is to work a little harder in the second (and subsequent)
commits run in the same process: keep track of files updated by the
last commit, and before starting the next commit, use
dirstate.maybelookup() to check their timestamps and possibly force an
expensive "read the contents" status check.

Incidentally, there is a simpler fix: call dirstate.normallookup() on
every file updated by commit() at the end of the commit.  The trouble
with that solution is that it imposes a performance penalty on the
common case: it means the next status-dependent hg command after every
"hg commit" will be a little bit slower.  The patch here is more
complex, but it only affects performance for the uncommon case of
multiple commits in the same process.

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)
+
             changes = self.status(match=match, clean=force)
             if force:
                 changes[0].extend(changes[6]) # mq may commit unchanged files
@@ -1018,7 +1031,8 @@
             if p2 == nullid:
                 parents = (p1,)
             bookmarks.update(self, parents, ret)
-            for f in changes[0] + changes[1]:
+            self._lastcommitfiles = changes[0] + changes[1]
+            for f in self._lastcommitfiles:
                 self.dirstate.normal(f)
             for f in changes[2]:
                 self.dirstate.forget(f)
diff --git a/tests/test-commit-multiple.t b/tests/test-commit-multiple.t
new file mode 100644
--- /dev/null
+++ b/tests/test-commit-multiple.t
@@ -0,0 +1,116 @@
+# reproduce issue2264, issue2516 (thanks to issue2516 for the original
+# script)
+
+create test repo
+  $ cat <<EOF >> $HGRCPATH
+  > [extensions]
+  > transplant =
+  > graphlog =
+  > EOF
+  $ hg init repo
+  $ cd repo
+  $ template="{rev}  {desc|firstline}  [{branches}]\n"
+
+# we need to start out with two changesets on the default branch
+# in order to avoid the cute little optimization where transplant
+# pulls rather than transplants
+add initial changesets
+  $ echo feature1 > file1
+  $ hg ci -Am"feature 1"
+  adding file1
+  $ echo feature2 >> file2
+  $ hg ci -Am"feature 2"
+  adding file2
+
+# The changes to 'bugfix' are enough to show the bug: in fact, with only
+# those changes, it's a very noisy crash ("RuntimeError: nothing
+# committed after transplant").  But if we modify a second file in the
+# transplanted changesets, the bug is much more subtle: transplant
+# silently drops the second change to 'bugfix' on the floor, and we only
+# see it when we run 'hg status' after transplanting.  Subtle data loss
+# bugs are worse than crashes, so reproduce the subtle case here.
+commit bug fixes on bug fix branch
+  $ hg branch fixes
+  marked working directory as branch fixes
+  $ echo fix1 > bugfix
+  $ echo fix1 >> file1
+  $ hg ci -Am"fix 1"
+  adding bugfix
+  $ echo fix2 > bugfix
+  $ echo fix2 >> file1
+  $ hg ci -Am"fix 2"
+  $ hg glog --template="$template"
+  @  3  fix 2  [fixes]
+  |
+  o  2  fix 1  [fixes]
+  |
+  o  1  feature 2  []
+  |
+  o  0  feature 1  []
+  
+transplant bug fixes onto release branch
+  $ hg update 0
+  1 files updated, 0 files merged, 2 files removed, 0 files unresolved
+  $ hg branch release
+  marked working directory as branch release
+  $ hg transplant 2 3
+  applying [0-9a-f]{12} (re)
+  [0-9a-f]{12} transplanted to [0-9a-f]{12} (re)
+  applying [0-9a-f]{12} (re)
+  [0-9a-f]{12} transplanted to [0-9a-f]{12} (re)
+  $ hg glog --template="$template"
+  @  5  fix 2  [release]
+  |
+  o  4  fix 1  [release]
+  |
+  | o  3  fix 2  [fixes]
+  | |
+  | o  2  fix 1  [fixes]
+  | |
+  | o  1  feature 2  []
+  |/
+  o  0  feature 1  []
+  
+  $ hg status
+  $ hg status --rev 0:4
+  M file1
+  A bugfix
+  $ hg status --rev 4:5
+  M bugfix
+  M file1
+
+now test that we fixed the bug for all scripts/extensions
+  $ cat > $TESTTMP/committwice.py <<__EOF__
+  > from mercurial import ui, hg, match, node
+  > 
+  > def replacebyte(fn, b):
+  >     f = open("file1", "rb+")
+  >     f.seek(0, 0)
+  >     f.write(b)
+  >     f.close()
+  > 
+  > repo = hg.repository(ui.ui(), '.')
+  > assert len(repo) == 6, \
+  >        "initial: len(repo) == %d, expected 6" % len(repo)
+  > try:
+  >     wlock = repo.wlock()
+  >     lock = repo.lock()
+  >     m = match.exact(repo.root, '', ['file1'])
+  >     replacebyte("file1", "x")
+  >     n = repo.commit(text="x", user="test", date=(0, 0), match=m)
+  >     print "commit 1: len(repo) == %d" % len(repo)
+  >     replacebyte("file1", "y")
+  >     n = repo.commit(text="y", user="test", date=(0, 0), match=m)
+  >     print "commit 2: len(repo) == %d" % len(repo)
+  > finally:
+  >     lock.release()
+  >     wlock.release()
+  > __EOF__
+  $ $PYTHON $TESTTMP/committwice.py
+  commit 1: len(repo) == 7
+  commit 2: len(repo) == 8
+  $ hg status
+  $ hg log --template "{rev}  {desc}  {files}\n" -r5:
+  5  fix 2  bugfix file1
+  6  x  file1
+  7  y  file1


More information about the Mercurial-devel mailing list