[MERGE] Add support for previewing the result of a merge

[John Arbash Meinel]

Aaron Bentley wrote:
> On Wed, Jan 02, 2008 at 11:07:52AM -0600, John Arbash Meinel wrote:
>> Aaron Bentley wrote:
>>> Okay, here's a version that includes that functionality in merge.  (Not
>>> diff, because it's more important to vary the merge parameters than the
>>> diff parameters).
>  
>> I'm a little hesitant to change a mutating command into a preview
>> command with a flag, but only because we don't have a lot of precedence
>> for it.
> 
> Not a lot of precedence, perhaps, but we do it for add and uncommit.  I
> think it's a pretty common practice in general.  patch, rsync, cvs, svn
> all do it.
> 
>> And I like not having an extra "bzr
>> preview-merge" command. Though "bzr preview-merge" might be more
>> discoverable than "bzr merge --preview".
> 
> I know I would look in the help for "merge" before I'd look for a
> different command.
> 

"bzr help commands" is a good way to match up "I want to do X" with the
command that does it.

I agree that "merge --preview" is decent. It could also be "merge
--dry-run" or "merge --display-only". I think with the other commands we
were going to use "-n" (for --no-action?)

It might be nice to have the "show me what you would do but don't do it"
option standardized across commands.


>> Though it also makes sense to not create another command that has to
>> duplicate all of the options of merge.
> 
> It needs to be accurate, so it really needs to be the same
> implementation.  But of course, we could do two commands with one
> implementation, if we wanted.
> 
>> It is also technically a backwards incompatible change to the Merger
>> api, since now do_merge must be handled. (Then again, I never really
>> liked changing the tree contents as part of __init__()).
> 
> No, it is not.  do_merge will be done automatically, *unless* you pass
> do_merge=False to __init__.
> 

If I have an implementation of a custom Merger, it will pass my function
a keyword argument that I did not implement before. Which means that
having a merge plugin installed will raise an exception rather than
"just working".

I wasn't talking about compatibility from callers, I was talking about
implementers.

>  
>>> +    def test_iter_changes(self):
>>> +        revision_tree = self.create_tree()
>>> +        root = revision_tree.inventory.root.file_id
>>> +        preview = TransformPreview(revision_tree)
>>> +        a_trans_id = preview.trans_id_file_id('a-id')
>>> +        preview.delete_contents(a_trans_id)
>>> +        preview.create_file('b content', a_trans_id)
>>> +        preview_tree = preview.get_preview_tree()
>>> +        self.assertEqual([('a-id', ('a', 'a'), True, (True, True),
>>> +                          (root, root), ('a', 'a'), ('file', 'file'),
>>> +                          (False, False))],
>>> +                          list(preview_tree._iter_changes(revision_tree)))
>>> +        # Test unsupported parameters
>>> +        e = self.assertRaises(ValueError, preview_tree._iter_changes,
>>> +                              preview_tree)
>  
>> ^- This seems like a test that you would really rather have as a set of
>> tests, rather than one big conglomeration.
> 
> I don't agree.  It's all testing the same thing.  Having to build the
> same fixture repeatedly just to test that a given option raises
> ValueError seems like a waste of runtime to me.
> 

I'll let the more test focused people (like Robert) chime in here. But
my understanding is that all-in-one tests like this are brittle. It
breaks in the middle, which doesn't show you if the rest is working or
not. You get 1 test failure which might actually be 5 things that need
to be fixed. Conversely if you are expecting side effects (not true in
this case), you may get a false failure. (It failed because of the set
up, but what you were actually testing in that test would have succeeded.)

>> Also, shouldn't you be iterating over the results? Otherwise it is only
>> checking to the point of the first 'yield'.
> 
> PreviewTree._iter_changes is a plain function, not a generator.  If it
> succeeds, it returns an iterator.  If it fails, it raises.  It never
> yields.  But even if it were a generator, I would expect it to raise
> before the first yield.

Is it always going to be implemented that way? I realize these are
directly testing PreviewTree. It just seems like we should be testing
the general API for that function.

>> +class TreeTransformBase(object):
>> +
>> +    def __init__(self, tree, limbodir, pb=DummyProgress(),
>>
>> ^- You should at least have a minimal doc string here.
> 
> Okay.
>  
>> +class TransformPreview(TreeTransformBase):
>>
>> And a nice descriptive one here.
> 
> Alright.
> 
>> _PreviewTree doesn't seem to support the full "Tree" interface
>> (get_file_lines, get_file_text, for example). It probably implements
>> enough for show_diff_trees(), but I'm wondering if we should be trying
>> to add it to the tree_implementations tests.
> 
> It certainly doesn't support the full interface, and I would expect it
> to fail most tests.
> 
>> Right *now* we don't need
>> it. But as we find this being useful, I think it is going to get more
>> people wanting it to act just like any other tree.
> 
> Probably, but still YAGNI.  It does enough to be useful for this
> purpose, and I think that's good enough reason to merge it.  This is the
> problem with fat interfaces like Tree and Branch; they make it hard to
> create a full implementation.  It's just not worth it for this.
> 
>> PS> It is interesting how you got *really* productive over the holidays,
>> while the rest of us sort of disappeared. My laptop's power supply died,
>> which didn't help. Thanks for all the effort, though.
> 
> I visited my mom for Christmas.  Bazaar works really well on trains, but
> my usual distractions don't ;-).
> 
> Aaron
> 

Nice.

John
=:->