[MERGE] Handle existing files cleanly in build_tree

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Arbash Meinel wrote:
> Aaron Bentley wrote:
> v- It might be better to read 'source/file' and write it to the target.

Done.  I had that at first, then I tried to clean it up.

> ^- And here, you want ...Equal(0, len(target2.conflicts())

Okay, all the assertEquals should be in the right order.

> Though, what is the format of 'conflicts'? If it is a list, I would
> rather see:
> self.assertEqual([], target2.conflicts())

That's doable; the result is a ConflictList, but they compare equal to
lists with the same contents.  I've assigned file-ids to many of the
files, so I get predictable conflicts.

> +        # Ensure new contents are suppressed for existing branches
> +        target = self.make_branch('target3')
> +        target = self.make_branch_and_tree('target3/dir1')
> +        self.build_tree(['target3/dir1/file2'])
> +        build_tree(source.basis_tree(), target)
> +        self.assertEqual(0, len(target.conflicts()))
> +        self.failIfExists('target3/dir1/file1')
> +        self.failUnlessExists('target3/dir1/file2')
> 
> ^- Don't you want to check that there is a conflict in the 'target3'
> directory? Since it cannot create 'dir1/file'?

Me and Rob have tweaked the behavior: if a file 'foo' cannot be created
because of a bzrdir in the way, it gets created as 'foo.diverted'.

> v- This is the second time I've seen you need a try/except. Is there a
> reason raising NoSuchFile is better than just returning None?

It's designed to be consistent with the Tree API, where Tree.kind raises
NoSuchFile and Tree.path2id returns None.

> +                try:
> +                    kind = self.final_kind(trans_id)
> +                except NoSuchFile:
> +                    kind = None
> +                file_id = self.final_file_id(trans_id)
> +                if kind is None and file_id is None:
> +                    continue
>                  if name == last_name:
>                      conflicts.append(('duplicate', last_trans_id, trans_id,
>                      name))
> -                try:
> -                    kind = self.final_kind(trans_id)
> -                except NoSuchFile:
> -                    kind = None
> -                file_id = self.final_file_id(trans_id)
> -                if kind is not None or file_id is not None:
> -                    last_name = name
> -                    last_trans_id = trans_id
> +                last_name = name
> +                last_trans_id = trans_id
>          return conflicts
> 
> ^- did you actually change the logic here? Or am I reading it correctly
> that you just re-ordered the code. (I guess you avoid adding 'duplicate'
> entries)

Yes, there was a slight change.  If the current entry has no file_id or
contents, but the previous entry had the same name, the old logic would
produce a spurious duplicate conflict.  The new logic will continue
before this can happen.

This was needed to pass the test suite, but with luck, it also squashes
https://launchpad.net/products/bzr/+bug/52895

> +    - Otherwise, if the content on disk matches the content we are
> building,
> +      it is silently replaced.
> 
> ^- Does this include if the existing content is versioned? Or only if it
> is unversioned? (It might be that you can't/(we don't) call build_tree
> on a working tree, only on a Branch).

It's now impossible to run build_tree on a populated tree.  (It can have
a root, but that's it.)

> ^- This is a side thing, but long-term is there a way we can do this
> without calling id2path. Just because id2path has to walk all over the
> inventory each call, rather than just knowing that you just processed
> the directory before you got to the children.

As discussed, I've switched to iter_entries_by_dir().

> +                if (suppress_children is not None and
> +                    tree_path.startswith(suppress_children)):
> +                    continue
> +                target_path = wt.abspath(tree.id2path(file_id))
> 
> ^- trailing whitespace here, you also are calling id2path 2 times for
> the same file_id. Would be better to re-use the 'tree_path' variable.

Done.

> v- Should this be a helper, or a member of TreeTransform?

I'd rather keep it out of TreeTransform.  TreeTransform's complicated
enough already.

I've attached a new bundle that should address all your concerns.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.2 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFE9Qxj0F+nu1YWqI0RAn9LAJ0YapeuGjf+tK6sTgBEM762P1++0wCeKtDy
XRz8ATFgVhj3MEHaY6NQJ80=
=PQHf
-----END PGP SIGNATURE-----
-------------- next part --------------
A non-text attachment was scrubbed...
Name: build-resolver.patch
Type: text/x-patch
Size: 31417 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060829/7787578c/attachment.bin