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

-----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-----