[merge] [#146176] fix Dirstate.set_state_from_inventory

[Robert Collins]

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