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

[Matt Armstrong]

Matt Armstrong <matt@rfc20.org> writes:

> Eason Huang <aqua0210@foxmail.com> writes:
>
>> I can reproduce the following steps, may be you can have try:
>>
>> 1. emacs -Q , launch Emacs
>> 2. M-x org-capture, and then type `t` (task)
>>    Now the buffer CAPTURE-.notes will open with contents as bellow:
>>
>>    ** TODO (now the cursor is here, you can type some characters, such as 
>> "test1", and then type C-C C-c )
>>   [2022-11-03 Thu]
>>
>> 3. C-x b, switch-to-buffer .notes
>>   The contents is as below:
>> * Tasks
>> ** TODO test1(move cusror here and type TAB)
>>   [2022-11-03 Thu]
>>
>> 4. Now the .notes buffer is like this:
>> * Tasks
>> ** TODO test1...
>> 5. Try step 2 again, now you will see the issue. Now the .notes buffer looks 
>> like this:
>> * Tasks
>> ** TODO test1...TODO
>>   [2022-11-03 Thu]
>>   test1
>
> I can reproduce this too.  Note that this is a new set of repro steps
> that are easier to do after "emacs -Q".  Thanks Eason!

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

Stefan, this and a couple other minor patches are in
https://git.sr.ht/~matta/emacs/log/scratch/matta/for_stefan

Now, with respect to Eason's repro above, Emacs is "bug/behavior
compatible" with itself before the noverlay merge to master.

There may still be an org-mode bug here.  In step 3 above org puts an
invisible overlay with "ellipsis" over the "TODO test1" heading and
content.  When it later appends the new heading via org-capture it does
so with `insert-before-markers' which causes the end of the "ellipsis"
overlay to cover the new entry too, which later confuses other org
functions until the heading is expanded and the overlay is deleted.
This seems wrong and doesn't happen in Emacs 27.  But, it does happen as
of commit 69121c33e4a11805bf6438131c8aec72411a0e5d (the predecessor to
Stefan's noverlay->master merge commit).

>From 23715529c7b87dfbed7810f674a161ddb4fbd909 Mon Sep 17 00:00:00 2001
From: Matt Armstrong <matt@rfc20.org>
Date: Thu, 3 Nov 2022 20:57:29 -0700
Subject: [PATCH 3/3] Fix insert-before-markers for the new overlays
 implementation.

* src/itree.h: Add a before_markers arg to itree_insert_gap.
* src/itree.c (itree_insert_gap): Use it, fixing bug#58928.
* src/lisp.h: Add a before_markers arg to adjust_overlays_for_insert.
* src/buffer.c (adjust_overlays_for_insert): Pass it through to
itree_insert_gap.
* src/insdel.c (insert_1_both): Pass before_markers through to
adjust_overlays_for_insert.
(insert_from_string_1): Ditto.
(insert_from_gap): Pass before_markers=false to
adjust_overlays_for_insert.
(insert_from_buffer_1): ditto.
(adjust_after_replace): ditto.
(replace_range): ditto.
(replace_range_2): ditto.
* test/src/buffer-tests.el (buffer-tests-overlay-string): New helper.
(test-overlay-insert-before-markers-at-start): New test.
(test-overlay-insert-before-markers-at-end): New test.
---
 src/buffer.c             | 11 ++++---
 src/insdel.c             | 14 ++++-----
 src/itree.c              | 28 +++++++++++-------
 src/itree.h              |  3 +-
 src/lisp.h               |  2 +-
 test/src/buffer-tests.el | 62 ++++++++++++++++++++++++++++++++++++++++
 6 files changed, 97 insertions(+), 23 deletions(-)

diff --git a/src/buffer.c b/src/buffer.c
index 3129aa2890e..5e15e9e1ec3 100644
--- a/src/buffer.c
+++ b/src/buffer.c
@@ -3454,20 +3454,23 @@ overlay_strings (ptrdiff_t pos, struct window *w, 
unsigned char **pstr)
 
 
 void
-adjust_overlays_for_insert (ptrdiff_t pos, ptrdiff_t length)
+adjust_overlays_for_insert (ptrdiff_t pos, ptrdiff_t length,
+                           bool before_markers)
 {
   if (!current_buffer->indirections)
-    itree_insert_gap (current_buffer->overlays, pos, length);
+    itree_insert_gap (current_buffer->overlays, pos, length,
+                     before_markers);
   else
     {
       struct buffer *base = current_buffer->base_buffer
                             ? current_buffer->base_buffer
                             : current_buffer;
       Lisp_Object tail, other;
-      itree_insert_gap (base->overlays, pos, length);
+      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);
+         itree_insert_gap (XBUFFER (other)->overlays, pos, length,
+                           before_markers);
     }
 }
 
diff --git a/src/insdel.c b/src/insdel.c
index 6d56a76c77a..33958f53e9c 100644
--- a/src/insdel.c
+++ b/src/insdel.c
@@ -917,7 +917,7 @@ insert_1_both (const char *string,
   if (Z - GPT < END_UNCHANGED)
     END_UNCHANGED = Z - GPT;
 
-  adjust_overlays_for_insert (PT, nchars);
+  adjust_overlays_for_insert (PT, nchars, before_markers);
   adjust_markers_for_insert (PT, PT_BYTE,
                             PT + nchars, PT_BYTE + nbytes,
                             before_markers);
@@ -1043,7 +1043,7 @@ insert_from_string_1 (Lisp_Object string, ptrdiff_t pos, 
ptrdiff_t pos_byte,
   if (Z - GPT < END_UNCHANGED)
     END_UNCHANGED = Z - GPT;
 
-  adjust_overlays_for_insert (PT, nchars);
+  adjust_overlays_for_insert (PT, nchars, before_markers);
   adjust_markers_for_insert (PT, PT_BYTE, PT + nchars,
                             PT_BYTE + outgoing_nbytes,
                             before_markers);
@@ -1115,7 +1115,7 @@ insert_from_gap (ptrdiff_t nchars, ptrdiff_t nbytes, bool 
text_at_gap_tail)
 
   insert_from_gap_1 (nchars, nbytes, text_at_gap_tail);
 
-  adjust_overlays_for_insert (ins_charpos, nchars);
+  adjust_overlays_for_insert (ins_charpos, nchars, false);
   adjust_markers_for_insert (ins_charpos, ins_bytepos,
                             ins_charpos + nchars, ins_bytepos + nbytes, 0);
 
@@ -1257,7 +1257,7 @@ insert_from_buffer_1 (struct buffer *buf,
   if (Z - GPT < END_UNCHANGED)
     END_UNCHANGED = Z - GPT;
 
-  adjust_overlays_for_insert (PT, nchars);
+  adjust_overlays_for_insert (PT, nchars, false);
   adjust_markers_for_insert (PT, PT_BYTE, PT + nchars,
                             PT_BYTE + outgoing_nbytes,
                             0);
@@ -1323,7 +1323,7 @@ adjust_after_replace (ptrdiff_t from, ptrdiff_t from_byte,
   record_insert (from, len);
 
   if (len > nchars_del)
-    adjust_overlays_for_insert (from, len - nchars_del);
+    adjust_overlays_for_insert (from, len - nchars_del, false);
   else if (len < nchars_del)
     adjust_overlays_for_delete (from, nchars_del - len);
 
@@ -1513,7 +1513,7 @@ replace_range (ptrdiff_t from, ptrdiff_t to, Lisp_Object 
new,
   /* Adjust the overlay center as needed.  This must be done after
      adjusting the markers that bound the overlays.  */
   adjust_overlays_for_delete (from, nchars_del);
-  adjust_overlays_for_insert (from, inschars);
+  adjust_overlays_for_insert (from, inschars, false);
 
   offset_intervals (current_buffer, from, inschars - nchars_del);
 
@@ -1648,7 +1648,7 @@ replace_range_2 (ptrdiff_t from, ptrdiff_t from_byte,
      adjusting the markers that bound the overlays.  */
   if (nchars_del != inschars)
     {
-      adjust_overlays_for_insert (from, inschars);
+      adjust_overlays_for_insert (from, inschars, false);
       adjust_overlays_for_delete (from + inschars, nchars_del);
     }
 
diff --git a/src/itree.c b/src/itree.c
index bd4e8cc5740..0b56feed43f 100644
--- a/src/itree.c
+++ b/src/itree.c
@@ -1190,11 +1190,14 @@ 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,
-                 ptrdiff_t pos, ptrdiff_t length)
+itree_insert_gap (struct itree_tree *tree, ptrdiff_t pos,
+                 ptrdiff_t length, bool before_markers)
 {
   if (!tree || length <= 0 || tree->root == NULL)
     return;
@@ -1202,14 +1205,16 @@ itree_insert_gap (struct itree_tree *tree,
 
   /* FIXME: Don't allocate iterator/stack anew every time. */
 
-  /* Nodes with front_advance starting at pos may mess up the tree
-     order, so we need to remove them first. */
+  /* Nodes starting at pos with front_advance (or before_markers) may
+     mess up the tree order, so we need to remove them first. */
   struct interval_stack *saved = interval_stack_create (0);
   struct itree_node *node = NULL;
   ITREE_FOREACH (node, tree, pos, pos + 1, PRE_ORDER)
     {
-      if (node->begin == pos && node->front_advance
-         && (node->begin != node->end || node->rear_advance))
+      if (node->begin == pos
+         && (node->front_advance || before_markers)
+         && (node->begin != node->end || node->rear_advance
+             || before_markers))
        interval_stack_push (saved, node);
     }
   for (int i = 0; i < saved->length; ++i)
@@ -1246,7 +1251,9 @@ itree_insert_gap (struct itree_tree *tree,
          /* node->begin == pos implies no front-advance. */
          if (node->begin > pos)
            node->begin += length;
-         if (node->end > pos || (node->end == pos && node->rear_advance))
+         if (node->end > pos
+             || (node->end == pos
+                 && (node->rear_advance || before_markers)))
            {
              node->end += length;
              eassert (node != NULL);
@@ -1256,7 +1263,8 @@ itree_insert_gap (struct itree_tree *tree,
       interval_stack_destroy (stack);
     }
 
-  /* Reinsert nodes starting at POS having front-advance. */
+  /* Reinsert nodes starting at POS with front-advance (or
+     before_markers). */
   uintmax_t notick = tree->otick;
   nodeptr_and_flag nav;
   while ((nav = interval_stack_pop (saved),
@@ -1264,7 +1272,7 @@ itree_insert_gap (struct itree_tree *tree,
     {
       eassert (node->otick == ootick);
       node->begin += length;
-      if (node->end != pos || node->rear_advance)
+      if (node->end != pos || (node->rear_advance || before_markers))
        node->end += length;
       node->otick = notick;
       interval_tree_insert (tree, node);
diff --git a/src/itree.h b/src/itree.h
index c6b68d36672..3ef9e76812a 100644
--- a/src/itree.h
+++ b/src/itree.h
@@ -119,7 +119,8 @@ #define ITREE_H
                          ptrdiff_t, ptrdiff_t);
 extern struct itree_node *itree_remove (struct itree_tree *,
                                        struct itree_node *);
-extern void itree_insert_gap (struct itree_tree *, ptrdiff_t, ptrdiff_t);
+extern void itree_insert_gap (struct itree_tree *, ptrdiff_t,
+                             ptrdiff_t, bool);
 extern void itree_delete_gap (struct itree_tree *, ptrdiff_t, ptrdiff_t);
 
 /* Iteration functions.  Almost all code should use ITREE_FOREACH
diff --git a/src/lisp.h b/src/lisp.h
index d87f9549382..472472d87f8 100644
--- a/src/lisp.h
+++ b/src/lisp.h
@@ -4690,7 +4690,7 @@ XMODULE_FUNCTION (Lisp_Object o)
 extern bool mouse_face_overlay_overlaps (Lisp_Object);
 extern Lisp_Object disable_line_numbers_overlay_at_eob (void);
 extern AVOID nsberror (Lisp_Object);
-extern void adjust_overlays_for_insert (ptrdiff_t, ptrdiff_t);
+extern void adjust_overlays_for_insert (ptrdiff_t, ptrdiff_t, bool);
 extern void adjust_overlays_for_delete (ptrdiff_t, ptrdiff_t);
 extern void fix_start_end_in_overlays (ptrdiff_t, ptrdiff_t);
 extern void report_overlay_modification (Lisp_Object, Lisp_Object, bool,
diff --git a/test/src/buffer-tests.el b/test/src/buffer-tests.el
index a4a1f609fd2..c8bd86ca9f1 100644
--- a/test/src/buffer-tests.el
+++ b/test/src/buffer-tests.el
@@ -1165,6 +1165,68 @@ test-delete-all-overlay-1
     (should-not (delete-all-overlays))))
 
 
+;; +==========================================================================+
+;; | insert-before-markers and overlays
+;; +==========================================================================+
+
+(defun buffer-tests-overlay-string (overlay)
+  (buffer-substring-no-properties
+   (overlay-start overlay)
+   (overlay-end overlay)))
+
+(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."
+  (with-temp-buffer
+    (insert "1234")
+    (let ((front-advance (make-overlay 2 3 nil t))
+          (default-advance (make-overlay 2 3 nil nil)))
+      (goto-char 2)
+      (insert-before-markers "x")
+      (should (equal "2" (buffer-tests-overlay-string front-advance)))
+      (should (equal "2" (buffer-tests-overlay-string default-advance))))))
+
+(ert-deftest test-overlay-insert-before-markers-at-end ()
+  "`insert-before-markers' always advances an overlay's start.
+Test both front-advance and non-front-advance overlays."
+  (with-temp-buffer
+    (insert "1234")
+    (let ((rear-advance (make-overlay 2 3 nil nil t))
+          (default-advance (make-overlay 2 3 nil nil nil)))
+      (goto-char 3)
+      (insert-before-markers "x")
+      (should (equal "2x" (buffer-tests-overlay-string rear-advance)))
+      (should (equal "2x" (buffer-tests-overlay-string default-advance))))))
+
+;; (ert-deftest test-overlay-insert-before-markers-at-start ()
+;;   (with-temp-buffer
+;;     (insert "1234")
+;;     (let ((front-advance (make-overlay 2 3 nil 'front-advance))
+;;           (default-advance (make-overlay 2 3 nil nil)))
+;;       (goto-char 2)
+;;       (insert-before-markers "x")
+;;       (should (equal -1 (overlay-start front-advance)))
+;;       (should (equal -1 (overlay-start default-advance)))
+;;       (should (equal -1 (overlay-end overlay)))
+;;       (should (equal -1 (overlay-end overlay)))
+;;       )))
+
+;; (ert-deftest test-overlay-insert-before-markers-empty ()
+;;   (with-temp-buffer
+;;     (insert "1234")
+;;     (let ((overlay (make-overlay 2 2)))
+;;       (goto-char 2)
+;;       (insert-before-markers "x")
+;;       (should (equal 3 (overlay-end overlay))))))
+
+;; (ert-deftest test-overlay-insert-before-markers-non-empty ()
+;;   (with-temp-buffer
+;;     (insert "1234")
+;;     (let ((overlay (make-overlay 2 3)))
+;;       (goto-char 3)
+;;       (insert-before-markers "x")
+;;       (should (equal 4 (overlay-end overlay))))))
+
 ;; +==========================================================================+
 ;; | get-pos-property
 ;; +==========================================================================+
-- 
2.35.1