Code review comment for lp:~spiv/bzr/no-sigwinch-583941

>>>>> Andrew Bennetts <email address hidden> writes:

    > Andrew Bennetts has proposed merging lp:~spiv/bzr/no-sigwinch-583941 into lp:bzr/2.1.
    > Requested reviews:
    > bzr-core (bzr-core)
    > Related bugs:
    > #583941 program crashes with "bzr: ERROR: socket.error: (4, 'Interrupted system call')"
    > https://bugs.launchpad.net/bugs/583941

    > Final, simple fix for EINTR?

    > This simply removes the SIGWINCH handler introduced in 2.1.0rc1,
    > and adjusts the osutils.terminal_width function accordingly.

    > I'm a little concerned that the logic inside terminal_width looks
    > somewhat different now, because it tries _terminal_size before it
    > tries $COLUMNS — but, strangely, the SIGWINCH handler had actually
    > been setting $COLUMNS, so I don't think this is actually so
    > different.

qblame to the rescue, have a look at 4747.4.3
(revid:<email address hidden>[1]):

,----
| * bzrlib/osutils.py:
| (terminal_width): COLUMNS takes priority on termios.TIOCGWINSZ as
| shown by tests under emacs (on no counter examples are known).
`----

This is indeed a known case where COLUMNS should take precedence. And
the comment you deleted was trying to hint at that. If you find a better
wording, please feel free to update it but don't just delete the hints
about the rationale.

Unfortunately, this couldn't be translated into a test since the tests
are supposed to be runned even without a controlling terminal ;(

We should check the env variable first *at least* when we start.

Now, if SIGWINCH occurs, we can't get an updated value for COLUMNS, so
we have to resort to _terminal_size (even if it's wrong now, it seems
less wrong than keeping a knowingly out-of-date COLUMNS) and setting
COLUMNS was the easiest way to go then.

[1]: Amazingly this revid includes the string 'bug' :)