[merge] [#146176] fix Dirstate.set_state_from_inventory
Robert Collins
robertc at robertcollins.net
Tue Oct 9 03:36:40 BST 2007
bb:tweak
I've made some comments but its in good enough shape that you can merge
whether you update based on them or not.
-Rob
On Mon, 2007-10-08 at 19:45 +1000, Martin Pool wrote:
> current_old = old_iterator.next()
> + # XXX: if we're generating things in order, why do we need to
> call
> + # update_minimal, rather than just generating the whole list
> in one
> + # go?
is it always in-order? what about renames that have occured, which means
the order can be either the dirstates order or the inventories order but
not both - which IIRC is what update-minimal handles. It would be better
I think if the XXX was a little bit more clear about *which* order it
means, or possibly a comment saying why update-minimal is used.
> # 5 cases, we dont have a value that is strictly greater
> than everything, so
> # we make both end conditions explicit
> - if not current_old:
> + if current_old is None:
> # old is finished: insert current_new into the state.
> self.update_minimal(new_entry_key, current_new_minikind,
> executable=current_new[1].executable,
> path_utf8=new_path_utf8, fingerprint=fingerprint)
> current_new = advance(new_iterator)
> - elif not current_new:
> + elif current_new is None:
Just a note here -
'not current_new' is measurably faster than 'current_new is None'. Is
there some reason to change these two conditions?
> + # XXX: Some callers pass '' as the packed_stat, and it seems to be
> + # sometimes present in the dirstate - this seems oddly inconsistent.
> + # mbp 20071008
Indeed this is, rather than an XXX, perhaps this would be better as a
trivial validate() item to log it, and/or a bug report.
> === modified file 'bzrlib/tests/test_dirstate.py'
> --- bzrlib/tests/test_dirstate.py 2007-10-08 04:24:19 +0000
> +++ bzrlib/tests/test_dirstate.py 2007-10-08 09:40:21 +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.edge.launchpad.net/bzr/+bug/146176
If you are putting url's in, please use a url everyone can access :).
> + # set_state_from_inventory should preserve the stat and hash value for
> + # workingtree files that are not changed by the inventory.
It might be worth noting somewhere that we should also preserve the stat
and hash value for changed files when:
- the kind was file, and stays as file
e.g. a rename of src/ to oldsrc/ should not invalidate the stat cache
for all the files within those directories.
> + tree = self.make_branch_and_tree('.')
> + # depends on the default format using dirstate...
This is easy to fix:
tree = self.make_branch_and_tree('.',
format=bzrdir.format_registry.make_bzrdir('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')
You can more pithily use
self.build_tree(['foo'])
Which will set deterministic content for you, but this is minor.
> ...
> + # no-op change is enough to make it lose state
> + tree._dirstate.set_state_from_inventory(inv)
I think the above comment is state, as you're fixing this bug right ? :)
-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20071009/752781b7/attachment.pgp
More information about the bazaar
mailing list