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

[Allen Li <vianchielfaura@gmail.com>]

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.