[MERGE] Handle existing files cleanly in build_tree
John Arbash Meinel
john at arbash-meinel.com
Tue Aug 29 22:31:51 BST 2006
Aaron Bentley wrote:
> Hi all,
>
> This patch addresses the need to handle existing content in directories
> when creating checkouts. It fixes #55460.
>
> At first, I was really horrified by why Robert wanted to do in a
> conflict resolver. Then I started to look at the behaviour as a tree
> merger, and it made a lot more sense like that.
>
> So the conflict resolution section is really bare-bones. It just
> detects duplicate entries and renames the old files to 'foo.moved'.
>
> The rest of it:
> - Ensures that bzrdirs are never replaced or populated by build_tree
> - Prevents conflicts when the file-to-add matches the already-present file
> - Merges the contents of directories
>
> Aaron
...
v- It might be better to read 'source/file' and write it to the target.
Specifically because build_tree() defaults to 'native' mode (for some
weird reason), which means that on Win32 this test will fail, because
the target is missing a '\r'. Also, the contents of files created with
build_tree might change in the future.
+ target_file = file('target2/file', 'wb')
+ try:
+ target_file.write('contents of source/file\n')
+ finally:
+ target_file.close()
+ build_tree(source.basis_tree(), target2)
+ self.assertEqual(len(target2.conflicts()), 0)
^- And here, you want ...Equal(0, len(target2.conflicts())
Though, what is the format of 'conflicts'? If it is a list, I would
rather see:
self.assertEqual([], target2.conflicts())
Or for a dictionary substitute "{}".
Because that way the error message will tell you what conflicts exist
that shouldn't be there. Rather than just an indication that there is a
conflict that you need to track down.
+
+ def test_symlink_conflict_handling(self):
+ """Ensure that when building trees, conflict handling is done"""
+ if not has_symlinks():
+ raise TestSkipped('Test requires symlink support')
+ source = self.make_branch_and_tree('source')
+ os.symlink('foo', 'source/symlink')
+ source.add('symlink')
+ source.commit('added file')
+ target = self.make_branch_and_tree('target')
+ os.symlink('bar', 'target/symlink')
+ build_tree(source.basis_tree(), target)
+ self.assertEqual(1, len(target.conflicts()))
+
+ target = self.make_branch_and_tree('target2')
+ os.symlink('foo', 'target2/symlink')
+ build_tree(source.basis_tree(), target)
+ self.assertEqual(0, len(target.conflicts()))
^- here you got the order right, but see my earlier comment about
comparing against a dict/list.
+
+ def test_directory_conflict_handling(self):
+ """Ensure that when building trees, conflict handling is done"""
+ source = self.make_branch_and_tree('source')
+ target = self.make_branch_and_tree('target')
+ self.build_tree(['source/dir1/', 'source/dir1/file',
'target/dir1/'])
+ source.add(['dir1', 'dir1/file'])
+ source.commit('added file')
+ build_tree(source.basis_tree(), target)
+ self.assertEqual(0, len(target.conflicts()))
+ self.failUnlessExists('target/dir1/file')
+
+ # Ensure contents are merged
+ target = self.make_branch_and_tree('target2')
+ self.build_tree(['target2/dir1/', 'target2/dir1/file2'])
+ build_tree(source.basis_tree(), target)
+ self.assertEqual(0, len(target.conflicts()))
+ self.failUnlessExists('target2/dir1/file2')
+ self.failUnlessExists('target2/dir1/file')
+
+ # 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'?
...
last_name = None
last_trans_id = None
for name, trans_id in name_ids:
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? I suppose
it might be an api change, so it is better to leave this in. But it
makes me wonder if you have to keep trapping NoSuchFile all the time.
Also, the fact that final_kind raises an exception, but final_file_id
doesn't is a little weird.
+ 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)
def _duplicate_ids(self):
@@ -934,36 +936,127 @@
file_ids.sort(key=tree.id2path)
return file_ids
+
def build_tree(tree, wt):
- """Create working tree for a branch, using a Transaction."""
+ """Create working tree for a branch, using a TreeTransform.
+
+ Existing files are handled like so:
+
+ - Existing bzrdirs take precedence over creating directories. New
+ directory contents are silently dropped.
^- it seems like it should be a conflict if I can't create a file where
I want to put it. And while you may not have a place to put the
conflicted file, it seems important enough to not just delete the file
from the parent.
+ - 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).
+ - Otherwise, conflict resolution will move the old file to
'oldname.moved'.
+ """
...
try:
for num, file_id in enumerate(file_ids):
pb.update("Building tree", num, len(file_ids))
entry = tree.inventory[file_id]
if entry.parent_id is None:
continue
+ reparent = False
+ tree_path = tree.id2path(file_id)
^- 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.
+ 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.
+ try:
+ kind = file_kind(target_path)
+ except NoSuchFile:
+ pass
+ else:
+ if kind == "directory":
+ try:
+ bzrdir.BzrDir.open(target_path)
+ except errors.NotBranchError:
+ pass
+ else:
+ suppress_children = tree_path + '/'
+ continue
+ if _content_match(tree, entry, file_id,
+ kind, target_path):
+
tt.delete_contents(tt.trans_id_tree_path(tree_path))
+ if kind == 'directory':
+ reparent = True
if entry.parent_id not in file_trans_id:
raise repr(entry.parent_id)
parent_id = file_trans_id[entry.parent_id]
file_trans_id[file_id] = new_by_entry(tt, entry,
parent_id,
tree)
+ if reparent:
+ new_trans_id = file_trans_id[file_id]
+ old_parent = tt.trans_id_tree_path(tree_path)
+ _reparent_children(tt, old_parent, new_trans_id)
finally:
pb.finished()
pp.next_phase()
+ raw_conflicts = resolve_conflicts(tt, pass_func=resolve_checkout)
+ conflicts = cook_conflicts(raw_conflicts, tt)
+ for conflict in conflicts:
+ warning(conflict)
+ try:
+ wt.add_conflicts(conflicts)
+ except errors.UnsupportedOperation:
+ pass
tt.apply()
finally:
tt.finalize()
top_pb.finished()
v- Should this be a helper, or a member of TreeTransform?
+
+def _reparent_children(tt, old_parent, new_parent):
+ for child in tt.iter_tree_children(old_parent):
+ tt.adjust_path(tt.final_name(child), new_parent, child)
+
So the only question is how do we want to handle files that are trying
to be put into a versioned directory. You seem to be advocating silently
deleting them. That may be okay, but I would at least like to discuss it.
Otherwise it looks pretty good.
John
=:->
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060829/280312dd/attachment.pgp
More information about the bazaar
mailing list