Rev 2899: Fix incorrect comparison in Dirstate.set_state_from_inventory, and bug #146176 in file:///home/pqm/archives/thelove/bzr/%2Btrunk/

Canonical.com Patch Queue Manager pqm at pqm.ubuntu.com
Tue Oct 9 09:08:23 BST 2007


At file:///home/pqm/archives/thelove/bzr/%2Btrunk/

------------------------------------------------------------
revno: 2899
revision-id: pqm at pqm.ubuntu.com-20071009080820-civ61gexahohpats
parent: pqm at pqm.ubuntu.com-20071009044446-uliu5z9a52bzmps8
parent: mbp at sourcefrog.net-20071009040940-yjwm33zkzp033j8q
committer: Canonical.com Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Tue 2007-10-09 09:08:20 +0100
message:
  Fix incorrect comparison in Dirstate.set_state_from_inventory, and bug #146176
modified:
  bzrlib/_dirstate_helpers_c.pyx dirstate_helpers.pyx-20070503201057-u425eni465q4idwn-3
  bzrlib/_dirstate_helpers_py.py _dirstate_helpers_py-20070710145033-90nz6cqglsk150jy-1
  bzrlib/dirstate.py             dirstate.py-20060728012006-d6mvoihjb3je9peu-1
  bzrlib/tests/test_dirstate.py  test_dirstate.py-20060728012006-d6mvoihjb3je9peu-2
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.11
    merged: mbp at sourcefrog.net-20071009040940-yjwm33zkzp033j8q
    parent: mbp at sourcefrog.net-20071009033533-zq8njirj71qmm2td
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Tue 2007-10-09 14:09:40 +1000
    message:
      Review documentation cleanups
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.10
    merged: mbp at sourcefrog.net-20071009033533-zq8njirj71qmm2td
    parent: mbp at sourcefrog.net-20071009024202-3t29natdi3lvwenl
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Tue 2007-10-09 13:35:33 +1000
    message:
      docstrings for cmp_ functions seem to be backwards
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.9
    merged: mbp at sourcefrog.net-20071009024202-3t29natdi3lvwenl
    parent: mbp at sourcefrog.net-20071008094021-d2mdlo8u4naiftv3
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Tue 2007-10-09 12:42:02 +1000
    message:
      Use cmp_by_dirs in set_state_from_inventory rather than hardcoding the equivalent
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.8
    merged: mbp at sourcefrog.net-20071008094021-d2mdlo8u4naiftv3
    parent: mbp at sourcefrog.net-20071008093743-b8m904v0grm602kw
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Mon 2007-10-08 19:40:21 +1000
    message:
      Clear up test code
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.7
    merged: mbp at sourcefrog.net-20071008093743-b8m904v0grm602kw
    parent: mbp at sourcefrog.net-20071008092834-oz18bb2l2y8eeem4
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Mon 2007-10-08 19:37:43 +1000
    message:
      More cleanups of dirstate work
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.6
    merged: mbp at sourcefrog.net-20071008092834-oz18bb2l2y8eeem4
    parent: mbp at sourcefrog.net-20071008092743-m5kd304bxitipjb9
    parent: pqm at pqm.ubuntu.com-20071008081200-04a0byt5lo5tyft9
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Mon 2007-10-08 19:28:34 +1000
    message:
      merge trunk
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.5
    merged: mbp at sourcefrog.net-20071008092743-m5kd304bxitipjb9
    parent: mbp at sourcefrog.net-20071008092114-em6foez3xs0vfsr2
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Mon 2007-10-08 19:27:43 +1000
    message:
      doc
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.4
    merged: mbp at sourcefrog.net-20071008092114-em6foez3xs0vfsr2
    parent: mbp at sourcefrog.net-20071008091737-lbed3avpxvwsbeoh
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Mon 2007-10-08 19:21:14 +1000
    message:
      Check parameters to update_minimal
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.3
    merged: mbp at sourcefrog.net-20071008091737-lbed3avpxvwsbeoh
    parent: mbp at sourcefrog.net-20071005094528-yi0g5jbe7adconwv
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Mon 2007-10-08 19:17:37 +1000
    message:
      Fix comparison for merge sort in Dirstate.set_state_from_inventory
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.2
    merged: mbp at sourcefrog.net-20071005094528-yi0g5jbe7adconwv
    parent: mbp at sourcefrog.net-20071005093611-sxy5dwz4k01g3cq0
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Fri 2007-10-05 19:45:28 +1000
    message:
      Treat set_state_from_inventory as evil, and del dead variables rather than just setting to null
    ------------------------------------------------------------
    revno: 2872.1.1.1.2.1.1
    merged: mbp at sourcefrog.net-20071005093611-sxy5dwz4k01g3cq0
    parent: mbp at sourcefrog.net-20070928075828-vp07yjxa2x29ckjq
    committer: Martin Pool <mbp at sourcefrog.net>
    branch nick: 146176-dirstate
    timestamp: Fri 2007-10-05 19:36:11 +1000
    message:
      Add xfail test for #146176
=== modified file 'bzrlib/_dirstate_helpers_c.pyx'
--- a/bzrlib/_dirstate_helpers_c.pyx	2007-08-02 21:00:51 +0000
+++ b/bzrlib/_dirstate_helpers_c.pyx	2007-10-09 03:35:33 +0000
@@ -196,9 +196,9 @@
 
     :param path1: first path
     :param path2: second path
-    :return: positive number if ``path1`` comes first,
+    :return: negative number if ``path1`` comes first,
         0 if paths are equal,
-        and negative number if ``path2`` sorts first
+        and positive number if ``path2`` sorts first
     """
     if not PyString_CheckExact(path1):
         raise TypeError("'path1' must be a plain string, not %s: %r"
@@ -224,9 +224,9 @@
 
     :param path1: first path
     :param path2: the second path
-    :return: positive number if ``path1`` comes first,
+    :return: negative number if ``path1`` comes first,
         0 if paths are equal
-        and a negative number if ``path2`` sorts first
+        and a positive number if ``path2`` sorts first
     """
     if not PyString_CheckExact(path1):
         raise TypeError("'path1' must be a plain string, not %s: %r"

=== modified file 'bzrlib/_dirstate_helpers_py.py'
--- a/bzrlib/_dirstate_helpers_py.py	2007-07-18 20:30:14 +0000
+++ b/bzrlib/_dirstate_helpers_py.py	2007-10-09 03:35:33 +0000
@@ -144,9 +144,9 @@
 
     :param path1: first path
     :param path2: second path
-    :return: positive number if ``path1`` comes first,
+    :return: negative number if ``path1`` comes first,
         0 if paths are equal,
-        and negative number if ``path2`` sorts first
+        and positive number if ``path2`` sorts first
     """
     if not isinstance(path1, str):
         raise TypeError("'path1' must be a plain string, not %s: %r"
@@ -166,9 +166,9 @@
 
     :param path1: first path
     :param path2: the second path
-    :return: positive number if ``path1`` comes first,
+    :return: negative number if ``path1`` comes first,
         0 if paths are equal
-        and a negative number if ``path2`` sorts first
+        and a positive number if ``path2`` sorts first
     """
     if not isinstance(path1, str):
         raise TypeError("'path1' must be a plain string, not %s: %r"

=== modified file 'bzrlib/dirstate.py'
--- a/bzrlib/dirstate.py	2007-09-28 07:09:05 +0000
+++ b/bzrlib/dirstate.py	2007-10-09 04:09:40 +0000
@@ -1860,10 +1860,20 @@
 
         :param new_inv: The inventory object to set current state from.
         """
+        if 'evil' in debug.debug_flags:
+            trace.mutter_callsite(1,
+                "set_state_from_inventory called; please mutate the tree instead")
         self._read_dirblocks_if_needed()
         # sketch:
-        # incremental algorithm:
-        # two iterators: current data and new data, both in dirblock order. 
+        # Two iterators: current data and new data, both in dirblock order. 
+        # We zip them together, which tells about entries that are new in the
+        # inventory, or removed in the inventory, or present in both and
+        # possibly changed.  
+        #
+        # You might think we could just synthesize a new dirstate directly
+        # since we're processing it in the right order.  However, we need to
+        # also consider there may be any number of parent trees and relocation
+        # pointers, and we don't want to duplicate that here.
         new_iterator = new_inv.iter_entries_by_dir()
         # we will be modifying the dirstate, so we need a stable iterator. In
         # future we might write one, for now we just clone the state into a
@@ -1894,10 +1904,15 @@
                 if current_new_minikind == 't':
                     fingerprint = current_new[1].reference_revision or ''
                 else:
+                    # We normally only insert or remove records, or update
+                    # them when it has significantly changed.  Then we want to
+                    # erase its fingerprint.  Unaffected records should
+                    # normally not be updated at all.
                     fingerprint = ''
             else:
                 # for safety disable variables
-                new_path_utf8 = new_dirname = new_basename = new_id = new_entry_key = None
+                new_path_utf8 = new_dirname = new_basename = new_id = \
+                    new_entry_key = None
             # 5 cases, we dont have a value that is strictly greater than everything, so
             # we make both end conditions explicit
             if not current_old:
@@ -1912,6 +1927,9 @@
                 current_old = advance(old_iterator)
             elif new_entry_key == current_old[0]:
                 # same -  common case
+                # We're looking at the same path and id in both the dirstate
+                # and inventory, so just need to update the fields in the
+                # dirstate from the one in the inventory.
                 # TODO: update the record if anything significant has changed.
                 # the minimal required trigger is if the execute bit or cached
                 # kind has changed.
@@ -1923,8 +1941,9 @@
                 # both sides are dealt with, move on
                 current_old = advance(old_iterator)
                 current_new = advance(new_iterator)
-            elif (new_entry_key[0].split('/') < current_old[0][0].split('/')
-                  and new_entry_key[1:] < current_old[0][1:]):
+            elif (cmp_by_dirs(new_dirname, current_old[0][0]) < 0
+                  or (new_dirname == current_old[0][0]
+                      and new_entry_key[1:] < current_old[0][1:])):
                 # new comes before:
                 # add a entry for this and advance new
                 self.update_minimal(new_entry_key, current_new_minikind,
@@ -1932,7 +1951,8 @@
                     path_utf8=new_path_utf8, fingerprint=fingerprint)
                 current_new = advance(new_iterator)
             else:
-                # old comes before:
+                # we've advanced past the place where the old key would be,
+                # without seeing it in the new list.  so it must be gone.
                 self._make_absent(current_old)
                 current_old = advance(old_iterator)
         self._dirblock_state = DirState.IN_MEMORY_MODIFIED
@@ -1998,15 +2018,22 @@
         :param minikind: The type for the entry ('f' == 'file', 'd' ==
                 'directory'), etc.
         :param executable: Should the executable bit be set?
-        :param fingerprint: Simple fingerprint for new entry.
-        :param packed_stat: packed stat value for new entry.
+        :param fingerprint: Simple fingerprint for new entry: sha1 for files, 
+            referenced revision id for subtrees, etc.
+        :param packed_stat: Packed stat value for new entry.
         :param size: Size information for new entry
         :param path_utf8: key[0] + '/' + key[1], just passed in to avoid doing
                 extra computation.
+
+        If packed_stat and fingerprint are not given, they're invalidated in
+        the entry.
         """
         block = self._find_block(key)[1]
         if packed_stat is None:
             packed_stat = DirState.NULLSTAT
+        # XXX: Some callers pass '' as the packed_stat, and it seems to be
+        # sometimes present in the dirstate - this seems oddly inconsistent.
+        # mbp 20071008
         entry_index, present = self._find_entry_index(key, block)
         new_details = (minikind, fingerprint, size, executable, packed_stat)
         id_index = self._get_id_index()

=== modified file 'bzrlib/tests/test_dirstate.py'
--- a/bzrlib/tests/test_dirstate.py	2007-10-08 04:24:19 +0000
+++ b/bzrlib/tests/test_dirstate.py	2007-10-09 04:09:40 +0000
@@ -700,6 +700,67 @@
             # This will unlock it
             self.check_state_with_reopen(expected_result, state)
 
+    def test_set_state_from_inventory_preserves_hashcache(self):
+        # https://bugs.launchpad.net/bzr/+bug/146176
+        # set_state_from_inventory should preserve the stat and hash value for
+        # workingtree files that are not changed by the inventory.
+       
+        tree = self.make_branch_and_tree('.')
+        # depends on the default format using dirstate...
+        tree.lock_write()
+        try:
+            # make a dirstate with some valid hashcache data 
+            # file on disk, but that's not needed for this test
+            foo_contents = 'contents of foo'
+            self.build_tree_contents([('foo', foo_contents)])
+            tree.add('foo', 'foo-id')
+
+            foo_stat = os.stat('foo')
+            foo_packed = dirstate.pack_stat(foo_stat)
+            foo_sha = osutils.sha_string(foo_contents)
+            foo_size = len(foo_contents)
+
+            # should not be cached yet, because the file's too fresh
+            self.assertEqual(
+                (('', 'foo', 'foo-id',),
+                 [('f', '', 0, False, dirstate.DirState.NULLSTAT)]),
+                tree._dirstate._get_entry(0, 'foo-id'))
+            # poke in some hashcache information - it wouldn't normally be
+            # stored because it's too fresh
+            tree._dirstate.update_minimal(
+                ('', 'foo', 'foo-id'),
+                'f', False, foo_sha, foo_packed, foo_size, 'foo')
+            # now should be cached
+            self.assertEqual(
+                (('', 'foo', 'foo-id',),
+                 [('f', foo_sha, foo_size, False, foo_packed)]),
+                tree._dirstate._get_entry(0, 'foo-id'))
+           
+            # extract the inventory, and add something to it
+            inv = tree._get_inventory()
+            # should see the file we poked in...
+            self.assertTrue(inv.has_id('foo-id'))
+            self.assertTrue(inv.has_filename('foo'))
+            inv.add_path('bar', 'file', 'bar-id')
+            # this used to cause it to lose its hashcache
+            tree._dirstate.set_state_from_inventory(inv)
+        finally:
+            tree.unlock()
+
+        tree.lock_read()
+        try:
+            # now check that the state still has the original hashcache value
+            state = tree._dirstate
+            foo_tuple = state._get_entry(0, path_utf8='foo')
+            self.assertEqual(
+                (('', 'foo', 'foo-id',),
+                 [('f', foo_sha, len(foo_contents), False,
+                   dirstate.pack_stat(foo_stat))]),
+                foo_tuple)
+        finally:
+            tree.unlock()
+
+
     def test_set_state_from_inventory_mixed_paths(self):
         tree1 = self.make_branch_and_tree('tree1')
         self.build_tree(['tree1/a/', 'tree1/a/b/', 'tree1/a-b/',




More information about the bazaar-commits mailing list