Merge lp:~vila/bzr/856261-unshelve-line-based into lp:bzr

[Vincent Ladeuil]

Allows bzr shelve to be used in test scripts and under emacs shells

I've been deprived from using shelve under emacs for years.

But when I ran into bug #856261 trying to reproduce bug #850594, I thought
it may be worth to have another go at it.

The fix is simple: since osutils.getchar() requires a true term (which
neither emacs not the test scripts provide), I handle the input as a
line-based one which is exactly what emacs does and also exactly what the
test script interface provides :)

I'd like feedback on how we want to configure this feature, knowing that an
environment variable may be more appropriate here (not everybody use
terminals under emacs only and the test script need a way to force the
behavior). Or should I just consider that the fix for #491196 is around the
corner and name the config variable... shelve.line_based_input ?
shelve.line_ui ? both boolean defaulting to False or shelve.input
(defaulting to 'char' accepting 'line' ?)

[Benoit Pierre (benoit.pierre)]

Excerpts from Vincent Ladeuil's message of Tue Sep 27 16:37:19 UTC 2011:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/856261-unshelve-line-based into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr/856261-unshelve-line-based/+merge/77195
>
> I've been deprived from using shelve under emacs for years.
>
> But when I ran into bug #856261 trying to reproduce bug #850594, I thought
> it may be worth to have another go at it.
>
> The fix is simple: since osutils.getchar() requires a true term (which
> neither emacs not the test scripts provide), I handle the input as a
> line-based one which is exactly what emacs does and also exactly what the
> test script interface provides :)
>
> I'd like feedback on how we want to configure this feature, knowing that an
> environment variable may be more appropriate here (not everybody use
> terminals under emacs only and the test script need a way to force the
> behavior). Or should I just consider that the fix for #491196 is around the
> corner and name the config variable... shelve.line_based_input ?
> shelve.line_ui ? both boolean defaulting to False or shelve.input
> (defaulting to 'char' accepting 'line' ?)

I also ran into problems using shelve with my VIM plugin. Other commands
use ui.get_boolean, but there is no corresponding method for handling
more complex choices. So even after providing my own UI class
implementation, I cannot use shelve.

In VIM there is a confirm function:

confirm({msg} [, {choices} [, {default} [, {type}]]])

Which can be used like this:

confirm("Save changes?", "&Yes\n&No\n&Cancel")

Where the letter after the '&' is the shortcut key for that choice.

So I have been working on a branch to implement something just like
this (see attached patch).

N.B.: your patch removes a call to gettext.
--
A: Because it destroys the flow of conversation.
Q: Why is top posting dumb?

[Vincent Ladeuil (vila)]

> I also ran into problems using shelve with my VIM plugin.

WOW ! (And that's coming from die-hard emacs user ;-D)

> Other commands
> use ui.get_boolean, but there is no corresponding method for handling
> more complex choices. So even after providing my own UI class
> implementation, I cannot use shelve.
>
> In VIM there is a confirm function:
>
> confirm({msg} [, {choices} [, {default} [, {type}]]])
>
> Which can be used like this:
>
> confirm("Save changes?", "&Yes\n&No\n&Cancel")
>
> Where the letter after the '&' is the shortcut key for that choice.
>
> So I have been working on a branch to implement something just like
> this (see attached patch).

This sounds *seriously* interesting and I urge you to pusblish your changes
in a branch and propose it for merge so we can properly review it (I'm
pleased tht launchpad displays your patch inline ! Wow too there) but I
really want your patch to be discussed as it seems we have perfect case of
triangulation here ;)

>
> N.B.: your patch removes a call to gettext.

Ouch, yeah, I initially wrote the patch on a 2.4-based branch where the
gettext call wasn't there >-/ I'll push a revision fixing that asap. Thanks
catching it !

[Martin Packman (gz)]

The problem with the shelf_ui code is it does something totally different to all the other bazaar user interactions, hence the issues with qbzr and test scripts and editors apparently. Making it use bzrlib.ui should be the goal, but doesn't need to happen in this branch.

Benoit, that bundle is really exciting and apart from some nitpicking I think should basically be merged. Please do propose it as Vincent said.

Notes just on the changes in this branch:

+ char_based = not(os.environ.get('INSIDE_EMACS', None) is not None)
+ if char_based and not sys.stdin.isatty():

Space after 'not' otherwise it looks like a function. :)

Having modules one by one need to learn about magic emacs variables is a good argument for this logic living elsewhere.

+ # XXX: Warn if more than one char is typed ?

To get similar logic to the current and get_boolean behaviour, you'd take the first character that matched the set, which you can't do at this level without remembering the last line and only prompting for a new one when there are no more characters. That's pretty unimportant though.

[Vincent Ladeuil (vila)]

> The problem with the shelf_ui code is it does something totally different to
> all the other bazaar user interactions, hence the issues with qbzr and test
> scripts and editors apparently. Making it use bzrlib.ui should be the goal,
> but doesn't need to happen in this branch.

/me nods

>
> Benoit, that bundle is really exciting and apart from some nitpicking I think
> should basically be merged. Please do propose it as Vincent said.

/me .. err, sorry :)

>
> Notes just on the changes in this branch:
>
> + char_based = not(os.environ.get('INSIDE_EMACS', None) is not None)
> + if char_based and not sys.stdin.isatty():
>
> Space after 'not' otherwise it looks like a function. :)

Yeah, thanks. But the plan is not to mention emacs at all there but instead
let user configure that themselves (INSIDE_EMACS is a far too big hammer and
was just a quick way to test but it's not reliable nor appropriate for all
cases), any feedback on the proposed config variables ?

>
> Having modules one by one need to learn about magic emacs variables is a good
> argument for this logic living elsewhere.
>
> + # XXX: Warn if more than one char is typed ?
>
> To get similar logic to the current and get_boolean behaviour, you'd take the
> first character that matched the set, which you can't do at this level without
> remembering the last line and only prompting for a new one when there are no
> more characters. That's pretty unimportant though.

And would keep the same UI which can drive any user nuts, it's really
intended to be line-based with one asnwer by *line* ending with CR, no
buffering of answers there.

What made shelve unsuable for me is that I needed to prepare the answers for
all the questions *before* seeing the questions, keeping this logic just
don't work ;)

So my question may be clearer as:

- given that we expect only a single char (followed by a \n), what should we
  do if we get more ? Warn ? Swallow silently ? Fail ?

Also, I need to add tests and that will probably remove the weird

  char = '\n'

just-in-case hack.

review: Needs Information

[Martin Packman (gz)]

So, to be clear, I think this branch may as well land as an improvement to the current isatty() hack, with the thought that Benoit's change will be the basis for a longer term solution.

> > + char_based = not(os.environ.get('INSIDE_EMACS', None) is not None)

...or just `is None` for that matter.

> Yeah, thanks. But the plan is not to mention emacs at all there but instead
> let user configure that themselves (INSIDE_EMACS is a far too big hammer and
> was just a quick way to test but it's not reliable nor appropriate for all
> cases), any feedback on the proposed config variables ?

For now it seems an appropriate hammer, just hang it elsewhere in a little private function. After this is all moved to bzrlib.ui then a config value to enable/disable character based input seems reasonable, but I wouldn't bother for now.

> - given that we expect only a single char (followed by a \n), what should we
> do if we get more ? Warn ? Swallow silently ? Fail ?

Swallow (and maybe warn) or fail and reprompt would all be fine.

> Also, I need to add tests and that will probably remove the weird
>
> char = '\n'
>
> just-in-case hack.

Right, you probably want a test for the EOF case, it needs to make sure it gets out of the shelf loop somehow.

review: Needs Fixing

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/shelf_ui.py'
--- bzrlib/shelf_ui.py	2011-09-16 15:39:47 +0000
+++ bzrlib/shelf_ui.py	2011-09-28 15:10:33 +0000
@@ -16,6 +16,7 @@
 
 
 from cStringIO import StringIO
+import os
 import shutil
 import sys
 import tempfile
@@ -251,20 +252,41 @@
         diff_file.seek(0)
         return patches.parse_patch(diff_file)
 
+    def _char_based(self):
+        # FIXME: A bit hackish to use INSIDE_EMACS here, but there is another
+        # work in progress moving this method (and more importantly prompt()
+        # below) into the ui area and address the issue in better ways.
+        # -- vila 2011-09-28
+        return os.environ.get('INSIDE_EMACS', None) is None
+
     def prompt(self, message):
         """Prompt the user for a character.
 
         :param message: The message to prompt a user with.
         :return: A character.
         """
-        if not sys.stdin.isatty():
-            # Since there is no controlling terminal we will hang when trying
-            # to prompt the user, better abort now.  See
+        char_based = self._char_based()
+        if char_based and not sys.stdin.isatty():
+            # Since there is no controlling terminal we will hang when
+            # trying to prompt the user, better abort now.  See
             # https://code.launchpad.net/~bialix/bzr/shelve-no-tty/+merge/14905
             # for more context.
             raise errors.BzrError(gettext("You need a controlling terminal."))
         sys.stdout.write(message)
-        char = osutils.getchar()
+        if char_based:
+            # We peek one char at a time which requires a real term here
+            char = osutils.getchar()
+        else:
+            # While running tests (or under emacs) the input is line buffered
+            # so we must not use osutils.getchar(). Instead we switch to a mode
+            # where each line is terminated by a new line
+            line = sys.stdin.readline()
+            if line:
+                # XXX: Warn if more than one char is typed ?
+                char = line[0]
+            else:
+                # Empty input, callers handle it as enter
+                char = ''
         sys.stdout.write("\r" + ' ' * len(message) + '\r')
         sys.stdout.flush()
         return char
 
=== modified file 'bzrlib/tests/script.py'
--- bzrlib/tests/script.py	2011-01-12 01:01:53 +0000
+++ bzrlib/tests/script.py	2011-09-28 15:10:33 +0000
@@ -481,6 +481,10 @@
     def setUp(self):
         super(TestCaseWithMemoryTransportAndScript, self).setUp()
         self.script_runner = ScriptRunner()
+        # FIXME: See shelf_ui.Shelver._char_based. This allow using shelve in
+        # scripts while providing a line-based input (better solution in
+        # progress). -- vila 2011-09-28
+        self.overrideEnv('INSIDE_EMACS', '1')
 
     def run_script(self, script, null_output_matches_anything=False):
         return self.script_runner.run_script(self, script, 
@@ -511,6 +515,10 @@
     def setUp(self):
         super(TestCaseWithTransportAndScript, self).setUp()
         self.script_runner = ScriptRunner()
+        # FIXME: See shelf_ui.Shelver._char_based. This allow using shelve in
+        # scripts while providing a line-based input (better solution in
+        # progress). -- vila 2011-09-28
+        self.overrideEnv('INSIDE_EMACS', '1')
 
     def run_script(self, script, null_output_matches_anything=False):
         return self.script_runner.run_script(self, script,
 
=== modified file 'bzrlib/tests/test_script.py'
--- bzrlib/tests/test_script.py	2011-05-16 13:39:39 +0000
+++ bzrlib/tests/test_script.py	2011-09-28 15:10:33 +0000
@@ -572,9 +572,9 @@
     def test_confirm_action(self):
         """You can write tests that demonstrate user confirmation.
         
-        Specifically, ScriptRunner does't care if the output line for the prompt
-        isn't terminated by a newline from the program; it's implicitly terminated 
-        by the input.
+        Specifically, ScriptRunner does't care if the output line for the
+        prompt isn't terminated by a newline from the program; it's implicitly
+        terminated by the input.
         """
         commands.builtin_command_registry.register(cmd_test_confirm)
         self.addCleanup(commands.builtin_command_registry.remove, 'test-confirm')
@@ -589,3 +589,56 @@
             ok, no
             """)
 
+class TestShelve(script.TestCaseWithTransportAndScript):
+
+    def setUp(self):
+        super(TestShelve, self).setUp()
+        self.run_script("""
+            $ bzr init test
+            Created a standalone tree (format: 2a)
+            $ cd test
+            $ echo foo > file
+            $ bzr add
+            adding file
+            $ bzr commit -m 'file added'
+            2>Committing to:...test/
+            2>added file
+            2>Committed revision 1.
+            $ echo bar > file
+            """)
+
+    def test_shelve(self):
+        self.run_script("""
+            $ bzr shelve -m 'shelve bar'
+            # Shelve? [yNfq?]
+            <y
+            # Shelve 1 change(s)? [yNfq?]
+            <y
+            2>Selected changes:
+            2> M  file
+            2>Changes shelved with id "1".
+            """,
+                        # shelve uses \r that can't be represented in the
+                        # script ?
+                        null_output_matches_anything=True)
+        self.run_script("""
+            $ bzr shelve --list
+              1: shelve bar
+            """)
+
+    def test_dont_shelve(self):
+        # We intentionally provide no input here to test EOF
+        self.run_script("""
+            $ bzr shelve -m 'shelve bar'
+            # Shelve? [yNfq?]
+            # Shelve 1 change(s)? [yNfq?]
+            2>No changes to shelve.
+            """,
+                        # shelve uses \r that can't be represented in the
+                        # script ?
+                        null_output_matches_anything=True)
+        self.run_script("""
+            $ bzr st
+            modified:
+              file
+            """)
 
=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-09-27 10:17:36 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-09-28 15:10:33 +0000
@@ -45,6 +45,10 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* ``bzr shelve`` can now be used in emacs shells as the input handling is
+  turned into a line-basde one when ``INSIDE_EMACS`` is set (which is the
+  case for all recent emacs versions). (Vincent Ladeuil, #856261)
+
 * Redirects between http and https no longer discard path information
   in some cases. (Jelmer Vernooij, #853765)
 
@@ -87,6 +91,9 @@
 Testing
 *******
 
+* Test scripts can now use ``bzr shelve`` and provide their input as
+  complete lines. (Vincent Ladeuil, #856261)
+
 .. Fixes and changes that are only relevant to bzr's test framework and 
    suite.  This can include new facilities for writing tests, fixes to 
    spurious test failures and changes to the way things should be tested.