tree-less merges
Michael Hudson
michael.hudson at canonical.com
Tue Nov 27 16:16:57 GMT 2007
Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Michael Hudson wrote:
>> Aaron Bentley wrote:
>> So I did these things and some further hacking (mostly slicing up bits
>> of merge.py into smaller methods, attached as a bundle against your
>> branch) that lets me write get_diff_as_merged like this:
>
>> and it works for the simple cases, which is pretty nice.
>
> Great!
>
>> It's not
>> tested at all, bad me, but I'd appreciate another sanity check if you
>> have time :)
>
> We try to maintain a stable (that is, append-only) API, but your change
> to the Merge3Merge constructor is an API break. Splitting the
> functionality into do_merge is fine, but please restore the old behavior
> as the default, and provide a new parameter to override it.
OK, should be easy enough.
>> + def do_merge(self):
>> + self.this_tree.lock_tree_write()
>
> this_tree is intended to be read-only source data. Especially, it may
> be a revision tree, if we can determine that the revision tree has the
> same data as the working tree. Please do not blur the distinction
> between this_tree (read-only source data) and working_tree (target for
> writing changes to).
Oh, OK. It looked to me like they were always the same, but I
probably didn't look hard enough.
>> @@ -507,6 +485,52 @@
>> self.this_tree.unlock()
>> self.pb.clear()
>>
>> + def make_preview_transform(self):
>> + self.base_tree.lock_read()
>> + self.other_tree.lock_read()
>> + self.tt = TransformPreview(self.this_tree)
>> + try:
>> + self.pp.next_phase()
>> + self.compute_transform()
>> + self.pp.next_phase()
>> + finally:
>> + self.other_tree.unlock()
>> + self.base_tree.unlock()
>> + self.pb.clear()
>> + return self.tt
>
> You're going to update this to set the conflicts, right?
Well, I'm going to find out how conflicts are handled at some
point... it's just something I haven't wrapped my brain around yet.
>> @@ -458,7 +459,6 @@
>> file.close()
>> mary_tree.commit("change file2")
>> # john should be able to merge with no conflicts.
>> - merge_type = Merge3Merger
>
> I don't understand why you removed this.
It's an entirely pointless line, isn't it?
>> + # XXX OW!
>> + self._TreeTransformBase__done = True
>
> Perhaps the best option is to call it _done instead of __done.
Oops, I'd forgotten about that line. Yes, what you say makes sense.
>> + def _set_mode(self, trans_id, mode_id, typefunc):
>> + """Set the mode of new file contents.
>> + The mode_id is the existing file to get the mode from (often the same
>> + as trans_id). The operation is only performed if there's a mode match
>> + according to typefunc.
>> + """
>> + # is it ok to ignore this? probably
>> + pass
>
> I think this can't be relevant for non-disk-based trees.
Good.
>> +
>> + def iter_tree_children(self, parent_id):
>> + """Iterate through the entry's tree children, if any"""
>> + # This can't possibly be right :) It's only called from
>> + # _add_tree_children (where the comment is "") which is called
>> + # from find_conflicts.
>
> Actually, it's used in several places.
Uh, right. But in the cases where a transformpreview is being used, I
think that this is the only callsite (except possibly
adjust_tree_root, I'm not sure when that is used).
> And the comment in add_tree_children is actually "ensure that all
> children are registered with the transaction"
Indeed, that's what I meant to paste in there.
>> + I don't understand the conflict
>> + # resolution well enough yet to know what the right thing to
>> + # do is, though in the simplest cases this seems to work.
>> + return []
>
> This function is essentially listdir, except that it returns paths
> relative to the tree root. You need to return the paths of the children
> of the specified directory in the original tree.
Partly here I was confused over what parent_id is. A trans_id?
> You ought to be able to reuse the original code, except for the
> listdir.
Sounds good.
> I think you can replace the listdir with
> wt.inventory['file_id'].children.keys()
But doesn't iter_tree_children also have to return the files that the
transform added? (On a tangent, using listdir seems a little strange,
isn't that going to pick up .pyc files and other ignored things?).
> (probably The Right Thing is using Tree.walkdirs, since that doesn't
> require Tree.inventory, but it's a bit like hitting a fly with a
> sledgehammer.)
Uh, yes it does a bit.
Thanks again for your time on this!
Cheers,
mwh
More information about the bazaar
mailing list