Merge lp:~vila/bzr/353370-notty-no-term-width into lp:bzr
- 353370-notty-no-term-width
- Merge into bzr.dev
Status: | Merged | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Approved by: | Martin Pool | ||||||||||||
Approved revision: | no longer in the source branch. | ||||||||||||
Merge reported by: | Vincent Ladeuil | ||||||||||||
Merged at revision: | not available | ||||||||||||
Proposed branch: | lp:~vila/bzr/353370-notty-no-term-width | ||||||||||||
Merge into: | lp:bzr | ||||||||||||
Diff against target: |
46 lines (+14/-5) 2 files modified
bzrlib/osutils.py (+9/-5) bzrlib/tests/test_osutils.py (+5/-0) |
||||||||||||
To merge this branch: | bzr merge lp:~vila/bzr/353370-notty-no-term-width | ||||||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Martin Pool | Approve | ||
Review via email: mp+13832@code.launchpad.net |
Commit message
Description of the change
Vincent Ladeuil (vila) wrote : | # |
John A Meinel (jameinel) wrote : | # |
So vila and I discussed this a bit on IRC, I'll try to summarize my part.
1) I'm concerned about respecting $COLUMNS even when redirecting. For example doing "bzr log --line >file", I probably would expect it to not truncate any lines. Note that this differs from what people probably want when they do "bzr log --line | less".
2) In general, I feel like most of our "truncated output" goes to stderr rather than stdout. Which means that checking stdout is going to give incorrect results. (The only thing we could come up with was 'bzr log --line' that truncates to stdout, everything else is stuff like 'progress' that goes to stderr.)
Of course, the bug #353370 is *explicitly* about 'bzr log --line | less' getting truncated. It would indicate (to me) that we should be detecting the terminal width on the file-handle that we are going to be writing out the truncated data.
3) 256 rather than 65536 seems fine to me. The specific width when things aren't truncated doesn't matter as long as it is "big enough".
I suppose I'm a bit concerned as to what output we are generating that causes vila to see the log files balloon into GB. Perhaps the progress bars are being redirected to the output file? (And thus the 'clean the bar' step is generating 64kB each time. Though, I don't think we want progress output in our debug logs anyway...
Vincent Ladeuil (vila) wrote : | # |
>>>>> "jam" == John A Meinel <email address hidden> writes:
<snip/>
jam> 1) I'm concerned about respecting $COLUMNS even when
jam> redirecting. For example doing "bzr log --line >file", I
jam> probably would expect it to not truncate any lines. Note
jam> that this differs from what people probably want when
jam> they do "bzr log --line | less".
That's a good point I don't have a good answer to :-) Except that
plugins like bzr-pager can set their own COLUMN value...
And for the bug use-case, as long as COLUMNS is not set (the
default setup for everybody) 'bzr log --line >file' will now
truncate less than before (256 instead of 80).
Also, I'd like to mention that after some experiments, it appears
that COLUMNS is seen by python only if set by the user (which I
think is rare enough, but worth respecting if present).
i.e: on Ubuntu, OSX, and FreeBSD we have:
vila:~ :) $ env|grep COLUMNS
vila:~ :( $ echo $COLUMNS
80
vila:~ :) $ python -c "import os; print os.environ[
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "/Library/
raise KeyError(key)
KeyError: 'COLUMNS'
vila:~ :( $
Oh my... experimenting some more:
vila:~ :) $ export COLUMNS=12
vila:~ :) $ env|grep COLUMNS
COLUMNS=12
vila:~ :) $ env|grep COLUMNS
COLUMNS=80
vila:~ :) $
.... restoring mental sanity :
vila:~ :) $ shopt -u checkwinsize
vila:~ :) $ export COLUMNS=12
vila:~ :) $ env|grep COLUMNS
COLUMNS=12
vila:~ :) $ env|grep COLUMNS
COLUMNS=12
So we may want to use some bzr specific value like BZR_COLUMNS
just to be on the safe side :-/
jam> 2) In general, I feel like most of our "truncated
jam> output" goes to stderr rather than stdout. Which means
jam> that checking stdout is going to give incorrect
jam> results.
Note that on windows we're checking stderr via get_console_size...
jam> (The only thing we could come up with was 'bzr log
jam> --line' that truncates to stdout, everything else is
jam> stuff like 'progress' that goes to stderr.)
But when we use termios we don't refer anymore to stdout nor
stderr...
jam> Of course, the bug #353370 is *explicitly* about 'bzr
jam> log --line less' getting truncated. It would indicate
jam> (to me) that we should be detecting the terminal width
jam> on the file-handle that we are going to be writing out
jam> the truncated data.
... which isn't always possible since we are talking about the
*terminal* here.
jam> 3) 256 rather than 65536 seems fine to me. The specific
jam> width when things aren't truncated doesn't matter as
jam> long as it is "big enough".
jam> I suppose I'm a bit concerned as to what output we are
jam> generating that causes vila to see the log files balloon
jam> into GB.
Mostly filling lines for each test with ~65000 spaces and then
append 'OK nn ms' or 'SKIP ..', etc. It took me a while to
understand what was going on because all I was seeing was:
- a scroll bar when looking at the log files (I didn't realize
the OK/SKIP were not there anymore even if the output looked a
...
Martin Pool (mbp) wrote : | # |
Sorry but I really think truncating to any width when there's no terminal to truncate for is bogus. If we are going to do it, better to do it at 80 so that it's obvious, rather than sweeping it under the carpet.
If you want progress for non-tty output I'd rather look at restoring something like the 'dots' progress bar.
Vincent Ladeuil (vila) wrote : | # |
>>>>> "martin" == Martin Pool <email address hidden> writes:
martin> Review: Disapprove
martin> Sorry but I really think truncating to any width when
martin> there's no terminal to truncate for is bogus.
I would have appreciated a bit more depth for that review (I've been
acting as patch pilot before the initiative was started so I'm not the
original author of the patch).
The case at point is not to truncate when there is no terminal, it is
that bzr want to right-justify when there is no terminal, without
terminal it's hard to derive a width and without width you can't
right-justify.
That's why I said several times that a deeper fix is needed *in the
callers* (the so-called "filling" cases, corrections welcome if it's
unclear) and that's where I was expecting your input.
martin> If we are going to do it, better to do it at 80 so that it's
martin> obvious, rather than sweeping it under the carpet.
That's definitely not a progress since that doesn't give any way to get
full test names for example.
martin> If you want progress for non-tty output I'd rather look at
martin> restoring something like the 'dots' progress bar.
Irrelevant. Having the name of the test output when the test start is
far enough, having the ability to display the *full* name is what I was
after, but only as an additional bonus.
Martin Pool (mbp) wrote : | # |
2009/11/24 Vincent Ladeuil <email address hidden>:
>>>>>> "martin" == Martin Pool <email address hidden> writes:
>
> martin> Review: Disapprove
>
> martin> Sorry but I really think truncating to any width when
> martin> there's no terminal to truncate for is bogus.
>
> I would have appreciated a bit more depth for that review (I've been
> acting as patch pilot before the initiative was started so I'm not the
> original author of the patch).
Sorry.
I'll grep for the callers and see if it would be practical to fix them.
--
Martin <http://
Martin Pool (mbp) wrote : | # |
I wonder if we actually need two concepts here: the 'align right' width, and the 'truncation' width. So on an infinitely wide terminal, or when the terminal size is not known, you might assume 80 (or 132 or something) for the first, and not truncate at all. And there's also the question of the progress bar and other possible future things where we try to erase the whole contents of a line.
The bugs this touches are
https:/
https:/
https:/
Martin Pool (mbp) wrote : | # |
So,
I don't want to shortcircuit the conversation, but as per my comments on those bugs, I think we should update the callers rather than making a guess at what width to use if there is no terminal. So to keep the patch queue clean, I'm going to mark this back to inprogress. If you object and want to make a case that we should merge it incrementally, please put it back to needsreview.
Vincent Ladeuil (vila) wrote : | # |
Right to the point :)
No, I think it's fair to fix the callers, I may not be able to do that as soon as I want, but soon enough.
I don't think it's mergeable as is if only because I misunderstood COLUMNS behavior when I wrote it.
Vincent Ladeuil (vila) wrote : | # |
I just push a new version that should address the points raised so far,
including the IRC discussion with Martin.
In summary, terminal_width() now returns None when no terminal can be found
to get the width from. I've also introduced a BZR_COLUMNS env variable that
takes precedence and can be used to force any value when *needed*
(pagers, pager users and babune I'm looking at you).
Martin Pool (mbp) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
2009/12/4 Vincent Ladeuil <email address hidden>:
> https:/
> You are reviewing the proposed merge of lp:~vila/bzr/353370-notty-no-term-width into lp:bzr.
Thanks, Vincent!
>
> === modified file 'NEWS'
> --- NEWS 2009-12-02 07:56:16 +0000
> +++ NEWS 2009-12-02 16:25:18 +0000
> @@ -26,6 +26,12 @@
> * ``bzr commit`` now has a ``--commit-time`` option.
> (Alexander Sack, #459276)
>
> +* The ``BZR_COLUMNS`` envrionment variable can be set to force bzr to
> + respect a given terminal width. This can be useful when output is
> + redirected or in obscure cases where the default value is not
> + appropriate.
> + (Vincent Ladeuil)
> +
> Bug Fixes
> *********
>
> @@ -45,6 +51,12 @@
> * 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.
> + 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.
> + (Joke de Buhr, Vincent Ladeuil, #353370, #62539)
> +
> * Terminate ssh subprocesses when no references to them remain, fixing
> subprocess and file descriptor leaks. (Andrew Bennetts, #426662)
You're cooler than this change lets on: explain why:
log --line uses the full width of the terminal, even when run through a pager.
and did it fix any other user-visible issues?
> +default_
> +"""The default terminal width for ttys.
> +
> +This is defined so that higher levels can share a common fallback value when
> +terminal_width() returns None.
> +"""
> +
> +
> def terminal_width():
> - """Return estimated terminal width."""
> + """Return terminal width.
> +
> + None is returned if the width can't established precisely.
> + """
> +
> + # If BZR_COLUMNS is set, take it, user is always right
> + try:
> + return int(os.
> + except (KeyError, ValueError):
> + pass
> +
> + # If COLUMNS is set, take it, the terminal knows better
> + try:
> + return int(os.
> + except (KeyError, ValueError):
> + pass
> +
> + isatty = getattr(sys.stdout, 'isatty', None)
> + if isatty is None or not isatty():
> + # Don't guess, setting BZR_COLUMNS is the recommended way to override.
> + return None
> +
This isn't necessarily wrong, but from the description above it
sounded like you were going to make BZR_COLUMNS 'stronger' than
COLUMNS, whereas it just has priority. I guess it probably is correct
to use $COLUMNS even if stdout is not a tty. So you're fine.
Otherwise good
review approve
merge approved
- - --
Martin <http://
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
iEYEARECAAYFAks
BpUAn3NREtpvLoI
=dgVy
-----END PGP SIGNATURE-----
Martin Pool (mbp) wrote : | # |
I think this is ok to land as it is, but eventually the priority we want is
1- BZR_COLUMNS
2- TIOCGWINSZ if possible (or the equivalent for win32)
3- COLUMNS otherwise
4- otherwise None
also fwiw http://
Vincent Ladeuil (vila) wrote : | # |
>>>>> "martin" == Martin Pool <email address hidden> writes:
martin> I think this is ok to land as it is, but eventually the priority we want is
martin> 1- BZR_COLUMNS
martin> 2- TIOCGWINSZ if possible (or the equivalent for win32)
martin> 3- COLUMNS otherwise
martin> 4- otherwise None
Damn, I didn't notice that earlier.
Well, I tried that but that's wrong, at least under emacs. That
means at least in one case COLUMNS is more correct than
TIOCGWINSZ (I *think* the rationale is that you can have COLUMNS
smaller than the window size because you use only part of that
window or bigger because the window has scroll bars).
So I reverted to my first proposal and align the tests so they
pass on PQM where things are a bit different too.
You
martin> also fwiw
martin> http://
martin> suggests checking TIOCGSIZE too
Hmm. Right it also seem to ignore LINES and COLUMNS to get to the
real terminal size, confirming that COLUMNS is used to override
TIOCGWINSZ, not used as a fallback.
I'll revisit that anyway if anybody raise concerns and include
that while handling the resize window event.
Vincent
John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Vincent Ladeuil wrote:
>>>>>> "martin" == Martin Pool <email address hidden> writes:
>
> martin> I think this is ok to land as it is, but eventually the priority we want is
> martin> 1- BZR_COLUMNS
> martin> 2- TIOCGWINSZ if possible (or the equivalent for win32)
> martin> 3- COLUMNS otherwise
> martin> 4- otherwise None
>
> Damn, I didn't notice that earlier.
>
> Well, I tried that but that's wrong, at least under emacs. That
> means at least in one case COLUMNS is more correct than
> TIOCGWINSZ (I *think* the rationale is that you can have COLUMNS
> smaller than the window size because you use only part of that
> window or bigger because the window has scroll bars).
>
> So I reverted to my first proposal and align the tests so they
> pass on PQM where things are a bit different too.
>
> You
>
> martin> also fwiw
> martin> http://
> martin> suggests checking TIOCGSIZE too
>
> Hmm. Right it also seem to ignore LINES and COLUMNS to get to the
> real terminal size, confirming that COLUMNS is used to override
> TIOCGWINSZ, not used as a fallback.
>
> I'll revisit that anyway if anybody raise concerns and include
> that while handling the resize window event.
>
> Vincent
I'll mention that this broke the test suite on Windows in a couple of ways.
See: https:/
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAks
dusAnRnaNiL469q
=0KcF
-----END PGP SIGNATURE-----
Martin Pool (mbp) wrote : | # |
2009/12/5 Vincent Ladeuil <email address hidden>:
>>>>>> "martin" == Martin Pool <email address hidden> writes:
>
> martin> I think this is ok to land as it is, but eventually the priority we want is
> martin> 1- BZR_COLUMNS
> martin> 2- TIOCGWINSZ if possible (or the equivalent for win32)
> martin> 3- COLUMNS otherwise
> martin> 4- otherwise None
>
> Damn, I didn't notice that earlier.
>
> Well, I tried that but that's wrong, at least under emacs. That
> means at least in one case COLUMNS is more correct than
> TIOCGWINSZ (I *think* the rationale is that you can have COLUMNS
> smaller than the window size because you use only part of that
> window or bigger because the window has scroll bars).
Is this emacs in its own frame, or emacs inside a termal?
If you run bzr inside a terminal and then resize it while bzr is
running, $COLUMNS does not change. (I am assuming the fcntl will give
you the right result.)
Anyhow, this is still a step forward.
--
Martin <http://
Vincent Ladeuil (vila) wrote : | # |
So, John filed a bug regarding the patch failing on windows so I'll mark this proposal as merged and will continue working on bug 492561.
Preview Diff
1 | === modified file 'bzrlib/osutils.py' |
2 | --- bzrlib/osutils.py 2009-12-04 10:09:11 +0000 |
3 | +++ bzrlib/osutils.py 2009-12-04 11:31:10 +0000 |
4 | @@ -1358,17 +1358,21 @@ |
5 | if sys.platform == 'win32': |
6 | return win32utils.get_console_size(defaultx=None)[0] |
7 | |
8 | + # If COLUMNS is set, take it, the terminal knows better (at least under |
9 | + # emacs, COLUMNS gives an accurate answer while the fcntl.ioctl call below |
10 | + # doesn't) -- vila 20091204 |
11 | + try: |
12 | + return int(os.environ['COLUMNS']) |
13 | + except (KeyError, ValueError): |
14 | + pass |
15 | + |
16 | try: |
17 | import struct, fcntl, termios |
18 | s = struct.pack('HHHH', 0, 0, 0, 0) |
19 | x = fcntl.ioctl(1, termios.TIOCGWINSZ, s) |
20 | width = struct.unpack('HHHH', x)[1] |
21 | except (IOError, AttributeError): |
22 | - # If COLUMNS is set, take it |
23 | - try: |
24 | - return int(os.environ['COLUMNS']) |
25 | - except (KeyError, ValueError): |
26 | - return None |
27 | + return None |
28 | |
29 | if width <= 0: |
30 | # Consider invalid values as meaning no width |
31 | |
32 | === modified file 'bzrlib/tests/test_osutils.py' |
33 | --- bzrlib/tests/test_osutils.py 2009-12-04 10:09:11 +0000 |
34 | +++ bzrlib/tests/test_osutils.py 2009-12-04 11:31:10 +0000 |
35 | @@ -1936,6 +1936,11 @@ |
36 | os.environ['BZR_COLUMNS'] = '12' |
37 | self.assertEquals(12, osutils.terminal_width()) |
38 | |
39 | + def test_falls_back_to_COLUMNS(self): |
40 | + del os.environ['BZR_COLUMNS'] |
41 | + os.environ['COLUMNS'] = '42' |
42 | + self.assertEquals(42, osutils.terminal_width()) |
43 | + |
44 | def test_tty_default_without_columns(self): |
45 | del os.environ['BZR_COLUMNS'] |
46 | del os.environ['COLUMNS'] |
Since there are no single answer valid for all cases, I went for two different ones:
- one for the tty case 80,
- one for the non-tty case 256.
The COLUMNS environment variable is always respected *first* so that users have a way
to override any heuristic we may use.
pagers can use either the COLUMNS variable or go for the bzrlib.osutils default values non_tty_ width).
(+default_tty_width or +default_
I added tests for most of the cases except windows (since I can't test that), if any reviewer can
add a such a test I'll gladly add it.
I also addressed bug #62539 while I was there.