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

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

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.

>>> 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?
>>> ^- 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.
>>
>>> 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.

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
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFHfDhX0F+nu1YWqI0RAt0mAJ4ra5eut/AiB7g26LDciG+DQgDE5gCfUByz
GY00+G/nQVTroGIvU2wWlhI=
=aJun
-----END PGP SIGNATURE-----