Skip to content

git_terminal_prompt fixes and Unicode support #141

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

Merged
merged 7 commits into from
May 9, 2015

Conversation

kblees
Copy link

@kblees kblees commented May 7, 2015

Followup to #74 mintty issues / @dscho's d56d2f8

  • Fixes a few minor issues with git_terminal_prompt (see individual fixup commits)
  • Use read -s instead of stty echo (for fewer processes and MSys1 compatibility)
  • Drop CONIN$ / CONOUT$ in favor of the shell based solution (for Unicode support) see discussion
  • Drop unnecessary SetConsoleCP() from git-wrapper see PR git-wrapper: don't set the console input code page #142

Tested with git-bash, git-cmd, bash in native console, and MSys1 bash (with compat/terminal.c backported to msysgit/git/master).

kblees added 3 commits May 7, 2015 14:09
Use 'read -r', so that '\' is interpreted verbatim rather than as escape
character.

Signed-off-by: Karsten Blees <[email protected]>
Also strip trailing CRLF instead of just LF or just CR.

Signed-off-by: Karsten Blees <[email protected]>
Use strbuf_reset() instead of strbuf_setlen(..., 0).

Signed-off-by: Karsten Blees <[email protected]>
@kblees kblees mentioned this pull request May 7, 2015
3 tasks
@dscho
Copy link
Member

dscho commented May 8, 2015

I am sympathetic to your aim to support Unicode better, but please let me restate my ultimate goal for Git for Windows: I want it to be independent from any Shell or Perl. Ideally, Git for Windows is a pure, monolithic executable (or at least a bunch of executables and libraries bundled in a small installer). Therefore it pains me to see that we will probably have to increase our dependency on Bash rather than decrease it as I really wish we could.

Having said that, after the concerns I raised, I will accept those changes (preferably as two, separate Pull Requests) because you make a strong point why it is better to rely on the shell rather than the Win32 API to access the console.

Thank you so much for your valuable work, I hope I manage to help make the changes better by my feedback.

@kblees
Copy link
Author

kblees commented May 8, 2015

For background, I wanted to run tests with the dash instead of bash ... Now, we might be able to patch in support for the -s option

I'd rather not start patching up yet another utility...

I kinda don't want to break compatibility with MSys1

stty (and tty) is available in the MSys1 coreutils package, we just don't have it in msysgit/msysgit yet. So we could do stty -echo && read -r && stty echo. I'll change that patch to just reduce the number of sh invocations.

We still test for this "xterm" part of TERM.

IMO TERM=xterm* is not a good way to check whether we run in an MSys pty or a native console window, especially as /etc/profile unconditionally sets TERM=xterm-256color.

I've played around with checking the terminal from native code (similar to checking pipe names in PR #102), but MSys2 manages the controlling terminal of a program group in a shared memory section. So this is basically a dead end, as the binary format may change with each release (see winsup/cygwin/shared_info.h / shared.cc).

Possible alternatives would be:

  • Check if sh -c tty returns /dev/cons* or /dev/pty*.
  • Use a small msys-compiled C-program that does all the work for us (basically the HAVE_DEV_TTY branch of compat/terminal.c, i.e. using the termios API to control the terminal).

I really want to limit our dependence on a shell.

With the long-term goal to provide a minimal Git distribution, we should also try to limit dependency on MSys(2) and MinTTY, right?

Why?

I moved the code in a separate commit to improve legibility of the subsequent patch.

Sorry, we cannot do that. As you can see here, Git for Windows is not the only user of compat/terminal.c.

The HAVE_DEV_TTY branch should work as before, I just eliminated the GIT_WINDOWS_NATIVE specific stuff.

But I agree that its better to keep the CONIN$/CONOUT$ variant, to support potential future minimal Git distros.

@dscho
Copy link
Member

dscho commented May 8, 2015

For background, I wanted to run tests with the dash instead of bash ... Now, we might be able to patch in support for the -s option

I'd rather not start patching up yet another utility...

I was not asking you to do that ;-) It's just that it really affects performance that we have to use shell so much in the test suite. The difference is truly stunning between running the test suite on Linux and on Windows.

I kinda don't want to break compatibility with MSys1

stty (and tty) is available in the MSys1 coreutils package, we just don't have it in msysgit/msysgit yet. So we could do stty -echo && read -r && stty echo. I'll change that patch to just reduce the number of sh invocations.

Oh, I meant to express clearly that you do not need to change that patch. We will not switch away from Bash anytime soon and you are correct that it is better to reduce the number of shell commands.

I really want to limit our dependence on a shell.

With the long-term goal to provide a minimal Git distribution, we should also try to limit dependency on MSys(2) and MinTTY, right?

There are two layers to Git for Windows: The git.exe executable and all it calls, and the option to run scripts on top of it. I would like the former ("core Git") to be as self-contained as possible, i.e. eventually not even requiring a sh.exe. The latter, however, is what I need for my own workflows: I use a metric ton of shell scripts on top of Git, some even more complicated than the shears.sh script; To support those, I need an interactive shell, and if I already have a shell, I also want a powerful xterm-like console window as MinTTY offers it. So even if nobody else would want to use MinTTY, I would still support it for myself.

That does not mean that I want to force every Git user to use a Bash and MinTTY. I want them to have the liberty to choose to use it, or to choose not to.

Sorry, we cannot do that. As you can see here, Git for Windows is not the only user of compat/terminal.c.

The HAVE_DEV_TTY branch should work as before, I just eliminated the GIT_WINDOWS_NATIVE specific stuff.

But I agree that its better to keep the CONIN$/CONOUT$ variant, to support potential future minimal Git distros.

Thank you.

kblees added 4 commits May 9, 2015 00:05
Move 'close(child.out)' down.

Signed-off-by: Karsten Blees <[email protected]>
Reduce shell invocations by chaining commands together.

Use 'echo' instead of 'printf \\n'.

Use 'read -s' instead of 'stty [-]echo', because we don't have 'stty'
in old msysgit.

Use 'bash' instead of 'sh' as 'read -s' is bash-specific (in case we
ever switch to dash or some other POSIX-only shell).

Signed-off-by: Karsten Blees <[email protected]>
'xterm_prompt' is not xterm-specific, change function name and error
messages to reflect this.

'start_command' already prints an error message on failure, we don't need
another one.

Also print the script's exit code on failure.

Signed-off-by: Karsten Blees <[email protected]>
Accessing the Windows console through the special CONIN$ / CONOUT$ devices
doesn't work properly for non-ASCII usernames an passwords.

It also doesn't work for terminal emulators that hide the native console
window (such as mintty), and 'TERM=xterm*' is not necessarily a reliable
indicator for such terminals.

The new shell_prompt() function, on the other hand, works fine for both
MSys1 and MSys2, in native console windows as well as mintty, and properly
supports Unicode. It just needs bash on the path (for 'read -s', which is
bash-specific).

On Windows, try to use the shell to read from the terminal. If that fails
with ENOENT (i.e. bash was not found), use CONIN/OUT as fallback.

Note: To test this, create a UTF-8 credential file with non-ASCII chars,
e.g. in git-bash: 'echo url=http://täst.com > cred.txt'. Then in git-cmd,
'git credential fill <cred.txt' works (shell version), while calling git
without the git-wrapper (i.e. 'mingw64\bin\git credential fill <cred.txt')
mangles non-ASCII chars in both console output and input.

Signed-off-by: Karsten Blees <[email protected]>
@kblees kblees force-pushed the kb/terminal-prompt branch from be2c77b to 761f1f5 Compare May 9, 2015 00:29
@kblees
Copy link
Author

kblees commented May 9, 2015

I hope I addressed all your concerns.

  • I changed sh to bash to ensure that read -s is supported.
  • Instead of checking TERM=xterm, we fall back to CONIN/OUT if bash is not on the PATH. So this should already work with a potential bash-less Git distro.

@dscho
Copy link
Member

dscho commented May 9, 2015

This is excellent; Very good idea to fall back to Win32 API only if bash was not found!

dscho added a commit that referenced this pull request May 9, 2015
git_terminal_prompt fixes and Unicode support
@dscho dscho merged commit 243924a into git-for-windows:master May 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants