bastibe / org-journal

A simple org-mode based journaling mode
BSD 3-Clause "New" or "Revised" License
1.23k stars 122 forks source link

Org 9.6 and carryover weird behaviour #406

Open mrvdb opened 1 year ago

mrvdb commented 1 year ago

I've recently upgraded my org package to 9.6 from 9.5.5 and org-journal carryover functionality started to behave erratically. Reverting back org solves the issue, so my hunch was some changed function behaviour that needed adaptation in org-journal. So far I haven't been able to figure it out.

My minimal case which shows the problem:

(setq org-journal-dir "/home/mrb/dat/org/journal/"
        org-agenda-file-regexp "^[0-9]+\\.org"
        org-journal-file-format  "%Y%m%d.org"
        org-journal-date-format "%A, %e-%m-%Y"
        org-journal-date-prefix "#+TITLE: "
        org-journal-time-format "[%R] "
        org-journal-time-prefix "* "
        org-journal-carryover-items "+carryover|+TODO=\"TODO\""
        org-journal-enable-agenda-integration t
        org-journal-carryover-delete-empty-journal 'always
        org-journal-enable-cache t))

Testfile 20221216.org (previous day):

#+TITLE: Friday, 16-12-2022
* Main                                                            :carryover:
** TODO Testing if this comes through.

With this config, I ran org-journal-new-entry

Contents of 20221217.org:

#+TITLE: Saturday, 17-12-2022
* Main                                                            :carryover:
** TODO Testing if this comes through.
* Main                                                            :carryover:
** TODO Testing if this comes through.

and an error message gets logged: org-journal-delete-old-carryover: Args out of range: #<buffer 20221216.org>, 29, 146"

Depending on the test file the carryover items which get in the new file is different. I have not been able to spot a clear pattern yet. It seems to double the main headline in any case with different items below it.

bastibe commented 1 year ago

How strange. Does it happen with the default org-journal-carryover-items as well, or is this specific to the carryover-tag matcher? (perhaps it now matches once for carryover, and once for TODO)

mrvdb commented 1 year ago

It is indeed the org-journal-carryover-items value that causes problems. Using the default it seems to work as expected, that is, it carries over TODO items and leaves the rest behind.

mrvdb commented 1 year ago

Forgot to ask, can you reproduce the error?

bastibe commented 1 year ago

I currently don't have much time to spare for open source development, and didn't try to reproduce the issue, sorry.

It is probably relatively easy to filter down the carryover results, such that we only ever copy each block once. If you'd like to try implementing this as a pull request, I'd be grateful for the help!

mrvdb commented 1 year ago

Fair enough.

jmay commented 1 year ago

I was unable to reproduce this error. I wrote this as an ert test case, which passes. I'm not seeing the duplicated entry. Have I missed anything?

(ert-deftest org-journal-carryover-args-out-of-range-test ()
  "Carrying over TODO items for daily files, with specified date format."

  (org-journal-test-macro
   (let (
         (org-journal-file-type 'daily)
         (org-journal-date-prefix "#+TITLE: ")
         (org-journal-time-prefix "* ")
         (org-journal-file-format  "%Y%m%d.org")
         (org-journal-date-format "%A, %e-%m-%Y")
         (org-journal-time-format "[%R] ")
         (org-journal-carryover-items "+carryover|+TODO=\"TODO\"")
         (org-journal-carryover-delete-empty-journal 'always)
         (org-journal-enable-cache t)
         )

     (copy-directory
      (directory-file-name "tests/journals/daily/carryover2")
      (file-name-as-directory org-journal-dir-test) nil nil t)

     (org-journal-new-entry t)

     (message (buffer-string))

     (should
      (cl-search
       (format-time-string org-journal-date-format)
       (buffer-substring-no-properties (point-min) (point-max))
       )
      )
     )))
jmay commented 1 year ago

@mrvdb I take back my "cannot reproduce" comment. Was using org 9.5.5. When I ensured that 9.6 was installed and being used by ERT, I'm getting several errors from the test suite, not just the carryover case. Not sure what is going on.

Command line to run the test suite:

emacs -q -batch -l ert --eval "(package-initialize)" -l org-journal.el -l tests/org-journal-test.el -f ert-run-tests-batch-and-exit
mrvdb commented 1 year ago

ERT results:

3 unexpected results:
   FAILED  org-journal-carryover-delete-empty-journal-test
   FAILED  org-journal-carryover-items-test
   FAILED  org-journal-scheduled-weekly-test

The common error in my ert results, at least for the carryover related tests seems to be:

"Calling ‘org-fold-core-region’ with missing SPEC"

bastibe commented 1 year ago

It might be extremely useful to incorporate these ERT tests into the Github CI.

jmay commented 1 year ago

Yes, confirming that I can reliably get those same test failures. This is definitely caused by new behavior in org 9.6, some missing values that are expected by functions in org-fold-core.el (if you're curious, poke around with injecting a drawer value into org-fold-core--specs). I'm trying to figure out a safe backward-compatible workaround until the core issue can be resolved.

mrvdb commented 1 year ago

How can I help get this resolved?

bastibe commented 1 year ago

We'd need someone to contribute a pull request that fixes the issue. If you'd like to take a stab at that, we'd be happy to review and merge it!

mrvdb commented 1 year ago

Some extra info:

git bisecting (orgmode repo) with my journal files showed

3c4290e668b15c64e6d48b1926291987742476e8 is the first bad commit
commit 3c4290e668b15c64e6d48b1926291987742476e8
Author: Ihor Radchenko <yantar92@gmail.com>
Date:   Sun Oct 17 00:00:01 2021 +0800

    org.el/org-scan-tags: Make use of fast `org-element-cache-map'

 lisp/org.el | 304 ++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 195 insertions(+), 109 deletions(-)

Using that info, a workaround for the issue is to set org-element-use-cache to nil (temporarily).

bastibe commented 1 year ago

Great work! Thank you for figuring this out!

Would you like to contribute the workaround to the README? Probably somewhere near the top, so as to warn users of this issue in Org 9.6.

mrvdb commented 1 year ago

[Bastian Bechtold]:

Would you like to contribute the workaround to the README? Probably somewhere near the top, so as to warn users of this issue in Org 9.6.

That's probably a good idea, I'm also checking if it is an option to wrap the 'carryover'; disabling the cache or invalidating some parts of it.

I can imagine the carryover should be a cache-invalidating operation, but I am not familiar enough with the org code to say for sure.

bastibe commented 1 year ago

We could maybe call org-element-cache-reset after the carryover (possibly on both the old and the new file). It might also be possible to call org-element-cache-refresh on just the changed areas, but that's probably more fragile.

Andsbf commented 10 months ago

thanks for updating the readme, saved me a couple hours of debugging, and assuring my self I was sure it worked before.

pglpm commented 1 month ago

Is this issue still present with org 9.7?

mrvdb commented 1 month ago

Since disabling the cache it has been a workable situation for me. Sporadically I get a repeat of the #TITLE line from the previous day (once a month or so), but could live with that workaround.

I have upgraded to Org mode version 9.8-pre (release_9.7.2-13-gc426f4) and re-enabled the cache (setq org-element-use-cache t) So far so good.

mrvdb commented 1 month ago

I'm considering this issue resolved from my point of view. ERT tests all succeed, I can't reproduce the problems anymore using org 9.7 and later and the cache setting is not needed anymore from what I can see.