Bazaar

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

  1. 353370-notty-no-term-width
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Approved by: John A Meinel
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~vila/bzr/353370-notty-no-term-width
Merge into: lp:bzr
Diff against target: 208 lines (+106/-38)
3 files modified
NEWS (+4/-2)
bzrlib/osutils.py (+56/-15)
bzrlib/tests/test_osutils.py (+46/-21)
To merge this branch: bzr merge lp:~vila/bzr/353370-notty-no-term-width
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
John A Meinel Approve
Review via email: mp+15858@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

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.

Revision history for this message
John A Meinel (jameinel) wrote :

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
Revision history for this message
Martin Pool (mbp) wrote :

capital 'Windows' when talking about that other software.

It seems reasonable. Let's try it.

review: Needs Fixing

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'NEWS'
2--- NEWS 2009-12-09 05:47:32 +0000
3+++ NEWS 2009-12-09 15:11:12 +0000
4@@ -64,11 +64,13 @@
5 * Lots of bugfixes for the test suite on Windows. We should once again
6 have a test suite with no failures on Windows. (John Arbash Meinel)
7
8-* ``osutils.terminal_width()`` obeys the BZR_COLUMNS envrionment
9+* ``osutils.terminal_width()`` obeys the BZR_COLUMNS environment
10 variable but returns None if the terminal is not a tty (when output is
11 redirected for example). Also fixes its usage under OSes that doesn't
12- provide termios.TIOCGWINSZ.
13+ provide termios.TIOCGWINSZ. Make sure the corresponding tests runs on
14+ windows too.
15 (Joke de Buhr, Vincent Ladeuil, #353370, #62539)
16+ (John Arbash Meinel, Vincent Ladeuil, #492561)
17
18 * Terminate ssh subprocesses when no references to them remain, fixing
19 subprocess and file descriptor leaks. (Andrew Bennetts, #426662)
20
21=== modified file 'bzrlib/osutils.py'
22--- bzrlib/osutils.py 2009-12-04 10:09:11 +0000
23+++ bzrlib/osutils.py 2009-12-09 15:11:12 +0000
24@@ -1342,6 +1342,23 @@
25 """Return terminal width.
26
27 None is returned if the width can't established precisely.
28+
29+ The rules are:
30+ - if BZR_COLUMNS is set, returns its value
31+ - if there is no controlling terminal, returns None
32+ - if COLUMNS is set, returns its value,
33+
34+ From there, we need to query the OS to get the size of the controlling
35+ terminal.
36+
37+ Unices:
38+ - get termios.TIOCGWINSZ
39+ - if an error occurs or a negative value is obtained, returns None
40+
41+ Windows:
42+
43+ - win32utils.get_console_size() decides,
44+ - returns None on error (provided default value)
45 """
46
47 # If BZR_COLUMNS is set, take it, user is always right
48@@ -1355,26 +1372,50 @@
49 # Don't guess, setting BZR_COLUMNS is the recommended way to override.
50 return None
51
52- if sys.platform == 'win32':
53- return win32utils.get_console_size(defaultx=None)[0]
54-
55+ # If COLUMNS is set, take it, the terminal knows better (even inside a
56+ # given terminal, the application can decide to set COLUMNS to a lower
57+ # value (splitted screen) or a bigger value (scroll bars))
58+ try:
59+ return int(os.environ['COLUMNS'])
60+ except (KeyError, ValueError):
61+ pass
62+
63+ width, height = _terminal_size(None, None)
64+ if width <= 0:
65+ # Consider invalid values as meaning no width
66+ return None
67+
68+ return width
69+
70+
71+def _win32_terminal_size(width, height):
72+ width, height = win32utils.get_console_size(defaultx=width, defaulty=height)
73+ return width, height
74+
75+
76+def _ioctl_terminal_size(width, height):
77 try:
78 import struct, fcntl, termios
79 s = struct.pack('HHHH', 0, 0, 0, 0)
80 x = fcntl.ioctl(1, termios.TIOCGWINSZ, s)
81- width = struct.unpack('HHHH', x)[1]
82+ height, width = struct.unpack('HHHH', x)[0:2]
83 except (IOError, AttributeError):
84- # If COLUMNS is set, take it
85- try:
86- return int(os.environ['COLUMNS'])
87- except (KeyError, ValueError):
88- return None
89-
90- if width <= 0:
91- # Consider invalid values as meaning no width
92- return None
93-
94- return width
95+ pass
96+ return width, height
97+
98+_terminal_size = None
99+"""Returns the terminal size as (width, height).
100+
101+:param width: Default value for width.
102+:param height: Default value for height.
103+
104+This is defined specifically for each OS and query the size of the controlling
105+terminal. If any error occurs, the provided default values should be returned.
106+"""
107+if sys.platform == 'win32':
108+ _terminal_size = _win32_terminal_size
109+else:
110+ _terminal_size = _ioctl_terminal_size
111
112
113 def supports_executable():
114
115=== modified file 'bzrlib/tests/test_osutils.py'
116--- bzrlib/tests/test_osutils.py 2009-12-08 21:46:07 +0000
117+++ bzrlib/tests/test_osutils.py 2009-12-09 15:11:12 +0000
118@@ -1928,39 +1928,63 @@
119
120 class TestTerminalWidth(tests.TestCase):
121
122+ def replace_stdout(self, new):
123+ orig_stdout = sys.stdout
124+ def restore():
125+ sys.stdout = orig_stdout
126+ self.addCleanup(restore)
127+ sys.stdout = new
128+
129+ def replace__terminal_size(self, new):
130+ orig__terminal_size = osutils._terminal_size
131+ def restore():
132+ osutils._terminal_size = orig__terminal_size
133+ self.addCleanup(restore)
134+ osutils._terminal_size = new
135+
136+ def set_fake_tty(self):
137+
138+ class I_am_a_tty(object):
139+ def isatty(self):
140+ return True
141+
142+ self.replace_stdout(I_am_a_tty())
143+
144 def test_default_values(self):
145- self.assertEquals(80, osutils.default_terminal_width)
146+ self.assertEqual(80, osutils.default_terminal_width)
147
148 def test_defaults_to_BZR_COLUMNS(self):
149 # BZR_COLUMNS is set by the test framework
150- self.assertEquals('80', os.environ['BZR_COLUMNS'])
151+ self.assertNotEqual('12', os.environ['BZR_COLUMNS'])
152 os.environ['BZR_COLUMNS'] = '12'
153- self.assertEquals(12, osutils.terminal_width())
154+ self.assertEqual(12, osutils.terminal_width())
155+
156+ def test_falls_back_to_COLUMNS(self):
157+ del os.environ['BZR_COLUMNS']
158+ self.assertNotEqual('42', os.environ['COLUMNS'])
159+ self.set_fake_tty()
160+ os.environ['COLUMNS'] = '42'
161+ self.assertEqual(42, osutils.terminal_width())
162
163 def test_tty_default_without_columns(self):
164 del os.environ['BZR_COLUMNS']
165 del os.environ['COLUMNS']
166- orig_stdout = sys.stdout
167- def restore():
168- sys.stdout = orig_stdout
169- self.addCleanup(restore)
170-
171- class I_am_a_tty(object):
172- def isatty(self):
173- return True
174-
175- sys.stdout = I_am_a_tty()
176- self.assertEquals(None, osutils.terminal_width())
177+
178+ def terminal_size(w, h):
179+ return 42, 42
180+
181+ self.set_fake_tty()
182+ # We need to override the osutils definition as it depends on the
183+ # running environment that we can't control (PQM running without a
184+ # controlling terminal is one example).
185+ self.replace__terminal_size(terminal_size)
186+ self.assertEqual(42, osutils.terminal_width())
187
188 def test_non_tty_default_without_columns(self):
189 del os.environ['BZR_COLUMNS']
190 del os.environ['COLUMNS']
191- orig_stdout = sys.stdout
192- def restore():
193- sys.stdout = orig_stdout
194- self.addCleanup(restore)
195- sys.stdout = None
196- self.assertEquals(None, osutils.terminal_width())
197+ self.replace_stdout(None)
198+ self.assertEqual(None, osutils.terminal_width())
199
200 def test_no_TIOCGWINSZ(self):
201 self.requireFeature(TermIOSFeature)
202@@ -1978,4 +2002,5 @@
203 del termios.TIOCGWINSZ
204 del os.environ['BZR_COLUMNS']
205 del os.environ['COLUMNS']
206- self.assertIs(None, osutils.terminal_width())
207+ # Whatever the result is, if we don't raise an exception, it's ok.
208+ osutils.terminal_width()