Re: Bug: org-archive-subtree-save-file-p logic [9.3.6 (release_9.3.6-399-ge6df03 @ /home/n/.emacs.d/straight/build/org/)]

[Kyle Meyer]

No Wayman <address@hidden> writes:

> The logic for saving the archive buffer in org-archive-subtree 
> does not consider the (default) value  of 'from-org for 
> org-archive-subtree-save-file-p.

Hmm, indeed, the change from 3d0282ef8 (New option
`org-archive-subtree-save-file-p', 2020-01-31) looks incomplete.

> While patching this, I realized I'm not sure I understand the 
> intended logic of each value for org-archive-subtree-save-file-p.

>From digging a bit, I think the history went like this:

  * All the way back in 4be4c5623 (version 4.12a, 2008-01-31),
    org-archive-subtree saved the archive buffer, except when it was the
    current buffer.

  [ The next two aren't relevant for the save part, but I'm noting
    because we should cleaned them up. ]

  * As of bf09955fe (Bugfix for `org-archive-subtree', 2008-03-01), the
    buffer was killed after being saved.

  * 343f3c478 (Keep archive buffer after archiving something to it,
    2009-10-28) reverted the killing, though it left behind a stale
    comment saying the buffer will be killed.

  * 63f6e851b (Do not save target buffer after archiving subtree,
    2017-11-25) stopped saving the buffer for performance reasons.

  * Some users preferred the older behavior.  An associated Debian bug
    was opened.

    https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=887332
    https://yhetil.org/orgmode/address@hidden/

    In response, Bastien committed 3d0282ef8 to offer more control over
    when saving happens.

> When setting it to 't', the defcustom :tag string claims "Always 
> save the archive buffer".
> This is not the case if archiving from within the current buffer. 
> Perhaps a clearer :tag string?
>
> #+begin_src emacs-lisp
> (defcustom org-archive-subtree-save-file-p 'from-org
>   ;;...
>   (const :tag "Save the archive buffer unless it is the current 
>   buffer" t)
>   ;;...
>   ))
> #+end_src

Yeah, I'd say that's an improvement, though the same "unless it is the
current buffer" would be true for from-org as well, were it wired up.

> The value 'from-org also still saves the archive buffer when 
> archiving from a buffer that is not in Org mode.

I'm confused by this statement.  To me it looks like save-buffer will
_never_ be called when org-archive-subtree-save-file-p is from-org:

    (unless (eq this-buffer buffer)
      (when (or (eq org-archive-subtree-save-file-p t)
                (and (boundp 'org-archive-from-agenda)
                     (eq org-archive-subtree-save-file-p 'from-agenda)))
        (save-buffer)))

> I'm not entirely sure of its purpose. If the intent is to allow an 
> option that prevents saving only when archiving from the agenda,
> I suggest a single option excluding that case and saving for 
> other, non-nil values:

Based on the history above, I believe the main purpose is to give users
a way to reverse the "no saving" behavior made in 63f6e851b (Do not save
target buffer after archiving subtree, 2017-11-25).  I'm _guessing_
that, on top of that, the idea adding a from-agenda value was that some
users may want to save only when archiving from the agenda, because in
that case they're a bit removed from the buffer and might not think to
save it, or something along those lines.

> #+begin_src emacs-lisp
> (defcustom org-archive-subtree-save-file-p 'unless-agenda
>   "Conditionally save the archive file after archiving a subtree.
> The value 'unless-agenda prevents saving from the agenda-view.
> Other non-nil values save the archive buffer unless it is the 
> current buffer."
>   :group 'org-archive
>   :package-version '(Org . "9.4")
>   :type '(choice
>           (const :tag "Do not save archive buffer when archiving 
>           from an agenda view" unless-agenda)
>           (const :tag "Save the archive buffer unless it is the 
>           current buffer" t)
>           (const :tag "Do not save the archive buffer")))
> #+end_src
>
> Then the saving logic in org-archive-subtree becomes:
>
> #+begin_src emacs-lisp
> (when org-archive-subtree-save-file-p
>   (unless (or (eq buffer this-buffer)
>               (and (eq org-archive-subtree-save-file-p 
>               'unless-agenda)
>                    ;;bound when called from org-agenda.el
>                    (boundp 'org-archive-from-agenda)))
>     (save-buffer)))
> #+end_src

Assuming what I said above is true, I think what you propose here loses
the ability to save only when archiving from the agenda.  And more
importantly, users would not be able to give a blanket "don't save" in
order to retain the behavior introduced by 63f6e851b (Do not save target
buffer after archiving subtree, 2017-11-25).