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

[Takashi Yano <takashi.yano@nifty.ne.jp>]

On Fri, 10 Dec 2021 12:12:44 +0100 (CET)
Johannes Schindelin wrote:
> On Fri, 10 Dec 2021, Takashi Yano wrote:
> > Could you please test if the following patch solves the issue?
> 
> It does!

It seems that you already apply this patch to msys2, however,
this is just an experimental patch to identify the cause of
the problem.

Please wait a while for actual patch.

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

The current pty code is too complicated indeed. :(
It is very difficult to explain how it works.

Basically, pty has two pairs of pipes, one is for cygwin apps
(io_handle/output_handle), the other is for non-cygwin app (
io_handle_nat/output_handle_nat). This is because these pipe-
pairs are processed differently even without ConPTY.

Outputs to output_handle_nat is forwarded to output_handle by
pty_master_fwd_thread after appropriate processing.

Input from keyboard is switched between two input pipes, i.e.
io_handle and io_handle_nat. When the cygwin-app is activated,
input from keyboard is sent to io_handle, and when the non-
cygwin app is activated, input from keyboard is sent to
io_handle_nat. This switching is done based on switch_to_pcon_in
and pcon_input_state. The name of this variable and related
function is *pcon*, however, these are also used for non-cygwin
apps even without ConPTY by historical reason.

While non-cygwin app is activated, switch_to_pcon_in is true
and pcon_input_state == to_nat. However, if the cygwin-app is
started from non-cygwin app, input from keyboard should be sent
to io_handle. This is done using mask_switch_to_pcon_in(). By
this function, input is temporary sent to io_handle even if
switch_to_pcon_in is true.

The function transfer_input() is used to transfer type ahead
inupt between two input pipes, i.e. io_handle and io_handle_nat.
Masking switch_to_pcon_in state by mask_switch_to_pcon_in() is
done in read() and select(), so input while read() or select()
is NOT called is sent to io_handle_nat rather than io_handle
if switch_to_pcon_in is true. The bug to be fixed now is just
the case.

So, transferring input from io_handle_nat to io_handle solves
the issue in this case. The patch I have sent yesterday is to
add this missing action.

In addition, this problem does not occur if ConPTY is enabled.

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