[PATCH v2] Cygwin: pty: Reduce unecessary input transfer.

[Johannes Schindelin]

Hi Takashi,

On Fri, 10 Dec 2021, Takashi Yano wrote:

> On Fri, 10 Dec 2021 00:05:27 +0100 (CET)
> Johannes Schindelin wrote:
> > sorry for responding to a patch you sent almost 10 months ago... but... I
> > am struggling with it.
> >
> > First of all, let me describe the problem I am seeing (see also
> > https://github.com/git-for-windows/git/issues/3579): after upgrading the
> > MSYS2 runtime to v3.3.3 in Git for Windows, whenever I ask `git.exe` to
> > spawn `vim.exe` to edit any file, after quitting `vim` I see spurious ANSI
> > sequences being "ghost-typed" into the terminal (which is a MinTTY running
> > under `TERM=xterm`).
> >
> > Apparently the ANSI sequences report the cursor position and the
> > foreground/background color in response to a CSI [ 6n sent from `vim`.
> >
> > Clearly, those sequences should go to `vim.exe`, but they mostly don't
> > arrive there (but in MinTTY instead, as if I had typed them). Sometimes,
> > the foreground/background color seems to arrive in the `vim` process, but
> > the cursor position almost always does not. I suspect that it is important
> > that `git.exe` is a non-MSYS2 process whereas `vim.exe` is an MSYS2
> > process, and something inside the MSYS2 runtime is at fault.
> >
> > I've bisected this incorrect behavior to the patch I am replying to.
> >
> > I tried to trigger the same bug in pure Cygwin (as opposed to MSYS2),
> > specifically using `disable_pcon` (because MSYS2 defaults to not using the
> > pseudo console support because I ran into too many issues to be confident
> > enough in it yet), but I think that Cygwin's `vim` is too old and
> > therefore might not even send that CSI [ 6n (although `:h t_RV` _does_
> > show the expected help).
> >
> > Now, the patch which I am responding to is completely obscure to me. It is
> > very, very unclear to me whether it really tries to only do one thing
> > (namely to transfer the input no longer in `read()` but in `setpgid()`),
> > or rather does many things at once. Even worse, I have not the faintest
> > clue how this patch is trying to accomplish what the commit message
> > describes (_because_ it does so many things at once), nor how that could
> > be related to the observed incorrect behavior, and as a consequence I have
> > no idea how I can hope to fix said observed incorrect behavior.
> >
> > Could you help shed some light into the problem?
>
> Thanks for the report.
> Could you please test if the following patch solves the issue?

It does!

However, I am a bit frustrated because there is still a lot light-shedding
to be done. In the current shape of the code, I do not even understand
what it does, let alone why it works around the problem.

For example, why is there such a long `pcon` stuff going on? I am in the
_disabled_ pseudo console mode, for starters. Like, why is there a
`pcon_input_state`? And why has the `disable_pcon` code path changed at
all (there was no need to touch it, was there)?

Also, `needs_xfer` clearly means `needs transfer`. What transfer? What's
`masked`? And how does it differ from `mask`?

I fear that the pseudo console/non-pseudo console code currently has a
lottery factor of 1. I spent a good part of three entire working days
pouring over it, and I still do not understand it. Usually, a combination
of reading the commit messages, reading the code, parsing
function/variable names with a sprinkling of intuition gets me very far in
understanding any kind of legacy code, but not here. And I do _a lot_ of
legacy code hacking, as part of maintaining Git for Windows. The pseudo
console code in Cygwin really is a class of its own in this regard.

And I have the very strong sense that it does not have to be that way.

I would really like it if the code in `fhandler_*` could see some tender,
loving care, bringing clarity about, for example clearly distinguishing
between the code paths that use pseudo console support vs not, and code
paths regarding Cygwin processes vs not.

I mean, even if your diff below is short, I cannot review it. Not the
context, not my study of three days of the surrounding code and the commit
messages, none of that equips me with enough knowledge to even spot an
obvious bug, because such a bug would still not be obvious to me.

I really hope that this can be fixed. Please let me know if there is
anything I can do to help bring this about.

Thank you,
Johannes

>
>
> diff --git a/winsup/cygwin/fhandler_tty.cc b/winsup/cygwin/fhandler_tty.cc
> index f523dafed..ba282b897 100644
> --- a/winsup/cygwin/fhandler_tty.cc
> +++ b/winsup/cygwin/fhandler_tty.cc
> @@ -1239,10 +1239,13 @@ fhandler_pty_slave::mask_switch_to_pcon_in (bool mask, bool xfer)
>    else if (InterlockedDecrement (&num_reader) == 0)
>      CloseHandle (slave_reading);
>
> +  bool need_xfer =
> +    get_ttyp ()->switch_to_pcon_in && !get_ttyp ()->pcon_activated;
> +
>    /* In GDB, transfer input based on setpgid() does not work because
>       GDB may not set terminal process group properly. Therefore,
>       transfer input here if isHybrid is set. */
> -  if (isHybrid && !!masked != mask && xfer
> +  if ((isHybrid || need_xfer) && !!masked != mask && xfer
>        && GetStdHandle (STD_INPUT_HANDLE) == get_handle ())
>      {
>        if (mask && get_ttyp ()->pcon_input_state_eq (tty::to_nat))
> @@ -1536,7 +1539,7 @@ out:
>    if (ptr0)
>      { /* Not tcflush() */
>        bool saw_eol = totalread > 0 && strchr ("\r\n", ptr0[totalread -1]);
> -      mask_switch_to_pcon_in (false, saw_eol);
> +      mask_switch_to_pcon_in (false, saw_eol || len == 0);
>      }
>  }
>
> @@ -2214,6 +2217,15 @@ fhandler_pty_master::write (const void *ptr, size_t len)
>        return len;
>      }
>
> +  if (to_be_read_from_pcon () && !get_ttyp ()->pcon_activated
> +      && get_ttyp ()->pcon_input_state == tty::to_cyg)
> +    {
> +      WaitForSingleObject (input_mutex, INFINITE);
> +      fhandler_pty_slave::transfer_input (tty::to_nat, from_master,
> +					  get_ttyp (), input_available_event);
> +      ReleaseMutex (input_mutex);
> +    }
> +
>    line_edit_status status = line_edit (p, len, ti, &ret);
>    if (status > line_edit_signalled && status != line_edit_pipe_full)
>      ret = -1;
>
> --
> Takashi Yano <takashi.yano@nifty.ne.jp>
>