GNU bug report logs - #16818
Undo in region after markers in undo history relocated

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 16818 in the body.
You can then email your comments to 16818 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: Undo in region after markers in undo history relocated
Date: Wed, 19 Feb 2014 17:15:52 -0500

[Message part 1 (text/plain, inline)]

Strange behavior with the following recipe, using trunk:

  * ./src/emacs -Q
  * Move point up two lines
  * Insert "aaa"
  * Undo
  * Move point down two lines
  * Insert "bbb"
  * Select "bbb" from end to beginning and delete
  * Insert "bbb"
  * Go to line where "aaa" was and select the line from beginning to
    end
  * Undo in region
  * Observe:
    * "Undo in region!" is echoed in the minibuffer
    * Unexpectedly, "aaa" is not reinserted back into the buffer
    * Strangely, the selection mark at BOL moves forward three chars
  * Undo in region again
  * Observe:
    * "aaa" is reinserted, despite selection no longer covering the
      region where "aaa" was

Prior to the first undo in region, buffer-undo-list is:

  (nil (192 . 195) nil (bbb . 192) (#<marker at 141 in *scratch*> . -3)
(#<marker at 192 in *scratch*> . -3) (#<marker at 190 in *scratch*> . -3)
nil (192 . 195) (t . 0) nil (aaa . 141) nil (141 . 144) (t . 0) nil (1 .
192) (t . 0))

And after undo-make-selective-list pending-undo-list becomes:

  (nil (#<marker at 141 in *scratch*> . -3) (#<marker at 190 in *scratch*>
. -3) nil (aaa . 141) nil (141 . 144) nil)

It seems the #<marker at 141 in *scratch*> is the mark used during
selection. When bbb is deleted, the mark is recorded into undo
history, and it's still there when the selection mark has moved to
position 141 before "aaa". So:

> * Unexpectedly, "aaa" is not reinserted back into the buffer

is due to the selection mark and another marker occupying their own
change group in the filtered pending-undo-list, and that change group
is chosen for the first undo in region.

> Strangely, the selection mark at BOL moves forward three chars

is due to the -3 marker adjustment.

> * "aaa" is reinserted, despite selection no longer covering the
    region where "aaa" was

is simply due to pending-undo-list being reused for the second
consecutive undo, unaware that the selection mark was adjusted.

I haven't determined what those other two markers are. But the general
question comes up: how is the undo history meant to know about marker
relocations?

The code in adjust_markers_for_delete indicates markers are only
recorded when they would be in a deleted region or immediately before
it, so I think the only circumstance markers form their own undesired
change group in the pending-undo-list is when recorded markers
were relocated.

[Message part 2 (text/html, inline)]

Message #8 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Wed, 19 Feb 2014 18:07:56 -0500

[Message part 1 (text/plain, inline)]

Another question: what is the purpose of undo-adjusted-markers?
simple.el is the only file I found that references it, and it's only
setting the variable without using it.

I found the specialized GC treatment of undo history markers in
compact_undo_list, so merely having references to the markers via
undo-adjusted-markers might be a motivation. But I can't think of a
particular reason for that.

[Message part 2 (text/html, inline)]

Message #11 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Wed, 19 Feb 2014 23:53:18 -0500

> Another question: what is the purpose of undo-adjusted-markers?
> simple.el is the only file I found that references it, and it's only
> setting the variable without using it.

I don't see that.  It's set *and* read in undo-elt-in-region.
Not sure exactly what is the purpose, tho.


        Stefan

Message #14 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Thu, 20 Feb 2014 09:38:52 -0500

[Message part 1 (text/plain, inline)]

> I don't see that. It's set *and* read in undo-elt-in-region. Not
> sure exactly what is the purpose, tho.

I found no purpose at all for it.

The more pertinent question is:

> the general question comes up: how is the undo history meant to know
> about marker relocations?

I think the marker adjustment entries in the undo history are invalid
when a marker is relocated elsewhere. I don't see code to deal with
that, so how you would like it to happen?

[Message part 2 (text/html, inline)]

Message #17 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Mon, 24 Feb 2014 17:46:11 -0500

[Message part 1 (text/plain, inline)]

> > the general question comes up: how is the undo history meant to
> > know about marker relocations?

> I think the marker adjustment entries in the undo history are
> invalid when a marker is relocated elsewhere. I don't see code to
> deal with that, so how you would like it to happen?

I found the Elisp manual says about moving markers:

  When you do this, be sure you know whether the marker is used
  outside of your program, and, if so, what effects will result from
  moving it--otherwise, confusing things may happen in other parts of
  Emacs.

So if we expand on the guidance as follows:

diff --git a/doc/lispref/markers.texi b/doc/lispref/markers.texi
index 51b87ab..19386d6 100644
--- a/doc/lispref/markers.texi
+++ b/doc/lispref/markers.texi
@@ -344,10 +344,12 @@ specify the insertion type, create them with
insertion type
 @section Moving Marker Positions

   This section describes how to change the position of an existing
-marker.  When you do this, be sure you know whether the marker is used
-outside of your program, and, if so, what effects will result from
-moving it---otherwise, confusing things may happen in other parts of
-Emacs.
+marker.  When you do this, be sure you know how the marker is used
+outside of your program.  For example, moving a marker to an unrelated
+new position can cause undo to later adjust the marker incorrectly.
+Often when you wish to relocate a marker to an unrelated position, it
+is preferable to make a new marker and set the prior one to point
+nowhere.

 @defun set-marker marker position &optional buffer
 This function moves @var{marker} to @var{position}

then we don't need to worry about markers in general, only the
particular offending ones of the recipe.

As I described, one offending marker is the mark-marker. From the
push-mark function:

(defun push-mark (&optional location nomsg activate)
  [...]
    (setq mark-ring (cons (copy-marker (mark-marker)) mark-ring))
  [...]
  (set-marker (mark-marker) (or location (point)) (current-buffer))
  [...]
  (setq global-mark-ring (cons (copy-marker (mark-marker))
global-mark-ring))
  )

Two copies are made and the copies go to the mark-ring and
global-mark-ring. The mark continues to be the same marker object
under eq as before, only mutated. Consequently, not only does undo
adjust a marker it shouldn't have, but it doesn't adjust the copies in
the rings when it should have.

What makes more sense to me is that the old value of mark-marker is
pushed onto the rings, then a new marker object is created to become
the value of mark-marker. Then the marker adjustment records would
reference the right markers. Effectively this means the mark would
follow the advice of the Elisp manual excerpt above.

I welcome your guidance about what the right way to solve this bug is.
Thank you.

[Message part 2 (text/html, inline)]

Message #20 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Tue, 25 Feb 2014 16:43:31 -0500

> What makes more sense to me is that the old value of mark-marker is
> pushed onto the rings, then a new marker object is created to become
> the value of mark-marker. Then the marker adjustment records would
> reference the right markers. Effectively this means the mark would
> follow the advice of the Elisp manual excerpt above.

That sounds right.  I don't have time to really understand the ins and
outs, but if you can write an ERT test case for it, we could install
the change.


        Stefan

Message #23 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Wed, 26 Feb 2014 10:18:12 -0500

[Message part 1 (text/plain, inline)]

> That sounds right. I don't have time to really understand the ins
> and outs, but if you can write an ERT test case for it, we could
> install the change.

Thanks. The doc patch is relevant with or without a fix, so I'll
install that soon if there's no objections.

[Message part 2 (text/html, inline)]

Message #26 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Tue, 11 Mar 2014 17:24:25 -0400

[Message part 1 (text/plain, inline)]

>> What makes more sense to me is that the old value of mark-marker is
>> pushed onto the rings, then a new marker object is created to
>> become the value of mark-marker. Then the marker adjustment records
>> would reference the right markers. Effectively this means the mark
>> would follow the advice of the Elisp manual excerpt above.

> That sounds right. I don't have time to really understand the ins
> and outs, but if you can write an ERT test case for it, we could
> install the change.

At the end is a patch to implement this. I wrote the marker-tests to
make sure I didn't break anything too badly, they pass with or without
the rest of the patch. The new undo-test-mark-adjustment implements
the recipe of this bug report. It fails with current trunk and passes
with the patch.

Of the original symptoms of this bug report, the patch solves:

>> Strangely, the selection mark at BOL moves forward three chars

but not:

>> * Unexpectedly, "aaa" is not reinserted back into the buffer
> is due to the selection mark and another marker occupying their own
> change group in the filtered pending-undo-list, and that change
> group is chosen for the first undo in region.

The selection mark is no longer "in the way" on the pending-undo-list,
but the another marker still is. It only is when the recipe is carried
out manually apparently, because the marker is not there when the
recipe is carried out in the undo-test-mark-adjustment. IOW, the ERT
test passes the same test case that fails when carried out manually.

Debugging the C sources, I find the offending marker in the
pending-undo-list is first created at the following C and Lisp
backtraces:

Obtained 31 stack frames.
./src/emacs [0x52c2b4]
./src/emacs(Fmake_overlay+0xf2) [0x526062]
./src/emacs(Ffuncall+0x371) [0x5729c1]
./src/emacs(exec_byte_code+0x3a5) [0x5a42e5]
./src/emacs(Ffuncall+0x20c) [0x57285c]
./src/emacs(exec_byte_code+0x3a5) [0x5a42e5]
./src/emacs(Ffuncall+0x20c) [0x57285c]
./src/emacs(exec_byte_code+0x3a5) [0x5a42e5]
./src/emacs(Ffuncall+0x20c) [0x57285c]
./src/emacs(eval_sub+0x6a8) [0x572118]
./src/emacs(internal_lisp_condition_case+0x21b) [0x574cbb]
./src/emacs(exec_byte_code+0x12fa) [0x5a523a]
./src/emacs(Ffuncall+0x20c) [0x57285c]
./src/emacs(Fapply+0x2c7) [0x572fe7]
./src/emacs(Ffuncall+0x422) [0x572a72]
./src/emacs(exec_byte_code+0x3a5) [0x5a42e5]
./src/emacs(Ffuncall+0x20c) [0x57285c]
./src/emacs(internal_condition_case_n+0xfe) [0x5713ce]
./src/emacs(safe_call+0x138) [0x463298]
./src/emacs [0x4814f4]
./src/emacs(read_char+0x965) [0x50f3b5]
./src/emacs [0x510bad]
./src/emacs(command_loop_1+0x27e) [0x51267e]
./src/emacs(internal_condition_case+0xda) [0x57101a]
./src/emacs [0x5097ca]
./src/emacs(internal_catch+0xd8) [0x570ef8]
./src/emacs(recursive_edit_1+0x120) [0x508d00]
./src/emacs(Frecursive_edit+0xc7) [0x50a687]
./src/emacs(main+0x8ed) [0x5016ad]
/lib64/libc.so.6(__libc_start_main+0xf4) [0x31cf01d994]
./src/emacs [0x43b369]
  make-overlay(192 193)
  #[1028 "Á
!„Â\!ˆÃ
Ä#ˆÃ
Ã
Æ#ˆ‰‡Ç
!p=ƒ4È
!=ƒ4É
!=„;Ê
p$ˆ‡"
[redisplay-unhighlight-region-function overlayp make-overlay overlay-put
window face region overlay-buffer overlay-start overlay-end move-overlay] 9
"

(fn START END WINDOW ROL)"](192 193 #<window 3 on *scratch*> nil)
  redisplay--update-region-highlight(#<window 3 on *scratch*>)
  #[0 "À¢„
ÃÄ !‡À¢<„Àà ƉÇ# ˆ ƒ\"ÈÃÀ¢\"‡É  )Ê À¢‰ S‰@‰Ä
=„>‰=ƒEÃ
!ˆ‚L
ËÌ\"!ˆ
A¶‚‚+²
‡" [((#<window 3 on *scratch*>))
highlight-nonselected-windows redisplay-unhighlight-region-function
redisplay--update-region-highlight selected-window window-list-1 nil t mapc
window-minibuffer-p minibuffer-selected-window window-parameter
internal-region-overlay] 7 "

(fn)"]()
  funcall(#[0 "À¢„
ÃÄ !‡À¢<„Àà ƉÇ# ˆ ƒ\"ÈÃÀ¢\"‡É  )Ê À¢‰ S‰@‰Ä
=„>‰=ƒEÃ
!ˆ‚L
ËÌ\"!ˆ
A¶‚‚+²
‡" [((#<window 3 on *scratch*>))
highlight-nonselected-windows redisplay-unhighlight-region-function
redisplay--update-region-highlight selected-window window-list-1 nil t mapc
window-minibuffer-p minibuffer-selected-window window-parameter
internal-region-overlay] 7 "

(fn)"])
  redisplay--update-region-highlights((#<window 3 on *scratch*>))
  apply(redisplay--update-region-highlights (#<window 3 on *scratch*>))
  #[128 "ÀÁ\"ˆÀÂ\"‡" [apply redisplay--update-region-highlights
ignore nil] 4 nil nil]((#<window 3 on *scratch*>))
  redisplay_internal\ \(C\ function\)()

Following the redisplay code indicated, it looks like the same overlay
object is reused for subsequent region selections. If so, the
explanation is similar as for the mark: the marker of the
internal-region-overlay is the same under eq when "bbb" is selected
and deleted as when the line formerly containing "aaa" is selected
followed by undo in region. The marker adjustment is incorrectly found
because the marker moved, and consequently the "aaa" undo record is
not found. Would the solution be analogous as for the mark: make a new
overlay for each new region selection?

Finally, here's the patch that fixes this problem wrt the mark:

diff --git a/lisp/simple.el b/lisp/simple.el
index f9447b1..5b7970c 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -4399,11 +4399,11 @@ If NO-TMM is non-nil, leave `transient-mark-mode'
alone."
       (setq transient-mark-mode 'lambda))
     (run-hooks 'activate-mark-hook)))

-(defun set-mark (pos)
-  "Set this buffer's mark to POS.  Don't use this function!
-That is to say, don't use this function unless you want
-the user to see that the mark has moved, and you want the previous
-mark position to be lost.
+(defun set-mark (pos-or-marker)
+  "Set this buffer's mark to POS-OR-MARKER.  Don't use this
+function!  That is to say, don't use this function unless you
+want the user to see that the mark has moved, and you want the
+previous mark position to be lost.

 Normally, when a new mark is set, the old one should go on the stack.
 This is why most applications should use `push-mark', not `set-mark'.
@@ -4415,9 +4415,10 @@ To remember a location for internal use in the Lisp
program,
 store it in a Lisp variable.  Example:

    (let ((beg (point))) (forward-line 1) (delete-region beg (point)))."
-
-  (set-marker (mark-marker) pos (current-buffer))
-  (if pos
+  (if (markerp pos-or-marker)
+      (setq mark pos-or-marker)
+    (set-marker (mark-marker) pos-or-marker (current-buffer)))
+  (if pos-or-marker
       (activate-mark 'no-tmm)
     ;; Normally we never clear mark-active except in Transient Mark mode.
     ;; But when we actually clear out the mark value too, we must
@@ -4639,10 +4640,12 @@ purposes.  See the documentation of `set-mark' for
more information.

 In Transient Mark mode, activate mark if optional third arg ACTIVATE
non-nil."
   (unless (null (mark t))
-    (setq mark-ring (cons (copy-marker (mark-marker)) mark-ring))
+    (push (mark-marker) mark-ring)
     (when (> (length mark-ring) mark-ring-max)
-      (move-marker (car (nthcdr mark-ring-max mark-ring)) nil)
+      ;; Remove from mark-ring.  Note that marker may be in
+      ;; global-mark-ring, so don't point it nowhere.
       (setcdr (nthcdr (1- mark-ring-max) mark-ring) nil)))
+  (set-mark (copy-marker (mark-marker)))
   (set-marker (mark-marker) (or location (point)) (current-buffer))
   ;; Now push the mark on the global mark ring.
   (if (and global-mark-ring
@@ -4650,9 +4653,8 @@ In Transient Mark mode, activate mark if optional
third arg ACTIVATE non-nil."
       ;; The last global mark pushed was in this same buffer.
       ;; Don't push another one.
       nil
-    (setq global-mark-ring (cons (copy-marker (mark-marker))
global-mark-ring))
+    (push (mark-marker) global-mark-ring)
     (when (> (length global-mark-ring) global-mark-ring-max)
-      (move-marker (car (nthcdr global-mark-ring-max global-mark-ring))
nil)
       (setcdr (nthcdr (1- global-mark-ring-max) global-mark-ring) nil)))
   (or nomsg executing-kbd-macro (> (minibuffer-depth) 0)
       (message "Mark set"))
@@ -4664,11 +4666,9 @@ In Transient Mark mode, activate mark if optional
third arg ACTIVATE non-nil."
   "Pop off mark ring into the buffer's actual mark.
 Does not set point.  Does nothing if mark ring is empty."
   (when mark-ring
-    (setq mark-ring (nconc mark-ring (list (copy-marker (mark-marker)))))
-    (set-marker (mark-marker) (+ 0 (car mark-ring)) (current-buffer))
-    (move-marker (car mark-ring) nil)
-    (if (null (mark t)) (ding))
-    (setq mark-ring (cdr mark-ring)))
+    (setq mark-ring (nconc mark-ring (list (mark-marker))))
+    (set-mark (pop mark-ring))
+    (if (null (mark t)) (ding)))
   (deactivate-mark))

 (define-obsolete-function-alias
diff --git a/src/buffer.c b/src/buffer.c
index 90c1542..d811515 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -5161,6 +5161,7 @@ init_buffer_once (void)
   bset_abbrev_table (&buffer_defaults, Qnil);
   bset_display_table (&buffer_defaults, Qnil);
   bset_undo_list (&buffer_defaults, Qnil);
+  bset_mark (&buffer_defaults, Qnil);
   bset_mark_active (&buffer_defaults, Qnil);
   bset_file_format (&buffer_defaults, Qnil);
   bset_auto_save_file_format (&buffer_defaults, Qt);
@@ -5219,6 +5220,7 @@ init_buffer_once (void)
   bset_major_mode (&buffer_local_flags, make_number (-1));
   bset_mode_name (&buffer_local_flags, make_number (-1));
   bset_undo_list (&buffer_local_flags, make_number (-1));
+  bset_mark (&buffer_local_flags, make_number (-1));
   bset_mark_active (&buffer_local_flags, make_number (-1));
   bset_point_before_scroll (&buffer_local_flags, make_number (-1));
   bset_file_truename (&buffer_local_flags, make_number (-1));
@@ -6124,6 +6126,9 @@ the changes between two undo boundaries as a single
step to be undone.

 If the value of the variable is t, undo information is not recorded.  */);

+  DEFVAR_PER_BUFFER ("mark", &BVAR (current_buffer, mark), Qnil,
+                    doc: /* The buffer's mark.  */);
+
   DEFVAR_PER_BUFFER ("mark-active", &BVAR (current_buffer, mark_active),
Qnil,
                     doc: /* Non-nil means the mark and region are
currently active in this buffer.  */);

diff --git a/test/automated/marker-tests.el b/test/automated/marker-tests.el
new file mode 100644
index 0000000..87ebf82
--- /dev/null
+++ b/test/automated/marker-tests.el
@@ -0,0 +1,59 @@
+;;; marker-tests.el --- tests for common markers such as the mark -*-
lexical-binding:t -*-
+
+;; Copyright (C) 2014 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <http://www.gnu.org/licenses/>.
+
+;;; Commentary:
+
+;;; Code:
+
+(ert-deftest marker-tests-mark-rings ()
+  (let ((buf1 (generate-new-buffer "buf1"))
+        (buf2 (generate-new-buffer "buf2")))
+    (set-buffer buf1)
+    (insert "abcdefg")
+    (push-mark 1)
+    (goto-char 2)
+    (push-mark)
+    (set-buffer buf2)
+    (insert "ABCDEFG")
+    (push-mark)
+    (set-buffer buf1)
+    (pop-global-mark)
+    (should (eq (current-buffer) (marker-buffer (mark-marker))))
+    (should (eq (current-buffer) buf2))
+    (goto-char 3)
+    (pop-mark) ; Shouldn't go anywhere, mark-ring empty
+    (should (= 3 (point)))
+    (pop-global-mark)
+    (should (eq (current-buffer) buf1))
+    (push-mark 4)
+    (should (= 4 (mark t)))
+    (goto-char 5)
+    (exchange-point-and-mark)
+    (should (= 4 (point)))
+    (should (= 5 (marker-position (mark-marker))))
+    (pop-mark)
+    (should (= 2 (mark t)))
+    (pop-mark)
+    (should (= 1 (mark t)))
+    (should (= 4 (point)))
+    (should (eq (current-buffer) buf1))))
+
+;;; marker-tests.el ends here
+
+
diff --git a/test/automated/undo-tests.el b/test/automated/undo-tests.el
index 8a963f1..a4ed95e 100644
--- a/test/automated/undo-tests.el
+++ b/test/automated/undo-tests.el
@@ -268,6 +268,46 @@
     (should (string= (buffer-string)
                      "This sentence corrupted?aaa"))))

+(ert-deftest undo-test-mark-adjustment ()
+  "Test that the mark's marker adjustment in undo history doesn't
+  obstruct undo in region from finding the correct change group.
+  Demonstrates bug 16818."
+  (with-temp-buffer
+    (buffer-enable-undo)
+    (transient-mark-mode 1)
+    (insert "First line\n")
+    (insert "Second line\n")
+    (undo-boundary)
+
+    (goto-char (point-min))
+    (insert "aaa")
+    (undo-boundary)
+
+    (undo)
+    (undo-boundary)
+
+    (goto-char (point-max))
+    (insert "bbb")
+    (undo-boundary)
+
+    (push-mark (point) t t)
+    (setq mark-active t)
+    (goto-char (- (point) 3))
+    (delete-forward-char 1)
+    (undo-boundary)
+
+    (insert "bbb")
+    (undo-boundary)
+
+    (goto-char (point-min))
+    (push-mark (point) t t)
+    (setq mark-active t)
+    (goto-char (+ (point) 3))
+    (undo)
+    (undo-boundary)
+
+    (should (string= (buffer-string) "aaaFirst line\nSecond line\nbbb"))))
+
 (defun undo-test-all (&optional interactive)
   "Run all tests for \\[undo]."
   (interactive "p")

[Message part 2 (text/html, inline)]

Message #29 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Wed, 12 Mar 2014 19:24:45 -0400

> At the end is a patch to implement this. I wrote the marker-tests to
> make sure I didn't break anything too badly, they pass with or without
> the rest of the patch. The new undo-test-mark-adjustment implements
> the recipe of this bug report. It fails with current trunk and passes
> with the patch.

Thinking more about this, I think this is fixing the symptom, but not
the cause.  The cause is that primitive-undo shouldn't blindly
obey a (MARKER . OFFSET) entry.  Instead it should only obey it if the
marker still points at the corresponding place.

Your patch works around the problem by trying to avoid moving the mark
(creating new markers each time instead) but that doesn't fix the
problem for the other markers.

Problem is: the undo log format as it stands does not record in
a reliable way what was the marker's position at that time, so it's not
easy to figure out whether the marker is still at the "same place"
or not.

This said, in practice, those (MARKER . OFFSET) entries are only
introduced for text deletion.  So we should normally find them
immediately after a (STRING . POS) and those (MARKER . OFFSET) should
only be applied if that MARKER was at POS before undoing the deletion
of STRING.

IOW, I think the right fix is to change primitive-undo's handling of
(STRING . POS) by first looking at subsequent (MARKER . OFFSET) entries
and dropping those whose MARKER is not currently at POS.

WDYT?


        Stefan

Message #32 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Wed, 12 Mar 2014 21:59:55 -0400

[Message part 1 (text/plain, inline)]

> Thinking more about this, I think this is fixing the symptom, but
> not the cause. The cause is that primitive-undo shouldn't blindly
> obey a (MARKER . OFFSET) entry. Instead it should only obey it if
> the marker still points at the corresponding place.

From what I can tell, the root cause is relocating markers to
unrelated locations in the buffer while another part of Emacs still
has a reference to it.

Your counter proposal is a step from the root. For instance, the
markers within the mark rings will not adjust correctly, whilst with
my patch they will.

I think your proposal sounds good nonetheless. I don't doubt there are
other markers that could get swept up into the undo history, become
relocated, and then annoy users of undo in region.

> (creating new markers each time instead)

My patch actually results in fewer markers created.

[Message part 2 (text/html, inline)]

Message #35 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Thu, 13 Mar 2014 09:24:28 -0400

>> Thinking more about this, I think this is fixing the symptom, but
>> not the cause. The cause is that primitive-undo shouldn't blindly
>> obey a (MARKER . OFFSET) entry. Instead it should only obey it if
>> the marker still points at the corresponding place.

> From what I can tell, the root cause is relocating markers to
> unrelated locations in the buffer while another part of Emacs still
> has a reference to it.

Not sure in which way what you're saying is different from what I'm saying.
Are we in violent agreement?

> Your counter proposal is a step from the root.  For instance, the
> markers within the mark rings will not adjust correctly, whilst with
> my patch they will.

Can you give an example incorrect adjustment?

>> (creating new markers each time instead)
> My patch actually results in fewer markers created.

Right, I didn't mean that creating new markers is a problem.


        Stefan

Message #38 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Thu, 13 Mar 2014 10:35:05 -0400

>> Your counter proposal is a step from the root.  For instance, the
>> markers within the mark rings will not adjust correctly, whilst with
>> my patch they will.
> Can you give an example incorrect adjustment?

Oh, I think I see: with the current code, the mark-ring gets copies of
markers and those copies are made "late", so if you do "C-SPC, some
deletion around point, and then C-SPC", a new marker for the first C-SPC
is pushed on the mark-ring, but the undo-log has an adjustment for
mark-marker rather than for that new marker.

IOW, I think the primitive-undo fix is needed and your fix is also needed.
I think your fix is too risky for the 24.4, tho (IOW, please wait for
the trunk to re-open before installing it).  The primitive-undo fix
should be safe enough for 24.4, so if you want to code this up and
install it, feel free.


        Stefan

Message #41 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Thu, 13 Mar 2014 12:55:57 -0400

[Message part 1 (text/plain, inline)]

> Oh, I think I see: with the current code, the mark-ring gets copies of
> markers and those copies are made "late", so if you do "C-SPC, some
> deletion around point, and then C-SPC", a new marker for the first C-SPC
> is pushed on the mark-ring, but the undo-log has an adjustment for
> mark-marker rather than for that new marker.

> IOW, I think the primitive-undo fix is needed and your fix is also
> needed.

Yes that's right. Parents cover their childrens' eyes at the violence
of our agreement.

I just discovered my patch has a bug. If I C-SPC first thing after
starting Emacs, an error is signaled because the mark is nil. I had
hoped the mark variable initialized equivalently except to make itself
available to Lisp. There are clearly holes in my understanding of the
initialization business. I'll debug that at a later time.

[Message part 2 (text/html, inline)]

Message #44 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Mon, 17 Mar 2014 19:05:34 -0400

[Message part 1 (text/plain, inline)]

> The primitive-undo fix should be safe enough for 24.4, so if you
> want to code this up and install it, feel free.

I have attached the patch for this, which I'll install if nothing
comes up from review.

[Message part 2 (text/html, inline)]

[undo-skip-markers.diff (text/plain, attachment)]

Message #47 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Stefan <monnier <at> iro.umontreal.ca>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Mon, 17 Mar 2014 20:02:43 -0400

>> The primitive-undo fix should be safe enough for 24.4, so if you
>> want to code this up and install it, feel free.
> I have attached the patch for this, which I'll install if nothing
> comes up from review.

Thanks.  A few comments below.

--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2229,6 +2229,7 @@
         (did-apply nil)
         (next nil))
     (while (> arg 0)
+      (let (del-pos)
       (while (setq next (pop list))     ;Exit inner loop at undo boundary.
         ;; Handle an integer by setting point to that value.
         (pcase next
@@ -2289,6 +2290,7 @@
            (when (let ((apos (abs pos)))
                    (or (< apos (point-min)) (> apos (point-max))))
              (error "Changes to be undone are outside visible portion of buffer"))
+             (setq del-pos pos)
            (if (< pos 0)
                (progn
                  (goto-char (- pos))
@@ -2303,11 +2305,14 @@
              (goto-char pos)))
           ;; (MARKER . OFFSET) means a marker MARKER was adjusted by OFFSET.
           (`(,(and marker (pred markerp)) . ,(and offset (pred integerp)))
-           (when (marker-buffer marker)
+             (when (and del-pos
+                        (integerp (marker-position marker))
+                        (= del-pos marker)
+                        (marker-buffer marker))
              (set-marker marker
                          (- marker offset)
                          (marker-buffer marker))))
-          (_ (error "Unrecognized entry in undo list %S" next))))
+            (_ (error "Unrecognized entry in undo list %S" next)))))
       (setq arg (1- arg)))
     ;; Make sure an apply entry produces at least one undo entry,
     ;; so the test in `undo' for continuing an undo series

The "move-after" markers will be at (+ del-pos (length inserted-text))
rather than at del-pos.  Also, a lone (MARKER . OFFSET) entry will
arbitrarily use some unrelated earlier del-pos.  Admittedly, such lone
(MARKER . OFFSET) entries should/will never happen, but I'd rather we
catch them and emit a warning than silently do something arbitrary.

IOW, I think we should only change the entry corresponding to a deletion
such that it directly handles all the immediately following
marker-adjustments (it can check them before doing the insertion, hence
using del-pos without having to care about insert-after vs
insert-before).  Then the current handling of marker-adjustments can be
changed to emit a warning.

> @@ -2351,18 +2354,30 @@ If we find an element that crosses an edge of this region,
>  we stop and ignore all further elements."
>    (let ((undo-list-copy (undo-copy-list buffer-undo-list))
>  	(undo-list (list nil))
> -	undo-adjusted-markers
> +        ;; The position of a deletion record (TEXT . POSITION) of the
> +        ;; current change group.
> +        ;;
> +        ;; This is used to check that marker adjustmenets are in the
> +        ;; region.  Bug 16818 describes why the marker's position is
> +        ;; not suitable.
> +        del-pos
>  	some-rejected
>  	undo-elt temp-undo-list delta)
>      (while undo-list-copy
>        (setq undo-elt (car undo-list-copy))
> +      ;; Update del-pos
> +      (if undo-elt
> +          (when (and (consp undo-elt) (stringp (car undo-elt)))
> +            (setq del-pos (cdr undo-elt)))
> +        ;; Undo boundary means new change group, so unset del-pos
> +        (setq del-pos nil))
>        (let ((keep-this
>  	     (cond ((and (consp undo-elt) (eq (car undo-elt) t))
>  		    ;; This is a "was unmodified" element.
>  		    ;; Keep it if we have kept everything thus far.
>  		    (not some-rejected))
>  		   (t
> -		    (undo-elt-in-region undo-elt start end)))))
> +		    (undo-elt-in-region undo-elt start end del-pos)))))
>  	(if keep-this
>  	    (progn
>  	      (setq end (+ end (cdr (undo-delta undo-elt))))

I'd have the same comment here, but if we emit a warning for sole
marker-adjustments in the "non-region" code, we don't really have to
worry about them here.

But this is also affected by the issue of "move-after" markers where
`del-pos' is not sufficient info since those markers won't be at del-pos
any more if they were at del-pos before we inserted the deleted text.


        Stefan

Message #50 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Wed, 19 Mar 2014 09:36:17 -0400

[Message part 1 (text/plain, inline)]

> The "move-after" markers will be at (+ del-pos (length
> inserted-text)) rather than at del-pos.

Right, thanks for catching that flaw.

> I'd have the same comment here, but if we emit a warning for sole
> marker-adjustments in the "non-region" code, we don't really have to
> worry about them here.

If you're saying changes under undo-make-selective-list are not
necessary, remember that currently it can create a list like:

  (nil (#<marker at 141 in *scratch*> . -3) (#<marker at 190 in *scratch*>
. -3) nil (aaa . 141) nil (141 . 144) nil)

I don't think the original recipe of this bug report should generate a
warning. Rather, I had undo-make-selective-list filter out the marker
adjustments so as the above list would have (aaa . 141) at the head.

> I think we should only change the entry corresponding to a deletion
> such that it directly handles all the immediately following
> marker-adjustments

They don't always immediately follow. An integer record can be between
them. For example, at the end of the undo-test-marker-adjustment-moved
test I posted previously, buffer-undo-list is:

  (nil (1 . 4) nil (abc . 1) 12 (#<marker at 7 in  *temp*-216909> . -1) nil
(1 . 12) (t . 0))

Also, (t sec-high sec-low microsec picosec) entries can be in between.
eg it was easy to bring about this buffer-undo-list:

  Value: (nil
   (#("   " 0 3
      (fontified t))
    . 12)
   (t 20985 26927 0 0)
   (#<marker at 12 in foo.py> . -2)
   (#<marker at 12 in foo.py> . -2)
   (#<marker in no buffer> . -3)
   (#<marker in no buffer> . -2))

I suppose some options are:

  * Implement your proposal but skip over the (t ...) and integer
    records
  * Restructure the C code so as marker adjustments are always
    immediately before deletion records
  * Revisit the approach of fixing markers that move to unrelated
    locations.

Let me know and thank you for your guidance.

[Message part 2 (text/html, inline)]

Message #53 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Wed, 19 Mar 2014 09:45:59 -0400

[Message part 1 (text/plain, inline)]

Clarifications:

>  * Implement your proposal but skip over the (t ...) and integer
    records

I mean "skip" only for the "look ahead to validate" part. Not to skip
completely.

>   * Restructure the C code so as marker adjustments are always
    immediately before deletion records

I meant "chronologically" before, not "before" in the list. I think
the desired ordering would be:

  Value: (nil
   (#("   " 0 3
      (fontified t))
    . 12)
   (#<marker at 12 in foo.py> . -2)
   (#<marker at 12 in foo.py> . -2)
   (#<marker in no buffer> . -3)
   (#<marker in no buffer> . -2)
   (t 20985 26927 0 0))

[Message part 2 (text/html, inline)]

Message #56 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Stefan <monnier <at> iro.umontreal.ca>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Wed, 19 Mar 2014 14:52:40 -0400

>> I'd have the same comment here, but if we emit a warning for sole
>> marker-adjustments in the "non-region" code, we don't really have to
>> worry about them here.
> If you're saying changes under undo-make-selective-list are not
> necessary, remember that currently it can create a list like:

No, I mean that it's OK to *assume* that any marker-adjustment we find
in the undo-region code is "right after a deletion".  Of course, that's
only relevant if that can help us simplify the code.

>> I think we should only change the entry corresponding to a deletion
>> such that it directly handles all the immediately following
>> marker-adjustments
> They don't always immediately follow. An integer record can be between
> them. For example, at the end of the undo-test-marker-adjustment-moved
> test I posted previously, buffer-undo-list is:
>   (nil (1 . 4) nil (abc . 1) 12 (#<marker at 7 in  *temp*-216909> . -1) nil
> (1 . 12) (t . 0))

Right, the integer record is indeed also added by the deletion, so
we should still consider the marker adjustments to "immediately follow".

>   * Implement your proposal but skip over the (t ...) and integer
>     records
>   * Restructure the C code so as marker adjustments are always
>     immediately before deletion records
>   * Revisit the approach of fixing markers that move to unrelated
>     locations.

I think the first option is best (hopefully, the set of "things that can
come between the deletion and the marker adjustments" won't keep growing).


        Stefan

Message #59 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Stefan <monnier <at> iro.umontreal.ca>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Wed, 19 Mar 2014 14:56:32 -0400

> I meant "chronologically" before, not "before" in the list. I think
> the desired ordering would be:

>   Value: (nil
>    (#("   " 0 3
>       (fontified t))
>     . 12)
>    (#<marker at 12 in foo.py> . -2)
>    (#<marker at 12 in foo.py> . -2)
>    (#<marker in no buffer> . -3)
>    (#<marker in no buffer> . -2)
>    (t 20985 26927 0 0))

Oh, right.  That would be good as well.  At least as good as option 1.
So I'd go with whichever of option 1 or 2 is simpler.

> * Revisit the approach of fixing markers that move to unrelated
>   locations.

Definitely not for 24.4.


        Stefan

Message #62 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Sun, 23 Mar 2014 18:49:32 -0400

[Message part 1 (text/plain, inline)]

>>   Value: (nil
>>    (#("   " 0 3
>>       (fontified t))
>>     . 12)
>>    (#<marker at 12 in foo.py> . -2)
>>    (#<marker at 12 in foo.py> . -2)
>>    (#<marker in no buffer> . -3)
>>    (#<marker in no buffer> . -2)
>>    (t 20985 26927 0 0))

> Oh, right. That would be good as well. At least as good as option 1.
> So I'd go with whichever of option 1 or 2 is simpler.

Attached is the new patch to solve this, implementing option 2 and
incorporating your feedback.

[Message part 2 (text/html, inline)]

[undo-marker-record.diff (text/plain, attachment)]

Message #65 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Stefan <monnier <at> IRO.UMontreal.CA>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Mon, 24 Mar 2014 09:03:50 -0400

> Attached is the new patch to solve this, implementing option 2 and
> incorporating your feedback.

Looks good, thank you very much.  Feel free to install into emacs-24,
but please see my comments below.


        Stefan


> +2014-03-13  Barry O'Reilly  <gundaetiapo <at> gmail.com>
> +	* markers.texi (Moving Marker Positions): The 2014-03-02 doc

Missing empty line between the two.

> +             (while (and (consp (car list))
> +                         (markerp (caar list))

(markerp (car-safe (car list)))

> +               (and (eq (marker-buffer (caar list))
> +                        (current-buffer))
> +                    (integerp (marker-position (caar list)))

I think this `integerp' test is redundant (marker-position can only
return nil or an integer and if it returns nil, then marker-buffer also
returns nil).

>            ;; (MARKER . OFFSET) means a marker MARKER was adjusted by OFFSET.
>            (`(,(and marker (pred markerp)) . ,(and offset (pred integerp)))
> -           (when (marker-buffer marker)
> -             (set-marker marker
> -                         (- marker offset)
> -                         (marker-buffer marker))))
> +           (warn "Encountered %S entry in undo list with no matching (TEXT . POS) entry"
> +                 next))

I think I'd add the warning without removing the `set-marker', just to
be conservative.

> +                (when (and (consp undo-elt)
> +                           (stringp (car undo-elt))

(stringp (car-safe undo-elt))

> +                           (integerp (cdr undo-elt)))
> +                  (let ((list-i (cdr undo-list-copy)))
> +                    (while (markerp (caar list-i))

I think you need (markerp (car-safe (car list-i))) because (car list-i)
may be an integer.

Also, I'd try to avoid repeatedly calling (car list-i) in the body of
the `while' loop(s) by let-binding it.  I'd probably do it via `pop', as
in

  (while (...)
    (let ((elem (pop undo-list)))
      ...))

>  	((and (consp undo-elt) (markerp (car undo-elt)))
> -	 ;; This is a marker-adjustment element (MARKER . ADJUSTMENT).
> -	 ;; See if MARKER is inside the region.
> -	 (let ((alist-elt (assq (car undo-elt) undo-adjusted-markers)))
> -	   (unless alist-elt
> -	     (setq alist-elt (cons (car undo-elt)
> -				   (marker-position (car undo-elt))))
> -	     (setq undo-adjusted-markers
> -		   (cons alist-elt undo-adjusted-markers)))
> -	   (and (cdr alist-elt)
> -		(>= (cdr alist-elt) start)
> -		(<= (cdr alist-elt) end))))
> +	 ;; (MARKER . ADJUSTMENT)
> +         nil)

It would also be more conservative to keep this unchanged, but I think
I agree with you here that we don't need to be *that* conservative.

> -  record_delete (beg, make_buffer_string (beg, beg + length, 1));
> +  record_delete (beg, make_buffer_string (beg, beg + length, 1), false);

Begs the question: why didn't (don't) we record marker adjustments here
(and other similar places)?


        Stefan

Message #68 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan <monnier <at> iro.umontreal.ca>, toby-undo-tree <at> dr-qubit.org
Cc: 16818 <at> debbugs.gnu.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Mon, 24 Mar 2014 18:10:30 -0400

[Message part 1 (text/plain, inline)]

[Adding Toby, since I mention undo-tree.]

> It would also be more conservative to keep this unchanged, but I
> think I agree with you here that we don't need to be *that*
> conservative.

This is how I addressed that:

diff --git a/lisp/simple.el b/lisp/simple.el
index 4a3a89c..6b5031e 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -2375,6 +2375,10 @@ we stop and ignore all further elements."
                    ;; This is a "was unmodified" element.
                    ;; Keep it if we have kept everything thus far.
                    (not some-rejected))
+                   ;; Skip over marker adjustments, instead relying on
+                   ;; finding them after (TEXT . POS) elements
+                   ((markerp (car-safe undo-elt))
+                    nil)
                   (t
                    (undo-elt-in-region undo-elt start end)))))
        (if keep-this
@@ -2461,7 +2465,7 @@ marker adjustment's corresponding (TEXT . POS)
element."
              (<= (abs (cdr undo-elt)) end)))
        ((and (consp undo-elt) (markerp (car undo-elt)))
         ;; (MARKER . ADJUSTMENT)
-         nil)
+         (<= start (car undo-elt) end))
        ((null (car undo-elt))
         ;; (nil PROPERTY VALUE BEG . END)
         (let ((tail (nthcdr 3 undo-elt)))

It maintains the same return from undo-elt-in-region for marker
adjustments as before.

I took a look at undo-tree. It doesn't use undo-make-selective-list,
but does use undo-elt-in-region. I guess that means this particular
bug won't be fixed in undo-tree for regional undos, until it also
scans forward from (TEXT . POS) as this patch's
undo-make-selective-list does.

> Begs the question: why didn't (don't) we record marker adjustments
> here (and other similar places)?

I considered pursuing that question earlier, but opted for those
callers to behave as before, at least for now.

There's a related comment nearby one of the callers:

/* Note that this does not yet handle markers quite right.
   Also it needs to record a single undo-entry that does a replacement
   rather than a separate delete and insert.
   That way, undo will also handle markers properly.

   But if MARKERS is 0, don't relocate markers.  */

I've attached the updated patch with your comments incorporated. I'll
install soon.

[Message part 2 (text/html, inline)]

[undo-marker-record-20140324.diff (text/plain, attachment)]

Message #71 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Stefan <monnier <at> IRO.UMontreal.CA>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org, toby-undo-tree <at> dr-qubit.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Mon, 24 Mar 2014 21:28:59 -0400

>> Begs the question: why didn't (don't) we record marker adjustments
>> here (and other similar places)?
> I considered pursuing that question earlier, but opted for those
> callers to behave as before, at least for now.

Yes, it's definitely not for `emacs-24' in any case.

> I've attached the updated patch with your comments incorporated.  I'll
> install soon.

Thank you,


        Stefan

Message #74 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Barry OReilly <gundaetiapo <at> gmail.com>
To: Stefan <monnier <at> iro.umontreal.ca>
Cc: 16818 <at> debbugs.gnu.org, toby-undo-tree <at> dr-qubit.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in undo
 history relocated)
Date: Mon, 24 Mar 2014 22:32:12 -0400

[Message part 1 (text/plain, inline)]

Rev 116855. Leaving bug open because of mark ring issue.

[Message part 2 (text/html, inline)]

Message #77 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org, Stefan <monnier <at> iro.umontreal.ca>,
 toby-undo-tree <at> dr-qubit.org
Subject: Re: bug#16818: Acknowledgement (Undo in region after markers in
 undo history relocated)
Date: Wed, 09 Sep 2020 14:25:00 +0200

Barry OReilly <gundaetiapo <at> gmail.com> writes:

> Rev 116855. Leaving bug open because of mark ring issue.

Skimming this bug report, it's not quite clear what the "mark ring"
issue is.  Is there more to be fixed here, or should the bug report be
closed?

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

Message #82 received at 16818 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Barry OReilly <gundaetiapo <at> gmail.com>
Cc: 16818 <at> debbugs.gnu.org, Stefan <monnier <at> iro.umontreal.ca>,
 toby-undo-tree <at> dr-qubit.org
Subject: Re: bug#16818: Undo in region after markers in undo history relocated
Date: Sun, 06 Dec 2020 16:57:21 +0100

Lars Ingebrigtsen <larsi <at> gnus.org> writes:

> Barry OReilly <gundaetiapo <at> gmail.com> writes:
>
>> Rev 116855. Leaving bug open because of mark ring issue.
>
> Skimming this bug report, it's not quite clear what the "mark ring"
> issue is.  Is there more to be fixed here, or should the bug report be
> closed?

More information was requested, but none was received, so I'm closing
this bug report.  If this is still an issue, please respond to the
debbugs address and we'll reopen the report.

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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.