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

Proposed by Vincent Ladeuil
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
Reviewer Review Type Date Requested Status
Martin Pool Approve
Review via email: mp+13832@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Vincent Ladeuil (vila) wrote :

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
(+default_tty_width or +default_non_tty_width).

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.

Revision history for this message
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...

Revision history for this message
Vincent Ladeuil (vila) wrote :
Download full text (4.7 KiB)

>>>>> "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['COLUMNS']"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/Library/Frameworks/Python.framework/Versions/2.5/lib/python2.5/UserDict.py", line 22, in __getitem__
    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
  ...

Read more...

Revision history for this message
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.

review: Disapprove
Revision history for this message
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.

Revision history for this message
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://launchpad.net/~mbp/>

Revision history for this message
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://bugs.edge.launchpad.net/bzr/+bug/62539 - trivial 'missing TIOCGWINSZ' "John A Meinel wrote on 2006-10-24: missed 0.12, is simple enough to happen soon." :-) Not controversial.

https://bugs.edge.launchpad.net/bzr-pager/+bug/353370 - assuming that non-ttys have 80 char width causes problems when bzr is run through a pager

https://bugs.edge.launchpad.net/bzr/+bug/458743 (now a dupe) - if Bazaar pretends the terminal is very wide for the sake of truncation, then it draws unreasonably wide progress bars.

Revision history for this message
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.

Revision history for this message
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.

Revision history for this message
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).

Revision history for this message
Martin Pool (mbp) wrote :

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

2009/12/4 Vincent Ladeuil <email address hidden>:
> https://code.edge.launchpad.net/~vila/bzr/353370-notty-no-term-width/+merge/13832
> 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.terminal_width()`` obeys the BZR_COLUMNS envrionment
> + 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_terminal_width = 80
> +"""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.environ['BZR_COLUMNS'])
> + except (KeyError, ValueError):
> + pass
> +
> + # If COLUMNS is set, take it, the terminal knows better
> + try:
> + return int(os.environ['COLUMNS'])
> + 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://launchpad.net/~mbp/>

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAksYo60ACgkQPGPKP6Cz6It7QwCggAjXKmuO/VEgng0ZeX87zf8E
BpUAn3NREtpvLoI+yLIYKItFROS9MsAF
=dgVy
-----END PGP SIGNATURE-----

review: Approve
Revision history for this message
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://www.ohse.de/uwe/software/resize.c.html suggests checking TIOCGSIZE too

Revision history for this message
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://www.ohse.de/uwe/software/resize.c.html
    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

Revision history for this message
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://www.ohse.de/uwe/software/resize.c.html
> 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://bugs.launchpad.net/bugs/492561

John
=:->

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

iEYEARECAAYFAksZcGQACgkQJdeBCYSNAANN2gCeOgkkdsU6UZqiWyZx836UJotH
dusAnRnaNiL469qWoqX0q4vm+69dgOVb
=0KcF
-----END PGP SIGNATURE-----

Revision history for this message
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://launchpad.net/~mbp/>

Revision history for this message
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

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
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']