public inbox for
 help / color / mirror / Atom feed
From: Takashi Yano <>
To: Johannes Schindelin <>
Subject: Re: [PATCH v2] Cygwin: pty: Reduce unecessary input transfer.
Date: Sat, 11 Dec 2021 22:40:30 +0900	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

  reply	other threads:[~2021-12-11 13:40 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-02-11  9:09 Takashi Yano
2021-02-12  9:43 ` Corinna Vinschen
2021-12-09 23:05 ` Johannes Schindelin
2021-12-10 10:20   ` Takashi Yano
2021-12-10 11:12     ` Johannes Schindelin
2021-12-11 13:40       ` Takashi Yano [this message]
2021-12-13  9:09         ` Takashi Yano
2021-12-13 12:33           ` Takashi Yano
2021-12-14 11:02             ` Takashi Yano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).