Code review comment for lp:~vila/bzr/353370-notty-no-term-width

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.

« Back to merge proposal