Improvements to the save-buffer protocol -- run with-silent-modifications

[akashpal-21]

Problem

During save -- before save the transclusions are removed from the buffer temporarily and after writing to disk -- the transclusions are enabled as and how they were -- this process generates excess entries to the buffer-undo-list -- if users save this list to disk as a simple version control system -- these excess entries should be suppressed.

Most easy solution without altering anything else in the infrastructure and increasing the entropy of the system --

(defun org-transclusion-before-save-buffer ()
  "Remove transclusions in `before-save-hook'.
This function is meant to clear the file clear of the
transclusions.  It also remembers the current point for
`org-transclusion-after-save-buffer' to move it back."
  (setq org-transclusion-remember-point (point))
  (setq org-transclusion-remember-transclusions
        (with-silent-modifications (org-transclusion-remove-all))))

(defun org-transclusion-after-save-buffer ()
  "Add transclusions back as they were `before-save-buffer'.
This function relies on `org-transclusion-remember-transclusions'
set in `before-save-hook'.  It also move the point back to
`org-transclusion-remember-point'."
  (unwind-protect
      (progn
        ;; Assume the list is in descending order.
        ;; pop and do from the bottom of buffer
        (let ((do-length (length org-transclusion-remember-transclusions))
              (do-count 0))
          (dolist (p org-transclusion-remember-transclusions)
            (save-excursion
              (goto-char p)
          (with-silent-modifications (org-transclusion-add))
              (move-marker p nil)
              (setq do-count (1+ do-count))
              (when (> do-count do-length)
                (error "org-transclusion: Aborting. You may be in an infinite loop"))))
          ;; After save and adding all transclusions, the modified flag should be
          ;; set to nil
          (restore-buffer-modified-p nil)
          (when org-transclusion-remember-point
            (goto-char org-transclusion-remember-point))))
    (progn
      (setq org-transclusion-remember-point nil)
      (setq org-transclusion-remember-transclusions nil))))

That is remove and add procedures are processed within with-silent-modifications

Another process we discussed was create a new function and alter the write-file-functions -- but this is an exotic process, and although works similarly -- regressions must be tested more thoroughly.

 (defun org-transclusion-activate ()
  "Activate Org-transclusion hooks and other setups in the current buffer.
This function does not add transclusions; it merely sets up hooks
and variables."
  (interactive)
  (add-hook 'write-file-functions #'org-transclusion-save-buffer nil t)
  (add-hook 'kill-buffer-hook #'org-transclusion-before-kill nil t)
  (add-hook 'kill-emacs-hook #'org-transclusion-before-kill nil t)
  (add-hook (if (version< org-version "9.6")
                'org-export-before-processing-hook
              'org-export-before-processing-functions)
            #'org-transclusion-inhibit-read-only nil t)
  (org-transclusion-yank-excluded-properties-set)
  (org-transclusion-load-extensions-maybe))

(defun org-transclusion-deactivate ()
  "Deactivate Org-transclusion hooks and other setups in the current buffer.
This function also removes all the transclusions in the current buffer."
  (interactive)
  (org-transclusion-remove-all)
  (remove-hook 'write-file-functions #'org-transclusion-save-buffer t)
  (remove-hook 'kill-buffer-hook #'org-transclusion-before-kill t)
  (remove-hook 'kill-emacs-hook #'org-transclusion-before-kill t)
  (remove-hook (if (version< org-version "9.6")
                   'org-export-before-processing-hook
                 'org-export-before-processing-functions)
               #'org-transclusion-inhibit-read-only t)
  (org-transclusion-yank-excluded-properties-remove))

(defun org-transclusion-save-buffer ()
  "Save buffer protocols. Ensures file on disk is cleaned of transclusions;
Before writing to disk run `org-transclusion-before-save-buffer' which removes
active transclusions and generates a list of transclusions that were removed;
after writing to disk is complete, re-enable transclusions as and how they were
by running `org-transclusion-after-save-buffer' over the previously generated
list.

Run within `with-silent-modifications' so that none of this is recorded in the
`buffer-undo-list' of the buffer concerned."
  (with-silent-modifications
    (save-restriction
      (widen)
      (org-transclusion-before-save-buffer)
      (write-file buffer-file-name)
      (org-transclusion-after-save-buffer)))
  t)

[akashpal-21]

Debug to test

;; Debug attach
(advice-add 'save-buffer :around #'(lambda (orig-fun &rest args)
                     "Debug: Compare `buffer-undo-list'"
                     (let ((orig-undo-list buffer-undo-list))
                       (apply orig-fun args)
                       (funcall #'(lambda (orig-undo-list)
                            (if (eq orig-undo-list buffer-undo-list)
                            (message "buffer-undo-list has not been modified")
                              (message "buffer-undo-list has been modified since")))
                        orig-undo-list))))

;; Debug remove
(advice-remove 'save-buffer #'(lambda (orig-fun &rest args)
                "Debug: Compare `buffer-undo-list'"
                (let ((orig-undo-list buffer-undo-list))
                            (apply orig-fun args)
                            (funcall #'(lambda (orig-undo-list)
                                     (if (eq orig-undo-list buffer-undo-list)
                                     (message "buffer-undo-list has not been modified")
                                       (message "buffer-undo-list has been modified since")))
                                 orig-undo-list))))

[akashpal-21]

Suggested patch

--- org-transclusion.el 2024-05-10 19:42:15.714701309 +0530
+++ org-transclusion-patched.el 2024-05-11 21:03:33.068559492 +0530
@@ -775,7 +775,7 @@
 `org-transclusion-after-save-buffer' to move it back."
   (setq org-transclusion-remember-point (point))
   (setq org-transclusion-remember-transclusions
-        (org-transclusion-remove-all)))
+        (with-silent-modifications (org-transclusion-remove-all))))

 (defun org-transclusion-after-save-buffer ()
   "Add transclusions back as they were `before-save-buffer'.
@@ -791,7 +791,7 @@
           (dolist (p org-transclusion-remember-transclusions)
             (save-excursion
               (goto-char p)
-              (org-transclusion-add)
+              (with-silent-modifications (org-transclusion-add))
               (move-marker p nil)
               (setq do-count (1+ do-count))
               (when (> do-count do-length)

This patch can be possibly pushed without much thought -- whether the choice is made between choosing the existing before-save-hooks and after-save-hooks or using the write-file-functions - this change should not interfere with that choice - we can run the function without with-silent-modifications there.

Please push an appropriate choice as time permits. The change is minor and wouldn't interfere with anything else.

[josephmturner]

In #207, I suggested using with-silent-modifications, in hopes that it would solve #106. In this comment @yantar92 pointed out that Org-mode caching relies on modification hooks to function. IIUC, your proposed patch would result in performance issues.

@yantar92 would you be willing to give feedback on this issue?

[akashpal-21]

@josephmturner please also check #243 It relates to not allowing org-element-cache to run on cloned buffers -- I suggested some fixes and also passed on a fix suggested by Yantar. Please give your opinion

[akashpal-21]

We can always store the value of buffer-undo-list before buffer operations in a local scope let - then restore as part of unwind-protect

Such hacky way is always available to us if we are not to use with-silent-modifications

Please consider pros and cons and suggest appropriate fix.

[akashpal-21]

After going through the issue once more - the reason Radchenko advised not to use (with-silent-modifications) is because inhibit-modification-hooks is set to t -- if this is the case then before-change-functions and after-change-functions would not work --

But we may still use this - because the cache is irrelevant here -- contents are restored as is -- nothing is really changed - every buffer change is superficial - it is just as quickly inversed.

But if we want to be on the side of caution -- there is a more trivial way - if we set buffer-undo-list to t within the scope of the function - the list is not updated with entries within the scope.

 (defun patch/org-transclusion-save-hook (fn &rest args)
    (let ((buffer-undo-list t))
      (apply fn args)))
  (advice-add 'org-transclusion-before-save-buffer :around #'patch/org-transclusion-save-hook)
  (advice-add 'org-transclusion-after-save-buffer :around #'patch/org-transclusion-save-hook)

[josephmturner]

@akashpal-21 Is this issue still relevant given @nobiot's infinite loop workaround?

[akashpal-21]

@akashpal-21 Is this issue still relevant given @nobiot's infinite loop workaround?

No - I was just about to close it, Could be a simple user side setting.