Re: [elpa] externals/org 46da64c 4/6: org-persist.el: Fix `add-hook' priority argument

Re: [elpa] externals/org 46da64c 4/6: org-persist.el: Fix `add-hook' priority argument

[Stefan Monnier]

Hi Ihor,

>     org-persist.el: Fix `add-hook' priority argument
> ---
>  lisp/org-persist.el | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lisp/org-persist.el b/lisp/org-persist.el
> index 9885386..e518974 100644
> --- a/lisp/org-persist.el
> +++ b/lisp/org-persist.el
> @@ -278,7 +278,7 @@ When BUFFER is `all', unregister VAR in all buffers."
>      (setq org-persist--index (nreverse new-index))))
>  
>  (add-hook 'kill-emacs-hook #'org-persist-gc)
> -(add-hook 'kill-emacs-hook #'org-persist-write-all 1000)
> +(add-hook 'kill-emacs-hook #'org-persist-write-all 100)

The docstring also says:

    Since nothing is \"always\" true, don't use 100 nor -100.

100 means that nothing will *ever* want to come later.  I.e. you claim
to be the one and only function that will ever deserve to always be the
last one on this hook, which is rather presumptuous, I think.

AFAICT the only thing that actually matters is that it should come after
`org-persist-gc`, so even just 0.01 would be good enough.  No need to
get anywhere near 100 (tho it would make sense to leave room for other
hooks that might need to come between `org-persist-gc` and
`org-persist-write-all`).  Also, I'd recommend you add a comment
explaining the constraints that were considered when choosing this depth
(e.g "Make sure it comes after `org-persist-gc`").


        Stefan

Re: [elpa] externals/org 46da64c 4/6: org-persist.el: Fix `add-hook' priority argument

[Ihor Radchenko]

Hi Stefan,

Thanks for providing comments on this new feature!

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>  (add-hook 'kill-emacs-hook #'org-persist-gc)
>> -(add-hook 'kill-emacs-hook #'org-persist-write-all 1000)
>> +(add-hook 'kill-emacs-hook #'org-persist-write-all 100)
>
> The docstring also says:
>
>     Since nothing is \"always\" true, don't use 100 nor -100.
>
> 100 means that nothing will *ever* want to come later.  I.e. you claim
> to be the one and only function that will ever deserve to always be the
> last one on this hook, which is rather presumptuous, I think.

> AFAICT the only thing that actually matters is that it should come after
> `org-persist-gc`, so even just 0.01 would be good enough.  No need to
> get anywhere near 100 (tho it would make sense to leave room for other
> hooks that might need to come between `org-persist-gc` and
> `org-persist-write-all`).  Also, I'd recommend you add a comment
> explaining the constraints that were considered when choosing this depth
> (e.g "Make sure it comes after `org-persist-gc`").

The idea behind 100 is related to Org usage.  We save buffer-local
`org-element--cache' to files.  It needs to be done _after_ all the
possible changes in Org buffers are made.  If any hook changes an Org
buffer after `org-persist-write-all', the saved value will become
invalid.  I am not sure what can be a reasonable DEPTH value in such
case.  Can it be something like 1?  Looking through Emacs code base, I
can mostly see non-nil values as DEPTH argument.  So, maybe 90? 91?

Best,
Ihor

Re: [elpa] externals/org 46da64c 4/6: org-persist.el: Fix `add-hook' priority argument

[Stefan Monnier]

> The idea behind 100 is related to Org usage.  We save buffer-local
> `org-element--cache' to files.  It needs to be done _after_ all the
> possible changes in Org buffers are made.  If any hook changes an Org
> buffer after `org-persist-write-all', the saved value will become
> invalid.  I am not sure what can be a reasonable DEPTH value in such
> case.  Can it be something like 1?  Looking through Emacs code base, I
> can mostly see non-nil values as DEPTH argument.  So, maybe 90? 91?

The value should be chosen in accordance with other possibly
interfering functions.
IOW you need for the functions to agree on a numbering-convention.
Since there is no such convention yet for your functions, you get to
define it.

So you can chose, for example 42 and from then on all function that need
to come before `org-persist-write-all` simply have to refrain from
using a value higher than 42.


        Stefan

Re: [elpa] externals/org 46da64c 4/6: org-persist.el: Fix `add-hook' priority argument

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> The idea behind 100 is related to Org usage.  We save buffer-local
>> `org-element--cache' to files.  It needs to be done _after_ all the
>> possible changes in Org buffers are made.  If any hook changes an Org
>> buffer after `org-persist-write-all', the saved value will become
>> invalid.  I am not sure what can be a reasonable DEPTH value in such
>> case.  Can it be something like 1?  Looking through Emacs code base, I
>> can mostly see non-nil values as DEPTH argument.  So, maybe 90? 91?
>
> The value should be chosen in accordance with other possibly
> interfering functions.
> IOW you need for the functions to agree on a numbering-convention.
> Since there is no such convention yet for your functions, you get to
> define it.
>
> So you can chose, for example 42 and from then on all function that need
> to come before `org-persist-write-all` simply have to refrain from
> using a value higher than 42.

Thanks for the clarification.

After more thinking and reading the manual, I have found that
kill-emacs-hook and kill-buffer-hook generally run after the buffer is
saved. So, I do not need to worry about something outside Org changing
the file contents. Depth 0 is good enough given that I put add-hook
calls in correct order.

Best,
Ihor