all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#29118: 25.2.50; Undoing shell flush output results in weird state
@ 2017-11-02 21:25 Allen Li
  2017-11-10  1:26 ` Noam Postavsky
  0 siblings, 1 reply; 11+ messages in thread
From: Allen Li @ 2017-11-02 21:25 UTC (permalink / raw)
  To: 29118

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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#29118: 25.2.50; Undoing shell flush output results in weird state
  2017-11-02 21:25 bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li
@ 2017-11-10  1:26 ` Noam Postavsky
  2017-11-10  8:10   ` Eli Zaretskii
  2017-11-11  7:20   ` bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li
  0 siblings, 2 replies; 11+ messages in thread
From: Noam Postavsky @ 2017-11-10  1:26 UTC (permalink / raw)
  To: Allen Li; +Cc: 29118

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.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#29118: 25.2.50; Undoing shell flush output results in weird state
  2017-11-10  1:26 ` Noam Postavsky
@ 2017-11-10  8:10   ` Eli Zaretskii
  2017-11-12  2:12     ` Noam Postavsky
  2017-11-11  7:20   ` bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li
  1 sibling, 1 reply; 11+ messages in thread
From: Eli Zaretskii @ 2017-11-10  8:10 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: 29118, vianchielfaura

> 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.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#29118: 25.2.50; Undoing shell flush output results in weird state
  2017-11-10  1:26 ` Noam Postavsky
  2017-11-10  8:10   ` Eli Zaretskii
@ 2017-11-11  7:20   ` Allen Li
  2017-11-12 18:30     ` Noam Postavsky
  1 sibling, 1 reply; 11+ messages in thread
From: Allen Li @ 2017-11-11  7:20 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: 29118

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).





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#29118: 25.2.50; Undoing shell flush output results in weird state
  2017-11-10  8:10   ` Eli Zaretskii
@ 2017-11-12  2:12     ` Noam Postavsky
  2017-11-12  4:47       ` Eli Zaretskii
  2017-11-26 19:59       ` bug#29461: Wishlist: better facilities for debugging undo problems Noam Postavsky
  0 siblings, 2 replies; 11+ messages in thread
From: Noam Postavsky @ 2017-11-12  2:12 UTC (permalink / raw)
  To: Eli Zaretskii; +Cc: 29118, vianchielfaura

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).





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#29118: 25.2.50; Undoing shell flush output results in weird state
  2017-11-12  2:12     ` Noam Postavsky
@ 2017-11-12  4:47       ` Eli Zaretskii
  2017-11-26 19:59       ` bug#29461: Wishlist: better facilities for debugging undo problems Noam Postavsky
  1 sibling, 0 replies; 11+ messages in thread
From: Eli Zaretskii @ 2017-11-12  4:47 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: 29118, vianchielfaura

> 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.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#29118: 25.2.50; Undoing shell flush output results in weird state
  2017-11-11  7:20   ` bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li
@ 2017-11-12 18:30     ` Noam Postavsky
  2017-11-13  8:29       ` Allen Li
  2017-11-13 13:26       ` Stefan Monnier
  0 siblings, 2 replies; 11+ messages in thread
From: Noam Postavsky @ 2017-11-12 18:30 UTC (permalink / raw)
  To: Allen Li; +Cc: 29118, Barry OReilly, Stefan

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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#29118: 25.2.50; Undoing shell flush output results in weird state
  2017-11-12 18:30     ` Noam Postavsky
@ 2017-11-13  8:29       ` Allen Li
  2017-11-13 13:26       ` Stefan Monnier
  1 sibling, 0 replies; 11+ messages in thread
From: Allen Li @ 2017-11-13  8:29 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: 29118, Barry OReilly, Stefan

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.





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#29118: 25.2.50; Undoing shell flush output results in weird state
  2017-11-12 18:30     ` Noam Postavsky
  2017-11-13  8:29       ` Allen Li
@ 2017-11-13 13:26       ` Stefan Monnier
  2017-11-15 13:05         ` Noam Postavsky
  1 sibling, 1 reply; 11+ messages in thread
From: Stefan Monnier @ 2017-11-13 13:26 UTC (permalink / raw)
  To: Noam Postavsky; +Cc: 29118, Allen Li, Barry OReilly

> --- 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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#29118: 25.2.50; Undoing shell flush output results in weird state
  2017-11-13 13:26       ` Stefan Monnier
@ 2017-11-15 13:05         ` Noam Postavsky
  0 siblings, 0 replies; 11+ messages in thread
From: Noam Postavsky @ 2017-11-15 13:05 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: 29118, Allen Li, Barry OReilly

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





^ permalink raw reply	[flat|nested] 11+ messages in thread

* bug#29461: Wishlist: better facilities for debugging undo problems
  2017-11-12  2:12     ` Noam Postavsky
  2017-11-12  4:47       ` Eli Zaretskii
@ 2017-11-26 19:59       ` Noam Postavsky
  1 sibling, 0 replies; 11+ messages in thread
From: Noam Postavsky @ 2017-11-26 19:59 UTC (permalink / raw)
  To: 29461

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.






^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2017-11-26 19:59 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-11-02 21:25 bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li
2017-11-10  1:26 ` Noam Postavsky
2017-11-10  8:10   ` Eli Zaretskii
2017-11-12  2:12     ` Noam Postavsky
2017-11-12  4:47       ` Eli Zaretskii
2017-11-26 19:59       ` bug#29461: Wishlist: better facilities for debugging undo problems Noam Postavsky
2017-11-11  7:20   ` bug#29118: 25.2.50; Undoing shell flush output results in weird state Allen Li
2017-11-12 18:30     ` Noam Postavsky
2017-11-13  8:29       ` Allen Li
2017-11-13 13:26       ` Stefan Monnier
2017-11-15 13:05         ` Noam Postavsky

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.