[BUG] org-copy-subtree in a file with local variables marks buffer as modified [9.5.3 (release_9.5.3-6-gef41f3 @ /home/ignacio/repos/emacs/lisp/org/)]

[BUG] org-copy-subtree in a file with local variables marks buffer as modified [9.5.3 (release_9.5.3-6-gef41f3 @ /home/ignacio/repos/emacs/lisp/org/)]

[Ignacio Casso]

Hello,

Copying a subtree with `org-copy-subtree' in an org file with local
variables marks the buffer as modified. This is because
`org-copy-subtree' calls `org-preserve-local-variables', which deletes
local variables, executes some body, and then inserts them again, which
results in a modified buffer even if the buffer was not modified before
and the body does not modify it either, like in the case of
`org-copy-subtree'.

What would you think about a change like the following in the definition of
`org-preserve-local-variables' to solve the issue?

+ (let ((modified-before-p) (buffer-modified-p))
    ;; current code that deletes local variables
+   (unless modified-before-p (set-buffer-modified-p nil))
    ;; current code that executes body
+   (let ((modified-after-p (buffer-modified-p)))
      ;; current code that restores local variables
+     (unless modified-after-p (set-buffer-modified-p nil))))

Could the current code or my proposed change have any more unintended
consequences, for example in the undo tree or mark ring? Sometimes when
I undo or jump to the mark, the point moves to the last visible, usually
folded headline of the file, even if I had not edited it recently, and I
have always assumed that it's because it wants to go to the end of the
buffer for some reason I've never been able to come up with or
debug. Could that have something to do with this?

Best regards,

Ignacio

Emacs  : GNU Emacs 29.0.50 (build 53, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 1.16.0)
 of 2022-05-22
Package: Org mode version 9.5.3 (release_9.5.3-6-gef41f3 @ /home/ignacio/repos/emacs/lisp/org/)

Re: [BUG] org-copy-subtree in a file with local variables marks buffer as modified [9.5.3 (release_9.5.3-6-gef41f3 @ /home/ignacio/repos/emacs/lisp/org/)]

[Ihor Radchenko]

Ignacio Casso <ignaciocasso@hotmail.com> writes:

> What would you think about a change like the following in the definition of
> `org-preserve-local-variables' to solve the issue?
>
> + (let ((modified-before-p) (buffer-modified-p))
>     ;; current code that deletes local variables
> +   (unless modified-before-p (set-buffer-modified-p nil))
>     ;; current code that executes body
> +   (let ((modified-after-p (buffer-modified-p)))
>       ;; current code that restores local variables
> +     (unless modified-after-p (set-buffer-modified-p nil))))
>
> Could the current code or my proposed change have any more unintended
> consequences, for example in the undo tree or mark ring? Sometimes when
> I undo or jump to the mark, the point moves to the last visible, usually
> folded headline of the file, even if I had not edited it recently, and I
> have always assumed that it's because it wants to go to the end of the
> buffer for some reason I've never been able to come up with or
> debug. Could that have something to do with this?

buffer-modified-p is not the only parameter affected by juggling around
the local variables. There will be undo history,
buffer-chars-modified-tick, before/after-change-hooks triggered by
`org-preserve-local-variables', etc.

However, I do not see any obvious way how your proposed change can
negatively affect all the above. Feel free to propose a patch.

Best,
Ihor

Re: [BUG] org-copy-subtree in a file with local variables marks buffer as modified [9.5.3 (release_9.5.3-6-gef41f3 @ /home/ignacio/repos/emacs/lisp/org/)]

[Ignacio Casso]

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


> buffer-modified-p is not the only parameter affected by juggling around
> the local variables. There will be undo history,
> buffer-chars-modified-tick, before/after-change-hooks triggered by
> `org-preserve-local-variables', etc.
>
> However, I do not see any obvious way how your proposed change can
> negatively affect all the above. Feel free to propose a patch.

I've written a patch proposal. It deals with buffer-modified-p and undo
history, but not the other two points you mention. I have tested it and
it works, but I had never dealt before with `buffer-undo-list' so maybe
there are some cases that I have not considered and for which this patch
could be problematic. Let me know what you think:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1839 bytes --]

From 14506bea13bf6278d95825257d90bbc3390ae8f1 Mon Sep 17 00:00:00 2001
From: Ignacio Casso <ignaciocasso@hotmail.com>
Date: Sun, 12 Jun 2022 10:38:53 +0200
Subject: [PATCH] Do not mark buffer as modified with
 org-preserve-local-variables

* lisp/org-macs.el (org-preserve-local-variables): Do not mark buffer
as modified or alter `buffer-undo-list' when body does not actually
modify the buffer.

This commit fixes a bug with `org-copy-subtree', which marked the
buffer as modified and added an entry to the undo list when the visited
file had local variables.
---
 lisp/org-macs.el | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 19e5f42e9..64beeff53 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -163,16 +163,24 @@
 	  (org-with-wide-buffer
 	   (goto-char (point-max))
 	   (let ((case-fold-search t))
-	     (and (re-search-backward "^[ \t]*# +Local Variables:"
+	     (when (re-search-backward "^[ \t]*# +Local Variables:"
 				      (max (- (point) 3000) 1)
 				      t)
-		  (delete-and-extract-region (point) (point-max)))))))
+               (undo-boundary)
+	       (delete-and-extract-region (point) (point-max))))))
+         (tick-counter (buffer-modified-tick))
+         modified)
      (unwind-protect (progn ,@body)
        (when local-variables
+         (setq modified (< tick-counter (buffer-modified-tick)))
 	 (org-with-wide-buffer
 	  (goto-char (point-max))
 	  (unless (bolp) (insert "\n"))
-	  (insert local-variables))))))
+	  (insert local-variables))
+         (unless modified
+           (set-buffer-modified-p nil)
+           (setq buffer-undo-list
+                 (seq-drop-while 'identity buffer-undo-list)))))))
 
 (defmacro org-no-popups (&rest body)
   "Suppress popup windows and evaluate BODY."
-- 
2.25.1

Re: [BUG] org-copy-subtree in a file with local variables marks buffer as modified [9.5.3 (release_9.5.3-6-gef41f3 @ /home/ignacio/repos/emacs/lisp/org/)]

[Ihor Radchenko]

Ignacio Casso <ignaciocasso@hotmail.com> writes:

> I've written a patch proposal. It deals with buffer-modified-p and undo
> history, but not the other two points you mention. I have tested it and
> it works, but I had never dealt before with `buffer-undo-list' so maybe
> there are some cases that I have not considered and for which this patch
> could be problematic. Let me know what you think:

Thanks!

> -		  (delete-and-extract-region (point) (point-max)))))))
> +               (undo-boundary)

> +           (setq buffer-undo-list
> +                 (seq-drop-while 'identity buffer-undo-list)))))))

This looks fragile and can be disasterous when buffer-undo-list is
large.

Maybe just use with-silent-modifications or some ideas from there (for
example, wrapping modifications inside let-bound (buffer-undo-list t))?
We should not inhibit modification hooks though. Otherwise, it will mess
up with caching code.

Best,
Ihor

Re: [BUG] org-copy-subtree in a file with local variables marks buffer as modified [9.5.3 (release_9.5.3-6-gef41f3 @ /home/ignacio/repos/emacs/lisp/org/)]

[Ignacio Casso]

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


>> -		  (delete-and-extract-region (point) (point-max)))))))
>> +               (undo-boundary)
>
>> +           (setq buffer-undo-list
>> +                 (seq-drop-while 'identity buffer-undo-list)))))))
>
> This looks fragile and can be disasterous when buffer-undo-list is
> large.

I agree that it is very fragile, but could you explain why is it also
problematic with a large `buffer-undo-list'? I guess you mean
efficiency-wise, but it would only iterate through the few items added
since the call to (undo-boundary). Is there something else I'm not seeing?

> Maybe just use with-silent-modifications or some ideas from there (for
> example, wrapping modifications inside let-bound (buffer-undo-list t))?
> We should not inhibit modification hooks though. Otherwise, it will mess
> up with caching code.

Done. I attach the new patch:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: New patch using (let ((buffer-undo-list t)) ...) --]
[-- Type: text/x-diff, Size: 1650 bytes --]

From 54e05dc54fe7091f2d1c7e0c44e01cf5abeb4907 Mon Sep 17 00:00:00 2001
From: Ignacio Casso <ignaciocasso@hotmail.com>
Date: Sun, 12 Jun 2022 10:38:53 +0200
Subject: [PATCH] Do not mark buffer as modified with
 org-preserve-local-variables

* lisp/org-macs.el (org-preserve-local-variables): Do not mark buffer
as modified or alter `buffer-undo-list' when body does not actually
modify the buffer.

This commit fixes a bug with `org-copy-subtree', which marked the
buffer as modified and added an entry to the undo list when the visited
file had local variables.
---
 lisp/org-macs.el | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 19e5f42e9..c34ff3ab7 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -166,13 +166,19 @@
 	     (and (re-search-backward "^[ \t]*# +Local Variables:"
 				      (max (- (point) 3000) 1)
 				      t)
-		  (delete-and-extract-region (point) (point-max)))))))
+               (let ((buffer-undo-list t))
+	         (delete-and-extract-region (point) (point-max)))))))
+         (tick-counter-before (buffer-modified-tick)))
      (unwind-protect (progn ,@body)
        (when local-variables
 	 (org-with-wide-buffer
 	  (goto-char (point-max))
 	  (unless (bolp) (insert "\n"))
-	  (insert local-variables))))))
+          (let ((modified (< tick-counter-before (buffer-modified-tick)))
+                (buffer-undo-list t))
+	    (insert local-variables)
+            (unless modified
+              (restore-buffer-modified-p nil))))))))
 
 (defmacro org-no-popups (&rest body)
   "Suppress popup windows and evaluate BODY."
-- 
2.25.1

Re: [BUG] org-copy-subtree in a file with local variables marks buffer as modified [9.5.3 (release_9.5.3-6-gef41f3 @ /home/ignacio/repos/emacs/lisp/org/)]

[Ihor Radchenko]

Ignacio Casso <ignaciocasso@hotmail.com> writes:

>>> +           (setq buffer-undo-list
>>> +                 (seq-drop-while 'identity buffer-undo-list)))))))
>>
>> This looks fragile and can be disasterous when buffer-undo-list is
>> large.
>
> I agree that it is very fragile, but could you explain why is it also
> problematic with a large `buffer-undo-list'? I guess you mean
> efficiency-wise, but it would only iterate through the few items added
> since the call to (undo-boundary). Is there something else I'm not seeing?

Simply because I saw iteration over a potentially large list. Looking
closer, you are indeed right and performance should not be a problem in
this particular scenario.

>> Maybe just use with-silent-modifications or some ideas from there (for
>> example, wrapping modifications inside let-bound (buffer-undo-list t))?
>> We should not inhibit modification hooks though. Otherwise, it will mess
>> up with caching code.
>
> Done. I attach the new patch:

LGTM!
Applied onto main via aa789b89d.

Best,
Ihor