Merge lp:~abentley/bzr/merge-interactive into lp:~bzr/bzr/trunk-old

[Aaron Bentley]

[Aaron Bentley (abentley)]

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

Hi all,

This patch implements interactive merge, based on the shelf UI. To make
the prompts more intelligible, it introduces a new vocabulary for
shelving, the "apply" vocabulary.

The current set of prompts is described in terms of a reverting to the
basis. For example, if the working tree does not have a file, but the
target tree does, the user is assumed to have removed the file. They
are therefore asked if they want to "Shelve removing" the file. If they
say "yes", the file is added to the working tree.

When the target tree is not the basis tree, this vocabulary makes little
sense. The user has not removed the file, so why would they "shelve"
its "removal"? Instead, the "apply" vocabulary asks if they want to
"Add" the file.

For similar reasons, the diff hunks are inverted.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpbc/8ACgkQ0F+nu1YWqI093gCbBHNNWi/g5yt+wHt/gps/Y83b
wlUAn1jNsmat6ZQth1VioNq7dAsJriUt
=lKq+
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

That's a really exciting feature, thanks for posting it.

It would be nice if it were mentioned in news and at least in the user manual to the extent of "you can do interactive merges with -i".

From the cover letter I thought you were saying merge would prompt with "Shelve removing..." but clearly it will actually say "Delete file %s".

I wonder if rather than having invert_diff=True and some of the messages backwards it could be a property of either this reporter or perhaps a parameter to Shelver that it's actually being applied as a negative. It just seems like the kind of thing that would easily be broken or just create an unnecessary puzzle or barrier to reuse.

For instance 'revert -i', which is really pretty close to shelve, might like to use this terminology.

You could have a decorator of the reporter that inverts it.

678 + def test_shelve_modify_target(self):
679 + tree = self.create_shelvable_tree()
680 + os.symlink('bar', 'tree/baz')

I think this needs to be guarded by having the symlink feature?

Perhaps a comment in do_interactive wouldn't go amiss: you're doing the whole merge into a preview tree, then iiuc selectively essentially reverting the working tree to look like the preview tree to bring those changes across. That's pretty elegant.

review: Approve

[Aaron Bentley (abentley)]

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

Thanks for your review.

Martin Pool wrote:
> It would be nice if it were mentioned in news and at least in the user manual to the extent of "you can do interactive merges with -i".

I've updated NEWS, and added a short paragraph to the command help:

 To select only some changes to merge, use "merge -i", which will prompt
 you to apply each diff hunk and file change, similar to "shelve".

>>From the cover letter I thought you were saying merge would prompt with "Shelve removing..." but clearly it will actually say "Delete file %s".
>
> I wonder if rather than having invert_diff=True and some of the messages backwards it could be a property of either this reporter or perhaps a parameter to Shelver that it's actually being applied as a negative. It just seems like the kind of thing that would easily be broken or just create an unnecessary puzzle or barrier to reuse.

It's not really being applied as a negative. The same keystrokes have
exactly the same meaning as they did before. It's just that shelving a
removal is the same thing as applying an addition.

> For instance 'revert -i', which is really pretty close to shelve, might like to use this terminology.

Do you mean the "apply" terminology? That it will work as-is. If you
have deleted a file, then "revert -i" would prompt you to add it. It
should not prompt you to delete the file.

If you meant the "shelve" terminology, it will need to be a different
variant, updated to say "revert" everywhere the current reporter says
"shelve".

> 678 + def test_shelve_modify_target(self):
> 679 + tree = self.create_shelvable_tree()
> 680 + os.symlink('bar', 'tree/baz')
>
> I think this needs to be guarded by having the symlink feature?

Fixed.

>
> Perhaps a comment in do_interactive wouldn't go amiss: you're doing the whole merge into a preview tree, then iiuc selectively essentially reverting the working tree to look like the preview tree to bring those changes across. That's pretty elegant.

done.

I've submitted, but I'm happy to land a follow-on patch.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkpcjwoACgkQ0F+nu1YWqI3+5QCfVGQEmVybFIDa2KtwaNyw022D
EX4AnRpQjGzQuAGUYK8dUt3IsRYDDLBF
=4eR1
-----END PGP SIGNATURE-----

=== modified file 'NEWS'
--- NEWS	2009-07-13 13:25:18 +0000
+++ NEWS	2009-07-14 14:35:13 +0000
@@ -16,6 +16,9 @@
 New Features
 ************
 
+* ``merge --interactive`` applies a user-selected portion of the merge.  The UI
+  is similar to ``shelve``.  (Aaron Bentley)
+
 Bug Fixes
 *********
 
 
=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-07-10 08:33:11 +0000
+++ bzrlib/builtins.py	2009-07-14 14:35:13 +0000
@@ -3573,6 +3573,9 @@
     merge refuses to run if there are any uncommitted changes, unless
     --force is given.
 
+    To select only some changes to merge, use "merge -i", which will prompt
+    you to apply each diff hunk and file change, similar to "shelve".
+
     :Examples:
         To merge the latest revision from bzr.dev::
 
@@ -3616,7 +3619,10 @@
                short_name='d',
                type=unicode,
                ),
-        Option('preview', help='Instead of merging, show a diff of the merge.')
+        Option('preview', help='Instead of merging, show a diff of the'
+               ' merge.'),
+        Option('interactive', help='Select changes interactively.',
+            short_name='i')
     ]
 
     def run(self, location=None, revision=None, force=False,
@@ -3624,6 +3630,7 @@
             uncommitted=False, pull=False,
             directory=None,
             preview=False,
+            interactive=False,
             ):
         if merge_type is None:
             merge_type = _mod_merge.Merge3Merger
@@ -3700,7 +3707,9 @@
                     return 0
             merger.check_basis(False)
             if preview:
-                return self._do_preview(merger)
+                return self._do_preview(merger, cleanups)
+            elif interactive:
+                return self._do_interactive(merger, cleanups)
             else:
                 return self._do_merge(merger, change_reporter, allow_pending,
                                       verified)
@@ -3708,16 +3717,18 @@
             for cleanup in reversed(cleanups):
                 cleanup()
 
-    def _do_preview(self, merger):
-        from bzrlib.diff import show_diff_trees
+    def _get_preview(self, merger, cleanups):
         tree_merger = merger.make_merger()
         tt = tree_merger.make_preview_transform()
-        try:
-            result_tree = tt.get_preview_tree()
-            show_diff_trees(merger.this_tree, result_tree, self.outf,
-                            old_label='', new_label='')
-        finally:
-            tt.finalize()
+        cleanups.append(tt.finalize)
+        result_tree = tt.get_preview_tree()
+        return result_tree
+
+    def _do_preview(self, merger, cleanups):
+        from bzrlib.diff import show_diff_trees
+        result_tree = self._get_preview(merger, cleanups)
+        show_diff_trees(merger.this_tree, result_tree, self.outf,
+                        old_label='', new_label='')
 
     def _do_merge(self, merger, change_reporter, allow_pending, verified):
         merger.change_reporter = change_reporter
@@ -3731,6 +3742,21 @@
         else:
             return 0
 
+    def _do_interactive(self, merger, cleanups):
+        """Perform an interactive merge.
+
+        This works by generating a preview tree of the merge, then using
+        Shelver to selectively remove the differences between the working tree
+        and the preview tree.
+        """
+        from bzrlib import shelf_ui
+        result_tree = self._get_preview(merger, cleanups)
+        writer = bzrlib.option.diff_writer_registry.get()
+        shelver = shelf_ui.Shelver(merger.this_tree, result_tree, destroy=True,
+                                   reporter=shelf_ui.ApplyReporter(),
+                                   diff_writer=writer(sys.stdout))
+        shelver.run()
+
     def sanity_check_merger(self, merger):
         if (merger.show_base and
             not merger.merge_type is _mod_merge.Merge3Merger):
 
=== modified file 'bzrlib/shelf.py'
--- bzrlib/shelf.py	2009-06-10 03:56:49 +0000
+++ bzrlib/shelf.py	2009-07-14 14:35:13 +0000
@@ -96,6 +96,21 @@
                 elif changed:
                     yield ('modify text', file_id)
 
+    def shelve_change(self, change):
+        """Shelve a change in the iter_shelvable format."""
+        if change[0] == 'rename':
+            self.shelve_rename(change[1])
+        elif change[0] == 'delete file':
+            self.shelve_deletion(change[1])
+        elif change[0] == 'add file':
+            self.shelve_creation(change[1])
+        elif change[0] == 'change kind':
+            self.shelve_content_change(change[1])
+        elif change[0] == 'modify target':
+            self.shelve_modify_target(change[1])
+        else:
+            raise ValueError('Unknown change kind: "%s"' % change[0])
+
     def shelve_rename(self, file_id):
         """Shelve a file rename.
 
 
=== modified file 'bzrlib/shelf_ui.py'
--- bzrlib/shelf_ui.py	2009-06-26 03:44:30 +0000
+++ bzrlib/shelf_ui.py	2009-07-14 14:35:13 +0000
@@ -37,20 +37,75 @@
 
 class ShelfReporter(object):
 
+    vocab = {'add file': 'Shelve adding file "%(path)s"?',
+             'binary': 'Shelve binary changes?',
+             'change kind': 'Shelve changing "%s" from %(other)s'
+             ' to %(this)s?',
+             'delete file': 'Shelve removing file "%(path)s"?',
+             'final': 'Shelve %d change(s)?',
+             'hunk': 'Shelve?',
+             'modify target': 'Shelve changing target of'
+             ' "%(path)s" from "%(other)s" to "%(this)s"?',
+             'rename': 'Shelve renaming "%(other)s" =>'
+                        ' "%(this)s"?'
+             }
+
+    invert_diff = False
+
     def __init__(self):
         self.delta_reporter = delta._ChangeReporter()
 
     def no_changes(self):
+        """Report that no changes were selected to apply."""
         trace.warning('No changes to shelve.')
 
     def shelved_id(self, shelf_id):
+        """Report the id changes were shelved to."""
         trace.note('Changes shelved with id "%d".' % shelf_id)
 
+    def changes_destroyed(self):
+        """Report that changes were made without shelving."""
+        trace.note('Selected changes destroyed.')
+
     def selected_changes(self, transform):
+        """Report the changes that were selected."""
         trace.note("Selected changes:")
         changes = transform.iter_changes()
         delta.report_changes(changes, self.delta_reporter)
 
+    def prompt_change(self, change):
+        """Determine the prompt for a change to apply."""
+        if change[0] == 'rename':
+            vals = {'this': change[3], 'other': change[2]}
+        elif change[0] == 'change kind':
+            vals = {'path': change[4], 'other': change[2], 'this': change[3]}
+        elif change[0] == 'modify target':
+            vals = {'path': change[2], 'other': change[3], 'this': change[4]}
+        else:
+            vals = {'path': change[3]}
+        prompt = self.vocab[change[0]] % vals
+        return prompt
+
+
+class ApplyReporter(ShelfReporter):
+
+    vocab = {'add file': 'Delete file "%(path)s"?',
+             'binary': 'Apply binary changes?',
+             'change kind': 'Change "%(path)s" from %(this)s'
+             ' to %(other)s?',
+             'delete file': 'Add file "%(path)s"?',
+             'final': 'Apply %d change(s)?',
+             'hunk': 'Apply change?',
+             'modify target': 'Change target of'
+             ' "%(path)s" from "%(this)s" to "%(other)s"?',
+             'rename': 'Rename "%(this)s" => "%(other)s"?',
+             }
+
+    invert_diff = True
+
+    def changes_destroyed(self):
+        pass
+
 
 class Shelver(object):
     """Interactively shelve the changes in a working tree."""
@@ -70,6 +125,7 @@
         :param destroy: Change the working tree without storing the shelved
             changes.
         :param manager: The shelf manager to use.
+        :param reporter: Object for reporting changes to user.
         """
         self.work_tree = work_tree
         self.target_tree = target_tree
@@ -121,41 +177,20 @@
                         changes_shelved += self.handle_modify_text(creator,
                                                                    change[1])
                     except errors.BinaryFile:
-                        if self.prompt_bool('Shelve binary changes?'):
+                        if self.prompt_bool(self.reporter.vocab['binary']):
                             changes_shelved += 1
                             creator.shelve_content_change(change[1])
-                if change[0] == 'add file':
-                    if self.prompt_bool('Shelve adding file "%s"?'
-                                        % change[3]):
-                        creator.shelve_creation(change[1])
-                        changes_shelved += 1
-                if change[0] == 'delete file':
-                    if self.prompt_bool('Shelve removing file "%s"?'
-                                        % change[3]):
-                        creator.shelve_deletion(change[1])
-                        changes_shelved += 1
-                if change[0] == 'change kind':
-                    if self.prompt_bool('Shelve changing "%s" from %s to %s? '
-                                        % (change[4], change[2], change[3])):
-                        creator.shelve_content_change(change[1])
-                        changes_shelved += 1
-                if change[0] == 'rename':
-                    if self.prompt_bool('Shelve renaming "%s" => "%s"?' %
-                                   change[2:]):
-                        creator.shelve_rename(change[1])
-                        changes_shelved += 1
-                if change[0] == 'modify target':
-                    if self.prompt_bool('Shelve changing target of "%s" '
-                            'from "%s" to "%s"?' % change[2:]):
-                        creator.shelve_modify_target(change[1])
+                else:
+                    if self.prompt_bool(self.reporter.prompt_change(change)):
+                        creator.shelve_change(change)
                         changes_shelved += 1
             if changes_shelved > 0:
                 self.reporter.selected_changes(creator.work_transform)
                 if (self.auto_apply or self.prompt_bool(
-                    'Shelve %d change(s)?' % changes_shelved)):
+                    self.reporter.vocab['final'] % changes_shelved)):
                     if self.destroy:
                         creator.transform()
-                        trace.note('Selected changes destroyed.')
+                        self.reporter.changes_destroyed()
                     else:
                         shelf_id = self.manager.shelve_changes(creator,
                                                                self.message)
@@ -166,17 +201,24 @@
             shutil.rmtree(self.tempdir)
             creator.finalize()
 
-    def get_parsed_patch(self, file_id):
+    def get_parsed_patch(self, file_id, invert=False):
         """Return a parsed version of a file's patch.
 
         :param file_id: The id of the file to generate a patch for.
+        :param invert: If True, provide an inverted patch (insertions displayed
+            as removals, removals displayed as insertions).
         :return: A patches.Patch.
         """
-        old_path = self.target_tree.id2path(file_id)
-        new_path = self.work_tree.id2path(file_id)
         diff_file = StringIO()
-        text_differ = diff.DiffText(self.target_tree, self.work_tree,
-                                    diff_file)
+        if invert:
+            old_tree = self.work_tree
+            new_tree = self.target_tree
+        else:
+            old_tree = self.target_tree
+            new_tree = self.work_tree
+        old_path = old_tree.id2path(file_id)
+        new_path = new_tree.id2path(file_id)
+        text_differ = diff.DiffText(old_tree, new_tree, diff_file)
         patch = text_differ.diff(file_id, old_path, new_path, 'file', 'file')
         diff_file.seek(0)
         return patches.parse_patch(diff_file)
@@ -223,30 +265,44 @@
     def handle_modify_text(self, creator, file_id):
         """Provide diff hunk selection for modified text.
 
+        If self.reporter.invert_diff is True, the diff is inverted so that
+        insertions are displayed as removals and vice versa.
+
         :param creator: a ShelfCreator
         :param file_id: The id of the file to shelve.
         :return: number of shelved hunks.
         """
-        target_lines = self.target_tree.get_file_lines(file_id)
+        if self.reporter.invert_diff:
+            target_lines = self.work_tree.get_file_lines(file_id)
+        else:
+            target_lines = self.target_tree.get_file_lines(file_id)
         textfile.check_text_lines(self.work_tree.get_file_lines(file_id))
         textfile.check_text_lines(target_lines)
-        parsed = self.get_parsed_patch(file_id)
+        parsed = self.get_parsed_patch(file_id, self.reporter.invert_diff)
         final_hunks = []
         if not self.auto:
             offset = 0
             self.diff_writer.write(parsed.get_header())
             for hunk in parsed.hunks:
                 self.diff_writer.write(str(hunk))
-                if not self.prompt_bool('Shelve?'):
+                selected = self.prompt_bool(self.reporter.vocab['hunk'])
+                if not self.reporter.invert_diff:
+                    selected = (not selected)
+                if selected:
                     hunk.mod_pos += offset
                     final_hunks.append(hunk)
                 else:
                     offset -= (hunk.mod_range - hunk.orig_range)
         sys.stdout.flush()
-        if len(parsed.hunks) == len(final_hunks):
+        if not self.reporter.invert_diff and (
+            len(parsed.hunks) == len(final_hunks)):
+            return 0
+        if self.reporter.invert_diff and len(final_hunks) == 0:
             return 0
         patched = patches.iter_patched_from_hunks(target_lines, final_hunks)
         creator.shelve_lines(file_id, list(patched))
+        if self.reporter.invert_diff:
+            return len(final_hunks)
         return len(parsed.hunks) - len(final_hunks)
 
 
 
=== modified file 'bzrlib/tests/test_shelf.py'
--- bzrlib/tests/test_shelf.py	2009-05-06 05:36:28 +0000
+++ bzrlib/tests/test_shelf.py	2009-07-14 14:35:13 +0000
@@ -40,7 +40,7 @@
 
 class TestPrepareShelf(tests.TestCaseWithTransport):
 
-    def test_shelve_rename(self):
+    def prepare_shelve_rename(self):
         tree = self.make_branch_and_tree('.')
         self.build_tree(['foo'])
         tree.add(['foo'], ['foo-id'])
@@ -50,7 +50,9 @@
         self.addCleanup(creator.finalize)
         self.assertEqual([('rename', 'foo-id', 'foo', 'bar')],
                           list(creator.iter_shelvable()))
-        creator.shelve_rename('foo-id')
+        return creator
+
+    def check_shelve_rename(self, creator):
         work_trans_id = creator.work_transform.trans_id_file_id('foo-id')
         self.assertEqual('foo', creator.work_transform.final_name(
                          work_trans_id))
@@ -58,7 +60,17 @@
         self.assertEqual('bar', creator.shelf_transform.final_name(
                          shelf_trans_id))
 
-    def test_shelve_move(self):
+    def test_shelve_rename(self):
+        creator = self.prepare_shelve_rename()
+        creator.shelve_rename('foo-id')
+        self.check_shelve_rename(creator)
+
+    def test_shelve_change_handles_rename(self):
+        creator = self.prepare_shelve_rename()
+        creator.shelve_change(('rename', 'foo-id', 'foo', 'bar'))
+        self.check_shelve_rename(creator)
+
+    def prepare_shelve_move(self):
         tree = self.make_branch_and_tree('.')
         self.build_tree(['foo/', 'bar/', 'foo/baz'])
         tree.add(['foo', 'bar', 'foo/baz'], ['foo-id', 'bar-id', 'baz-id'])
@@ -68,7 +80,9 @@
         self.addCleanup(creator.finalize)
         self.assertEqual([('rename', 'baz-id', 'foo/baz', 'bar/baz')],
                          list(creator.iter_shelvable()))
-        creator.shelve_rename('baz-id')
+        return creator, tree
+
+    def check_shelve_move(self, creator, tree):
         work_trans_id = creator.work_transform.trans_id_file_id('baz-id')
         work_foo = creator.work_transform.trans_id_file_id('foo-id')
         self.assertEqual(work_foo, creator.work_transform.final_parent(
@@ -80,6 +94,16 @@
         creator.transform()
         self.assertEqual('foo/baz', tree.id2path('baz-id'))
 
+    def test_shelve_move(self):
+        creator, tree = self.prepare_shelve_move()
+        creator.shelve_rename('baz-id')
+        self.check_shelve_move(creator, tree)
+
+    def test_shelve_change_handles_move(self):
+        creator, tree = self.prepare_shelve_move()
+        creator.shelve_change(('rename', 'baz-id', 'foo/baz', 'bar/baz'))
+        self.check_shelve_move(creator, tree)
+
     def assertShelvedFileEqual(self, expected_content, creator, file_id):
         s_trans_id = creator.shelf_transform.trans_id_file_id(file_id)
         shelf_file = creator.shelf_transform._limbo_name(s_trans_id)
@@ -102,7 +126,8 @@
         self.assertFileEqual('a\nc\n', 'foo')
         self.assertShelvedFileEqual('b\na\n', creator, 'foo-id')
 
-    def test_shelve_creation(self):
+
+    def prepare_shelve_creation(self):
         tree = self.make_branch_and_tree('.')
         tree.lock_write()
         self.addCleanup(tree.unlock)
@@ -114,9 +139,9 @@
         self.assertEqual([('add file', 'bar-id', 'directory', 'bar'),
                           ('add file', 'foo-id', 'file', 'foo')],
                           sorted(list(creator.iter_shelvable())))
-        creator.shelve_creation('foo-id')
-        creator.shelve_creation('bar-id')
-        creator.transform()
+        return creator, tree
+
+    def check_shelve_creation(self, creator, tree):
         self.assertRaises(StopIteration,
                           tree.iter_entries_by_dir(['foo-id']).next)
         s_trans_id = creator.shelf_transform.trans_id_file_id('foo-id')
@@ -129,7 +154,22 @@
         self.assertEqual('directory',
             creator.shelf_transform.final_kind(s_bar_trans_id))
 
-    def _test_shelve_symlink_creation(self, link_name, link_target):
+    def test_shelve_creation(self):
+        creator, tree = self.prepare_shelve_creation()
+        creator.shelve_creation('foo-id')
+        creator.shelve_creation('bar-id')
+        creator.transform()
+        self.check_shelve_creation(creator, tree)
+
+    def test_shelve_change_handles_creation(self):
+        creator, tree = self.prepare_shelve_creation()
+        creator.shelve_change(('add file', 'foo-id', 'file', 'foo'))
+        creator.shelve_change(('add file', 'bar-id', 'directory', 'bar'))
+        creator.transform()
+        self.check_shelve_creation(creator, tree)
+
+    def _test_shelve_symlink_creation(self, link_name, link_target,
+                                      shelve_change=False):
         self.requireFeature(tests.SymlinkFeature)
         tree = self.make_branch_and_tree('.')
         tree.lock_write()
@@ -141,7 +181,10 @@
         self.addCleanup(creator.finalize)
         self.assertEqual([('add file', 'foo-id', 'symlink', link_name)],
                          list(creator.iter_shelvable()))
-        creator.shelve_creation('foo-id')
+        if shelve_change:
+            creator.shelve_change(('add file', 'foo-id', 'symlink', link_name))
+        else:
+            creator.shelve_creation('foo-id')
         creator.transform()
         s_trans_id = creator.shelf_transform.trans_id_file_id('foo-id')
         self.failIfExists(link_name)
@@ -158,8 +201,12 @@
         self._test_shelve_symlink_creation(u'fo\N{Euro Sign}o',
                                            u'b\N{Euro Sign}ar')
 
+    def test_shelve_change_handles_symlink_creation(self):
+        self._test_shelve_symlink_creation('foo', 'bar', shelve_change=True)
+
     def _test_shelve_symlink_target_change(self, link_name,
-                                           old_target, new_target):
+                                           old_target, new_target,
+                                           shelve_change=False):
         self.requireFeature(tests.SymlinkFeature)
         tree = self.make_branch_and_tree('.')
         tree.lock_write()
@@ -174,7 +221,11 @@
         self.assertEqual([('modify target', 'foo-id', link_name,
                            old_target, new_target)],
                          list(creator.iter_shelvable()))
-        creator.shelve_modify_target('foo-id')
+        if shelve_change:
+            creator.shelve_change(('modify target', 'foo-id', link_name,
+                                   old_target, new_target))
+        else:
+            creator.shelve_modify_target('foo-id')
         creator.transform()
         self.assertEqual(old_target, osutils.readlink(link_name))
         s_trans_id = creator.shelf_transform.trans_id_file_id('foo-id')
@@ -191,6 +242,10 @@
         self._test_shelve_symlink_target_change(
             u'fo\N{Euro Sign}o', u'b\N{Euro Sign}ar', u'b\N{Euro Sign}az')
 
+    def test_shelve_change_handles_symlink_target_change(self):
+        self._test_shelve_symlink_target_change('foo', 'bar', 'baz',
+                                                shelve_change=True)
+
     def test_shelve_creation_no_contents(self):
         tree = self.make_branch_and_tree('.')
         tree.lock_write()
@@ -213,7 +268,7 @@
                          creator.shelf_transform.final_file_id(s_trans_id))
         self.failIfExists('foo')
 
-    def test_shelve_deletion(self):
+    def prepare_shelve_deletion(self):
         tree = self.make_branch_and_tree('tree')
         tree.lock_write()
         self.addCleanup(tree.unlock)
@@ -228,13 +283,27 @@
         self.assertEqual([('delete file', 'bar-id', 'file', 'foo/bar'),
                           ('delete file', 'foo-id', 'directory', 'foo')],
                           sorted(list(creator.iter_shelvable())))
-        creator.shelve_deletion('foo-id')
-        creator.shelve_deletion('bar-id')
-        creator.transform()
+        return creator, tree
+
+    def check_shelve_deletion(self, tree):
         self.assertTrue('foo-id' in tree)
         self.assertTrue('bar-id' in tree)
         self.assertFileEqual('baz', 'tree/foo/bar')
 
+    def test_shelve_deletion(self):
+        creator, tree = self.prepare_shelve_deletion()
+        creator.shelve_deletion('foo-id')
+        creator.shelve_deletion('bar-id')
+        creator.transform()
+        self.check_shelve_deletion(tree)
+
+    def test_shelve_change_handles_deletion(self):
+        creator, tree = self.prepare_shelve_deletion()
+        creator.shelve_change(('delete file', 'foo-id', 'directory', 'foo'))
+        creator.shelve_change(('delete file', 'bar-id', 'file', 'foo/bar'))
+        creator.transform()
+        self.check_shelve_deletion(tree)
+
     def test_shelve_delete_contents(self):
         tree = self.make_branch_and_tree('tree')
         self.build_tree(['tree/foo',])
@@ -249,7 +318,7 @@
         creator.transform()
         self.failUnlessExists('tree/foo')
 
-    def test_shelve_change_kind(self):
+    def prepare_shelve_change_kind(self):
         tree = self.make_branch_and_tree('tree')
         self.build_tree_contents([('tree/foo', 'bar')])
         tree.add('foo', 'foo-id')
@@ -260,12 +329,33 @@
         self.addCleanup(creator.finalize)
         self.assertEqual([('change kind', 'foo-id', 'file', 'directory',
                            'foo')], sorted(list(creator.iter_shelvable())))
+        return creator
+
+    def check_shelve_change_kind(self, creator):
+        self.assertFileEqual('bar', 'tree/foo')
+        s_trans_id = creator.shelf_transform.trans_id_file_id('foo-id')
+        self.assertEqual('directory',
+                         creator.shelf_transform._new_contents[s_trans_id])
+
+    def test_shelve_change_kind(self):
+        creator = self.prepare_shelve_change_kind()
         creator.shelve_content_change('foo-id')
         creator.transform()
-        self.assertFileEqual('bar', 'tree/foo')
-        s_trans_id = creator.shelf_transform.trans_id_file_id('foo-id')
-        self.assertEqual('directory',
-                         creator.shelf_transform._new_contents[s_trans_id])
+        self.check_shelve_change_kind(creator)
+
+    def test_shelve_change_handles_change_kind(self):
+        creator = self.prepare_shelve_change_kind()
+        creator.shelve_change(('change kind', 'foo-id', 'file', 'directory',
+                               'foo'))
+        creator.transform()
+        self.check_shelve_change_kind(creator)
+
+    def test_shelve_change_unknown_change(self):
+        tree = self.make_branch_and_tree('tree')
+        creator = shelf.ShelfCreator(tree, tree.basis_tree())
+        self.addCleanup(creator.finalize)
+        e = self.assertRaises(ValueError, creator.shelve_change, ('unknown',))
+        self.assertEqual('Unknown change kind: "unknown"', str(e))
 
     def test_shelve_unversion(self):
         tree = self.make_branch_and_tree('tree')
 
=== modified file 'bzrlib/tests/test_shelf_ui.py'
--- bzrlib/tests/test_shelf_ui.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/test_shelf_ui.py	2009-07-14 14:35:13 +0000
@@ -27,10 +27,10 @@
 
     def __init__(self, work_tree, target_tree, diff_writer=None,
                  auto=False, auto_apply=False, file_list=None, message=None,
-                 destroy=False):
+                 destroy=False, reporter=None):
         shelf_ui.Shelver.__init__(self, work_tree, target_tree, diff_writer,
                                   auto, auto_apply, file_list, message,
-                                  destroy)
+                                  destroy, reporter=reporter)
         self.expected = []
         self.diff_writer = StringIO()
 
@@ -165,6 +165,7 @@
         shelver.expect('Shelve 1 change(s)? [yNfq?]', 'y')
 
     def test_shelve_modify_target(self):
+        self.requireFeature(tests.SymlinkFeature)
         tree = self.create_shelvable_tree()
         os.symlink('bar', 'tree/baz')
         tree.add('baz', 'baz-id')
@@ -224,6 +225,108 @@
         self.assertFileEqual(LINES_AJ, 'tree/foo')
 
 
+class TestApplyReporter(TestShelver):
+
+    def test_shelve_not_diff(self):
+        tree = self.create_shelvable_tree()
+        shelver = ExpectShelver(tree, tree.basis_tree(),
+                                reporter=shelf_ui.ApplyReporter())
+        shelver.expect('Apply change? [yNfq?]', 'n')
+        shelver.expect('Apply change? [yNfq?]', 'n')
+        # No final shelving prompt because no changes were selected
+        shelver.run()
+        self.assertFileEqual(LINES_ZY, 'tree/foo')
+
+    def test_shelve_diff_no(self):
+        tree = self.create_shelvable_tree()
+        shelver = ExpectShelver(tree, tree.basis_tree(),
+                                reporter=shelf_ui.ApplyReporter())
+        shelver.expect('Apply change? [yNfq?]', 'y')
+        shelver.expect('Apply change? [yNfq?]', 'y')
+        shelver.expect('Apply 2 change(s)? [yNfq?]', 'n')
+        shelver.run()
+        self.assertFileEqual(LINES_ZY, 'tree/foo')
+
+    def test_shelve_diff(self):
+        tree = self.create_shelvable_tree()
+        shelver = ExpectShelver(tree, tree.basis_tree(),
+                                reporter=shelf_ui.ApplyReporter())
+        shelver.expect('Apply change? [yNfq?]', 'y')
+        shelver.expect('Apply change? [yNfq?]', 'y')
+        shelver.expect('Apply 2 change(s)? [yNfq?]', 'y')
+        shelver.run()
+        self.assertFileEqual(LINES_AJ, 'tree/foo')
+
+    def test_shelve_binary_change(self):
+        tree = self.create_shelvable_tree()
+        self.build_tree_contents([('tree/foo', '\x00')])
+        shelver = ExpectShelver(tree, tree.basis_tree(),
+                                reporter=shelf_ui.ApplyReporter())
+        shelver.expect('Apply binary changes? [yNfq?]', 'y')
+        shelver.expect('Apply 1 change(s)? [yNfq?]', 'y')
+        shelver.run()
+        self.assertFileEqual(LINES_AJ, 'tree/foo')
+
+    def test_shelve_rename(self):
+        tree = self.create_shelvable_tree()
+        tree.rename_one('foo', 'bar')
+        shelver = ExpectShelver(tree, tree.basis_tree(),
+                                reporter=shelf_ui.ApplyReporter())
+        shelver.expect('Rename "bar" => "foo"? [yNfq?]', 'y')
+        shelver.expect('Apply change? [yNfq?]', 'y')
+        shelver.expect('Apply change? [yNfq?]', 'y')
+        shelver.expect('Apply 3 change(s)? [yNfq?]', 'y')
+        shelver.run()
+        self.assertFileEqual(LINES_AJ, 'tree/foo')
+
+    def test_shelve_deletion(self):
+        tree = self.create_shelvable_tree()
+        os.unlink('tree/foo')
+        shelver = ExpectShelver(tree, tree.basis_tree(),
+                                reporter=shelf_ui.ApplyReporter())
+        shelver.expect('Add file "foo"? [yNfq?]', 'y')
+        shelver.expect('Apply 1 change(s)? [yNfq?]', 'y')
+        shelver.run()
+        self.assertFileEqual(LINES_AJ, 'tree/foo')
+
+    def test_shelve_creation(self):
+        tree = self.make_branch_and_tree('tree')
+        tree.commit('add tree root')
+        self.build_tree(['tree/foo'])
+        tree.add('foo')
+        shelver = ExpectShelver(tree, tree.basis_tree(),
+                                reporter=shelf_ui.ApplyReporter())
+        shelver.expect('Delete file "foo"? [yNfq?]', 'y')
+        shelver.expect('Apply 1 change(s)? [yNfq?]', 'y')
+        shelver.run()
+        self.failIfExists('tree/foo')
+
+    def test_shelve_kind_change(self):
+        tree = self.create_shelvable_tree()
+        os.unlink('tree/foo')
+        os.mkdir('tree/foo')
+        shelver = ExpectShelver(tree, tree.basis_tree(),
+                               reporter=shelf_ui.ApplyReporter())
+        shelver.expect('Change "foo" from directory to a file? [yNfq?]', 'y')
+        shelver.expect('Apply 1 change(s)? [yNfq?]', 'y')
+
+    def test_shelve_modify_target(self):
+        self.requireFeature(tests.SymlinkFeature)
+        tree = self.create_shelvable_tree()
+        os.symlink('bar', 'tree/baz')
+        tree.add('baz', 'baz-id')
+        tree.commit("Add symlink")
+        os.unlink('tree/baz')
+        os.symlink('vax', 'tree/baz')
+        shelver = ExpectShelver(tree, tree.basis_tree(),
+                                reporter=shelf_ui.ApplyReporter())
+        shelver.expect('Change target of "baz" from "vax" to "bar"? [yNfq?]',
+                       'y')
+        shelver.expect('Apply 1 change(s)? [yNfq?]', 'y')
+        shelver.run()
+        self.assertEqual('bar', os.readlink('tree/baz'))
+
+
 class TestUnshelver(tests.TestCaseWithTransport):
 
     def create_tree_with_shelf(self):