all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries
@ 2022-06-26 18:36 hokomo
  2022-06-27  8:04 ` Lars Ingebrigtsen
  0 siblings, 1 reply; 10+ messages in thread
From: hokomo @ 2022-06-26 18:36 UTC (permalink / raw)
  To: 56241

[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]

icalendar is capable of exporting arbitrary diary sexp entries, but it 
looks like there's a bug in the code. Each conversion function 
`icalendar--convert-*-to-ical' can return either a dotted pair or a list 
of such pairs, but the code fails to differentiate the two cases properly.

To reproduce:

(require 'icalendar)

(defun test-diary-sexp (sexp)
   (message "Testing %S\n" sexp)
   (let ((file (make-temp-file "export.ics")))
     (with-temp-buffer
       (insert sexp)
       (icalendar-export-region (point-min) (point-max) file))
     (with-current-buffer (get-buffer "*icalendar-errors*")
       (message "export: %S\n" (with-temp-buffer
                             (insert-file-contents file)
                             (buffer-string)))
       (message "errors: %S" (buffer-string))))
   (terpri))

(defun my-float (&rest args)
   (apply #'diary-float args))

(let ((icalendar-export-sexp-enumeration-days 366))
   (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1")
   (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2"))

Exporting the my-float sexp will fail with:

"Error in line 0 -- (wrong-type-argument listp First Sunday in July 2): 
‘%%(my-float 7 0 1) First Sunday in July 2"

because the returned list is confused for a dotted pair.

`icalendar-export-sexp-enumeration-days' is set to 366 to guarantee that 
the sexp event occurs at least once. It looks like there's a different 
bug (?) where, even if an entry is recognized as an arbitrary diary 
sexp, if it doesn't produce any events, the converter will go ahead with 
trying to interpret it in a different way and eventually fail. E.g., 
lowering the enumeration days to 0 gives:

"Error in line 0 -- (error Could not parse date): ‘%%(my-float 7 0 1) 
First Sunday in July 2’"

after exhausting all of the known entry types. Should I file this as a 
separate bug?

Regards,
hokomo

[-- Attachment #2: 0001-Fix-detecting-dotted-pairs.patch --]
[-- Type: text/x-patch, Size: 1083 bytes --]

From 691f34a301359893bcffff8e4e0c8794a1fd5e68 Mon Sep 17 00:00:00 2001
From: hokomo <hokomo@airmail.cc>
Date: Sat, 25 Jun 2022 09:44:54 +0200
Subject: [PATCH] Fix detecting dotted pairs

* lisp/calendar/icalendar.el (icalendar-export-region): Fix detecting
dotted pairs.
---
 lisp/calendar/icalendar.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lisp/calendar/icalendar.el b/lisp/calendar/icalendar.el
index 1a5a071e20..cf54293989 100644
--- a/lisp/calendar/icalendar.el
+++ b/lisp/calendar/icalendar.el
@@ -1144,7 +1144,8 @@ icalendar-export-region
                                      (cdr contents-n-summary))))
                       (setq result (concat result header contents alarm
                                            "\nEND:VEVENT")))
-                    (if (consp cns-cons-or-list)
+                    (if (and (consp cns-cons-or-list)
+                             (not (listp (cdr cns-cons-or-list))))
                         (list cns-cons-or-list)
                       cns-cons-or-list)))
           ;; handle errors
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries
  2022-06-26 18:36 bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries hokomo
@ 2022-06-27  8:04 ` Lars Ingebrigtsen
  2022-06-27  9:42   ` hokomo
  0 siblings, 1 reply; 10+ messages in thread
From: Lars Ingebrigtsen @ 2022-06-27  8:04 UTC (permalink / raw)
  To: hokomo; +Cc: 56241

hokomo <hokomo@airmail.cc> writes:

> Exporting the my-float sexp will fail with:
>
> "Error in line 0 -- (wrong-type-argument listp First Sunday in July
> 2): ‘%%(my-float 7 0 1) First Sunday in July 2"
>
> because the returned list is confused for a dotted pair.

Thanks; patch pushed to Emacs 29.

> `icalendar-export-sexp-enumeration-days' is set to 366 to guarantee
> that the sexp event occurs at least once. It looks like there's a
> different bug (?) where, even if an entry is recognized as an
> arbitrary diary sexp, if it doesn't produce any events, the converter
> will go ahead with trying to interpret it in a different way and
> eventually fail. E.g., lowering the enumeration days to 0 gives:
>
> "Error in line 0 -- (error Could not parse date): ‘%%(my-float 7 0 1)
> First Sunday in July 2’"
>
> after exhausting all of the known entry types. Should I file this as a
> separate bug?

No, we can work on this problem here in this bug report.

Do you have a recipe to reproduce the problem, starting from "emacs -Q"?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries
  2022-06-27  8:04 ` Lars Ingebrigtsen
@ 2022-06-27  9:42   ` hokomo
  2022-06-28 11:21     ` Lars Ingebrigtsen
  0 siblings, 1 reply; 10+ messages in thread
From: hokomo @ 2022-06-27  9:42 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 56241

[-- Attachment #1: Type: text/plain, Size: 3207 bytes --]

>> `icalendar-export-sexp-enumeration-days' is set to 366 to guarantee
>> that the sexp event occurs at least once. It looks like there's a
>> different bug (?) where, even if an entry is recognized as an
>> arbitrary diary sexp, if it doesn't produce any events, the converter
>> will go ahead with trying to interpret it in a different way and
>> eventually fail. E.g., lowering the enumeration days to 0 gives:
>>
>> "Error in line 0 -- (error Could not parse date): ‘%%(my-float 7 0 1)
>> First Sunday in July 2’"
>>
>> after exhausting all of the known entry types. Should I file this as a
>> separate bug?
> 
> No, we can work on this problem here in this bug report.
> 
> Do you have a recipe to reproduce the problem, starting from "emacs -Q"?

Yep, you can use the same testing script and just replace the last 
expression with:

(let ((icalendar-export-sexp-enumeration-days 0))
   (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1")
   (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2"))

which should give you the error I mentioned. I don't have a patch for 
this on hand but it would probably require restructuring the surrounding 
code a little bit.

There's another bug that concerns sexps that contain more than a single 
closing parenthesis. Seems like the icalendar code uses a bunch of 
regexes to parse the sexp (see `icalendar--convert-sexp-to-ical'), 
rather than something like `read' or `read-from-string' (which is what 
diary does, as well as org-agenda). To reproduce (with the same testing 
code as before):

(let ((icalendar-export-sexp-enumeration-days 366))
   (test-diary-sexp "%%(= (calendar-day-of-week date) 0) Sunday 1")
   (test-diary-sexp "%%(= 0 (calendar-day-of-week date)) Sunday 2"))

No matter whether the closing parentheses are bunched together or 
separated, parsing fails either way. I've attached a draft of a patch 
that modifies as little as possible and makes the above two cases work, 
but that's not enough as there's special handling of `and' forms. 
Furthermore, sexps that span multiple lines fail for the same reason, 
even though diary handles them just fine of course. Ideally the whole 
function should be rewritten to just use `read-from-string'. I don't 
understand why `and' forms are handled specially though, so I'm not sure 
how to proceed here.

A last thing to note -- even though org-agenda properly handles 
multiline sexps and sexps with an arbitrary number of closing 
parentheses (because it does the parsing manually and uses 
`forward-sexp'), Org's parser itself does not properly handle multiline 
sexps (but does handle multiple closing parentheses). To confirm, use 
`org-element-at-point' at the beginning of these two diary sexps in an 
org-mode buffer:

%%(let ((a 123) (b 456)) (+ a b))

%%(let ((a 123)
         (b 456))
    (+ a b))

All of these bugs also propagate to ox-icalendar (which is how I got 
into exploring this whole thing) since it relies on the Org parse tree 
to pull out 'diary-sexp elements and on icalendar to export them into 
iCalendar format.

Let me know if you want me to report any of these as separate bugs and 
what would be the best thing to do here.

Regards,
hokomo

[-- Attachment #2: 0002-Draft-fix-nested-diary-sexps.patch --]
[-- Type: text/x-patch, Size: 1197 bytes --]

From e35ea1f3a9c62d9c07d10d9ec89bf879058775c2 Mon Sep 17 00:00:00 2001
From: hokomo <hokomo@airmail.cc>
Date: Sat, 25 Jun 2022 13:43:28 +0200
Subject: [PATCH] Fix nested diary sexps

---
 lisp/calendar/icalendar.el | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lisp/calendar/icalendar.el b/lisp/calendar/icalendar.el
index 46bcf4566f..2b6af79812 100644
--- a/lisp/calendar/icalendar.el
+++ b/lisp/calendar/icalendar.el
@@ -1641,9 +1641,11 @@ icalendar--convert-sexp-to-ical
                        entry-main)
          ;; regular sexp entry
          (icalendar--dmsg "diary-sexp %s" entry-main)
-         (let ((p1 (substring entry-main (match-beginning 1) (match-end 1)))
-               (p2 (substring entry-main (match-beginning 2) (match-end 2)))
-               (now (or start (current-time))))
+         (let* ((entry-main (substring entry-main 2))
+                (res (read-from-string entry-main))
+                (p1 (prin1-to-string (car res)))
+                (p2 (substring entry-main (cdr res)))
+                (now (or start (current-time))))
            (delete nil
                    (mapcar
                     (lambda (offset)
-- 
2.35.3


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries
  2022-06-27  9:42   ` hokomo
@ 2022-06-28 11:21     ` Lars Ingebrigtsen
  2022-07-06 18:58       ` hokomo
  2022-07-24 18:14       ` Ulf Jasper
  0 siblings, 2 replies; 10+ messages in thread
From: Lars Ingebrigtsen @ 2022-06-28 11:21 UTC (permalink / raw)
  To: hokomo; +Cc: Ulf Jasper, 56241

hokomo <hokomo@airmail.cc> writes:

> Yep, you can use the same testing script and just replace the last
> expression with:
>
> (let ((icalendar-export-sexp-enumeration-days 0))
>   (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1")
>   (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2"))
>
> which should give you the error I mentioned.

Right; I can reproduce that problem.  I've added Ulf to the CCs; perhaps
he has some comments.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries
  2022-06-28 11:21     ` Lars Ingebrigtsen
@ 2022-07-06 18:58       ` hokomo
  2022-07-07  8:08         ` Lars Ingebrigtsen
  2022-07-24 18:14       ` Ulf Jasper
  1 sibling, 1 reply; 10+ messages in thread
From: hokomo @ 2022-07-06 18:58 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: Ulf Jasper, 56241

> Right; I can reproduce that problem.  I've added Ulf to the CCs; perhaps
> he has some comments.

It's been a week since then. Any ideas on how to move forward or should 
we give it more time? :-)

To summarize, the problems are:

(1) A diary sexp not generating any events for the configured number of 
enumeration days is treated effectively as a parsing failure, meaning 
that the next type of diary entry is tried instead of just not producing 
any events.

(2) Improper parsing of diary sexps (using regexes instead of e.g. 
`read') leads to diary sexps not being able to span multiple lines or 
have more than a single closing parenthesis (i.e. any sort of nested 
expression).

(3) Not strictly in the scope of icalendar itself, but still related the 
handling of diary sexps: Org's parser seems to assume that each diary 
sexp is on a single line. This and the two limitations above end up 
propagating to ox-icalendar.

hokomo





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries
  2022-07-06 18:58       ` hokomo
@ 2022-07-07  8:08         ` Lars Ingebrigtsen
  0 siblings, 0 replies; 10+ messages in thread
From: Lars Ingebrigtsen @ 2022-07-07  8:08 UTC (permalink / raw)
  To: hokomo; +Cc: Ulf Jasper, 56241

hokomo <hokomo@airmail.cc> writes:

> It's been a week since then. Any ideas on how to move forward or
> should we give it more time? :-)

If you have a possible patch for the issues, that'd be appreciated.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries
  2022-06-28 11:21     ` Lars Ingebrigtsen
  2022-07-06 18:58       ` hokomo
@ 2022-07-24 18:14       ` Ulf Jasper
  2022-11-11 13:38         ` Stefan Kangas
  1 sibling, 1 reply; 10+ messages in thread
From: Ulf Jasper @ 2022-07-24 18:14 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: 56241, hokomo

Am 28.06.2022 um 13:21 (+0200) schrieb Lars Ingebrigtsen:
> hokomo <hokomo@airmail.cc> writes:
>
>> Yep, you can use the same testing script and just replace the last
>> expression with:
>>
>> (let ((icalendar-export-sexp-enumeration-days 0))
>>   (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1")
>>   (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2"))
>>
>> which should give you the error I mentioned.
>
> Right; I can reproduce that problem.  I've added Ulf to the CCs; perhaps
> he has some comments.

Sorry for the late reply.  Thanks for the bug report, the patch and for
analysing the problems.  I'll try to have a look at the icalendar code
the next days (or so).





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries
  2022-07-24 18:14       ` Ulf Jasper
@ 2022-11-11 13:38         ` Stefan Kangas
  2022-11-23 20:24           ` Ulf Jasper
  0 siblings, 1 reply; 10+ messages in thread
From: Stefan Kangas @ 2022-11-11 13:38 UTC (permalink / raw)
  To: Ulf Jasper; +Cc: Lars Ingebrigtsen, 56241, hokomo

Ulf Jasper <ulf.jasper@web.de> writes:

> Am 28.06.2022 um 13:21 (+0200) schrieb Lars Ingebrigtsen:
>> hokomo <hokomo@airmail.cc> writes:
>>
>>> Yep, you can use the same testing script and just replace the last
>>> expression with:
>>>
>>> (let ((icalendar-export-sexp-enumeration-days 0))
>>>   (test-diary-sexp "%%(diary-float 7 0 1) First Sunday in July 1")
>>>   (test-diary-sexp "%%(my-float 7 0 1) First Sunday in July 2"))
>>>
>>> which should give you the error I mentioned.
>>
>> Right; I can reproduce that problem.  I've added Ulf to the CCs; perhaps
>> he has some comments.
>
> Sorry for the late reply.  Thanks for the bug report, the patch and for
> analysing the problems.  I'll try to have a look at the icalendar code
> the next days (or so).

That was 15 weeks ago, so here's a friendly ping.





^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries
  2022-11-11 13:38         ` Stefan Kangas
@ 2022-11-23 20:24           ` Ulf Jasper
  2022-11-24 18:07             ` Ulf Jasper
  0 siblings, 1 reply; 10+ messages in thread
From: Ulf Jasper @ 2022-11-23 20:24 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: Lars Ingebrigtsen, 56241, hokomo

I added a test to icalendar-tests.el that verifies that patch
0001-Fix-detecting-dotted-pairs.patch works as expected.

I also prepared two additional tests for the other examples where
icalendar-export fails.  For the time being they are commented out as
there is no fix available yet.  Patch
0002-Draft-fix-nested-diary-sexps.patch seems to fix one of the examples
but apparently breaks test 'icalendar-real-world'.  So a look at
'icalendar--convert-sexp-to-ical' seems to be necessary.

Best,
Ulf






^ permalink raw reply	[flat|nested] 10+ messages in thread

* bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries
  2022-11-23 20:24           ` Ulf Jasper
@ 2022-11-24 18:07             ` Ulf Jasper
  0 siblings, 0 replies; 10+ messages in thread
From: Ulf Jasper @ 2022-11-24 18:07 UTC (permalink / raw)
  To: Stefan Kangas; +Cc: Lars Ingebrigtsen, 56241, hokomo

Am 23.11.2022 um 21:24 (+0100) schrieb Ulf Jasper:
> I added a test to icalendar-tests.el that verifies that patch
> 0001-Fix-detecting-dotted-pairs.patch works as expected.
>
> I also prepared two additional tests for the other examples where
> icalendar-export fails.  For the time being they are commented out as
> there is no fix available yet.  Patch
> 0002-Draft-fix-nested-diary-sexps.patch seems to fix one of the examples
> but apparently breaks test 'icalendar-real-world'.  So a look at
> 'icalendar--convert-sexp-to-ical' seems to be necessary.

There was a testcase in 'icalendar-real-world' that relied on
icalendar-export to fail on a particular sexp.  With patch 0002... that
testcase behaves differently (i.e. export works correctly now).  In
other words: breaking that test was correct.

Applied patch 0002... and updated icalendar-tests.

Best,
Ulf





^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-11-24 18:07 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-26 18:36 bug#56241: [PATCH] icalendar doesn't correctly process arbitrary diary sexp entries hokomo
2022-06-27  8:04 ` Lars Ingebrigtsen
2022-06-27  9:42   ` hokomo
2022-06-28 11:21     ` Lars Ingebrigtsen
2022-07-06 18:58       ` hokomo
2022-07-07  8:08         ` Lars Ingebrigtsen
2022-07-24 18:14       ` Ulf Jasper
2022-11-11 13:38         ` Stefan Kangas
2022-11-23 20:24           ` Ulf Jasper
2022-11-24 18:07             ` Ulf Jasper

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.