tree-less merges

[Michael Hudson]

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