bug#7485: 23.2; Fix removing unrecognized ANSI sequences in ansi-color-apply

bug#7485: 23.2; Fix removing unrecognized ANSI sequences in ansi-color-apply

[Leo]

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

In ansi-color-apply, (string-match "\033" string start) finds the wrong
portion of context if unrecognized ANSI sequences is not removed before
the match.

This can cause, for example, eshell's prompt to disappear if
ansi-color-apply is used in eshell-preoutput-filter-functions. The
attached patch tries to fix this.

Leo


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

diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index 146c6c9..8d5cbe1 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,8 @@
+2010-11-26  Leo <sdl.web@gmail.com>
+
+	* ansi-color.el (ansi-color-apply): Also eliminate unrecognized
+	ANSI sequences for remaining string.
+
 2010-10-03  Chong Yidong  <cyd@stupidchicken.com>
 
 	* minibuffer.el (completion--some, completion--do-completion)
diff --git a/lisp/ansi-color.el b/lisp/ansi-color.el
index 00162c9..40c0066 100644
--- a/lisp/ansi-color.el
+++ b/lisp/ansi-color.el
@@ -341,12 +341,15 @@ See `ansi-color-unfontify-region' for a way around this."
       (put-text-property start (length string) 'ansi-color t string)
       (put-text-property start (length string) 'face face string))
     ;; save context, add the remainder of the string to the result
-    (let (fragment)
-      (if (string-match "\033" string start)
+    (let ((remaining (substring string start))
+	  fragment)
+      (while (string-match ansi-color-drop-regexp remaining)
+	(setq remaining (replace-match "" nil nil remaining)))
+      (if (string-match "\033" remaining)
 	  (let ((pos (match-beginning 0)))
-	    (setq fragment (substring string pos))
-	    (push (substring string start pos) result))
-	(push (substring string start) result))
+	    (setq fragment (substring remaining pos))
+	    (push (substring remaining 0 pos) result))
+	(push remaining result))
       (if (or face fragment)
 	  (setq ansi-color-context (list face fragment))
 	(setq ansi-color-context nil)))

bug#7485: 23.2; Fix removing unrecognized ANSI sequences in ansi-color-apply

[Stefan Monnier]

> In ansi-color-apply, (string-match "\033" string start) finds the wrong
> portion of context if unrecognized ANSI sequences is not removed before
> the match.

You mean, because \033 can appear in that unrecognized ANSI sequence?

> This can cause, for example, eshell's prompt to disappear if
> ansi-color-apply is used in eshell-preoutput-filter-functions.
> The attached patch tries to fix this.

I don't quite understand your patch.  And your saying "tries to fix"
doesn't make me more confident.

> diff --git a/lisp/ansi-color.el b/lisp/ansi-color.el
> index 00162c9..40c0066 100644
> *** a/lisp/ansi-color.el
> --- b/lisp/ansi-color.el
> ***************
> *** 341,352 ****
>         (put-text-property start (length string) 'ansi-color t string)
>         (put-text-property start (length string) 'face face string))
>       ;; save context, add the remainder of the string to the result
> !     (let (fragment)
> !       (if (string-match "\033" string start)
>   	  (let ((pos (match-beginning 0)))
> ! 	    (setq fragment (substring string pos))
> ! 	    (push (substring string start pos) result))
> ! 	(push (substring string start) result))
>         (if (or face fragment)
>   	  (setq ansi-color-context (list face fragment))
>   	(setq ansi-color-context nil)))
> --- 341,355 ----
>         (put-text-property start (length string) 'ansi-color t string)
>         (put-text-property start (length string) 'face face string))
>       ;; save context, add the remainder of the string to the result
> !     (let ((remaining (substring string start))
> ! 	  fragment)
> !       (while (string-match ansi-color-drop-regexp remaining)
> ! 	(setq remaining (replace-match "" nil nil remaining)))
> !       (if (string-match "\033" remaining)
>   	  (let ((pos (match-beginning 0)))
> ! 	    (setq fragment (substring remaining pos))
> ! 	    (push (substring remaining 0 pos) result))
> ! 	(push remaining result))
>         (if (or face fragment)
>   	  (setq ansi-color-context (list face fragment))
>   	(setq ansi-color-context nil)))

This appears to "drop control sequences" even tho they weren't dropped
before, so it does more than "ignore false-positive \033 from
unrecognized control sequences".  IIUC those unrecognized control
sequences are dropped elsewhere, right?  So are maybe suggesting that
they are currently not dropped at the right time (i.e. dropped too
late), or that the \033 handling takes place too early, or that we
currently forget to drop those control sequences or ... ?


        Stefan

bug#7485: 23.2; Fix removing unrecognized ANSI sequences in ansi-color-apply

[Leo]

On 2010-11-26 18:55 +0000, Stefan Monnier wrote:
>> In ansi-color-apply, (string-match "\033" string start) finds the wrong
>> portion of context if unrecognized ANSI sequences is not removed before
>> the match.
>
> You mean, because \033 can appear in that unrecognized ANSI sequence?

Yes.

>> This can cause, for example, eshell's prompt to disappear if
>> ansi-color-apply is used in eshell-preoutput-filter-functions.
>> The attached patch tries to fix this.
>
> I don't quite understand your patch.  And your saying "tries to fix"
> doesn't make me more confident.

which means I only briefly tested the patch and it seems not to make
things worse while fixing a bug that annoys me ;)

>> diff --git a/lisp/ansi-color.el b/lisp/ansi-color.el
>> index 00162c9..40c0066 100644
>> *** a/lisp/ansi-color.el
>> --- b/lisp/ansi-color.el
>> ***************
>> *** 341,352 ****
>>         (put-text-property start (length string) 'ansi-color t string)
>>         (put-text-property start (length string) 'face face string))
>>       ;; save context, add the remainder of the string to the result
>> !     (let (fragment)
>> !       (if (string-match "\033" string start)
>>   	  (let ((pos (match-beginning 0)))
>> ! 	    (setq fragment (substring string pos))
>> ! 	    (push (substring string start pos) result))
>> ! 	(push (substring string start) result))
>>         (if (or face fragment)
>>   	  (setq ansi-color-context (list face fragment))
>>   	(setq ansi-color-context nil)))
>> --- 341,355 ----
>>         (put-text-property start (length string) 'ansi-color t string)
>>         (put-text-property start (length string) 'face face string))
>>       ;; save context, add the remainder of the string to the result
>> !     (let ((remaining (substring string start))
>> ! 	  fragment)
>> !       (while (string-match ansi-color-drop-regexp remaining)
>> ! 	(setq remaining (replace-match "" nil nil remaining)))
>> !       (if (string-match "\033" remaining)
>>   	  (let ((pos (match-beginning 0)))
>> ! 	    (setq fragment (substring remaining pos))
>> ! 	    (push (substring remaining 0 pos) result))
>> ! 	(push remaining result))
>>         (if (or face fragment)
>>   	  (setq ansi-color-context (list face fragment))
>>   	(setq ansi-color-context nil)))
>
> This appears to "drop control sequences" even tho they weren't dropped
> before, so it does more than "ignore false-positive \033 from
> unrecognized control sequences".  IIUC those unrecognized control
> sequences are dropped elsewhere, right?  So are maybe suggesting that
> they are currently not dropped at the right time (i.e. dropped too
> late), or that the \033 handling takes place too early, or that we
> currently forget to drop those control sequences or ... ?
>
>
>         Stefan

Unrecognized control sequences also begin with char \033. The FRAGMENT
part of ansi-color-context wasn't supposed to contain actual text,
otherwise they might get lost. It should always be a fragment of a whole
ansi control sequence if non-nil.

Let me show how this stops eshell prompt from showing.

Assume ansi-color-apply is in eshell-preoutput-filter-functions. Suppose
the output of shell command 'ack linux' ends with an unrecognized
control sequence ^[[K. The whole output string that goes through
ansi-color-apply is

   output of 'ack linux' + 'eshell prompt'.

After 'ack linux' finishes, the FRAGMENT of ansi-color-context becomes:

#("^[[K\n~/.emacs.d $ " 4 17
   (read-only t face eshell-prompt rear-nonsticky
              (face read-only)))

The string after being processed by ansi-color-apply is thus without an
eshell prompt.

Leo

bug#7485: 23.2; Fix removing unrecognized ANSI sequences in ansi-color-apply

[npostavs]

close 7485 
quit

Leo <sdl.web@gmail.com> writes:

> Unrecognized control sequences also begin with char \033. The FRAGMENT
> part of ansi-color-context wasn't supposed to contain actual text,
> otherwise they might get lost. It should always be a fragment of a whole
> ansi control sequence if non-nil.
>
> Let me show how this stops eshell prompt from showing.
>
> Assume ansi-color-apply is in eshell-preoutput-filter-functions. Suppose
> the output of shell command 'ack linux' ends with an unrecognized
> control sequence ^[[K. The whole output string that goes through
> ansi-color-apply is
>
>    output of 'ack linux' + 'eshell prompt'.
>
> After 'ack linux' finishes, the FRAGMENT of ansi-color-context becomes:
>
> #("^[[K\n~/.emacs.d $ " 4 17
>    (read-only t face eshell-prompt rear-nonsticky
>               (face read-only)))
>
> The string after being processed by ansi-color-apply is thus without an
> eshell prompt.

I think this patch is no longer applicable now that I've pushed
[1:35ed01dfb3], ansi-color-apply now skips over every escape sequence,
not just color escape sequences.  Although I'm unable to produce the
problem of missing prompts even before my patch (e.g., in 25.2):

    ~/src $ printf 'ack linux\e[K\n'
    ack linux
    ~/src $ 

[1: 35ed01dfb3]: 2017-07-03 10:09:40 -0400
  Fix and simplify ansi escape detection (Bug#21381)
  http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=35ed01dfb3f811a997e26d843e9971eb6b81b125