On Thu, 8 May 2025 09:19:47 +0200 (CEST)
Johannes Schindelin wrote:
> Hi Takashi,
>
> On Wed, 7 May 2025, Takashi Yano wrote:
>
> > ... and restore it when app exits. The commit 0bfd91d57863 has a bug
> > that the console mode is stored into the shared memory when both:
> > (1) cygwin process is started from non-cygwin process.
> > (2) cygwin process started from non-cygwin process exits.
> > (1) is intended, but (2) is not. Due to (2), the stored console mode
> > is unexpectedly broken when the cygwin process exits. Then the mode
> > restored will be not as expected. This causes undesired console mode
> > in the use case that cygwin and non-cygwin apps are mixed.
> >
> > With this patch, the console mode will stored only in the case (1).
> > This is done by putting the code, which stores the console mode, into
> > fhandler_console::open() rather than fhandler_console::set_input_mode()
> > and fhandler_console::set_output_mode().
> >
> > Fixes: 0bfd91d57863 ("Cygwin: console: tty::restore really restores the
> > previous mode")
> > Reported-by: Johannes Schindelin <[email protected]>
> > Signed-off-by: Takashi Yano <[email protected]>
>
> Thank you! I can confirm that this re-fixes the problem fixed in
> 3312f2d21f (Cygwin: console: Redesign mode set strategy on close().,
> 2025-03-03), which was in response to a report I received in
> https://github.com/microsoft/git/issues/730.
>
> I also asked the reporter of the bug you fixed in 114cbda779 (Cygwin:
> console: tty::restore really restores the previous mode, 2025-03-21)
> (which was the cause for the regression I reported) to verify that _their_
> bug did not reappear, see
> https://github.com/msys2/msys2-runtime/issues/268#issuecomment-2858071841
>
> Finally, I spent quite a few hours to write an automated test that
> imitates my reproducer, and integrated it into Git for Windows' CI builds:
> https://github.com/git-for-windows/msys2-runtime/pull/95/commits/7acbb031654404c9fd711eee9974c88475de98fd
Thanks for testing.
> However, I still have concerns about the patch. It still lets the Cygwin
> runtime operate under the assumption that it can control the console, even
> though other process can interact with the same console in _overlapping_
> lifecycles.
>
> Keep in mind that the bug I reported twice involves a Cygwin process that
> spawns a non-Cygwin process which writes to the console _after_ the Cygwin
> process exited (and restored the console mode).
>
> It looks plausible to me that this storing, setting, then re-setting of
> the console mode by the Cygwin runtime will never cease to be fragile and
> will always be prone to surface bugs that are similar, bug not identical
> to the two bugs referenced above.
>
> Therefore I fail to convince myself that the current design is okay, and
> expect that, in the long run, the Cygwin runtime will stop to toggle the
> console modes as if the console were not serving other, non-Cygwin
> processes, too.
>
> I do see that you changed some code so that it is only run when Cygwin is
> the console owner. That triggers the following questions, answers to which
> I could not find in the commit message (and neither in the diff):
>
> - Now that the mode is left alone when the console is owned by a
> non-Cygwin process, which code is broken? There must have been a reason
> to change the console mode in the first place, after all.
What situation do you assume? I think I designed so that the console mode
is restored to tty::cygwin when non-cygwin process exits.
> - If it is okay to leave the console mode alone when Cygwin is _not_ the
> console owner, why bother changing the mode at all?
--
Takashi Yano <[email protected]>
