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

 merge rejected

Martin [gz] wrote:
> Review: Abstain
> Thanks for writing this, it's interesting to see. It's unfortunate we can't do
> the work that actually needs to happen in response to the signal (setting the
> COLUMNS envvar) as it's not an atomic operation. So, this essentially moves us
> back to polling, at which point, why bother with signals at all? Moving the
> width variable to a Python int wouldn't help either.

Well, it changes the cost of osutils.terminal_width from invoking the
ioctl (and associated struct.pack/unpack) every time, to only when the
the width may have changed. So in the common case the poll is check
glancing at an int, rather than interrogating the OS. Micro-benchmarks
with timeit suggest this is about 200x faster.

Martin P makes a good point that we don't really check the
terminal_width often enough for this matter though --
_ioctl_terminal_size only takes 11 microseconds on my fairly slow
laptop, and we simply don't call it very often.

So I'm going to abandon this patch (even though I'm quite confident in
its correctness) and instead propose one to take out all SIGWINCH code
entirely.

> A few misc comments on the patch:
>
> +if compiler.has_function('sigaction'):
>
> Think that should be instead:
>
> if compiler.has_function('sigaction', includes=['signal.h']):
>
> Also, putting it in the not-win32 section would at least mean no stderr kipple for me. ;)

Good points, something I'll keep in mind for next time :)

> Have been reading ncurses source, and they check for SA_RESTART on top
> of sigaction. Probably not something to worry about, have fewer
> platforms and less history with Bazaar.

That was my hope too ;)