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

Escape sequences in magit-process-buffer #3549

Closed
romandecker opened this issue Aug 9, 2018 · 27 comments
Closed

Escape sequences in magit-process-buffer #3549

romandecker opened this issue Aug 9, 2018 · 27 comments
Labels
. maintainer's bookmark enhancement New feature or request wont add This feature will not be worked on

Comments

@romandecker
Copy link

We use a git hook to lint staged files before committing, when I run git commit from cli, the output looks as follows:

CLI-Output

The library we use here (https://github.com/okonet/lint-staged) makes heavy use of escape sequences to render that progress indicator. This causes the output shown in magit-process-buffer to be mangled:

magit-process-buffer output

I've already tried setting magit-process-finish-apply-ansi-colors as indicated in #3159 but that just makes things worse: Everything is a lot slower, and the escape sequences are still not interpreted correctly (I think that only affects colors but not cursor-movement-escape sequences).

Magit version: Magit 20180630.1235, Git 2.18.0, Emacs 26.1, darwin

@tarsius tarsius added the enhancement New feature or request label Aug 9, 2018
@tarsius
Copy link
Member

tarsius commented Aug 9, 2018

I'll probably do something about that eventually, but for now check if your script can be told that it isn't being run in a terminal that fully understands escape sequences.

@asottile
Copy link

I think the root cause is magit is invoking with a tty. Most tools use "is this a tty" as an appropriate check for "can I output color".

For example, the venerable coreutils ls:

$ ls --help | grep -C2 -- --color=auto

Using color to distinguish file types is disabled both by default and
with --color=never.  With --color=auto, ls emits color codes only when
standard output is connected to a terminal.  The LS_COLORS environment
variable can change the settings.  Use the dircolors command to set it.

@asottile
Copy link

Traced this to here:

magit/lisp/magit-process.el

Lines 526 to 531 in 8adbe43

(let ((process-connection-type
;; Don't use a pty, because it would set icrnl
;; which would modify the input (issue #20).
(and (not input) magit-process-connection-type))
(process-environment (magit-process-environment))
(default-process-coding-system (magit--process-coding-system)))

magit is using a pty to invoke git commit which is inappropriate since it is written to a buffer and then not interpreted by a tty

@tarsius
Copy link
Member

tarsius commented Sep 17, 2018

We need a pty because we have to send user input to the git process in some case.

@asottile
Copy link

You don't need a pty to communicate, you can send that over a pipe. pre-commit hooks are also invoked with stdin closed

@npostavs
Copy link
Member

You don't need a pty to communicate, you can send that over a pipe

The problem is that git and ssh will only prompt for passwords on a tty.

@asottile
Copy link

asottile commented Sep 17, 2018

Can commit trigger a prompt? Can you configure the pty so stdout is a pipe but stdin is tty-like?

@tarsius
Copy link
Member

tarsius commented Sep 17, 2018

Can commit trigger a prompt?

It might: Your commit message doesn't satisfy the guidelines. Proceed anyway? (y/n) .

@asottile
Copy link

Where does that prompt come from? I don't see it in git or magit?

@tarsius
Copy link
Member

tarsius commented Sep 17, 2018

I invented that prompt on the fly. My point is:
Yes, a commit can trigger a prompt because creating a commit can run arbitrary hook functions which may or may not prompt.

@asottile
Copy link

They can't because git hooks don't have stdin attached:

$ tail -n999 .git/hooks/{commit-msg,pre-commit}
==> .git/hooks/commit-msg <==
#!/usr/bin/env python3
import sys
print(f'isatty: {sys.stdin.isatty()}')
print(f'hello from {sys.argv[0]}')

==> .git/hooks/pre-commit <==
#!/usr/bin/env python3
import sys
print(f'isatty: {sys.stdin.isatty()}')
print(f'hello from {sys.argv[0]}')

$ git commit -mtest --allow-empty
isatty: False
hello from .git/hooks/pre-commit
isatty: False
hello from .git/hooks/commit-msg
[master c91af40] test

@tarsius
Copy link
Member

tarsius commented Sep 17, 2018

This works:

#!/bin/sh

exec < /dev/tty

echo "Password: "
read password
echo "password is $password"
git … commit --
Password: 
password is 1234
hint: Waiting for your editor to close the file...
Waiting for Emacs...
[remove-module ce46df07e] sdfdsf
 1 file changed, 1 insertion(+)

@asottile
Copy link

fair, that's cheating I would think 😆

Anyway, all I'm asking here is that stdout be a pipe, stdin can continue to be a pty if that's necessary

I'm not at all familiar with elisp but here's how this would be done with python:

t.py

#!/usr/bin/env python3
import sys

print(f'in: {sys.stdin.isatty()}')
print(f'out: {sys.stdout.isatty()}')
print(f'err: {sys.stderr.isatty()}')


print(f'read: {sys.stdin.readline()}')

pty_ex.py

import os
import pty
import subprocess


master, slave = pty.openpty()
proc = subprocess.Popen(
    ('./t.py',),
    stdin=slave,
    stdout=subprocess.PIPE,
    stderr=subprocess.PIPE,
)

os.close(slave)
with os.fdopen(master, 'wb', 0) as stdin:
    stdin.write(b'hello\n')
    out, err = proc.communicate()

print('RETC:')
print(proc.returncode)
print('OUT:')
print(out.decode(), end='')
print('ERR:')
print(err.decode(), end='')

output

RETC:
0
OUT:
in: True
out: False
err: False
read: hello

ERR:

@tarsius
Copy link
Member

tarsius commented Sep 19, 2018

Anyway, all I'm asking here is that stdout be a pipe, stdin can continue to be a pty if that's necessary

I don't think that's possible in elisp.

@asottile
Copy link

😆 surely it can, elisp is turing complete!

@vermiculus
Copy link
Contributor

Go ahead and re-implement a better emacs/elisp in elisp 😉

@tarsius tarsius added the wont add This feature will not be worked on label Oct 9, 2018
@tarsius
Copy link
Member

tarsius commented Oct 9, 2018

Closing as wontadd.

In a far away future I might actually start dealing with control sequences in the output of git (and by extension its subprocesses, like hook commands for example). But when and if I decide to actually go down that road, then I will open a new issue that does not focus on one particular third-party utility.

@tarsius tarsius closed this as completed Oct 9, 2018
@tarsius tarsius added the . maintainer's bookmark label Oct 9, 2018
@tarsius
Copy link
Member

tarsius commented Oct 9, 2018

Also added the semi-secret . (look at this again some time) label.

@Fuco1
Copy link
Contributor

Fuco1 commented Jun 10, 2019

For anyone interested, I've hacked on this a bit and created https://gist.github.com/Fuco1/f0cc2c866926f433a32ffa882887e960 which is at least lint-staged aware... it does not implement all the ANSI escapes but it is good enough for my needs.

Feel free to extend it and submit patches.

To use this copy the form somewhere in your emacs and eval it. Probably don't do this if you don't know what you're doing.

@saper
Copy link

saper commented Jul 4, 2019

Unfortunately more and more tools decide to include various ANSI goodness and there is no way to tell if that things is really interactive or not. https://no-color.org/ is one initiative to solve at least the color problem.

@tavurth
Copy link

tavurth commented Apr 29, 2021

@twmr
Copy link

twmr commented Apr 30, 2021

Here are some facts of the interesting discussion between @asottile and @tarsius

  • A git pre-commit hook is invoked with:
    • stdin closed (stdin.isatty()=False)
    • pty is attached to stdout (stdout.isatty()=True)
    • However, if git commit is called without a pty (process-connection-type=nil) attached to stdout, stdout of the pre-commit hook is not a pty (stdout.isatty()=False)!
  • You can send data to the stdin of a subprocess either via a pipe or via a pty.
  • Since the pre commit hook is called with stdin closed, it is not possible to communicate with it.
  • escape sequences are usually generated by programs if a pty is attached to stdout (see manpage of ls).
  • Therefore, if we don't want to output colors (or escape sequences) but keep support for necessary communication via stdin (e.g. for entering ssh passphrases) with git, we have to call git with
    stdin->pty, stdout->PIPE

Since it seems that it is not possible in emacs to create subprocesses where stdin is attached to a pty but not stdout, I'll create a bug report.

@tarsius
Copy link
Member

tarsius commented May 2, 2021

Please post a link when you have done that.

@twmr
Copy link

twmr commented May 2, 2021

Sure. Here it is: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=48129

@twmr
Copy link

twmr commented May 5, 2021

Eli asked in the thread if we could use make-pipe-process, but he didn't provide any details. I answered him that I think that it doesn't work for our use-case, but he hasn't replied to my answer yet.

@ezmiller
Copy link

ezmiller commented Jan 7, 2023

I've been running into this issue too. Would be great if there were a solution.

@ezmiller
Copy link

ezmiller commented Jan 9, 2023

I'm not sure if this is the right solution, but adding this code from @LaloHao (from here) worked for me:

(defun color-buffer (proc &rest args)
  (interactive)
  (with-current-buffer (process-buffer proc)
    (read-only-mode -1)
    (ansi-color-apply-on-region (point-min) (point-max))
    (read-only-mode 1)))

(advice-add 'magit-process-filter :after #'color-buffer)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
. maintainer's bookmark enhancement New feature or request wont add This feature will not be worked on
Development

No branches or pull requests

10 participants