[PATCH] Cygwin: pty: Disable pseudo console if TERM is dumb or not set.

[Corinna Vinschen]

Hi Takashi,

On Aug 31 12:26, Takashi Yano via Cygwin-patches wrote:
> Hi Corinna,
> 
> On Sun, 30 Aug 2020 14:49:25 +0200
> Corinna Vinschen wrote:
> > I like the idea in general, but isn't there a noticable perfomance hit?
> 
> I have measured the startup time of non-cygwin process
> with v2 and v8 patch.
> 
> mintty with v2    : 92.7ms
> emacs-dumb with v2: 22.8ms
> 
> mintty with v8    : 94.6ms
> emacs-dumb with v8: 22.7ms
> 
> There is not noticeable difference more than measurement
> error.

Great.

> By the way, I have implemented timeout strategy for CSI6n, you
> mentioned in the previous comment, for a test. This check is
> done only on the first execution of non-cygwin apps in the pty.
> With this patch, first checks if the terminfo has cursor_home
> (ESC [H). If terminfo has cursor_home, ANSI escape sequence is
> supposed to be supported. In this case, I expect not to display
> garbage "^[[6n" even if CSI6n is sent because the parser ignores
> unsupported CSI sequences.
> 
> With this implementation, pseudo console works in tmux as well
> even if TERM=screen.
> 
> Please have a look v9 patch attached.
> 
> The performance of v9 is also checked.
> 
> mintty with v9    : 93.9ms
> emacs-dumb with v9: 22.8ms
> ANSI without CSI6n: 22.8ms
> 
> [the first time in v9]
> mintty            : 94.8ms
> emacs-dumb        : 22.5ms
> ANSI without CSI6n: 63.5ms
> 
> Most of the results are the same as v2 and v8 except for the
> first execution of non-cygwin apps in ansi terminal without
> CSI6n. It takes about 40ms (timeout) longer than dumb terminal
> in ANSI terminal without CSI6n support.
> 
> However, this causes only on the first execution of non-cygwin
> apps in pty.
> 
> I think this is the most reasonable one I have ever proposed.

This looks good, but I have a few nits:

- Don't use GetTickCount().  Use GetTickCount64().  Otherwise the code
  is prone to the 49 days tick counter overflow problem(*).

- get_ttyp ()->pcon_start is set to true twice in term_has_pcon_cap().

- Following the maybe_dumb label, you're setting has_csi6n and
  has_set_title to false, but these are the default values anyway.
  Also, setting has_set_title to false in the preceeding else branch
  seems unnecessary.  Do you want that for clarity?  If not I'd drop
  them.

With these minor problems fixed, I'm happy to push the change.

(*) I just noticed belatedly that GetTickCount() is used in the tty code
    already.  Can you please change pcon_last_time to ULONGLONG and use
    GetTickCount64() instead?


Thanks,
Corinna