bug#29118: 25.2.50; Undoing shell flush output results in weird state

bug#29118: 25.2.50; Undoing shell flush output results in weird state

[Allen Li]

Repro:

1. emacs -Q
2. M-x shell RET
3. echo hi RET
4. C-c C-o
5. C-/
6. echo bye RET

The shell buffer ends up in a weird state that's hard to describe.

In GNU Emacs 25.2.50.1 (x86_64-pc-linux-gnu, GTK+ Version 3.10.8),
modified by Debian
Windowing system distributor 'The X.Org Foundation', version 11.0.11803000
System Description: Ubuntu 14.04 LTS

bug#29118: 25.2.50; Undoing shell flush output results in weird state

[Noam Postavsky]

found 29118 26.0.90
tags 29118 + confirmed
quit

Allen Li <vianchielfaura@gmail.com> writes:

> Repro:
>
> 1. emacs -Q
> 2. M-x shell RET
> 3. echo hi RET
> 4. C-c C-o
> 5. C-/
> 6. echo bye RET
>
> The shell buffer ends up in a weird state that's hard to describe.

The process mark ends up in the wrong place.  That is,

    (process-mark (get-buffer-process (current-buffer)))

gives a different answer after step 3 vs after step 5.  Not really sure
why, debugging undo-list stuff is kind of a pain.

bug#29118: 25.2.50; Undoing shell flush output results in weird state

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Date: Thu, 09 Nov 2017 20:26:53 -0500
> Cc: 29118@debbugs.gnu.org
> 
> debugging undo-list stuff is kind of a pain.

Why is that a pain?  What can we do to make its debugging experience
better?

Thanks.

bug#29118: 25.2.50; Undoing shell flush output results in weird state

[Noam Postavsky]

Eli Zaretskii <eliz@gnu.org> writes:

>> debugging undo-list stuff is kind of a pain.
>
> Why is that a pain?

Just from this very simple recipe, buffer-undo-list contains all of this
(assuming you made no mistakes typing it in, it's unclear to me how to
reproduce it non-interactively).  And I find it pretty difficult to
interpret buffer positions in my head.  So interpreting what all the
element means is ... a pain.

(nil
 (22 . 25)
 (22 . 35)
 (#("*** output flushed ***
~/src/emacs$ " 0 23
 (fontified t front-sticky #1=(field inhibit-line-move-field-capture)
	    rear-nonsticky t field output inhibit-line-move-field-capture t)
 23 36
 (font-lock-face
  (comint-highlight-prompt)
  inhibit-line-move-field-capture t field output rear-nonsticky t front-sticky #1# fontified nil))
   . 22)
 (#<marker at 38 in *shell*> . -36)
 (#<marker at 22 in *shell*> . -36)
 (#<marker at 22 in *shell*> . -23)
 (#<marker at 22 in *shell*> . -36)
 (nil font-lock-face
      (comint-highlight-prompt . #2=(comint-highlight-prompt))
      45 . 58)
 nil
 (nil font-lock-face #2# 45 . 58)
 (22 . 58)
 (#("~/src/emacs$ " 0 13
    (fontified nil front-sticky
	       (field inhibit-line-move-field-capture)
	       rear-nonsticky t field output inhibit-line-move-field-capture t font-lock-face
	       (comint-highlight-prompt)))
   . -22)
 (#<marker at 38 in *shell*> . -13)
 (#<marker at 22 in *shell*> . -13)
 (#("hi
" 0 3
(fontified t front-sticky
	   (field inhibit-line-move-field-capture)
	   rear-nonsticky t field output inhibit-line-move-field-capture t))
   . -22)
 38 nil
 (nil font-lock-face nil 25 . 38)
 (21 . 38)
 (#("echo hi" 0 7
    (fontified nil))
   . 14)
 (#<marker at 38 in *shell*> . -7)
 (21 . 28)
 nil
 (14 . 21)
 nil
 (nil font-lock-face nil 1 . 14)
 (1 . 14)
 (t . 0))

> What can we do to make its debugging experience better?

Some way of visualizing the effect of undo list elements.  A way to
replay and rewind without affecting the undo list.

Implementing such a thing would probably be pretty difficult.  I have
some crude visualization for markers in yasnippet-debug.el (in GNU
ELPA).

bug#29118: 25.2.50; Undoing shell flush output results in weird state

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@users.sourceforge.net>
> Cc: 29118@debbugs.gnu.org,  vianchielfaura@gmail.com
> Date: Sat, 11 Nov 2017 21:12:30 -0500
> 
> > What can we do to make its debugging experience better?
> 
> Some way of visualizing the effect of undo list elements.  A way to
> replay and rewind without affecting the undo list.
> 
> Implementing such a thing would probably be pretty difficult.  I have
> some crude visualization for markers in yasnippet-debug.el (in GNU
> ELPA).

Maybe you should file a wishlist bug about this, then.

bug#29461: Wishlist: better facilities for debugging undo problems

[Noam Postavsky]

Severity: wishlist

See also https://debbugs.gnu.org/cgi/bugreport.cgi?bug=29118#21

To make debugging trouble with undo easier, it would nice to have a way
to "replay" and "rewind" operations to easily see what they do.  And a
way to visualize the meaning of undo list elements without actually
"playing" them, as it's hard to see what a pair of buffer positions
refers to at a glance.

I've written some crude visualization code for markers under
elpa/packages/yasnippet/yasnippet-debug.el.

bug#29118: 25.2.50; Undoing shell flush output results in weird state

[Allen Li]

On Thu, Nov 9, 2017 at 5:26 PM, Noam Postavsky
<npostavs@users.sourceforge.net> wrote:
> found 29118 26.0.90
> tags 29118 + confirmed
> quit
>
> Allen Li <vianchielfaura@gmail.com> writes:
>
>> Repro:
>>
>> 1. emacs -Q
>> 2. M-x shell RET
>> 3. echo hi RET
>> 4. C-c C-o
>> 5. C-/
>> 6. echo bye RET
>>
>> The shell buffer ends up in a weird state that's hard to describe.
>
> The process mark ends up in the wrong place.  That is,
>
>     (process-mark (get-buffer-process (current-buffer)))
>
> gives a different answer after step 3 vs after step 5.  Not really sure
> why, debugging undo-list stuff is kind of a pain.

Thanks.  A couple of followup observations:

1. As a workaround, to fix the shell buffer, you can use
comint-set-process-mark with point at where the end of the current
prompt is.
2. Changing the process-mark insertion type before undoing prevents the bug:

(set-marker-insertion-type (process-mark (get-buffer-process
(current-buffer))) t)

However, you need to set it back to nil after undoing or all user
input will get inserted before process-mark.

So one way to fix this would be to define a comint-undo that sets and
resets the process-mark insertion type.
That's probably not the most elegant solution.

I think undo in general does not behave well with markers. undo will
always rely on the insertion type of the markers,
whereas the text to re-insert may well have been on either side of the marker.

Perhaps undo should be improved to track which side of a marker
deleted text was at.  I think that might fix a whole class of
undo-related bugs.
However, this is very difficult to implement (I think, but I would be
glad to be proven wrong).

bug#29118: 25.2.50; Undoing shell flush output results in weird state

[Noam Postavsky]

found 29118 24.4
quit

Allen Li <vianchielfaura@gmail.com> writes:

> 2. Changing the process-mark insertion type before undoing prevents the bug:
>
> (set-marker-insertion-type (process-mark (get-buffer-process
> (current-buffer))) t)

Ah, interesting.

> However, you need to set it back to nil after undoing or all user
> input will get inserted before process-mark.
>
> So one way to fix this would be to define a comint-undo that sets and
> resets the process-mark insertion type.
> That's probably not the most elegant solution.
>
> I think undo in general does not behave well with markers. undo will
> always rely on the insertion type of the markers,
> whereas the text to re-insert may well have been on either side of the marker.
>
> Perhaps undo should be improved to track which side of a marker
> deleted text was at.

Hmm, as far as I can tell, in this case the text was inserted on the
"normal" side of the marker, but then the marker is moved afterwards.
So I'm not sure if such tracking would help.

    (defun comint-output-filter (process string)
       ...
            ;; insert-before-markers is a bad thing. XXX
            ;; Luckily we don't have to use it any more, we use
            ;; window-point-insertion-type instead.
            (insert string)

            ;; Advance process-mark
            (set-marker (process-mark process) (point))


However, while looking at the undo code, I noticed that it checks for
valid marker adjustments by checking that the marker position is the
same as the POSITION of the deletion record.  However, for a negative
POSITION (indicating text was at the end of the deleted text) this
comparison will always be false (markers can't have negative positions).

  (defun primitive-undo (n list)
    ...          
            (`(,(and string (pred stringp)) . ,(and pos (pred integerp)))
                   ...
                   (and (eq (marker-buffer m) (current-buffer))
                        (= pos m) ; <<<<<<<<<<<< Always nil for negative `pos'
                        (push marker-adj valid-marker-adjustments))))

I don't see why the position of point should affect the adjustment of
other markers.  This seems to have been added by [1: 37ea8275f7] to fix
Bug#16818.  Before that, if I understand correctly, all marker
adjustments were applied without checking location at all.  And indeed,
this bug does not occur in Emacs 24.3.

So I would suggest the following patch, which does seem to actually fix
this bug.  It might break some other things though; I have not tested it
apart from the scenario in this bug.  I'm adding the participants of
Bug#16818 to Cc, in the hope they might have some more insight.

--- i/lisp/simple.el
+++ w/lisp/simple.el
@@ -2579,7 +2579,7 @@ primitive-undo
                (let* ((marker-adj (pop list))
                       (m (car marker-adj)))
                  (and (eq (marker-buffer m) (current-buffer))
-                      (= pos m)
+                      (= (abs pos) m)
                       (push marker-adj valid-marker-adjustments))))
              ;; Insert string and adjust point
              (if (< pos 0)

[1: 37ea8275f7]: 2014-03-24 22:47:39 -0400
  Undo in region after markers in undo history relocated
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=37ea8275f7faad1192ddaba9f4a0789580675e17

bug#29118: 25.2.50; Undoing shell flush output results in weird state

[Allen Li]

On Sun, Nov 12, 2017 at 10:30 AM, Noam Postavsky
<npostavs@users.sourceforge.net> wrote:
> Hmm, as far as I can tell, in this case the text was inserted on the
> "normal" side of the marker, but then the marker is moved afterwards.
> So I'm not sure if such tracking would help.
>
>     (defun comint-output-filter (process string)
>        ...
>             ;; insert-before-markers is a bad thing. XXX
>             ;; Luckily we don't have to use it any more, we use
>             ;; window-point-insertion-type instead.
>             (insert string)
>
>             ;; Advance process-mark
>             (set-marker (process-mark process) (point))
>
>
> However, while looking at the undo code, I noticed that it checks for
> valid marker adjustments by checking that the marker position is the
> same as the POSITION of the deletion record.  However, for a negative
> POSITION (indicating text was at the end of the deleted text) this
> comparison will always be false (markers can't have negative positions).

I was mistaken, undo does track marker movement correctly; or mostly
correctly, excluding this bug.

>
>   (defun primitive-undo (n list)
>     ...
>             (`(,(and string (pred stringp)) . ,(and pos (pred integerp)))
>                    ...
>                    (and (eq (marker-buffer m) (current-buffer))
>                         (= pos m) ; <<<<<<<<<<<< Always nil for negative `pos'
>                         (push marker-adj valid-marker-adjustments))))
>
> I don't see why the position of point should affect the adjustment of
> other markers.  This seems to have been added by [1: 37ea8275f7] to fix
> Bug#16818.  Before that, if I understand correctly, all marker
> adjustments were applied without checking location at all.  And indeed,
> this bug does not occur in Emacs 24.3.

I agree with your analysis.  The intent of the code is to make sure that
the marker hasn't been moved from the original location before trying to
restore it.  In that case, comparing against the absolute value of pos
makes sense, as we use the sign to carry extra information in band that
is not relevant to the marker adjustment.

> So I would suggest the following patch, which does seem to actually fix
> this bug.  It might break some other things though; I have not tested it
> apart from the scenario in this bug.  I'm adding the participants of
> Bug#16818 to Cc, in the hope they might have some more insight.
>
> --- i/lisp/simple.el
> +++ w/lisp/simple.el
> @@ -2579,7 +2579,7 @@ primitive-undo
>                 (let* ((marker-adj (pop list))
>                        (m (car marker-adj)))
>                   (and (eq (marker-buffer m) (current-buffer))
> -                      (= pos m)
> +                      (= (abs pos) m)
>                        (push marker-adj valid-marker-adjustments))))
>               ;; Insert string and adjust point
>               (if (< pos 0)
>
> [1: 37ea8275f7]: 2014-03-24 22:47:39 -0400
>   Undo in region after markers in undo history relocated
>   https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=37ea8275f7faad1192ddaba9f4a0789580675e17

I have added this to my trusty personal monkeypatch library.  I can
confirm that it fixes the current bug.  I can use this patch for a while
and see if I run into any obvious issues.  The patch seems quite
straightforward and correct, but perhaps I am naive.

bug#29118: 25.2.50; Undoing shell flush output results in weird state

[Stefan Monnier]

> --- i/lisp/simple.el
> +++ w/lisp/simple.el
> @@ -2579,7 +2579,7 @@ primitive-undo
>                 (let* ((marker-adj (pop list))
>                        (m (car marker-adj)))
>                   (and (eq (marker-buffer m) (current-buffer))
> -                      (= pos m)
> +                      (= (abs pos) m)
>                        (push marker-adj valid-marker-adjustments))))
>               ;; Insert string and adjust point
>               (if (< pos 0)

Looks correct to me, this belongs in emacs-26, thanks.


        Stefan

bug#29118: 25.2.50; Undoing shell flush output results in weird state

[Noam Postavsky]

tags 29118 fixed
close 29118 26.1
quit

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>                   (and (eq (marker-buffer m) (current-buffer))
>> -                      (= pos m)
>> +                      (= (abs pos) m)
>>                        (push marker-adj valid-marker-adjustments))))

> Looks correct to me, this belongs in emacs-26, thanks.

Thanks, pushed to emacs-26.

[1: 1faade8821]: 2017-11-15 07:05:35 -0500
  Fix marker adjustment for undo (Bug#29118)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=1faade882165de2e9bd63702c59c8f26531c18ed