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

>>>>> "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
  bit weird (but of course I couldn't put my finger on *what* was
  weird)),

- FreeBSD slaves failing all over the place with timeouts related
  to low level em0 device (network...)

Of course poking in and there on either the master configs or
slaves did nothing... until I suddenly realize the huge size of
the log files....

    jam> Perhaps the progress bars are being redirected to the
    jam> output file? (And thus the 'clean the bar' step is
    jam> generating 64kB each time. Though, I don't think we want
    jam> progress output in our debug logs anyway...

I have a good use case for that as it helps detecting hanging
operations on babune.... or differentiate a hanging checkout from
a looooong one (more than 1200 secs say) and I've used that
feature from babune's day one (of, three or four but for some
times now).

Activating the progress reporting helps here because we output
*something* while doing the checkout.

Overall, that code was new to me (and still is partly) and we may
want to think about enhancing ui_factory to handle more details.

John also mentioned handling SIGWINCH but I didn't want to make
that patch bigger than it was especially given the side-effects
for what was originally a one-line fix...

So, like John, I'd like a broader discussion here, but unlike
him, I'd still like to land that patch without adding too much
features :-)

As noted, a deeper fix will be to rewrite all callers so that the
'no width' case is better handled than using a high value for
that, but then, that's still left the problem of the filling
cases.