bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Ian Dunn]

When I use `revert-buffer' on a buffer for a file encrypted with GnuPG, every marker in the buffer is moved to the end of the buffer.  A normal file restores markers to their original positions after reverting.

To replicate:

1. Open a GnuPG encrypted file
2. Run the following:
   (setq test-marker (make-marker))
   (move-marker test-marker (point-marker))
   ;; test-marker points to `point'
3. Revert the buffer
   ;; test-marker now points to the end of the buffer

-- 
Ian Dunn

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Lars Ingebrigtsen]

Ian Dunn <dunni@gnu.org> writes:

> When I use `revert-buffer' on a buffer for a file encrypted with
> GnuPG, every marker in the buffer is moved to the end of the buffer.
> A normal file restores markers to their original positions after
> reverting.
>
> To replicate:
>
> 1. Open a GnuPG encrypted file
> 2. Run the following:
>    (setq test-marker (make-marker))
>    (move-marker test-marker (point-marker))
>    ;; test-marker points to `point'
> 3. Revert the buffer
>    ;; test-marker now points to the end of the buffer

I can confirm that this bug is still present in Emacs 27 -- but only if
point is not at point-min, oddly enough.

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

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>> 1. Open a GnuPG encrypted file
>> 2. Run the following:
>>    (setq test-marker (make-marker))
>>    (move-marker test-marker (point-marker))
>>    ;; test-marker points to `point'
>> 3. Revert the buffer
>>    ;; test-marker now points to the end of the buffer

I took a stab at this with the patch below (which is the wrong solution
because it doesn't clear the undo list etc).

But I don't quite understand why this is so much work.  :-)

The problem is twofold: The first is in epa.el itself, which deletes and
then inserts, which destroys the markers.

The second is the call to `decode-coding-region' in
`decode-coding-inserted-region', which also destroys the markers.

The third is that the interface to the new replace-buffer-contents
function is really awkward -- it only takes a buffer as its SOURCE,
which means that if you want to feed it something, you have to skip
around to temporary buffers instead of feeding it a string, which would
be natural.  You have to be a with-temp-buffer/with-current-buffer/let
contortionist to get it to be safe.

And replace-buffer-contents does so much more than just restoring
markers, anyway, so I'm not sure it's the right solution here, anyway.

Would anybody mind if I just write a `with-saved-markers' macro in
subr-x, which would make all these problems to away and make the
solution a two-liner in epa itself?

(Unless such a macro exists somewhere already.)

diff --git a/lisp/epa-file.el b/lisp/epa-file.el
index d9886d3d67..03f53c40a9 100644
--- a/lisp/epa-file.el
+++ b/lisp/epa-file.el
@@ -24,6 +24,7 @@
 
 (require 'epa)
 (require 'epa-hook)
+(require 'mule)
 
 (defcustom epa-file-cache-passphrase-for-symmetric-encryption nil
   "If non-nil, cache passphrase for symmetric encryption.
@@ -102,16 +103,21 @@ epa-file-run-real-handler
     (apply operation args)))
 
 (defun epa-file-decode-and-insert (string file visit beg end replace)
-  (if (fboundp 'decode-coding-inserted-region)
-      (save-restriction
-	(narrow-to-region (point) (point))
-	(insert string)
-	(decode-coding-inserted-region
-	 (point-min) (point-max)
-	 (substring file 0 (string-match epa-file-name-regexp file))
-	 visit beg end replace))
-    (insert (epa-file--decode-coding-string string (or coding-system-for-read
-						       'undecided)))))
+  (let ((buffer (current-buffer)))
+    (with-temp-buffer
+      (insert string)
+      (let ((coding (decode-coding-inserted-region-coding-system
+                     (substring
+                      file 0 (string-match epa-file-name-regexp file))
+                     visit beg end replace)))
+        (if coding
+	    (decode-coding-region (point-min) (point-max) coding)
+	  (setq last-coding-system-used coding))
+        (let ((temp (current-buffer)))
+          (with-current-buffer buffer
+            (if replace
+                (replace-buffer-contents temp)
+              (insert-buffer-substring temp))))))))
 
 (defvar epa-file-error nil)
 (defun epa-file--find-file-not-found-function ()
@@ -147,8 +153,6 @@ epa-file-insert-file-contents
 	   (format "Decrypting %s" file)))
     (unwind-protect
 	(progn
-	  (if replace
-	      (goto-char (point-min)))
 	  (condition-case error
 	      (setq string (epg-decrypt-file context local-file nil))
 	    (error
@@ -187,12 +191,9 @@ epa-file-insert-file-contents
 	    ;; really edit the buffer.
 	    (let ((buffer-file-name
 		   (if visit nil buffer-file-name)))
-	      (save-restriction
-		(narrow-to-region (point) (point))
-		(epa-file-decode-and-insert string file visit beg end replace)
-		(setq length (- (point-max) (point-min))))
-	      (if replace
-		  (delete-region (point) (point-max))))
+	      (setq length
+                    (epa-file-decode-and-insert
+                     string file visit beg end replace)))
 	    (if visit
 		(set-visited-file-modtime))))
       (if (and local-copy
diff --git a/lisp/international/mule.el b/lisp/international/mule.el
index ec6f647688..5338e54cf6 100644
--- a/lisp/international/mule.el
+++ b/lisp/international/mule.el
@@ -2244,6 +2244,25 @@ modify-coding-system-alist
 		   (cons (cons regexp coding-system)
 			 network-coding-system-alist)))))))
 
+(defun decode-coding-inserted-region-coding-system (filename
+                                                    visit beg end replace)
+  "Return a coding system for the current buffer as if it is read from FILENAME."
+  (let ((coding coding-system-for-read))
+    (unless coding
+      (setq coding (funcall set-auto-coding-function
+			    filename (- (point-max) (point-min)))))
+    (unless coding
+      (setq coding (car (find-operation-coding-system
+		         'insert-file-contents
+		         (cons filename (current-buffer))
+		         visit beg end replace))))
+    (if (coding-system-p coding)
+        (or enable-multibyte-characters
+	    (setq coding
+		  (coding-system-change-text-conversion coding 'raw-text)))
+      (setq coding nil))
+    coding))
+
 (defun decode-coding-inserted-region (from to filename
 					   &optional visit beg end replace)
   "Decode the region between FROM and TO as if it is read from file FILENAME.
@@ -2253,8 +2272,7 @@ decode-coding-inserted-region
 Part of the job of this function is setting `buffer-undo-list' appropriately."
   (save-excursion
     (save-restriction
-      (let ((coding coding-system-for-read)
-	    undo-list-saved)
+      (let (coding undo-list-saved)
 	(if visit
 	    ;; Temporarily turn off undo recording, if we're decoding the
 	    ;; text of a visited file.
@@ -2268,19 +2286,8 @@ decode-coding-inserted-region
 		      buffer-undo-list t))))
 	(narrow-to-region from to)
 	(goto-char (point-min))
-	(or coding
-	    (setq coding (funcall set-auto-coding-function
-				  filename (- (point-max) (point-min)))))
-	(or coding
-	    (setq coding (car (find-operation-coding-system
-			       'insert-file-contents
-			       (cons filename (current-buffer))
-			       visit beg end replace))))
-	(if (coding-system-p coding)
-	    (or enable-multibyte-characters
-		(setq coding
-		      (coding-system-change-text-conversion coding 'raw-text)))
-	  (setq coding nil))
+        (setq coding (decode-coding-inserted-region-coding-system
+                      filename visit beg end replace))
 	(if coding
 	    (decode-coding-region (point-min) (point-max) coding)
 	  (setq last-coding-system-used coding))

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

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 26 Aug 2019 09:20:29 +0200
> Cc: 34720@debbugs.gnu.org
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> >> 1. Open a GnuPG encrypted file
> >> 2. Run the following:
> >>    (setq test-marker (make-marker))
> >>    (move-marker test-marker (point-marker))
> >>    ;; test-marker points to `point'
> >> 3. Revert the buffer
> >>    ;; test-marker now points to the end of the buffer

Markers cannot be preserved in every situation, there's no way around
this basic fact.

> The third is that the interface to the new replace-buffer-contents
> function is really awkward -- it only takes a buffer as its SOURCE,
> which means that if you want to feed it something, you have to skip
> around to temporary buffers instead of feeding it a string, which would
> be natural.  You have to be a with-temp-buffer/with-current-buffer/let
> contortionist to get it to be safe.

replace-buffer-contents is a primitive, so it can legitimately rely on
Lisp programs to set up whatever preconditions it needs for it to
work.  It MUST have a buffer to work with; if you want to replace with
a string, insert that string into a buffer and call the primitive.
Why is that a problem?

> Would anybody mind if I just write a `with-saved-markers' macro in
> subr-x, which would make all these problems to away and make the
> solution a two-liner in epa itself?

What would that macro do, exactly?

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Markers cannot be preserved in every situation, there's no way around
> this basic fact.

No, but commands like `revert-buffer' (via insert-file-contents) try to
keep most of them around.

> replace-buffer-contents is a primitive, so it can legitimately rely on
> Lisp programs to set up whatever preconditions it needs for it to
> work.  It MUST have a buffer to work with; if you want to replace with
> a string, insert that string into a buffer and call the primitive.
> Why is that a problem?

Why MUST it have a buffer to get the input data from?

You get a text from somewhere, and it often ends up in a string (as in
the epa case).  If you want to safely feed that to this function, you
have to say something along the lines of

(let ((buffer (current-buffer)))
  (with-temp-buffer
    (insert string)
    (let ((temp (current-buffer)))
      (with-current-buffer buffer
	(replace-buffer-contents temp)))))

which is horrible, horrible, and horrible.  No wonder this function has
gotten one single usage after it was introduced two years ago.  (Well,
one usage to replace-region-contents, which then calls the function.)
(Unless I'm grepping wrong.)

>> Would anybody mind if I just write a `with-saved-markers' macro in
>> subr-x, which would make all these problems to away and make the
>> solution a two-liner in epa itself?
>
> What would that macro do, exactly?

Gather the markers, execute the body, and then put the markers back
where they were.  Which is what replace-buffer-contents does, but for a
whole lot more stuff.

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

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: dunni@gnu.org,  34720@debbugs.gnu.org
> Date: Mon, 26 Aug 2019 10:14:10 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Markers cannot be preserved in every situation, there's no way around
> > this basic fact.
> 
> No, but commands like `revert-buffer' (via insert-file-contents) try to
> keep most of them around.

"As much as possible".  In some situation, it's impossible, at least
for some of the markers.  That happens when the text where markers
pointed to is entirely replaced.

IOW, there's no guarantee that markers will be preserved across
operations that replace text.

> > replace-buffer-contents is a primitive, so it can legitimately rely on
> > Lisp programs to set up whatever preconditions it needs for it to
> > work.  It MUST have a buffer to work with; if you want to replace with
> > a string, insert that string into a buffer and call the primitive.
> > Why is that a problem?
> 
> Why MUST it have a buffer to get the input data from?

Because no one has written code to support a string, I guess.  Feel
free to work on that if you think it's important enough.

> You get a text from somewhere, and it often ends up in a string (as in
> the epa case).  If you want to safely feed that to this function, you
> have to say something along the lines of
> 
> (let ((buffer (current-buffer)))
>   (with-temp-buffer
>     (insert string)
>     (let ((temp (current-buffer)))
>       (with-current-buffer buffer
> 	(replace-buffer-contents temp)))))
> 
> which is horrible, horrible, and horrible.

I see nothing horrible here.  More importantly, I suspect the string
actually started in some buffer, in which case all of the
complications above could be avoided.  (I generally consider code in
Emacs that manipulates large text strings to be broken by design.)

But I'm not opposed to adding support for string as the source for
replacement.  Just be aware that the code which access such a string
must be highly optimized, because it is executed in the innermost loop
of the code.

> No wonder this function has gotten one single usage after it was
> introduced two years ago.  (Well, one usage to
> replace-region-contents, which then calls the function.)  (Unless
> I'm grepping wrong.)

replace-region-contents is used in json.el.

As for the users of replace-buffer-contents, I could think of several
alternative reasons for its rare usage:

  . the function is relatively new, and was horribly slow before Emacs 27
  . the use cases for it are relatively rare, otherwise we'd have it
    long ago

> >> Would anybody mind if I just write a `with-saved-markers' macro in
> >> subr-x, which would make all these problems to away and make the
> >> solution a two-liner in epa itself?
> >
> > What would that macro do, exactly?
> 
> Gather the markers, execute the body, and then put the markers back
> where they were.

How do you "put the markers back where they were"?  That's the crucial
point, and I don't see how would you pull that out when the text is
significantly modified.

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> IOW, there's no guarantee that markers will be preserved across
> operations that replace text.

No, but we have a small handful of functions that do best effort...  but
they're deep in the C code and not accessible.

Finsert_file_contents has this:

  /* Replacement should preserve point as it preserves markers.  */
  if (!NILP (replace))
    {
      window_markers = get_window_points_and_markers ();
      record_unwind_protect (restore_point_unwind,
			     XCAR (XCAR (window_markers)));
    }
    ...
 handled:

  if (inserted > 0)
    restore_window_points (window_markers, inserted,
			   BYTE_TO_CHAR (same_at_start),
			   same_at_end_charpos);

The problem that this bug report addresses is that Lisp level functions
that implement special handlers for insert-file-contents (in this case,
epa-file-insert-file-contents) doesn't have access to the internals
necessary to give the user the same experience that the built-in version
gives the user.

My suggestion is basically to make DEFUN shims over
get_window_points_and_markers/restore_window_points, and create a new
macro `with-saved-markers' that would use that pair to do this cheap,
best-effort thing that Finsert_file_contents does.

> But I'm not opposed to adding support for string as the source for
> replacement.  Just be aware that the code which access such a string
> must be highly optimized, because it is executed in the innermost loop
> of the code.

I just had a peek at the code, and it indeed highly optimised...

>> No wonder this function has gotten one single usage after it was
>> introduced two years ago.  (Well, one usage to
>> replace-region-contents, which then calls the function.)  (Unless
>> I'm grepping wrong.)
>
> replace-region-contents is used in json.el.

Yes, that's the one single usage of this machinery.

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

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: dunni@gnu.org,  34720@debbugs.gnu.org
> Date: Tue, 27 Aug 2019 09:14:22 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > IOW, there's no guarantee that markers will be preserved across
> > operations that replace text.
> 
> No, but we have a small handful of functions that do best effort...  but
> they're deep in the C code and not accessible.
> 
> Finsert_file_contents has this:
> 
>   /* Replacement should preserve point as it preserves markers.  */
>   if (!NILP (replace))
>     {
>       window_markers = get_window_points_and_markers ();
>       record_unwind_protect (restore_point_unwind,
> 			     XCAR (XCAR (window_markers)));
>     }
>     ...
>  handled:
> 
>   if (inserted > 0)
>     restore_window_points (window_markers, inserted,
> 			   BYTE_TO_CHAR (same_at_start),
> 			   same_at_end_charpos);

These just restore a single marker: the point marker.  That's a far
cry from restoring all the markers.  I don't think the latter is
possible in all cases without violating the principle of least
astonishment (by placing the markers at locations that have nothing in
comm on with where they have been before the editing operation).

> The problem that this bug report addresses is that Lisp level functions
> that implement special handlers for insert-file-contents (in this case,
> epa-file-insert-file-contents) doesn't have access to the internals
> necessary to give the user the same experience that the built-in version
> gives the user.

That's because markers can only be preserved by operations on the C
level.  If the C-level handling cannot preserve a marker, then it is
unsafe, to say the least, to try to second-guess that by moving the
markers manually from Lisp.

Specifically, if the editing operation deletes the text on both sides
of a marker, there's no reasonable place, in general, to have this
marker after some other text is inserted.  You may think otherwise if
the inserted text happens to be very similar to the one which was
deleted, but that's an illusion.  If some Lisp program wants to
preserve markers during editing, then it must call the likes of
replace-buffer-contents to do the job.

> My suggestion is basically to make DEFUN shims over
> get_window_points_and_markers/restore_window_points, and create a new
> macro `with-saved-markers' that would use that pair to do this cheap,
> best-effort thing that Finsert_file_contents does.

That has come up before.  Giving Lisp programs direct access to
markers is dangerous, because Emacs uses markers internally for
various bookkeeping purposes, such as conversion between character and
byte positions.

But maybe I misunderstand what you want to do, because you didn't
answer my question about restoring markers after replacement.

> >> No wonder this function has gotten one single usage after it was
> >> introduced two years ago.  (Well, one usage to
> >> replace-region-contents, which then calls the function.)  (Unless
> >> I'm grepping wrong.)
> >
> > replace-region-contents is used in json.el.
> 
> Yes, that's the one single usage of this machinery.

It's a rather niche capability, so I'm not surprised it doesn't yet
have a lot of users.

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> These just restore a single marker: the point marker.  That's a far
> cry from restoring all the markers.  I don't think the latter is
> possible in all cases without violating the principle of least
> astonishment (by placing the markers at locations that have nothing in
> comm on with where they have been before the editing operation).

Hm.  Doesn't the code below restore all markers (that it can restore)?

static void
restore_window_points (Lisp_Object window_markers, ptrdiff_t inserted,
		       ptrdiff_t same_at_start, ptrdiff_t same_at_end)
{
  for (; CONSP (window_markers); window_markers = XCDR (window_markers))
    if (CONSP (XCAR (window_markers)))
      {
	Lisp_Object car = XCAR (window_markers);
	Lisp_Object marker = XCAR (car);
	Lisp_Object oldpos = XCDR (car);
	if (MARKERP (marker) && FIXNUMP (oldpos)
	    && XFIXNUM (oldpos) > same_at_start
	    && XFIXNUM (oldpos) < same_at_end)
	  {
	    ptrdiff_t oldsize = same_at_end - same_at_start;
	    ptrdiff_t newsize = inserted;
	    double growth = newsize / (double)oldsize;
	    ptrdiff_t newpos
	      = same_at_start + growth * (XFIXNUM (oldpos) - same_at_start);
	    Fset_marker (marker, make_fixnum (newpos), Qnil);
	  }
      }
}

And I just tested with the test case in this bug report, which is

(progn
  (setq test-marker (make-marker))
  (move-marker test-marker (point)))

append to the file, and then `M-x revert-buffer': test-marker remains at
the same position.

So I think Finsert_file_contents really restores (for some value of
"restores") all the markers?

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

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Lars Ingebrigtsen]

Continuing to read the code in the massive Finsert_file_contents, even
with a way to get and restore the markers, replicating all that it does
seems like a rather hair-raising task.  For instance:

  /* If requested, replace the accessible part of the buffer
     with the file contents.  Avoid replacing text at the
     beginning or end of the buffer that matches the file contents;
     that preserves markers pointing to the unchanged parts.

     Here we implement this feature in an optimized way
     for the case where code conversion is NOT needed.
     The following if-statement handles the case of conversion
     in a less optimal way.

     If the code conversion is "automatic" then we try using this
     method and hope for the best.
     But if we discover the need for conversion, we give up on this method
     and let the following if-statement handle the replace job.  */

Oh:

/* FIXME: insert-file-contents should be split with the top-level moved to
   Elisp and only the core kept in C.  */

Right.

Hm...  Now what's this?

  /* If the file name has special constructs in it,
     call the corresponding file name handler.  */
  handler = Ffind_file_name_handler (filename, Qinsert_file_contents);
  if (!NILP (handler))
    {
      val = call6 (handler, Qinsert_file_contents, filename,
		   visit, beg, end, replace);
      if (CONSP (val) && CONSP (XCDR (val))
	  && RANGED_FIXNUMP (0, XCAR (XCDR (val)), ZV - PT))
	inserted = XFIXNUM (XCAR (XCDR (val)));
      goto handled;
    }

That's the code that calls out to `epa-file-insert-file-contents', isn't
it?  Hm, yes it is.  And handled: will restore the markers if inserted
is larger than zero.

But it seems to be larger than zero, so I think I need to do more
debugging.

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

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: dunni@gnu.org,  34720@debbugs.gnu.org
> Date: Tue, 27 Aug 2019 10:23:28 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > These just restore a single marker: the point marker.  That's a far
> > cry from restoring all the markers.  I don't think the latter is
> > possible in all cases without violating the principle of least
> > astonishment (by placing the markers at locations that have nothing in
> > comm on with where they have been before the editing operation).
> 
> Hm.  Doesn't the code below restore all markers (that it can restore)?

No, it restores only the window-point marker in each window that shows
the same buffer.  See get_window_points_and_markers, which collects
those markers.

> And I just tested with the test case in this bug report, which is
> 
> (progn
>   (setq test-marker (make-marker))
>   (move-marker test-marker (point)))
> 
> append to the file, and then `M-x revert-buffer': test-marker remains at
> the same position.

Must be sheer luck.  Or maybe I'm missing something, but you will have
to show me the code that moves this test marker to convince me.  (I
don't have the necessary software installed to repeat the recipe myself.)

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Must be sheer luck.  Or maybe I'm missing something, but you will have
> to show me the code that moves this test marker to convince me.  (I
> don't have the necessary software installed to repeat the recipe myself.)

(progn
  (find-file "/tmp/foo.txt")
  (kill-buffer (current-buffer))
  (when (file-exists-p "/tmp/foo.txt")
    (delete-file "/tmp/foo.txt"))
  (find-file "/tmp/foo.txt")
  (insert "Test line.")
  (setq test-marker1 (make-marker))
  (move-marker test-marker1 4)
  (setq test-marker2 (make-marker))
  (move-marker test-marker2 5)
  (save-buffer t)
  (shell-command "echo new >> /tmp/foo.txt")
  (revert-buffer nil t)
  (list test-marker1 test-marker2))

=> (#<marker at 4 in foo.txt> #<marker at 5 in foo.txt>)

So the markers seem to be restored on `revert-buffer'?

The reason this doesn't happen in the epa case seems to be a bug in
Finsert_file_contents: When there's an external handler, it skips
directly to handled: and neglects to do the same_at_start computation,
which leaves that at point-min and restore_window_points ignores all
markers that have values that are larger than same_at_start.

If I'm reading the code right.

If the new text doesn't match the old text, the markers are not
restored:

(progn
  (find-file "/tmp/foo.txt")
  (kill-buffer (current-buffer))
  (when (file-exists-p "/tmp/foo.txt")
    (delete-file "/tmp/foo.txt"))
  (find-file "/tmp/foo.txt")
  (insert "Test line.")
  (setq test-marker1 (make-marker))
  (move-marker test-marker1 4)
  (setq test-marker2 (make-marker))
  (move-marker test-marker2 5)
  (save-buffer t)
  (shell-command "echo Here is a new text > /tmp/foo.txt")
  (revert-buffer nil t)
  (list test-marker1 test-marker2))

(#<marker at 1 in foo.txt> #<marker at 1 in foo.txt>)

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

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> No, it restores only the window-point marker in each window that shows
> the same buffer.  See get_window_points_and_markers, which collects
> those markers.

Oh, oh, you're totally right, of course -- that function only collects
the point markers.

The reason the other markers magically survive Finsert_file_contents is
because that function takes care not to replace any text in the buffer
that's identical to the text already there?  So the text isn't deleted,
the markers stay where they are, and everything works nice.

So the fix here is to make epa follow the same logic, perhaps?  That is,
first get the text we're supposed to insert, then compare with the data
in the buffer, and only start replacing at the point where we find the
first difference?

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

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: dunni@gnu.org,  34720@debbugs.gnu.org
> Date: Tue, 27 Aug 2019 11:05:33 +0200
> 
> (progn
>   (find-file "/tmp/foo.txt")
>   (kill-buffer (current-buffer))
>   (when (file-exists-p "/tmp/foo.txt")
>     (delete-file "/tmp/foo.txt"))
>   (find-file "/tmp/foo.txt")
>   (insert "Test line.")
>   (setq test-marker1 (make-marker))
>   (move-marker test-marker1 4)
>   (setq test-marker2 (make-marker))
>   (move-marker test-marker2 5)
>   (save-buffer t)
>   (shell-command "echo new >> /tmp/foo.txt")
>   (revert-buffer nil t)
>   (list test-marker1 test-marker2))
> 
> => (#<marker at 4 in foo.txt> #<marker at 5 in foo.txt>)
> 
> So the markers seem to be restored on `revert-buffer'?

The original text is unchanged, you just added something at the end of
the file.  So markers at positions 4 and 5 will be preserved, yes.

> The reason this doesn't happen in the epa case seems to be a bug in
> Finsert_file_contents: When there's an external handler, it skips
> directly to handled: and neglects to do the same_at_start computation,
> which leaves that at point-min and restore_window_points ignores all
> markers that have values that are larger than same_at_start.

AFAIU, the problem with the epa case is that the buffer is erased and
the text re-inserted.  Isn't that the case?

In any case, how can insert-file-contents know what the handler did
wrt which part(s) of the buffer remained unchanged?

> If the new text doesn't match the old text, the markers are not
> restored:

Of course.

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 34720@debbugs.gnu.org,  dunni@gnu.org
> Date: Tue, 27 Aug 2019 11:15:29 +0200
> 
> The reason the other markers magically survive Finsert_file_contents is
> because that function takes care not to replace any text in the buffer
> that's identical to the text already there?  So the text isn't deleted,
> the markers stay where they are, and everything works nice.

Right.

> So the fix here is to make epa follow the same logic, perhaps?  That is,
> first get the text we're supposed to insert, then compare with the data
> in the buffer, and only start replacing at the point where we find the
> first difference?

You want to replace the insert-file-contents with custom-tailored Lisp
code?  Even if possible and efficient enough, this would be a
specialized solution for only a single use case.  Right?  Other use
cases, with other insert-file-contents handlers, will each one have to
have their separate custom solutions, right?  All that just to keep
markers intact?

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> So the fix here is to make epa follow the same logic, perhaps?  That is,
>> first get the text we're supposed to insert, then compare with the data
>> in the buffer, and only start replacing at the point where we find the
>> first difference?
>
> You want to replace the insert-file-contents with custom-tailored Lisp
> code?  Even if possible and efficient enough, this would be a
> specialized solution for only a single use case.  Right?  Other use
> cases, with other insert-file-contents handlers, will each one have to
> have their separate custom solutions, right?  All that just to keep
> markers intact?

It's a problem common to all the insert-file-content handlers, I think?
Now that I understand what the problem is (i.e., "don't replace the text
at the start of the buffer with identical text"), I think fixing this in
the epa case should hopefully be easy enough, and perhaps something more
general can be extracted from that solution.  Which I have not written
yet, so we'll see.  :-)

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

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Lars Ingebrigtsen]

That didn't seem to be too complicated, so I've now pushed the change.
The original test case now works (i.e., more markers are now preserved,
so it more closely emulates Finsert_file_contents).

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

bug#34720: 26.1; Reverting a GPG buffer moves all markers to the end of the file

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: 34720@debbugs.gnu.org,  dunni@gnu.org
> Date: Fri, 30 Aug 2019 11:48:07 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> So the fix here is to make epa follow the same logic, perhaps?  That is,
> >> first get the text we're supposed to insert, then compare with the data
> >> in the buffer, and only start replacing at the point where we find the
> >> first difference?
> >
> > You want to replace the insert-file-contents with custom-tailored Lisp
> > code?  Even if possible and efficient enough, this would be a
> > specialized solution for only a single use case.  Right?  Other use
> > cases, with other insert-file-contents handlers, will each one have to
> > have their separate custom solutions, right?  All that just to keep
> > markers intact?
> 
> It's a problem common to all the insert-file-content handlers, I think?
> Now that I understand what the problem is (i.e., "don't replace the text
> at the start of the buffer with identical text"), I think fixing this in
> the epa case should hopefully be easy enough, and perhaps something more
> general can be extracted from that solution.  Which I have not written
> yet, so we'll see.  :-)

The problem is that getting at the new text to compare with is
specific to each handler, and some of them cannot be rewound (i.e.,
you cannot examine the same text more than once).