Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Git hooks can corrupt command line on Windows #730

Open
marcelais opened this issue Feb 27, 2025 · 5 comments · May be fixed by git-for-windows/msys2-runtime#87
Open

Git hooks can corrupt command line on Windows #730

marcelais opened this issue Feb 27, 2025 · 5 comments · May be fixed by git-for-windows/msys2-runtime#87

Comments

@marcelais
Copy link

On Windows, if you have a git hook which spawns a background process (in our case, to report telemetry for the tool asynchronously), after running a Git command, the cmd command prompt is "corrupted" in that control keys like ESC and up/down arrow no longer are interpreted correctly but instead result in ^[ or ^[[A being displayed.

Running "chcp" restores the correct behavior of the command prompt.

This is a regression in v2.47.2.vfs.0.3. This version (and newer) show the bad behavior while v2.47.2.vfs.0.2 (and earlier) work properly.
@dscho
Copy link
Member

dscho commented Feb 27, 2025

Unfortunately, there was a rather big MSYS2 runtime version jump between .0.2 and .0.3:

-msys2-runtime 3.5.4-5
+msys2-runtime 3.5.7-1

Just to be sure, could you download v3.5.4-5 of msys-2.0.dll and temporarily replace your <Git>\usr\bin\msys-2.0.dll with it to verify that this works around the regression?

@marcelais
Copy link
Author

marcelais commented Feb 27, 2025 •
edited
Loading

I swapped out the dll and that did fix the regression. It does look like that dependency upgrade is what introduced the problem.

@dscho
Copy link
Member

dscho commented Feb 27, 2025 •
edited
Loading

@marcelais thank you for confirming, and thank you for jumping into the private chat with me. I think I got a pretty reliable reproducer:

  1. The basic setup is a regular Cygwin and a regular Git for Windows installation.
  2. Then, open a regular CMD.
  3. Now set PATH=C:\Program Files\Git\mingw64\bin;D:\cygwin64\bin;%PATH% (adjust to wherever your Cygwin is).
  4. Also, set MSYSTEM=MINGW64 to prevent git.exe from using its own Bash to run Git hooks.
  5. Then, initialize a new Git repository: git init puzzling
  6. Now, switch to that repository: cd puzzling
  7. Next, install this pre-commit hook in it: printf "%s\\n" "#!/bin/sh" "git ls-remote https://github.com/git/git &" >.git/hooks/pre-commit
  8. And now, trigger that pre-commit hook, e.g. via git commit --allow-empty -m zOMG
  9. Finally, after you see the output of that ls-remote (which should be a loooong list ending in something like "refs/tags/v2.9.5^{}"), press the Cursor Up key and then Enter.

What I observe is that the git commit returns, but then the background'ed git ls-remote prints to stdout and after pressing that Cursor Up key everything is broken, CMD does not react to any keyboard strokes anymore, except if you hit Ctrl+Z twice. And even then, Cursor Up does not work anymore. You have to call either chcp or stty sane to restore the intended behavior upon Cursor Up/Down.

What I suspect is happening is that Cygwin assumes full control over some Console state that it wants to temporarily switch while the (MINGW) git.exe process is connected, and then switch back. But the assumption that this works is incorrect because that process started another Cygwin process (the hook) which spawned another MINGW process (the ls-remote call) which did not return before Cygwin did all of the re-setting because the first MINGW process has long exited.

Even if that suspicion is wrong: There is something highly iffy going on, messing up Console state like that, from which you can only recover by force-terminating the interactive Bash.

Since this reproduces with the regular Cygwin runtime (and not only with the friendly fork thereof that is the MSYS2 runtime that Git for Windows and microsoft/git use), I asked the Cygwin developers for help, and will keep you posted.

github-cygwin pushed a commit to cygwin/cygwin that referenced this issue Mar 3, 2025
@tyan0
Cygwin: console: Redesign mode set strategy on close().
3312f2d 
The required console mode for a non-cygwin process is different from
that for a cygwin process. There are currently three modes: tty::cygwin,
tty::native, and tty::restore. The latter two are for the non-cygwin
processes. tty::restore is the mode for the non-cygwin processes that
started the cygwin process, used to restore the previous behaviour.
tty::native is the mode that reflects some terminfo flags. The issue
below is caused because the console mode fails to be restored to the
previous console mode used by cmd.exe.
This patch redesigns the strategy to determine which mode should be
set on console close() to fix all similar problems. Previously, the
number of handle count is used to determine the appropriate console
mode. However, the handle count seems uncertain for that purpose.
In the new design, the relation ship between the master process and
the process that is about to close the console is mainly used. This
can provide more certain result than previous one.

Addresses: microsoft/git#730
Fixes: 30d2669 ("Cygwin: console: Fix clean up conditions in close()")
Reported-by: Mike Marcelais, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
dscho pushed a commit to dscho/msys2-runtime that referenced this issue Mar 3, 2025
@tyan0 @dscho
Cygwin: console: Redesign mode set strategy on close().
f551f2a 
The required console mode for a non-cygwin process is different from
that for a cygwin process. There are currently three modes: tty::cygwin,
tty::native, and tty::restore. The latter two are for the non-cygwin
processes. tty::restore is the mode for the non-cygwin processes that
started the cygwin process, used to restore the previous behaviour.
tty::native is the mode that reflects some terminfo flags. The issue
below is caused because the console mode fails to be restored to the
previous console mode used by cmd.exe.
This patch redesigns the strategy to determine which mode should be
set on console close() to fix all similar problems. Previously, the
number of handle count is used to determine the appropriate console
mode. However, the handle count seems uncertain for that purpose.
In the new design, the relation ship between the master process and
the process that is about to close the console is mainly used. This
can provide more certain result than previous one.

Addresses: microsoft/git#730
Backported-from: 3312f2d21f (Cygwin: console: Redesign mode set strategy on close()., 2025-03-03)
Fixes: 30d2669 ("Cygwin: console: Fix clean up conditions in close()")
Reported-by: Mike Marcelais, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho linked a pull request Mar 3, 2025 that will close this issue
Open
@dscho
Copy link
Member

dscho commented Mar 3, 2025

@marcelais could I ask you to download the install artifact from https://github.com/git-for-windows/msys2-runtime/actions/runs/13628995018?pr=87#artifacts, extract its usr\bin\msys-2.0.dll and (temporarily) replace your regular Git installation's version, then try again? In my tests, it fixes the problem, but I know that you have a more complicated setup where the bug caused disruption.

@dscho dscho mentioned this issue Mar 3, 2025
Merged
lazka pushed a commit to msys2/msys2-runtime that referenced this issue Mar 3, 2025
@tyan0 @lazka
Cygwin: console: Redesign mode set strategy on close().
cd64230 
The required console mode for a non-cygwin process is different from
that for a cygwin process. There are currently three modes: tty::cygwin,
tty::native, and tty::restore. The latter two are for the non-cygwin
processes. tty::restore is the mode for the non-cygwin processes that
started the cygwin process, used to restore the previous behaviour.
tty::native is the mode that reflects some terminfo flags. The issue
below is caused because the console mode fails to be restored to the
previous console mode used by cmd.exe.
This patch redesigns the strategy to determine which mode should be
set on console close() to fix all similar problems. Previously, the
number of handle count is used to determine the appropriate console
mode. However, the handle count seems uncertain for that purpose.
In the new design, the relation ship between the master process and
the process that is about to close the console is mainly used. This
can provide more certain result than previous one.

Addresses: microsoft/git#730
Backported-from: 3312f2d (Cygwin: console: Redesign mode set strategy on close()., 2025-03-03)
Fixes: 30d2669 ("Cygwin: console: Fix clean up conditions in close()")
Reported-by: Mike Marcelais, Johannes Schindelin <Johannes.Schindelin@gmx.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@marcelais
Copy link
Author

Yes, this fixes the problem in our real hooks as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix console mode restoring dscho/msys2-runtime
2 participants
@dscho @marcelais