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.

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

Bell sound on git hooks scripts not working #11202

Closed
KeterSCP opened this issue Sep 11, 2021 · 7 comments
Closed

Bell sound on git hooks scripts not working #11202

KeterSCP opened this issue Sep 11, 2021 · 7 comments
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-2 A description (P2) Resolution-External For issues that are outside this codebase

Comments

@KeterSCP
Copy link

Windows Terminal version (or Windows build number)

1.10.2383.0

Other Software

git 2.31.1.windows.1

Steps to reproduce

  1. Enable Audible bell notification style for the powershell or cmd profile in the windows terminal.
  2. Create an empty repo.
  3. In .git/hooks folder add post-checkout file with following content:
#!/bin/bash                                                                      

tput setaf 1 # set red foreground

echo 'Here the bell sound should appear'

tput bel # make a sound

tput sgr0 # reset
  1. Add a dummy file and commit it to initialize the branch.
  2. Open repo folder in powershell or cmd profile in windows terminal.
  3. Execute git checkout command

Expected Behavior

The bell should be triggered

Actual Behavior

Bell is not triggered.

When repeating mentioned steps from windows powershell or cmd (not from the windows terminal), the bell is being triggered.
bell-issue

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Sep 11, 2021
@zadjii-msft
Copy link
Member

weird. What Windows version are you on?

@zadjii-msft
Copy link
Member

Woah okay this is something else. In this scenario, the Terminal doesn't even get a BEL. Like, one never gets written to the conpty connection. I worry that the bash.exe that msys ships doesn't actually bell with BEL. Hmmmmmmm

@zadjii-msft zadjii-msft added Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Sep 14, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 14, 2021
@zadjii-msft zadjii-msft added this to the Terminal v2.0 milestone Sep 14, 2021
@zadjii-msft zadjii-msft added Help Wanted We encourage anyone to jump in on these. Needs-Tag-Fix Doesn't match tag requirements labels Sep 14, 2021
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Sep 14, 2021
@elsaco
Copy link

elsaco commented Sep 14, 2021

FYI: Git for Windows has the bell style set to visible, by default. See /etc/inputrc for details.

Sample reading on git version 2.30.1.windows.1:

# none, visible or audible
set bell-style visible

@zadjii-msft
Copy link
Member

FYI: Git for Windows has the bell style set to visible, by default.

I'm not sure that's relevant in this case - I believe that only applies to the bell emitted on input. For example, typing asdf at a git bash prompt, then hitting tab.

Changing that does emit a bell sound correctly in the Terminal, but not in this weird git checkout repro. Heck, even a tput bel in git bash will emit a bell, but again, not in this weird scenario. I'm wondering if they're detecting they're not attached to a console and skipping it somehow?

Clearly something is special about this case, because slapping a infocmp in the post-checkout shows that the Terminal is running with TERM=cygwin in a post-checkout, which is I guess fine because bel=^G, in both xterm-256color and cygwin

@elsaco

This comment has been minimized.

@zadjii-msft

This comment has been minimized.

@DHowett
Copy link
Member

DHowett commented Oct 15, 2021

the Terminal is running with TERM=cygwin in a post-checkout

This is absolutely the issue. When TERM=cygwin, all output from Cygwin (also msys2) applications is routed through their "console file handler." It implements, among other things, its own VT parser.

It also handles bell on its own.

https://github.com/Alexpux/Cygwin/blob/b61dc22adaf82114eee3edce91cc3433bcd27fe5/winsup/cygwin/fhandler_console.cc#L2566-L2568

It manually writes C:\windows\media\ding.wav into the user's AppEvents configuration, and then triggers a MessageBeep. It literally reconfigures the OS to set MessageBeep to ding.wav and then calls it.

image

This is 100% on the Cygwin/MSYS2 runtime. It is equal parts (also 100%!) on Git-for-windows for choosing TERM=cygwin for checkout hooks.

@DHowett DHowett closed this as completed Oct 15, 2021
@DHowett DHowett added Resolution-External For issues that are outside this codebase and removed Product-Terminal The new Windows Terminal. labels Oct 15, 2021
@DHowett DHowett removed this from the Terminal v2.0 milestone Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Priority-2 A description (P2) Resolution-External For issues that are outside this codebase
Projects
None yet
Development

No branches or pull requests

4 participants