Rev 4720: Bare minimum to change IndirectedDirState to use a directory. in http://bazaar.launchpad.net/~lifeless/bzr/dirstate2

Robert Collins robertc at robertcollins.net
Tue Sep 29 04:32:40 BST 2009


At http://bazaar.launchpad.net/~lifeless/bzr/dirstate2

------------------------------------------------------------
revno: 4720
revision-id: robertc at robertcollins.net-20090929033218-b1os6edaaf8n2ame
parent: robertc at robertcollins.net-20090928232256-qclqkb7y0su13qjl
committer: Robert Collins <robertc at robertcollins.net>
branch nick: dirstate2
timestamp: Tue 2009-09-29 13:32:18 +1000
message:
  Bare minimum to change IndirectedDirState to use a directory.
=== modified file 'bzrlib/dirstate.py'
--- a/bzrlib/dirstate.py	2009-09-28 23:22:56 +0000
+++ b/bzrlib/dirstate.py	2009-09-29 03:32:18 +0000
@@ -221,6 +221,7 @@
     lock,
     osutils,
     trace,
+    transport,
     )
 
 
@@ -389,7 +390,8 @@
         self._parents = []
         self._state_file = None
         self._filename = path
-        self._lock_token = None
+        # None == unlocked,'w' for writing, 'r' for reading(and state cache
+        # updates)
         self._lock_state = None
         self._id_index = None
         # a map from packed_stat to sha's.
@@ -1238,8 +1240,8 @@
         self._last_entry_index = entry_index
         return entry_index, present
 
-    @staticmethod
-    def from_tree(tree, dir_state_filename, sha1_provider=None):
+    @classmethod
+    def from_tree(cls, tree, dir_state_filename, sha1_provider=None):
         """Create a dirstate from a bzr Tree.
 
         :param tree: The tree which should provide parent information and
@@ -1249,7 +1251,7 @@
         :return: a DirState object which is currently locked for writing.
             (it was locked by DirState.initialize)
         """
-        result = DirState.initialize(dir_state_filename,
+        result = cls.initialize(dir_state_filename,
             sha1_provider=sha1_provider)
         try:
             tree.lock_read()
@@ -2036,6 +2038,7 @@
         if sha1_provider is None:
             sha1_provider = DefaultSHA1Provider()
         result = cls(path, sha1_provider)
+        result._initialize()
         # root dir and root dir contents with no children.
         empty_tree_dirblocks = [('', []), ('', [])]
         # a new root directory, with a NULLSTAT.
@@ -2052,6 +2055,9 @@
             raise
         return result
 
+    def _initialize(self):
+        """Create any on-disk structures needed for the initialize method."""
+
     @staticmethod
     def _inv_entry_to_details(inv_entry):
         """Convert an inventory entry (from a revision tree) to state details.
@@ -2173,8 +2179,8 @@
         """The number of parent entries in each record row."""
         return len(self._parents) - len(self._ghosts)
 
-    @staticmethod
-    def on_file(path, sha1_provider=None):
+    @classmethod
+    def on_file(cls, path, sha1_provider=None):
         """Construct a DirState on the file at path "path".
 
         :param path: The path at which the dirstate file on disk should live.
@@ -2184,7 +2190,7 @@
         """
         if sha1_provider is None:
             sha1_provider = DefaultSHA1Provider()
-        result = DirState(path, sha1_provider)
+        result = cls(path, sha1_provider)
         return result
 
     def _read_dirblocks_if_needed(self):
@@ -2221,7 +2227,7 @@
     def _read_header_if_needed(self):
         """Read the header of the dirstate file if needed."""
         # inline this as it will be called a lot
-        if not self._lock_token:
+        if not self._lock_state:
             raise errors.ObjectNotLocked(self)
         if self._header_state == DirState.NOT_IN_MEMORY:
             self._read_header()
@@ -2287,30 +2293,30 @@
 
             grabbed_write_lock = False
             if self._lock_state != 'w':
-                grabbed_write_lock, new_lock = self._lock_token.temporary_write_lock()
-                # Switch over to the new lock, as the old one may be closed.
-                # TODO: jam 20070315 We should validate the disk file has
-                #       not changed contents. Since temporary_write_lock may
-                #       not be an atomic operation.
-                self._lock_token = new_lock
-                self._state_file = new_lock.f
+                grabbed_write_lock = self._upgrade_physical_lock()
                 if not grabbed_write_lock:
-                    # We couldn't grab a write lock, so we switch back to a read one
+                    # We couldn't grab a write lock, so we can't write.
                     return
             try:
-                self._state_file.seek(0)
-                self._state_file.writelines(self.get_lines())
-                self._state_file.truncate()
-                self._state_file.flush()
-                self._header_state = DirState.IN_MEMORY_UNMODIFIED
-                self._dirblock_state = DirState.IN_MEMORY_UNMODIFIED
+                if not self._save():
+                    self._header_state = DirState.IN_MEMORY_UNMODIFIED
+                    self._dirblock_state = DirState.IN_MEMORY_UNMODIFIED
             finally:
                 if grabbed_write_lock:
-                    self._lock_token = self._lock_token.restore_read_lock()
-                    self._state_file = self._lock_token.f
-                    # TODO: jam 20070315 We should validate the disk file has
-                    #       not changed contents. Since restore_read_lock may
-                    #       not be an atomic operation.
+                    self._restore_upgraded_lock()
+
+    def _save(self):
+        """Core of save - perform the writing of dirstate data.
+
+        This is called from save, when the dirstate has ensure there are
+        changes to save, that saving hasn't been aborted, and that a write
+        lock has been acquired.
+
+        :return: True if the save was not performed (because the change was
+            purely a state cache update and a write-locked change won, or
+            similar).
+        """
+        raise NotImplementedError(self._save)
 
     def _set_data(self, parent_ids, dirblocks):
         """Set the full dirstate data in memory.
@@ -3029,48 +3035,57 @@
 
     def lock_read(self):
         """Acquire a read lock on the dirstate."""
-        if self._lock_token is not None:
-            raise errors.LockContention(self._lock_token)
-        # TODO: jam 20070301 Rather than wiping completely, if the blocks are
-        #       already in memory, we could read just the header and check for
-        #       any modification. If not modified, we can just leave things
-        #       alone
-        self._lock_token = lock.ReadLock(self._filename)
+        if self._lock_state is not None:
+            raise errors.LockContention(self._filename, lock_state)
+        self._lock_read()
+        self._wipe_state()
         self._lock_state = 'r'
-        self._state_file = self._lock_token.f
-        self._wipe_state()
+
+    def _lock_read(self):
+        raise NotImplementedError(self._lock_read)
 
     def lock_write(self):
         """Acquire a write lock on the dirstate."""
-        if self._lock_token is not None:
-            raise errors.LockContention(self._lock_token)
-        # TODO: jam 20070301 Rather than wiping completely, if the blocks are
-        #       already in memory, we could read just the header and check for
-        #       any modification. If not modified, we can just leave things
-        #       alone
-        self._lock_token = lock.WriteLock(self._filename)
+        if self._lock_state is not None:
+            raise errors.LockContention(self._filename, lock_state)
+        self._lock_write()
+        self._wipe_state()
         self._lock_state = 'w'
-        self._state_file = self._lock_token.f
-        self._wipe_state()
+
+    def _lock_write(self):
+        raise NotImplementedError(self._lock_write)
+
+    def _requires_lock(self):
+        """Check that a lock is currently held by someone on the dirstate."""
+        if not self._lock_state:
+            raise errors.ObjectNotLocked(self)
+
+    def _restore_upgraded_lock(self):
+        """Restore a lock upgraded by _upgrade_physical_lock.
+
+        MUST be called if _upgrade_physical_lock returned True.
+        """
+        raise NotImplementedError(self._restore_upgraded_lock)
 
     def unlock(self):
         """Drop any locks held on the dirstate."""
-        if self._lock_token is None:
+        if self._lock_state is None:
             raise errors.LockNotHeld(self)
-        # TODO: jam 20070301 Rather than wiping completely, if the blocks are
-        #       already in memory, we could read just the header and check for
-        #       any modification. If not modified, we can just leave things
-        #       alone
-        self._state_file = None
+        self._unlock()
         self._lock_state = None
-        self._lock_token.unlock()
-        self._lock_token = None
-        self._split_path_cache = {}
-
-    def _requires_lock(self):
-        """Check that a lock is currently held by someone on the dirstate."""
-        if not self._lock_token:
-            raise errors.ObjectNotLocked(self)
+
+    def _unlock(self):
+        raise NotImplementedError(self._unlock)
+
+    def _upgrade_physical_lock(self):
+        """Called before writing if _lock_state isn't 'w'.
+
+        This method should make sure that a stat cache update is possible
+        and return False if it is not. Any state that it needs to pivot
+        to accomplish this should be stashed, and restored by
+        _restore_upgraded_lock.
+        """
+        raise NotImplementedError(self._upgrade_physical_lock)
 
 
 class DirState(AbstractDirState):
@@ -3084,6 +3099,67 @@
     a corrupt data structure on disk.
     """
 
+    def __init__(self, path, sha1_provider):
+        AbstractDirState.__init__(self, path, sha1_provider)
+        # Lock object that can be used to escalate locks etc.
+        self._lock_token = None
+
+    def _lock_read(self):
+        # TODO: jam 20070301 Rather than wiping completely, if the blocks are
+        #       already in memory, we could read just the header and check for
+        #       any modification. If not modified, we can just leave things
+        #       alone
+        self._lock_token = lock.ReadLock(self._filename)
+        self._state_file = self._lock_token.f
+
+    def _lock_write(self):
+        # TODO: jam 20070301 Rather than wiping completely, if the blocks are
+        #       already in memory, we could read just the header and check for
+        #       any modification. If not modified, we can just leave things
+        #       alone
+        self._lock_token = lock.WriteLock(self._filename)
+        self._state_file = self._lock_token.f
+
+    def _restore_upgraded_lock(self):
+        self._lock_token = self._lock_token.restore_read_lock()
+        self._state_file = self._lock_token.f
+        # TODO: jam 20070315 We should validate the disk file has
+        #       not changed contents. Since restore_read_lock may
+        #       not be an atomic operation.
+
+    def _save(self):
+        self._state_file.seek(0)
+        self._state_file.writelines(self.get_lines())
+        self._state_file.truncate()
+        self._state_file.flush()
+
+    def _unlock(self):
+        # TODO: jam 20070301 Rather than wiping completely, if the blocks are
+        #       already in memory, we could read just the header and check for
+        #       any modification. If not modified, we can just leave things
+        #       alone
+        self._state_file = None
+        self._lock_token.unlock()
+        self._lock_token = None
+        self._split_path_cache = {}
+
+    def _upgrade_physical_lock(self):
+        """Upgrade a 'r' lock to a write lock.
+
+        This does not change the lock_state - a conceptual read lock stays as
+        a read lock.
+        
+        :return: True if the upgrade succeeded.
+        """
+        grabbed_write_lock, new_lock = self._lock_token.temporary_write_lock()
+        # Switch over to the new lock, as the old one may be closed.
+        # TODO: jam 20070315 We should validate the disk file has
+        #       not changed contents. Since temporary_write_lock may
+        #       not be an atomic operation.
+        self._lock_token = new_lock
+        self._state_file = new_lock.f
+        return grabbed_write_lock
+
 
 class IndirectedDirState(AbstractDirState):
     """DirState implemented using an indirection pointer and hash-named files.
@@ -3093,36 +3169,108 @@
     file, and many content files named with their hash. Updates to
     IndirectedDirState are very robust and do not lock out other readers.
 
+    This is intended to be used with a WorkingTree object that has an explicit
+    LockDir lock to mutually exclude semantic writers. Thus there are two forms
+    of changes that can be made: changes from logical read-locks, which are
+    limited to stat cache updates, and changes from logical write-locks which
+    can add file ids, change parents etc. The design and semantics therefore
+    assume that semantic changes are externally limited to only one process
+    at a time, and that stat cache updates may happen anytime.
+
     Disk layout
     -----------
     basepath/
-      format = "bzr IndirectDirstate 1"
-      current = "MD5hash...\n". If missing then there will be one HASH.old
-        file which has the value current had if the read had started earlier,
-        and may be used.
-      MD5HASH.old = temp file used while replacing current. May only ever be
-        one - it is created by renaming current.
-      MD5HASH = a dirstate state file
-      MD5HASH.current = a candidate replacement for current, about to be
+      format = "bzr IndirectDirState 1"
+      current = "$MD5HASH\\n".
+      $MD5HASH.check = "$OTHERMD5HASH\\n". Used when checking if current should
+        be replaced.
+      $MD5HASH.delete = "$OTHERMD5HASH\\n". Used when a state file is queued for
+        deletion.
+      $MD5HASH = a dirstate state file
+      $MD5HASH.current = a candidate replacement for current, about to be
         moved into place.
 
+    Invariants
+    ----------
+
+    Only one of the following statements is true at any time:
+     - There is a file 'current'
+     - There is a file '*.check'
+     - There is a file pair 'MD5HASH.current + MD5HASH.delete'
+    This invariant gives us an unambiguous pointer at any point in time.
+    File system arbitrary-ordering can in principle break this invariant, as
+    we don't have fbarriers. Excluding that, a crash after renaming a .check
+    to .delete leaves the dirstate with no current, forcing recovery, and
+    preventing the creation of more .delete files. A crash after renaming
+    MD5HASH.current to current means that we cannot have both MD5HASH.current
+    and MD5HASH.delete, keeping the pointer unambiguous.
+
+    Operations
+    ----------
+
+    The insert sequence is:
+     - statefilename = md5(content)
+     - write new statefilename
+     - write statefilename.current
+     - rename current statefilename.check
+       If this step fails, then another process is attempting to write, or
+       the dirstate is in need of recovery. If a logical write is being
+       attempted, spinning is recommended; if a state cache update is in
+       progress, rollback is the most appropriate action.
+     - At this point we're in a mutex - no other processes can enter this
+       state without performing recovery. Checks such as 'was the current
+       pointer changed concurrently' can be performed here. This is needed
+       so that stat-cache only changes ('logical read-lock') can rollback
+       if the current pointer was changed (so that they do not overwrite
+       changes from a logical write-lock).
+     [rollback]
+      - rename statefilename.check current if current was renamed succesfully.
+        nb: other processes may be reading the .check file and thus this step
+        requires a small spinloop.
+      - remove statefilename.current
+      - remove statefilename
+     [complete]
+      - rename statefilename.check statefilename.delete
+        nb: other processes may be reading the .check file and thus this step
+        requires a small spinloop.
+      - rename statefilename.current current
+      - remove the old statefile named in statefilename.delete
+      - remove statefilename.delete
+
+    Read sequence:
+     - get the current pointer: read current, or if it is absent *.check (
+       handling a listed-but-gone when reading .check file as an absent pointer
+       - that is loop).
+     - read the pointer file, or if it is absent loop on getting a pointer
+
+    Closing a dirstate - lazy deletion of in-use MD5HASH files.
+     - consult the current pointer, if it does not refer to the last file read
+       then attempt to delete that file, as it is no longer in use.
+
+    Recovery:
+     - if there is a current, delete everything but the format and the file
+       current points at.
+     - elif there is a MD5HASH .check and no current, mv the .check to current
+       and then remove the rest.
+     - elif there is a MD5HASH .delete with a MD5HASH.current, mv the
+       MD5HASH.current to current and remove the rest. If there is more than
+       one, bail and seek human help. (See invariants)
+
+    Commentary
+    ----------
+
     The 'current' file contains a hash which is the current actual dirstate
-    to read. Updates have two forms - stat cache updates (when a logical
+    to read. 'current' may contain the special value INITIALIZING\\n during
+    creation. Updates have two forms - stat cache updates (when a logical
     read lock is in place), and semantic updates - when a write lock is in 
-    place. Updates write a new current file '$hash.current', move the 'current'
-    file to '$hash.old', and then move '$hash.current' to 'current', finally
-    removing '$hash.old'. If the Update is a stat cache update, a check is
-    done after moving 'current' to '$hash.old' - if the old hash is not the
-    hash that was meant to be replaced, then some other task has updated the
-    dirstate (and may have done more than a stat cache update). When this is
-    detected, '$hash.old' is moved back to 'current', and the stat cache
-    update discarded. Updaters are not permitted to write directly to 'current'
-    or to rename a '$hash.current' file to 'current' unless they currently
-    have '$hash.old'.
+    place. The decision about whether to update is taken while current is
+    temporarily renamed to the NEWMD5HASH.check. If it will be replaced, it
+    is replaced and then renamed to .delete to indicate that it should be
+    deleted by later readers doing recovery.
     After successfully updating 'current', the old state contained in the
-    content file named in '$hash.old' should be removed. If the removal
+    content file named in '$hash.delete' should be removed. If the removal
     errors due to other processing holding the file open, the errors can be
-    removed.
+    ignored.
     Finally, when closing a dirstate state file, 'current' should be checked,
     and if it is different to the dirstate file that was read initially, an
     attempt to remove that dirstate file should be made - last one out close
@@ -3131,16 +3279,124 @@
     When reading an IndirectedDirState, check for a format file to determine
     if the dirstate is a known format. If there is no current file, it is
     either in-progress or an interrupted update has occured. Readers should
-    look for *.old to get the last-written dirstate, and read that instead.
-    Concurrency concerns here mean that by the time .old is processed current
-    may have been updated and the old state file removed, so a short loop is
-    recommended.
+    look for *.check to get the last-written dirstate, and read that
+    instead.  Concurrency concerns here mean that by the time .check is
+    processed current may have been updated and the old state file removed, so
+    a short loop is recommended.
+    
+    Content files can possibly flip-flop by being restored to an identical
+    state, we may add a random component to guard against that as it can make
+    deleting unsafe.
 
-    If a write is fatally interrupted, recovery *must* rollback, not forward,
-    as a later client cannot tell if the update would have rolled back when
-    checking the '$hash.old' file.
+    :ivar _last_pointer: The MD5HASH of the last opened state file.
     """
 
+    DIRFORMAT = "bzr IndirectedDirState 1"
+
+    def __init__(self, path, sha1_provider):
+        AbstractDirState.__init__(self, path, sha1_provider)
+        self._last_pointer = None
+        self._transport = transport.get_transport(path)
+
+    def _check_pointer_safe(self, check_name):
+        # Prevent hostile data causing arbitrary file to be unlinked
+        # this may also need a urlunescape to be thorough. However  it won't
+        # permit arbitrary data to be _written_ under any circumstances.
+        if '.' in check_name or '/' in check_name or '\\' in check_name:
+            raise Exception('invalid check_name %r' % check_name)
+
+    def _initialize(self):
+        """Create any on-disk structures needed for the initialize method."""
+        self._transport.mkdir('.')
+        self._transport.put_bytes_non_atomic('current', 'INITIALIZING\n')
+
+    def _lock_read(self):
+        self._set_state_file()
+
+    def _lock_write(self):
+        self._set_state_file()
+
+    def _restore_upgraded_lock(self):
+        # As _upgrade_physical_lock is a no-op, this method is too.
+        pass
+
+    def _save(self):
+        bytes = ''.join(self.get_lines())
+        name = osutils.md5(bytes).hexdigest()
+        name_check = name + '.check'
+        current_new = name + '.current'
+        name_delete = name + '.delete'
+        self._transport.put_bytes_non_atomic(name, bytes)
+        self._transport.put_bytes_non_atomic(current_new, name + '\n')
+        # TODO spin on this in 'w' locks, for concurrency with stat cache
+        # updates.
+        self._transport.rename('current', name_check)
+        old_pointer = self._transport.get_bytes(name_check).strip()
+        self._check_pointer_safe(old_pointer)
+        if self._lock_state != 'w':
+            # Hash cache update - check we can update.
+            if old_pointer != self._last_pointer:
+                # TODO: spin on this, to handle resource busy on windows
+                # when another process is reading the pointer from the check
+                # file.
+                self._transport.rename(name_check, 'current')
+                self._transport.delete(current_new)
+                self._read_link.delete(name)
+                # not saving in _save in a 'r' lock is not an error.
+                return True
+        # Claim that we're committing
+        self._transport.rename(name_check, name_delete)
+        # Linkin the new current pointer
+        self._transport.rename(current_new, 'current')
+        # Attempt to remove the old state file
+        try:
+            self._transport.delete(old_pointer)
+        except (errors.NoSuchFile, errors.ResourceBusy,
+            errors.PermissionDenied):
+            pass
+        self._transport.delete(name_delete)
+        if old_pointer == 'INITIALIZING':
+            # We've completed initialization.
+            self._transport.put_bytes_non_atomic('format',
+                IndirectedDirState.DIRFORMAT)
+        self._state_file = self._transport.get(name)
+        
+    def _set_try_state_file(self):
+        pointer = None
+        try:
+            pointer = self._transport.get_bytes('current')[:-1]
+        except errors.NoSuchFile:
+            names = self._transport.list_dir('.')
+            for name in names:
+                if name.endswith('.check'):
+                    try:
+                        pointer = self._transport.get_bytes(name)[:-1]
+                    except errors.NoSuchFile:
+                        pass
+            if pointer is None:
+                return False
+        self._check_pointer_safe(pointer)
+        try:
+            self._state_file = self._transport.get(pointer)
+        except errors.NoSuchFile:
+            return False
+        self._last_pointer = pointer
+        return True
+
+    def _set_state_file(self):
+        """Set self._state_file to a state file from disk."""
+        for spin in range(10):
+            if self._set_try_state_file():
+                return
+
+    def _unlock(self):
+        self._state_file = None
+        # XXX: TODO garbage collect state files on disk.
+
+    def _upgrade_physical_lock(self):
+        # As no physical locks are needed, this is a no-op
+        return True
+
 
 def py_update_entry(state, entry, abspath, stat_value,
                  _stat_to_minikind=DirState._stat_to_minikind,

=== modified file 'bzrlib/tests/test_dirstate.py'
--- a/bzrlib/tests/test_dirstate.py	2009-09-28 23:22:56 +0000
+++ b/bzrlib/tests/test_dirstate.py	2009-09-29 03:32:18 +0000
@@ -29,6 +29,7 @@
     tests,
     )
 from bzrlib.tests import test_osutils
+from bzrlib.transport import get_transport
 
 
 # TODO:
@@ -192,7 +193,7 @@
         """
         # The state should already be write locked, since we just had to do
         # some operation to get here.
-        self.assertTrue(state._lock_token is not None)
+        self.assertTrue(state._lock_state is not None)
         try:
             self.assertEqual(expected_result[0],  state.get_parent_ids())
             # there should be no ghosts in this tree.
@@ -329,10 +330,20 @@
             expected[path2] = (new_key, [orig[1][0],
                                          dirstate.DirState.NULL_PARENT_DETAILS])
 
-        # We will replace the 'dirstate' file underneath 'state', but that is
-        # okay as lock as we unlock 'state' first.
+        self.replace_state_from_tree(state, tree)
+        return tree, state, expected
+
+    def replace_state_from_tree(self, state, tree):
+        # We will replace the 'dirstate' underneath 'state', but that is okay
+        # as lock as we unlock 'state' first.
         state.unlock()
         try:
+            try:
+                # Delete dirstate 2 directory, because that isn't triviall
+                # replaceable.
+                get_transport(self.get_url('dirstate')).delete_tree('.')
+            except errors.NoSuchFile:
+                pass
             new_state = self._dirstate_class.from_tree(tree, 'dirstate')
             try:
                 new_state.save()
@@ -342,7 +353,6 @@
             # But we need to leave state in a read-lock because we already have
             # a cleanup scheduled
             state.lock_read()
-        return tree, state, expected
 
     def create_renamed_dirstate(self):
         """Create a dirstate with a few internal renames.
@@ -369,16 +379,7 @@
                              old_e[1][1]])
         expected['h/e'] = (('h', 'e', 'e-id'), [old_e[1][0],
                                                 ('r', 'b/d/e', 0, False, '')])
-
-        state.unlock()
-        try:
-            new_state = self._dirstate_class.from_tree(tree, 'dirstate')
-            try:
-                new_state.save()
-            finally:
-                new_state.unlock()
-        finally:
-            state.lock_read()
+        self.replace_state_from_tree(state, tree)
         return tree, state, expected
 
 
@@ -546,13 +547,11 @@
 
     def test_construct_with_path(self):
         tree = self.make_branch_and_tree('tree')
-        state = self._dirstate_class.from_tree(tree, 'dirstate.from_tree')
-        # we want to be able to get the lines of the dirstate that we will
-        # write to disk.
-        lines = state.get_lines()
+        # Create a dirstate
+        state = self._dirstate_class.from_tree(tree, 'dirstate')
+        state.save()
         state.unlock()
-        self.build_tree_contents([('dirstate', ''.join(lines))])
-        # get a state object
+        # get a state structure we exepct on_file to find.
         # no parents, default tree content
         expected_result = ([], [
             (('', '', tree.get_root_id()), # common details
@@ -656,13 +655,10 @@
             state2 = self._dirstate_class.on_file('dirstate')
             state2.lock_read()
             try:
-                # This won't actually write anything, because it couldn't grab
-                # a write lock. But it shouldn't raise an error, either.
-                # TODO: jam 20070315 We should probably distinguish between
-                #       being dirty because of 'update_entry'. And dirty
-                #       because of real modification. So that save() *does*
-                #       raise a real error if it fails when we have real
-                #       modifications.
+                # The could write, or could silently not write, depending
+                # on whether its Dirstate or IndirectedDirState. Either way
+                # it must not error. We know its not a semantic change because
+                # the object is read-locked.
                 state.save()
             finally:
                 state2.unlock()
@@ -732,10 +728,6 @@
             lines = state.get_lines()
         finally:
             state.unlock()
-        # On win32 you can't read from a locked file, even within the same
-        # process. So we have to unlock and release before we check the file
-        # contents.
-        self.assertFileEqual(''.join(lines), 'dirstate')
         state.lock_read() # check_state_with_reopen will unlock
         self.check_state_with_reopen(expected_result, state)
 
@@ -2309,3 +2301,30 @@
         self.assertTrue(len(statvalue) >= 10)
         self.assertEqual(len(text), statvalue.st_size)
         self.assertEqual(expected_sha, sha1)
+
+
+class TestOneFileDirstate(tests.TestCaseWithTransport):
+    """Tests for specific behaviour unique to DirState."""
+
+    def test_initialize(self):
+        state = dirstate.DirState.initialize('dirstate')
+        lines = state.get_lines()
+        state.unlock()
+        # On win32 you can't read from a locked file, even within the same
+        # process. So we have to unlock and release before we check the file
+        # contents.
+        self.assertFileEqual(''.join(lines), 'dirstate')
+
+
+class TestIndirectDirState(tests.TestCaseWithMemoryTransport):
+    """Tests for specific behaviour unique to IndirectedDirState."""
+
+    def test_initialize(self):
+        # The parameterised tests test the semantic value of 'initialise',
+        # this test tests that the physical layout matches our expectations.
+        state = dirstate.IndirectedDirState.initialize(self.get_url('foo'))
+        t = get_transport(self.get_url('foo'))
+        self.assertEqual('bzr IndirectedDirState 1', t.get_bytes('format'))
+        current_hash = t.get_bytes('current').strip()
+        self.assertNotEqual('INITIALIZING', current_hash)
+        self.assertTrue(t.has(current_hash))




More information about the bazaar-commits mailing list