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/)
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
[-- 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
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
[-- 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
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