bug#58928: 29.0.50; overlays in org-mode are disrupted after call `org-capture`

[Matt Armstrong <matt@rfc20.org>]

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

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

>> See attached fix to fix the bug found by Eason's steps quoted above.
>> The bug was that we ignored the "before markers" part of
>> `insert-before-markers'.
>
> I pushed a similar fix a few hours earlier, sorry :-)

Your fix was better!  ;)


>> Stefan, this and a couple other minor patches are in
>> https://git.sr.ht/~matta/emacs/log/scratch/matta/for_stefan
>
> Could you rebase the remaining minor patches?

Done (patches here and at my sr.ht).  Basically, I rewrote the test in a
way that was clearer, at lest for me, and more exhaustive.  Moved them
down to the other tests related to inserting and moving overlays.  Used
explicit buffer positions instead of (point) as a relative reference
(which confused me and seemed unrelated, since neither
`insert-before-markers` nor the overlay code is ever concerned with
(point).  If you don't like that, we I think we should still tweak the
one new test to not attempt (goto-char (2+ (point))) when point is at
eob.  :-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Tweak-the-overlay-related-insert-before-markers-test.patch --]
[-- Type: text/x-diff, Size: 4378 bytes --]

From 4e66e39cdcd27d04f21d2459516d36ea22727bca Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Fri, 4 Nov 2022 15:02:17 -0700
Subject: [PATCH 1/2] Tweak the overlay related `insert-before-markers' tests

* test/src/buffer-tests.el (test-overlay-insert-before-markers-empty):
Move code down to the other tests related to insertion.  Test all
front/rear insert combinations.  To make the test more clear, at least
to me, hard code all character positions.
(test-overlay-insert-before-markers-at-start): For both front-advance
modes verify that `insert-before-markers' at and overlay's start
advances it.
(test-overlay-insert-before-markers-at-end): For both rear-advance
modes test that `insert-before-markers' at an overlay's end advances
it.
(test-overlay-insert-before-markers-non-empty): Delete, replaced by
the two tests above.
---
 test/src/buffer-tests.el | 61 +++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/test/src/buffer-tests.el b/test/src/buffer-tests.el
index a39d7d51de1..0e9e84ef7a1 100644
--- a/test/src/buffer-tests.el
+++ b/test/src/buffer-tests.el
@@ -528,28 +528,6 @@ K
 (deftest-overlay-start/end-1 L (1 0) (1 1))
 (deftest-overlay-start/end-1 M (0 0) (1 1))
 
-(ert-deftest test-overlay-insert-before-markers-empty ()
-  (with-temp-buffer
-    (insert "1234")
-    (goto-char (1+ (point-min)))
-    (let ((overlay (make-overlay (point) (point))))
-      (insert-before-markers "x")
-      (should (equal (point) (overlay-end overlay)))
-      (should (equal (point) (overlay-start overlay))))))
-
-(ert-deftest test-overlay-insert-before-markers-non-empty ()
-  (with-temp-buffer
-    (insert "1234")
-    (goto-char (+ 2 (point)))
-    (let ((overlay (make-overlay (1- (point)) (point))))
-      (insert-before-markers "x")
-      (should (equal (point) (overlay-end overlay)))
-      (should (equal (- (point) 2) (overlay-start overlay)))
-      (forward-char -2)
-      (insert-before-markers "y")
-      (should (equal (+ 2 (point)) (overlay-end overlay)))
-      (should (equal (point) (overlay-start overlay))))))
-
 (ert-deftest test-overlay-start/end-2 ()
   (should-not (overlay-start (with-temp-buffer (make-overlay 1 1))))
   (should-not (overlay-end (with-temp-buffer (make-overlay 1 1)))))
@@ -1315,7 +1293,46 @@ test-moving-insert-2
       (delete-overlay left)
       (should (= 2 (length (overlays-in 1 (point-max))))))))
 
+;; +==========================================================================+
+;; | Moving overlays with insert-before-markers
+;; +==========================================================================+
 
+(ert-deftest test-overlay-insert-before-markers-at-start ()
+  "`insert-before-markers' always advances an overlay's start.
+Test both front-advance and non-front-advance overlays."
+  (dolist (front-advance '(nil t))
+    (ert-info ((format "front-advance %S" front-advance))
+      (with-temp-buffer
+        (insert "1234")
+        (let ((overlay (make-overlay 2 3 nil front-advance nil)))
+          (goto-char 2)
+          (insert-before-markers "x")
+          (should (equal 3 (overlay-start overlay)))
+          (should (equal 4 (overlay-end overlay))))))))
+
+(ert-deftest test-overlay-insert-before-markers-at-end ()
+  "`insert-before-markers' always advances an overlay's end.
+Test both rear-advance and non-rear-advance overlays."
+  (dolist (rear-advance '(nil t))
+    (ert-info ((format "rear-advance %S" rear-advance))
+      (with-temp-buffer
+        (insert "1234")
+        (let ((overlay (make-overlay 2 3 nil nil rear-advance)))
+          (goto-char 3)
+          (insert-before-markers "x")
+          (should (equal 2 (overlay-start overlay)))
+          (should (equal 4 (overlay-end overlay))))))))
+
+(ert-deftest test-overlay-insert-before-markers-empty ()
+  (dolist (advance-args '((nil nil) (t nil) (nil t) (t t)))
+    (ert-info ((format "advance args %S" advance-args))
+      (with-temp-buffer
+        (insert "1234")
+        (let ((overlay (apply #'make-overlay 2 2 nil advance-args)))
+          (goto-char 2)
+          (insert-before-markers "x")
+          (should (equal 3 (overlay-start overlay)))
+          (should (equal 3 (overlay-end overlay))))))))
 
 ;; +==========================================================================+
 ;; | Moving by deletions
-- 
2.35.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Minor-tweaks-to-the-fix-for-insert-before-markers-ov.patch --]
[-- Type: text/x-diff, Size: 1598 bytes --]

From e7e66c30430b4e331a9285159851492fd4c78c3c Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Fri, 4 Nov 2022 15:24:40 -0700
Subject: [PATCH 2/2] Minor tweaks to the fix for `insert-before-markers'
 overlay fix

(bug#58928)

* src/buffer.c (adjust_overlays_for_insert): wrap to less than 80
chars.
* src/itree.c: document BEFORE_MARKERS.
---
 src/buffer.c | 3 ++-
 src/itree.c  | 5 ++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 745e62f53f7..390ccff5c8a 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -3467,7 +3467,8 @@ adjust_overlays_for_insert (ptrdiff_t pos, ptrdiff_t length, bool before_markers
       itree_insert_gap (base->overlays, pos, length, before_markers);
       FOR_EACH_LIVE_BUFFER (tail, other)
         if (XBUFFER (other)->base_buffer == base)
-          itree_insert_gap (XBUFFER (other)->overlays, pos, length, before_markers);
+	  itree_insert_gap (XBUFFER (other)->overlays, pos, length,
+			    before_markers);
     }
 }
 
diff --git a/src/itree.c b/src/itree.c
index cd37da18b89..b744b8423a2 100644
--- a/src/itree.c
+++ b/src/itree.c
@@ -1183,7 +1183,10 @@ itree_iterator_finish (struct itree_iterator *iter)
 
 /* Insert a gap at POS of length LENGTH expanding all intervals
    intersecting it, while respecting their rear_advance and
-   front_advance setting. */
+   front_advance setting.
+
+   When BEFORE_MARKERS, all overlays beginning/ending at POS are
+   treated as if their front_advance/rear_advance was true. */
 
 void
 itree_insert_gap (struct itree_tree *tree,
-- 
2.35.1