bug#27407: 25.2; SGR reset is ignored if it terminates Eshell's output

bug#27407: 25.2; SGR reset is ignored if it terminates Eshell's output

[Pierre Neidhardt]

Steps to reproduce:

- M-x eshell
- Enter "^[[33mhello^[[0m"

The new prompt becomes yellow.

It does not happen if the SGR reset sequence is not last.
This is fine for instance:

- echo "^[[33mhello^[[0m"X
- echo -n "^[[33mhello^[[0m"

Maybe an off-by-one error in the `ansi-*' function?



In GNU Emacs 25.2.1 (x86_64-unknown-linux-gnu, GTK+ Version 3.22.10)
 of 2017-04-22 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.11903000
System Description:	Arch Linux

Configured using:
 'configure --prefix=/usr --sysconfdir=/etc --libexecdir=/usr/lib
 --localstatedir=/var --with-x-toolkit=gtk3 --with-xft --with-modules
 'CFLAGS=-march=x86-64 -mtune=generic -O2 -pipe
 -fstack-protector-strong' CPPFLAGS=-D_FORTIFY_SOURCE=2
 LDFLAGS=-Wl,-O1,--sort-common,--as-needed,-z,relro'

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GCONF GSETTINGS
NOTIFY ACL GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES

Important settings:
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

bug#27407: 25.2; SGR reset is ignored if it terminates Eshell's output

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

tags 27407 + patch
severity 27407 minor
quit

Pierre Neidhardt <ambrevar@gmail.com> writes:

> Steps to reproduce:
>
> - M-x eshell
> - Enter "hello"
>
> The new prompt becomes yellow.

You meant enter the text produced with (insert "echo \"\e[33mhello\e[0m\"").

> It does not happen if the SGR reset sequence is not last.
> This is fine for instance:
>
> - echo "hello"X
> - echo -n "hello"
>
> Maybe an off-by-one error in the `ansi-*' function?

The escape code is implemented by applying an overlay to the text
following it (see `ansi-color-apply-on-region').  The problem seems to
be that the reset sequence is translated to no face, so nothing is
applied (see `ansi-color-apply-overlay-face'), and the yellow overlay
gets extended despite the `ansi-color-freeze-overlay' modification-hook
because `eshell-output-filter' (and `eshell-send-input') let-bind
`inhibit-modification-hooks'.

I'm not sure why `inhibit-modification-hooks' needs to be let-bound at
all, but just binding it to nil around string insertion seems to be
enough:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1788 bytes --]

From 6e3892b8044fc918a5231a79963d0560adea2403 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sat, 17 Jun 2017 12:06:37 -0400
Subject: [PATCH v1] Let ansi-color overlay hooks work in eshell (Bug#27407)

* lisp/ansi-color.el (ansi-color-make-extent): Add
`ansi-color-freeze-overlay' to `insert-behind-hooks' as well.
* lisp/eshell/esh-mode.el (eshell-output-filter): Let-bind
`inhibit-modification-hooks' to nil while inserting the string.
---
 lisp/ansi-color.el      | 1 +
 lisp/eshell/esh-mode.el | 4 +++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/ansi-color.el b/lisp/ansi-color.el
index a1b4933175..72d70c2102 100644
--- a/lisp/ansi-color.el
+++ b/lisp/ansi-color.el
@@ -481,6 +481,7 @@ (defun ansi-color-make-extent (from to &optional object)
     ;; property to make sure it works.
     (let ((overlay (make-overlay from to object)))
       (overlay-put overlay 'modification-hooks '(ansi-color-freeze-overlay))
+      (overlay-put overlay 'insert-behind-hooks '(ansi-color-freeze-overlay))
       overlay)))
 
 (defun ansi-color-freeze-overlay (overlay is-after begin end &optional len)
diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
index 0fd0c18301..0999f9c4a8 100644
--- a/lisp/eshell/esh-mode.el
+++ b/lisp/eshell/esh-mode.el
@@ -726,7 +726,9 @@ (defun eshell-output-filter (process string)
 		  (setq obeg (+ obeg nchars)))
 	      (if (<= (point) oend)
 		  (setq oend (+ oend nchars)))
-	      (insert-before-markers string)
+              ;; Let the ansi-color overlay hooks run.
+              (let ((inhibit-modification-hooks nil))
+                (insert-before-markers string))
 	      (if (= (window-start) (point))
 		  (set-window-start (selected-window)
 				    (- (point) nchars)))
-- 
2.11.1

bug#27407: 25.2; SGR reset is ignored if it terminates Eshell's output

[Pierre Neidhardt]

On 17-06-17 14:19:54, npostavs@users.sourceforge.net wrote:
> You meant enter the text produced with (insert "echo \"\e[33mhello\e[0m\"").

Yes.

I haven't tried your patch, but in the general case Eshell should not extend the
ANSI coloring to the prompt, SGR reset or not. I.e. the following should not
color the prompt:

	(insert "echo \"\e[33mhello")

--
Pierre Neidhardt

bug#27407: 25.2; SGR reset is ignored if it terminates Eshell's output

[npostavs]

Pierre Neidhardt <ambrevar@gmail.com> writes:

> On 17-06-17 14:19:54, npostavs@users.sourceforge.net wrote:
>
> I haven't tried your patch, but in the general case Eshell should not extend the
> ANSI coloring to the prompt, SGR reset or not. I.e. the following should not
> color the prompt:
>
> 	(insert "echo \"\e[33mhello")

My patch does not solve this scenario, but on the other hand, when I try
this experiment in a terminal with bash, not putting a reset does give a
coloured prompt, i.e., in the following the 2nd "~/src$" is in yellow:

    ~/src$ echo $'\e[33mhello'
    hello
    ~/src$ 

So should eshell really be any different?

bug#27407: 25.2; SGR reset is ignored if it terminates Eshell's output

[Pierre Neidhardt]

> My patch does not solve this scenario, but on the other hand, when I try
> this experiment in a terminal with bash, not putting a reset does give a
> coloured prompt, i.e., in the following the 2nd "~/src$" is in yellow:
>
>     ~/src$ echo $'\e[33mhello'
>     hello
>     ~/src$
>
> So should eshell really be any different?

Yes. Definitely. Eshell _already_ is very different and that's why we use
it. Bash is hardly a role model in the world of shells.

In the case of bash, the issue probably stems back from historical limitations
of VT* terminals. Something that does not make sense to emulate in modern
computers.

I don't see any use case for letting the SGR color the shell. If you want to
change you shell from a command, Elisp is your friend...

--
Pierre Neidhardt

bug#27407: 25.2; SGR reset is ignored if it terminates Eshell's output

[npostavs]

[-- Attachment #1: Type: text/plain, Size: 866 bytes --]

Pierre Neidhardt <ambrevar@gmail.com> writes:

>> My patch does not solve this scenario, but on the other hand, when I try
>> this experiment in a terminal with bash, not putting a reset does give a
>> coloured prompt, i.e., in the following the 2nd "~/src$" is in yellow:
>>
>>     ~/src$ echo $'\e[33mhello'
>>     hello
>>     ~/src$
>>
>> So should eshell really be any different?
>
> Yes. Definitely. Eshell _already_ is very different and that's why we use
> it. Bash is hardly a role model in the world of shells.
>
> In the case of bash, the issue probably stems back from historical limitations
> of VT* terminals. Something that does not make sense to emulate in modern
> computers.
>
> I don't see any use case for letting the SGR color the shell. If you want to
> change you shell from a command, Elisp is your friend...

Hmm, seems easy enough to fix:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 933 bytes --]

From 34f41c8573936285076b4e24c2e7ff4d34a48b8d Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Sun, 18 Jun 2017 15:29:55 -0400
Subject: [PATCH v1] Reset ansi escape context before printing eshell prompt
 (Bug#27407)

* lisp/eshell/em-prompt.el (eshell-emit-prompt): Reset
`ansi-color-context-region'.
---
 lisp/eshell/em-prompt.el | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lisp/eshell/em-prompt.el b/lisp/eshell/em-prompt.el
index 53a83e6a67..8c81b43b1f 100644
--- a/lisp/eshell/em-prompt.el
+++ b/lisp/eshell/em-prompt.el
@@ -117,6 +117,8 @@ (defun eshell-prompt-initialize ()
 
 (defun eshell-emit-prompt ()
   "Emit a prompt if eshell is being used interactively."
+  (when (boundp 'ansi-color-context-region)
+    (setq ansi-color-context-region nil))
   (run-hooks 'eshell-before-prompt-hook)
   (if (not eshell-prompt-function)
       (set-marker eshell-last-output-end (point))
-- 
2.11.1

bug#27407: 25.2; SGR reset is ignored if it terminates Eshell's output

[npostavs]

tags 27407 fixed
close 27407 26.1
quit

npostavs@users.sourceforge.net writes:

>>
>> I don't see any use case for letting the SGR color the shell. If you want to
>> change you shell from a command, Elisp is your friend...
>
> Hmm, seems easy enough to fix:

Pushed to master.

[1: 4ebdcc46ab]: 2017-07-03 10:09:40 -0400
  Let ansi-color overlay hooks work in eshell (Bug#27407)
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=4ebdcc46ab345849332332d580bd1e3c2c9adb1e

[2: 37cdfec7c7]: 2017-07-03 10:09:41 -0400
  Reset ansi escape context before printing eshell prompt (Bug#27407)
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=37cdfec7c73205668498da9b27387f5f3ccfebee