Merge lp:~vila/bzr/353370-notty-no-term-width into lp:bzr

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

This fix (which includes jam's one) should address bug #492561
and provides more robust tests.

There have been several points raised in several discussions so
I'll try to summarize below.

1) I confirm that COLUMNS should be taken into account *before*
   querying for termios.TIOCGWINSZ.

2) TIOCGSIZE seems to be a thing of the distant past and is
   either equivalent to TIOCGWINSZ or SUN specific (I couldn't
   find it in /usr/include on Karmic nor FreeBSD nor gentoo :)

   Only on OSX 10.6 did I came back with:

     ./sys/ioctl.h:85:#define TIOCGSIZE TIOCGWINSZ

   So, no support for TIOCGSIZE sounds right.

3) I documented the rules to get the terminal size and hopefully
   write the right tests for that.

Keep in mind though that we are limited by the real controlling
terminal here unless we try to override the OS specific ways to
query for terminal size. I did that to test the default values
but trying to do more sounds a bit overkill (emulating the
controlling terminal sounds like too much work for no real
benefit).

That means we need "manual" confirmation that the ioctl call is
right for unices (it is for all the tests I could think of).

For the automated tests, so far, only PQM runs without a
controlling terminal, each of us always have one and even babune
seems to always have one defined somehow (I was a bit surprised
by that).

We rely on win32utils.get_console_size() for windows and I didn't
test the error cases here (help and feedback welcome). Instead I
trusted the function and just seed it with None as a default
value if anything goes wrong.

On windows, I'm not sure get_console_size() will do the right
thing if we use some xterm-like application and we may want to
use fcntl.ioctl(termios.TIOCGWINSZ) ? Some cygwin users can
answer that ?

Also, as Alexander mentioned at least twice, we rely on stderr
not being redirected to get the terminal size. But since we now
have BZR_COLUMNS to force a width I wonder if it's still needed
since it shouldn't be called if stdout is redirected anyway.

[John A Meinel (jameinel)]

So arguably, we need to change the command a bit, and add a flag that says whether we want to check stderr or stdout. So that progress bars stay the right width even when redirecting, but 'bzr log --line' gives the wide info.

Note that some of the motivation was also because of pagers. And "bzr log --line | less" goes to a terminal, even though "bzr log --line > file" does not. AFAIK there is no way to determine in process whether stdout is a pipe rather than a file. And even if there was, what do you do for "bzr log --line | tee file" ? As that goes both to the terminal and to a file. (Note this is even worse for getting the file encoding right on windows, as default file content encoding != default terminal encoding != filename encoding. So doing "bzr log > file && gvim file you would want content encoding, but bzr log | less you would want terminal encoding." Probably one of the few reasons to spawn our own pager, as then we know it is still going to a term...)

Anyway, I feel that the current patch is 'good enough'. It allows BZR_COLUMNS if people really need to tweak the width. It respects COLUMNS if available, and still uses TIOCGWINSZ.

review: Approve

[Martin Pool (mbp)]

capital 'Windows' when talking about that other software.

It seems reasonable. Let's try it.

review: Needs Fixing

=== modified file 'NEWS'
--- NEWS	2009-12-09 05:47:32 +0000
+++ NEWS	2009-12-09 15:11:12 +0000
@@ -64,11 +64,13 @@
 * Lots of bugfixes for the test suite on Windows. We should once again
   have a test suite with no failures on Windows. (John Arbash Meinel)
 
-* ``osutils.terminal_width()`` obeys the BZR_COLUMNS envrionment
+* ``osutils.terminal_width()`` obeys the BZR_COLUMNS environment
   variable but returns None if the terminal is not a tty (when output is
   redirected for example). Also fixes its usage under OSes that doesn't
-  provide termios.TIOCGWINSZ.  
+  provide termios.TIOCGWINSZ. Make sure the corresponding tests runs on
+  windows too.
   (Joke de Buhr, Vincent Ladeuil, #353370, #62539)
+  (John Arbash Meinel, Vincent Ladeuil, #492561)
 
 * Terminate ssh subprocesses when no references to them remain, fixing
   subprocess and file descriptor leaks.  (Andrew Bennetts, #426662)
 
=== modified file 'bzrlib/osutils.py'
--- bzrlib/osutils.py	2009-12-04 10:09:11 +0000
+++ bzrlib/osutils.py	2009-12-09 15:11:12 +0000
@@ -1342,6 +1342,23 @@
     """Return terminal width.
 
     None is returned if the width can't established precisely.
+
+    The rules are:
+    - if BZR_COLUMNS is set, returns its value
+    - if there is no controlling terminal, returns None
+    - if COLUMNS is set, returns its value,
+
+    From there, we need to query the OS to get the size of the controlling
+    terminal.
+
+    Unices:
+    - get termios.TIOCGWINSZ
+    - if an error occurs or a negative value is obtained, returns None
+
+    Windows:
+    
+    - win32utils.get_console_size() decides,
+    - returns None on error (provided default value)
     """
 
     # If BZR_COLUMNS is set, take it, user is always right
@@ -1355,26 +1372,50 @@
         # Don't guess, setting BZR_COLUMNS is the recommended way to override.
         return None
 
-    if sys.platform == 'win32':
-        return win32utils.get_console_size(defaultx=None)[0]
-
+    # If COLUMNS is set, take it, the terminal knows better (even inside a
+    # given terminal, the application can decide to set COLUMNS to a lower
+    # value (splitted screen) or a bigger value (scroll bars))
+    try:
+        return int(os.environ['COLUMNS'])
+    except (KeyError, ValueError):
+        pass
+
+    width, height = _terminal_size(None, None)
+    if width <= 0:
+        # Consider invalid values as meaning no width
+        return None
+
+    return width
+
+
+def _win32_terminal_size(width, height):
+    width, height = win32utils.get_console_size(defaultx=width, defaulty=height)
+    return width, height
+
+
+def _ioctl_terminal_size(width, height):
     try:
         import struct, fcntl, termios
         s = struct.pack('HHHH', 0, 0, 0, 0)
         x = fcntl.ioctl(1, termios.TIOCGWINSZ, s)
-        width = struct.unpack('HHHH', x)[1]
+        height, width = struct.unpack('HHHH', x)[0:2]
     except (IOError, AttributeError):
-        # If COLUMNS is set, take it
-        try:
-            return int(os.environ['COLUMNS'])
-        except (KeyError, ValueError):
-            return None
-
-    if width <= 0:
-        # Consider invalid values as meaning no width
-        return None
-
-    return width
+        pass
+    return width, height
+
+_terminal_size = None
+"""Returns the terminal size as (width, height).
+
+:param width: Default value for width.
+:param height: Default value for height.
+
+This is defined specifically for each OS and query the size of the controlling
+terminal. If any error occurs, the provided default values should be returned.
+"""
+if sys.platform == 'win32':
+    _terminal_size = _win32_terminal_size
+else:
+    _terminal_size = _ioctl_terminal_size
 
 
 def supports_executable():
 
=== modified file 'bzrlib/tests/test_osutils.py'
--- bzrlib/tests/test_osutils.py	2009-12-08 21:46:07 +0000
+++ bzrlib/tests/test_osutils.py	2009-12-09 15:11:12 +0000
@@ -1928,39 +1928,63 @@
 
 class TestTerminalWidth(tests.TestCase):
 
+    def replace_stdout(self, new):
+        orig_stdout = sys.stdout
+        def restore():
+            sys.stdout = orig_stdout
+        self.addCleanup(restore)
+        sys.stdout = new
+
+    def replace__terminal_size(self, new):
+        orig__terminal_size = osutils._terminal_size
+        def restore():
+            osutils._terminal_size = orig__terminal_size
+        self.addCleanup(restore)
+        osutils._terminal_size = new
+
+    def set_fake_tty(self):
+
+        class I_am_a_tty(object):
+            def isatty(self):
+                return True
+
+        self.replace_stdout(I_am_a_tty())
+
     def test_default_values(self):
-        self.assertEquals(80, osutils.default_terminal_width)
+        self.assertEqual(80, osutils.default_terminal_width)
 
     def test_defaults_to_BZR_COLUMNS(self):
         # BZR_COLUMNS is set by the test framework
-        self.assertEquals('80', os.environ['BZR_COLUMNS'])
+        self.assertNotEqual('12', os.environ['BZR_COLUMNS'])
         os.environ['BZR_COLUMNS'] = '12'
-        self.assertEquals(12, osutils.terminal_width())
+        self.assertEqual(12, osutils.terminal_width())
+
+    def test_falls_back_to_COLUMNS(self):
+        del os.environ['BZR_COLUMNS']
+        self.assertNotEqual('42', os.environ['COLUMNS'])
+        self.set_fake_tty()
+        os.environ['COLUMNS'] = '42'
+        self.assertEqual(42, osutils.terminal_width())
 
     def test_tty_default_without_columns(self):
         del os.environ['BZR_COLUMNS']
         del os.environ['COLUMNS']
-        orig_stdout = sys.stdout
-        def restore():
-            sys.stdout = orig_stdout
-        self.addCleanup(restore)
-
-        class I_am_a_tty(object):
-            def isatty(self):
-                return True
-
-        sys.stdout = I_am_a_tty()
-        self.assertEquals(None, osutils.terminal_width())
+
+        def terminal_size(w, h):
+            return 42, 42
+
+        self.set_fake_tty()
+        # We need to override the osutils definition as it depends on the
+        # running environment that we can't control (PQM running without a
+        # controlling terminal is one example).
+        self.replace__terminal_size(terminal_size)
+        self.assertEqual(42, osutils.terminal_width())
 
     def test_non_tty_default_without_columns(self):
         del os.environ['BZR_COLUMNS']
         del os.environ['COLUMNS']
-        orig_stdout = sys.stdout
-        def restore():
-            sys.stdout = orig_stdout
-        self.addCleanup(restore)
-        sys.stdout = None
-        self.assertEquals(None, osutils.terminal_width())
+        self.replace_stdout(None)
+        self.assertEqual(None, osutils.terminal_width())
 
     def test_no_TIOCGWINSZ(self):
         self.requireFeature(TermIOSFeature)
@@ -1978,4 +2002,5 @@
             del termios.TIOCGWINSZ
         del os.environ['BZR_COLUMNS']
         del os.environ['COLUMNS']
-        self.assertIs(None, osutils.terminal_width())
+        # Whatever the result is, if we don't raise an exception, it's ok.
+        osutils.terminal_width()