[MERGE] Add support for previewing the result of a merge
John Arbash Meinel
john at arbash-meinel.com
Wed Jan 2 19:38:42 GMT 2008
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
=:->
More information about the bazaar
mailing list