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

[John Arbash Meinel]

Aaron Bentley wrote:
> John Arbash Meinel wrote:
>> Aaron Bentley wrote:
>>> On Wed, Jan 02, 2008 at 11:07:52AM -0600, John Arbash Meinel wrote:
> 
>>>> 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.
> 
> Yeah, but I expect dry-runs to done using a flag to the normal command,
> so the flag is more discoverable to someone like me.
> 
>> 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.
> 
> Yes.  I suppose it's a bit pedantic, but this isn't just "show me what
> you would do but don't do it" to me.  It shows a diff, which is a
> behavior change.  But we can use -n if you really want.

Isn't showing a diff a "show me what you would have done" ?
I suppose it is outputting a diff to stdout which isn't the same as
--dry-run.

I don't know that "-n" was the finally determined short option for it.
Just that if we do start using this, I think it would be nice to our
users if all options that fall under "show me what you would have done"
were unified.


> 
>>>> 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__.
> 
>> I wasn't talking about compatibility from callers, I was talking about
>> implementers.
> 
> Ah.  Misunderstood.  AFAIK, there aren't any independent Merger
> implementations, so pragmatically, I'd just flag it as an API break in
> NEWS.  Does that suit?

I think so.

...

>> 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.
> 
> That seems like over-testing to me.  All of the functionality unique to
> _PreviewTree._iter_changes is tested.  All of the functionality of
> TreeTransform._iter_changes is also tested.
> 
> At some point, we may try to make _PreviewTree into a more general tree
> type.  And then, this test will break, and rightly so.  But then we'll
> be able to add it to the InterTree._iter_changes tests.
> 
> So no, I don't think testing the API makes sense.
> 
> Aaron
> 

I think as a general rule, testing the API leads to better (less
fragile) tests. It doesn't always lead to as much coverage, but that
might indicate the API needs to be designed differently. (Look at my
recent discussions with Vincent on SFTP readv, and how it needs to be
broken up, which is partly exposed because testing different code paths
is difficult.)

Nothing I would strictly block on, but I would be interested to hear
what Andrew or Robert have to say on the subject.

John
=:->