Parse malformed clocklines (was: Re: [PATCH] lisp/org-clock.el (org-clock-sum): Rewrite regex using rx)

[Morgan Smith <morgan.j.smith@outlook.com>]

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

Ihor Radchenko <yantar92@posteo.net> writes:

> Morgan Smith <morgan.j.smith@outlook.com> writes:
>
>>> That's expected.
>>> We have the following _syntax_ description for clock lines:
>>>
>>> https://orgmode.org/worg/org-syntax.html#Clocks...>> clock: INACTIVE-TIMESTAMP-RANGE DURATION
>> ...
>> My specific issue is that the ":*-end" stuff can be set when the
>> "*-start" stuff is not.
>> ...
>> CLOCK: [2023-11-15 Wed 15rr:26]--[2023-11-15 Wed 16:12] =>  0:46
>> calculated time: 58320.0
>> ...
>> What this shows is that an invalid end will cause the entry to be
>> ignored, but an invalid start will not.
>
> Yup. Everything makes sense, but only within syntax spec. Actual code
> that handles such clock lines is another story.
>
> Warning is important because a number of people use clock functionality
> for billing - miscalculating clock sums can literally affect people's
> income :)
>
> I think that the best course of action when a problematic timestamp
> without opening/closing time is encountered is:
>
> 1. Warn user
> 2. Still calculate the duration, assuming 0s in time (simply because
>    previous versions of Org mode did it)
>
> (2) is kind of debatable, but I can imagine some users might make use of
>
> such feature by putting clocks by hand:
> CLOCK: [2023-11-15 Wed]--[2023-11-16 Thu] => 24:00
>
> These users may then simply suppress the warning.

I'm very tempted to just make it hard rule that clocklines need to be
fully specified.  If you take a look at the attached patch, it shows one
way we could do this.

First, I modify the timestamp parser so the ":*-end" variables don't
always inherit the ":*-start" variables.  They can be nil independent of
the start.

Then in the clockline parser I ensure that every single field is set.
If it isn't, then I set the timestamp to nil so it won't be used.

This preserves the flexibility you want in timestamp parsing while also
catching many malformed clocklines.

Let me know what you think of this approach.

Also a couple questions about this approach:

1. Will changing the timestamp parser have far-reaching implications?
Is this something I should avoid?

2. Is there a way to give user errors in the parser code?  I'm using
`org-element--cache-warn' in my patch but I'm not sure that's the right
thing to do.  (if it is the right thing then we need to define it
earlier in the file)



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Warn-about-invalid-clock-lines.patch --]
[-- Type: text/x-patch, Size: 3056 bytes --]

From 82231fb4b11b780488c66b4e5f0ee6fcf6643f1d Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Wed, 19 Jun 2024 13:41:50 -0400
Subject: [PATCH] Warn about invalid clock lines

---
 lisp/org-element.el | 32 +++++++++++++++++++++++++-------
 1 file changed, 25 insertions(+), 7 deletions(-)

diff --git a/lisp/org-element.el b/lisp/org-element.el
index d6ad9824a..113cd6059 100644
--- a/lisp/org-element.el
+++ b/lisp/org-element.el
@@ -2312,6 +2312,25 @@ Return a new syntax node of `clock' type containing `:status',
 			 (unless (bolp) (skip-chars-forward " \t"))
 			 (count-lines before-blank (point))))
 	   (end (point)))
+      (when (and value ;; Clock lines don't need timestamps
+                 (or
+                  (not
+                   (and (org-element-property :year-start value)
+                        (org-element-property :month-start value)
+                        (org-element-property :day-start value)
+                        (org-element-property :hour-start value)
+                        (org-element-property :minute-start value)))
+                  (and (eq status 'closed)
+                       (not (and (org-element-property :year-end value)
+                                 (org-element-property :month-end value)
+                                 (org-element-property :day-end value)
+                                 (org-element-property :hour-end value)
+                                 (org-element-property :minute-end value))))))
+        (setq value nil)
+        (org-element--cache-warn "Invalid clock element at %s:%d: \"%s\""
+                                 (buffer-name)
+                                 (line-number-at-pos begin t)
+                                 (buffer-substring-no-properties begin end)))
       (org-element-create
        'clock
        (list :status status
@@ -4393,15 +4412,14 @@ Assume point is at the beginning of the timestamp."
 		  hour-start (nth 2 date)
 		  minute-start (nth 1 date))))
 	;; Compute date-end.  It can be provided directly in timestamp,
-	;; or extracted from time range.  Otherwise, it defaults to the
-	;; same values as date-start.
+	;; or extracted from time range.
 	(unless diaryp
 	  (let ((date (and date-end (org-parse-time-string date-end t))))
-	    (setq year-end (or (nth 5 date) year-start)
-		  month-end (or (nth 4 date) month-start)
-		  day-end (or (nth 3 date) day-start)
-		  hour-end (or (nth 2 date) (car time-range) hour-start)
-		  minute-end (or (nth 1 date) (cdr time-range) minute-start))))
+	    (setq year-end (or (nth 5 date) (and time-range year-start))
+		  month-end (or (nth 4 date) (and time-range month-start))
+		  day-end (or (nth 3 date) (and time-range day-start))
+		  hour-end (or (nth 2 date) (car time-range))
+		  minute-end (or (nth 1 date) (cdr time-range)))))
         ;; Diary timestamp with time.
         (when (and diaryp
                    (string-match "\\([012]?[0-9]\\):\\([0-5][0-9]\\)\\(-\\([012]?[0-9]\\):\\([0-5][0-9]\\)\\)?" date-start))
-- 
2.45.1