[MERGE] Shelf 5 / 5 for reals

[John Arbash Meinel]

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

Aaron Bentley wrote:
> Aaron Bentley wrote:
>> Since shelving is a heavily-interactive process, I had to test a derived
>> class of Shelver, ExpectShelver.  ExpectShelver does some basic
>> scripting-- you tell it what prompt to expect, and how to reply.
>> Prompts are processed in order of insertion.
> 
>> Unshelver is simpler (for now!) because it doesn't allow changes to be
>> selected.
> 
> Since no one has reviewed it thus far, here's an updated version of the
> shelf UI.  Mainly I've added documentation, but I've also made the API
> less bogus and fixed a bug shelving renamed and modified files.
> 
> Note that the outcome of the Emacs discussion was that these commands
> are acceptably compatible with Emacs.
> 
> Aaron

Overall, I think this is looking very nice. Well polished, and a lot of
direct testing. Thanks for working on this.

BB:tweak


=== added file 'bzrlib/shelf_ui.py'

The copyright statement at the top of this file doesn't match the rest
of the bzr codebase. "bzr selftest -s bt.test_source" will fail on that.

+from bzrlib import (
+    builtins,
+    delta,
+    diff,
+    errors,
+    osutils,
+    patches,
+    shelf,
+    textfile,
+    trace,
+    ui,
+    workingtree)

^- Usually we leave each one on its own line with a trailing ',' like:

 ui,
 workingtree,
 )

So that when adding one, you don't have to edit a different one.

...

+try:
+    from bzrlib.plugins.bzrtools import colordiff
+except ImportError:
+    colordiff = None


^- I can't say that there is a strictly better way to do this, but it
does seem a bit odd to have core code depend on an external library. Was
any progress made in getting some sort of colordiff support into core?


...

+        :param target_tree: The "unchanged" / old tree to compare the
+            work_tree to.
+        :param auto: If True, shelve each possible change.
+        :param auto_apply: If True, shelve changes with no final prompt.
+        :param file_list: If supplied, only files in this list may be
shelved.
+        :param message: The message to associate with the shelved changes.

^- I think you have 2 spaces around "list may be  shelved"


...

+    def prompt_bool(self, question):
+        """Prompt the user with a yes/no question.
+
+        This may be overridden by self.auto, or auto may be supplied.
+        It may also *set* self.auto.  It may also raise SystemExit.
+        :param question: The question to ask the user.
+        :return: True or False

^- You no longer allow "auto" to be passed in, and the docstring just
needs to be updated.


...

+    def handle_modify_text(self, creator, file_id):
+        """Provide diff hunk selection for modified text.
+
+        :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)
+        work_file = self.work_tree.get_file(file_id)
+        try:
+            textfile.text_file(work_file)
+        finally:
+            work_file.close()
+        textfile.check_text_lines(target_lines)
+        parsed = self.get_parsed_patch(file_id)


^- It seems a bit odd to work in terms of both lines of a file, and a
File object, and to not pass any of that to 'get_parsed_patch' which
needs to extract it yet again. I'm not sure that there is much you can
do, but there is a bit of redundancy here.

...

+    def test_shelve_binary_change(self):
+        tree = self.create_shelvable_tree()
+        self.build_tree_contents([('tree/foo', '\x00')])
+        shelver = ExpectShelver(tree, tree.basis_tree())
+        shelver.expect('Shelve binary changes? [yNfq]', 'y')
+        shelver.expect('Shelve 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())
+        shelver.expect('Shelve renaming foo => bar? [yNfq]', 'y')

^- You have an extra blank space here

I'm also thinking we may want to add tests for Unicode filenames as part
of the UI. I believe we need to set 'outf' to 'exact' because we will be
writing out diffs, so we need to be careful that we *do* encode
filenames when we need to. (This can come in after the rest of the
shelf, code, though.)

I also notice that you are doing:

'Shelve renaming foo => bar? [yNfq]'

but

'Shelve removing file "foo"?  [yNfq]'

We should probably be consistent with how we quote the filenames we
display. I'll also note that you don't test the display of the filename
as part of the 'diff' code, though perhaps that is tested elsewhere.

...

+    def test_shelve_quit(self):
+        tree = self.create_shelvable_tree()
+        shelver = ExpectShelver(tree, tree.basis_tree())
+        shelver.expect('Shelve? [yNfq]', 'q')
+        self.assertRaises(SystemExit, shelver.run)
+        self.assertFileEqual(LINES_ZY, 'tree/foo')
+

Do we want to be raising SystemExit, rather than a more bzr-specific
exit exception?

John
=:->

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

iEYEARECAAYFAkkPWokACgkQJdeBCYSNAAOHcQCfTbv28P4vXh8/+KLWG22vc2jl
uQEAoIkQAFWOWO+mLFv6oPejJ952jc0P
=x+5m
-----END PGP SIGNATURE-----