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

[Takashi Yano]

Hi Corinna,

Thank you for checking the patch.

On Mon, 31 Aug 2020 10:16:51 +0200
Corinna Vinschen wrote:
> 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(*).

This does not matter because the code is
  if (GetTickCount() - t0 > 40)
rather than
  if (GetTickCount() > t0 + 40)
and because DWORD is 32bit unsigned integer.

If the overflow occurs within the timeout period, for example,
t0 = 0xfffffff0 and GetTickCount() becomes 0x00000002,
GetTickCount() - t0 equals to 18 (0x12) as expected.

If the code is
  if (GetTickCount() > t0 + 40)
and t0 = 0xfffffff0 and GetTickCount() is 0xfffffff1,
t0 + 40 equals to 24 (0x18)
so GetTickCount() > t0 + 40 is true against expectation.

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

pcon_start is cleared to false in master write() if responce to CSI6n
is sent. Therefore, it is necessary to set again after the responce.
I will added the comment in the source.

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

You are right. I will submit the v10 patch.

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

Same here. That does not matter.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>