bug#18135: Emacs bug # 3735 definitely not fixed (5 years later)

bug#18135: Emacs bug # 3735 definitely not fixed (5 years later)

[Don Hopkins]

[-- Attachment #1.1: Type: text/plain, Size: 758 bytes --]

Daniel submitted this excellent bug report for a bug that has really been causing me problems, which was claimed to have been fixed, but definitely is not fixed in Emacs 23.3.1 or 24.3.1. 

http://debbugs.gnu.org/cgi/bugreport.cgi?bug=3735

Chong mentioned it was fixed in Emacs 23, but said he didn't recall who fixed it.
Apparently it was put back in and has been back for a long time. 
Could you please investigate to see who fixed it, and who put it back in, and WHY? 
This bug REALLY needs to be fixed, as it makes shell mode practically unusable, and as Daniel described, can lead to some very dangerous and inexplicable accidents. 
And whoever put the bug back in after it was fixed last time needs to stop doing that, please. 

-Don




[-- Attachment #1.2: Type: text/html, Size: 1202 bytes --]

[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 495 bytes --]

bug#18135: Emacs bug # 3735 definitely not fixed (5 years later)

[Chong Yidong]

I was going through old emails and just saw this; sorry for the huge
delay in triaging this bug.

For what it's worth, I can't reproduce the original bug recipe on Emacs 25.2:

- At command prompt in shell mode ...
- Insert "echo ls".
- Type RET.
- (Emacs submits whole command line "echo ls".)
- (Shell gives output line "ls".)
- Go up to output line "ls" (C-p).
- Go to end of output line (C-e).
- Insert " -la".
- Type RET.

The original bug report says

     3.5 PROBLEM: Note that Emacs submits just " -la", instead of whole
         command line "ls -la" (as intended, expected, and in Emacs 20).

Instead, I see the output of ls -la.

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

bug#18135: bug#3735: shell-mode editing, movement, and command entry broken by prompt/output changes

[Lars Ingebrigtsen]

Chong Yidong <cyd@gnu.org> writes:

> I was going through old emails and just saw this; sorry for the huge
> delay in triaging this bug.
>
> For what it's worth, I can't reproduce the original bug recipe on Emacs 25.2:
>
> - At command prompt in shell mode ...
> - Insert "echo ls".
> - Type RET.
> - (Emacs submits whole command line "echo ls".)
> - (Shell gives output line "ls".)
> - Go up to output line "ls" (C-p).
> - Go to end of output line (C-e).
> - Insert " -la".
> - Type RET.
>
> The original bug report says
>
>      3.5 PROBLEM: Note that Emacs submits just " -la", instead of whole
>          command line "ls -la" (as intended, expected, and in Emacs 20).
>
> Instead, I see the output of ls -la.

(I'm going through old bug reports that unfortunately weren't resolved
at the time.)

I'm not able to reproduce this problem in Emacs 29, either.

Don, are there any additional steps needed to reproduce the problem
(starting from "emacs -Q")?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18135: bug#3735: shell-mode editing, movement, and command entry broken by prompt/output changes

[Don Hopkins]

Thanks for looking into this! 

I can’t reproduce the problem through that recipe, but weird things still happen in other situations. 

Try putting “ -la” in the kill buffer and then yanking it instead of typing it at the end of the output of “echo ls”. 

It totally ignores the whole line, as if you entered an empty line.

But the thing that usually happens to me is that it enters the text that I yanked, but I’m not sure what the exact conditions to trigger it are. 

But at any rate, yanking “ -la” and hitting return should have the exact same behavior as typing “ -la” and hitting return, so it’s probably caused by the same underlying problem.

I’ll let you know if I figure out other test cases that get it to enter the text you yanked but not the text before that. It usually has something to do with yanking text, possibly at the end or also in the middle of the line.

-Don


> On Dec 4, 2021, at 22:15, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> 
> Chong Yidong <cyd@gnu.org> writes:
> 
>> I was going through old emails and just saw this; sorry for the huge
>> delay in triaging this bug.
>> 
>> For what it's worth, I can't reproduce the original bug recipe on Emacs 25.2:
>> 
>> - At command prompt in shell mode ...
>> - Insert "echo ls".
>> - Type RET.
>> - (Emacs submits whole command line "echo ls".)
>> - (Shell gives output line "ls".)
>> - Go up to output line "ls" (C-p).
>> - Go to end of output line (C-e).
>> - Insert " -la".
>> - Type RET.
>> 
>> The original bug report says
>> 
>>     3.5 PROBLEM: Note that Emacs submits just " -la", instead of whole
>>         command line "ls -la" (as intended, expected, and in Emacs 20).
>> 
>> Instead, I see the output of ls -la.
> 
> (I'm going through old bug reports that unfortunately weren't resolved
> at the time.)
> 
> I'm not able to reproduce this problem in Emacs 29, either.
> 
> Don, are there any additional steps needed to reproduce the problem
> (starting from "emacs -Q")?
> 
> -- 
> (domestic pets only, the antidote for overdose, milk.)
>   bloggy blog: http://lars.ingebrigtsen.no

bug#18135: bug#3735: shell-mode editing, movement, and command entry broken by prompt/output changes

[miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Don Hopkins <don@donhopkins.com> writes:

> Thanks for looking into this! 
>
> I can’t reproduce the problem through that recipe, but weird things still happen in other situations. 
>
> Try putting “ -la” in the kill buffer and then yanking it instead of typing it at the end of the output of “echo ls”. 
>
> It totally ignores the whole line, as if you entered an empty line.
>
> But the thing that usually happens to me is that it enters the text that I yanked, but I’m not sure what the exact conditions to trigger it are. 
>
> But at any rate, yanking “ -la” and hitting return should have the exact same behavior as typing “ -la” and hitting return, so it’s probably caused by the same underlying problem.
>
> I’ll let you know if I figure out other test cases that get it to enter the text you yanked but not the text before that. It usually has something to do with yanking text, possibly at the end or also in the middle of the line.

Reproducer:

- copy " -la" into kill ring
- type "echo ls" into M-x shell
- (Shell gives output line "ls".)
- Go up to output line "ls" (C-p).
- yank " -la"
- press C-b
- the line now "ls -la" with point after "l"
- press RET

Only " -la" is sent to the process instead of "ls -la"

This is because the 'field' text property of "ls" is 'output' but the
" -la" is inserted with insert-for-yank and doesn't have a 'field' text
property.

A similar thing happens if we insert a space using M-SPC
(just-one-space) in the middle of process output. The space is inserted
with 'insert' and doesn't inherit the 'field' text property of the
surrounding text.

This didn't cause a problem in older Emacs because the field text
property wasn't used yet at that time in comint.

I am willing to write up a patch to fix this. I'm thinking of having
comint-mode register a function in after-change-functions to mark text
inserted in the 'output' field as 'output'. Would this be okay or are
there any obvious flaws with this approach?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

bug#18135: bug#3735: shell-mode editing, movement, and command entry broken by prompt/output changes

[Lars Ingebrigtsen]

miha@kamnitnik.top writes:

> Reproducer:
>
> - copy " -la" into kill ring
> - type "echo ls" into M-x shell
> - (Shell gives output line "ls".)
> - Go up to output line "ls" (C-p).
> - yank " -la"
> - press C-b
> - the line now "ls -la" with point after "l"
> - press RET
>
> Only " -la" is sent to the process instead of "ls -la"

Thanks; with that I'm also able to reproduce the problem.

> I am willing to write up a patch to fix this. I'm thinking of having
> comint-mode register a function in after-change-functions to mark text
> inserted in the 'output' field as 'output'. Would this be okay or are
> there any obvious flaws with this approach?

I think that sounds like a promising approach, but I wonder whether
something could be done with rear-nonsticky here.  `field' isn't
rear-sticky here for probably good reasons, but perhaps something could
be tweaked there?  (I haven't actually tried, though.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18135: bug#3735: shell-mode editing, movement, and command entry broken by prompt/output changes

[miha--- via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1.1: Type: text/plain, Size: 1305 bytes --]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> miha@kamnitnik.top writes:
>
>> Reproducer:
>>
>> - copy " -la" into kill ring
>> - type "echo ls" into M-x shell
>> - (Shell gives output line "ls".)
>> - Go up to output line "ls" (C-p).
>> - yank " -la"
>> - press C-b
>> - the line now "ls -la" with point after "l"
>> - press RET
>>
>> Only " -la" is sent to the process instead of "ls -la"
>
> Thanks; with that I'm also able to reproduce the problem.
>
>> I am willing to write up a patch to fix this. I'm thinking of having
>> comint-mode register a function in after-change-functions to mark text
>> inserted in the 'output' field as 'output'. Would this be okay or are
>> there any obvious flaws with this approach?
>
> I think that sounds like a promising approach, but I wonder whether
> something could be done with rear-nonsticky here. `field' isn't
> rear-sticky here for probably good reasons, but perhaps something
> could be tweaked there? (I haven't actually tried, though.)

Yeah I thought about that too. However, commands such as 'yank' or
'just-one-space' use insert rather than insert-and-inherit and thus
always bypass inheritance of surrounding text properties regardless of
it's stickiness. That is why I settled for using the
'insert-in-front-hooks' text property, patch attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Improve-yanking-in-the-middle-of-comint-process-outp.patch --]
[-- Type: text/x-patch, Size: 4576 bytes --]

From 7914fe85ba096f93d4b6817bc0d7b7976f3d316a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Miha=20Rihtar=C5=A1i=C4=8D?= <miha@kamnitnik.top>
Date: Tue, 7 Dec 2021 16:59:39 +0100
Subject: [PATCH] Improve yanking in the middle of comint process output

* lisp/comint.el
(comint--unmark-string-as-output): New function to remove unwanted
properties from text yanked from comint buffers.

(comint-mode): Use it as a 'filter-buffer-substring-function'.

(comint-output-filter): Set 'insert-in-front-hooks' text property on
process output such that text yanked by the user in the middle of
process output is marked as process output (Bug#3735).

(comint--mark-as-output): New function.
(comint--mark-yanked-as-output): New function.
---
 lisp/comint.el | 54 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/lisp/comint.el b/lisp/comint.el
index 544f0b8b82..5f99f560cf 100644
--- a/lisp/comint.el
+++ b/lisp/comint.el
@@ -730,6 +730,8 @@ comint-mode
               (or (file-remote-p default-directory) ""))
   (setq-local comint-accum-marker (make-marker))
   (setq-local font-lock-defaults '(nil t))
+  (add-function :filter-return (local 'filter-buffer-substring-function)
+                #'comint--unmark-string-as-output)
   (add-hook 'change-major-mode-hook 'font-lock-defontify nil t)
   (add-hook 'isearch-mode-hook 'comint-history-isearch-setup nil t)
   (add-hook 'completion-at-point-functions 'comint-completion-at-point nil t)
@@ -1815,7 +1817,8 @@ comint-add-to-input-history
     (ring-insert comint-input-ring cmd)))
 
 (defconst comint--prompt-rear-nonsticky
-  '(field inhibit-line-move-field-capture read-only font-lock-face)
+  '( field inhibit-line-move-field-capture read-only font-lock-face
+     insert-in-front-hooks)
   "Text properties we set on the prompt and don't want to leak past it.")
 
 (defun comint-send-input (&optional no-newline artificial)
@@ -2152,14 +2155,7 @@ comint-output-filter
 	    (goto-char (process-mark process)) ; In case a filter moved it.
 
 	    (unless comint-use-prompt-regexp
-              (with-silent-modifications
-                (add-text-properties comint-last-output-start (point)
-                                     `(rear-nonsticky
-				       ,comint--prompt-rear-nonsticky
-				       front-sticky
-				       (field inhibit-line-move-field-capture)
-				       field output
-				       inhibit-line-move-field-capture t))))
+              (comint--mark-as-output comint-last-output-start (point)))
 
 	    ;; Highlight the prompt, where we define `prompt' to mean
 	    ;; the most recent output that doesn't end with a newline.
@@ -2191,6 +2187,46 @@ comint-output-filter
 	                             ,comint--prompt-rear-nonsticky)))
 	    (goto-char saved-point)))))))
 
+(defun comint--mark-as-output (beg end)
+  (with-silent-modifications
+    (add-text-properties
+     beg end
+     `(rear-nonsticky
+       ,comint--prompt-rear-nonsticky
+       front-sticky
+       (field inhibit-line-move-field-capture)
+       field output
+       inhibit-line-move-field-capture t
+       ;; Text inserted by a user in the middle of process output
+       ;; should be marked as output.  This is needed for commands
+       ;; such as `yank' or `just-one-space' which don't use
+       ;; `insert-and-inherit' and thus bypass default text property
+       ;; inheritance.
+       insert-in-front-hooks
+       (,#'comint--mark-as-output ,#'comint--mark-yanked-as-output)))))
+
+(defun comint--mark-yanked-as-output (beg end)
+  ;; `yank' removes the field text property from the text it inserts
+  ;; due to `yank-excluded-properties', so arrange for this text
+  ;; property to be reapplied in the `after-change-functions'.
+  (let (fun)
+    (setq
+     fun
+     (lambda (beg1 end1 _len1)
+       (remove-hook 'after-change-functions fun t)
+       (when (and (= beg beg1)
+                  (= end end1))
+         (comint--mark-as-output beg1 end1))))
+    (add-hook 'after-change-functions fun nil t)))
+
+(defun comint--unmark-string-as-output (string)
+  (remove-list-of-text-properties
+   0 (length string)
+   '( rear-nonsticky front-sticky field
+      inhibit-line-move-field-capture insert-in-front-hooks)
+   string)
+  string)
+
 (defun comint-preinput-scroll-to-bottom ()
   "Go to the end of buffer in all windows showing it.
 Movement occurs if point in the selected window is not after the process mark,
-- 
2.34.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

bug#3735: bug#18135: bug#3735: shell-mode editing, movement, and command entry broken by prompt/output changes

[Lars Ingebrigtsen]

<miha@kamnitnik.top> writes:

> Yeah I thought about that too. However, commands such as 'yank' or
> 'just-one-space' use insert rather than insert-and-inherit and thus
> always bypass inheritance of surrounding text properties regardless of
> it's stickiness. That is why I settled for using the
> 'insert-in-front-hooks' text property, patch attached.

Thanks; works well here, too, so I've pushed this to Emacs 29 now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#18135: bug#3735: shell-mode editing, movement, and command entry broken by prompt/output changes

[Don Hopkins]

Thank you! I’m looking forward to upgrading to emacs 29. 

-Don


> On Dec 7, 2021, at 21:16, Lars Ingebrigtsen <larsi@gnus.org> wrote:
> 
> <miha@kamnitnik.top> writes:
> 
>> Yeah I thought about that too. However, commands such as 'yank' or
>> 'just-one-space' use insert rather than insert-and-inherit and thus
>> always bypass inheritance of surrounding text properties regardless of
>> it's stickiness. That is why I settled for using the
>> 'insert-in-front-hooks' text property, patch attached.
> 
> Thanks; works well here, too, so I've pushed this to Emacs 29 now.
> 
> -- 
> (domestic pets only, the antidote for overdose, milk.)
>   bloggy blog: http://lars.ingebrigtsen.no