[PATCH] lisp/org-clock.el (org-clock-sum): Rewrite regex using rx

[PATCH] lisp/org-clock.el (org-clock-sum): Rewrite regex using rx

[Morgan Smith]

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

Hello!

See two attached patches.  All tests pass on my computer.

Every once in a while I feel obligated to go back to org-clock-sum to
try and optimize it.  I have a file with 8 clocktables in it and it
takes forever to update.  This time I decided instead of trying to
optimize, I'm just going to try and understand.

The regex has been altered slightly.

1. Instead of using "[ \t]", I decided to use [[:blank:]].  No real
reason.  I just think it's easier to read and maybe slightly more
correct?

2. For the timestamps, instead of ".*?" (using a non-greedy ".*") I
decided to use "[^]]*" (accept everything except "]").  I did this simply
because I'm not used to using non-greedy regex's.  Maybe this way
performs better?  I didn't test that.

3. I used the variable `org-outline-regexp' but that doesn't actually
change the regex.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-org-clock.el-org-clock-sum-Rewrite-regex-using-.patch --]
[-- Type: text/x-patch, Size: 1689 bytes --]

From 3c3d7abed25cafb2be1096ca079a0e8be907c644 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Thu, 11 Apr 2024 12:23:21 -0400
Subject: [PATCH 1/2] lisp/org-clock.el (org-clock-sum): Rewrite regex using rx

---
 lisp/org-clock.el | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index 65a54579a..5ef987ab8 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -2008,9 +2008,23 @@ each headline in the time range with point at the headline.  Headlines for
 which HEADLINE-FILTER returns nil are excluded from the clock summation.
 PROPNAME lets you set a custom text property instead of :org-clock-minutes."
   (with-silent-modifications
-    (let* ((re (concat "^\\(\\*+\\)[ \t]\\|^[ \t]*"
-		       org-clock-string
-		       "[ \t]*\\(?:\\(\\[.*?\\]\\)-+\\(\\[.*?\\]\\)\\|=>[ \t]+\\([0-9]+\\):\\([0-9]+\\)\\)"))
+    (let* ((re (rx line-start
+                   (or
+                    (group (regexp org-outline-regexp))
+                    (seq (* blank)
+                         (literal org-clock-string)
+                         (* blank)
+                         (or
+                          (seq
+                           (group "[" (* (not "]")) "]")
+                           (+ "-")
+                           (group "[" (* (not "]")) "]"))
+                          (seq
+                           "=>"
+                           (+ blank)
+                           (group (+ digit))
+                           ":"
+                           (group (+ digit))))))))
 	   (lmax 30)
 	   (ltimes (make-vector lmax 0))
 	   (level 0)
-- 
2.41.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Test-clock-times-without-timestamps.patch --]
[-- Type: text/x-patch, Size: 1237 bytes --]

From e5298920568e4c5a34589640f11edfa09a98d0d1 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Thu, 11 Apr 2024 12:51:18 -0400
Subject: [PATCH 2/2] Test clock times without timestamps

* testing/lisp/test-org-clock.el (test-org-clock/clocktable/insert):
Add a clock time that does not include timestamps.
---
 testing/lisp/test-org-clock.el | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/testing/lisp/test-org-clock.el b/testing/lisp/test-org-clock.el
index 44c62e7bc..be8acb529 100644
--- a/testing/lisp/test-org-clock.el
+++ b/testing/lisp/test-org-clock.el
@@ -345,13 +345,12 @@ CLOCK: [2022-11-03 %s 06:00]--[2022-11-03 %s 06:01] =>  0:01
    (equal
     "| Headline     | Time   |
 |--------------+--------|
-| *Total time* | *1:00* |
+| *Total time* | *2:00* |
 |--------------+--------|
-| H1           | 1:00   |"
+| H1           | 2:00   |"
     (org-test-with-temp-text "* H1\n<point>"
-      (insert (org-test-clock-create-clock ". 1:00" ". 2:00"))
-
-      (goto-line 2)
+      (insert (org-test-clock-create-clock ". 1:00" ". 2:00")
+              "CLOCK: => 1:00\n")
       (require 'org-clock)
       (org-dynamic-block-insert-dblock "clocktable")
 
-- 
2.41.0

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

[Ihor Radchenko]

Morgan Smith <Morgan.J.Smith@outlook.com> writes:

> See two attached patches.  All tests pass on my computer.
>
> Every once in a while I feel obligated to go back to org-clock-sum to
> try and optimize it.  I have a file with 8 clocktables in it and it
> takes forever to update.  This time I decided instead of trying to
> optimize, I'm just going to try and understand.
>
> The regex has been altered slightly.
>
> 1. Instead of using "[ \t]", I decided to use [[:blank:]].  No real
> reason.  I just think it's easier to read and maybe slightly more
> correct?
>
> 2. For the timestamps, instead of ".*?" (using a non-greedy ".*") I
> decided to use "[^]]*" (accept everything except "]").  I did this simply
> because I'm not used to using non-greedy regex's.  Maybe this way
> performs better?  I didn't test that.
>
> 3. I used the variable `org-outline-regexp' but that doesn't actually
> change the regex.

Thanks for the patch!
I think that a better approach would be re-using the parser constant
`org-element-clock-line-re'. 

> * testing/lisp/test-org-clock.el (test-org-clock/clocktable/insert):
> Add a clock time that does not include timestamps.
> ...
> -
> -      (goto-line 2)
> +      (insert (org-test-clock-create-clock ". 1:00" ". 2:00")
> +              "CLOCK: => 1:00\n")

This is not a valid clock format. Matching such lines is a bug.
See https://list.orgmode.org/orgmode/87wpkkhafc.fsf@saiph.selenimh/

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Morgan Smith]

Ihor Radchenko <yantar92@posteo.net> writes:

>> * testing/lisp/test-org-clock.el (test-org-clock/clocktable/insert):
>> Add a clock time that does not include timestamps.
>> ...
>> -
>> -      (goto-line 2)
>> +      (insert (org-test-clock-create-clock ". 1:00" ". 2:00")
>> +              "CLOCK: => 1:00\n")
>
> This is not a valid clock format. Matching such lines is a bug.
> See https://list.orgmode.org/orgmode/87wpkkhafc.fsf@saiph.selenimh/

Let me preface this defense with the fact that I don't like this format
and I don't think we should support it.  Rewriting `org-clock-sum' would
be much easier if we drop support for it.  However, I do believe we
currently support it.

First of all, it currently does work.

Accord to the "Version 4.78" release notes as found on worg, this is
valid.

```
   - You may specify clocking times by hand (i.e. without
     clocking in and out) using this syntax.

     : CLOCK: => 2:00

     Thanks to Scott Jaderholm for this proposal.
```

Also last time I went to rewrite `org-clock-sum' you said
(https://list.orgmode.org/orgmode/87bkg7xbxo.fsf@localhost/):

```
Further, you dropped the

	     ((match-end 4)
	      ;; A naked time.

branch of the code, which accounts for CLOCK: => HH:MM lines that are not clock elements.
```

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

[Ihor Radchenko]

Morgan Smith <morgan.j.smith@outlook.com> writes:

>>> -      (goto-line 2)
>>> +      (insert (org-test-clock-create-clock ". 1:00" ". 2:00")
>>> +              "CLOCK: => 1:00\n")
>>
>> This is not a valid clock format. Matching such lines is a bug.
>> See https://list.orgmode.org/orgmode/87wpkkhafc.fsf@saiph.selenimh/
>
> Let me preface this defense with the fact that I don't like this format
> and I don't think we should support it.  Rewriting `org-clock-sum' would
> be much easier if we drop support for it.  However, I do believe we
> currently support it.
>
> First of all, it currently does work.
>
> Accord to the "Version 4.78" release notes as found on worg, this is
> valid.
>
> ```
>    - You may specify clocking times by hand (i.e. without
>      clocking in and out) using this syntax.
>
>      : CLOCK: => 2:00
>
>      Thanks to Scott Jaderholm for this proposal.
> ```

This is convincing. I did not know that this format is explicitly
mentioned in the news.

Our general rule is that we do not drop existing features in Org mode
except extraordinary circumstances:
https://bzg.fr/en/the-software-maintainers-pledge/
Especially when they are documented.

So, in the message I linked, Nicolas (the major Org mode contributor)
was not right. I hence need to fix the parser and update Org syntax
page. This includes fixing `org-element-clock-line-re' to account for
CLOCK: => 1:00 syntax.

Luckily, it does not look like we are going to break the existing
external exporter packages as long as they are using ox.el API -
`org-export-translate' works just fine with missing timestamps.

> Also last time I went to rewrite `org-clock-sum' you said
> (https://list.orgmode.org/orgmode/87bkg7xbxo.fsf@localhost/):
>
> ```
> Further, you dropped the
>
> 	     ((match-end 4)
> 	      ;; A naked time.
>
> branch of the code, which accounts for CLOCK: => HH:MM lines that are not clock elements.
> ```

Yup. Although I did not see Nicolas' message that time. My judgment was
simply based on looking at the code and seeing that CLOCK: => HH:MM
matching was clearly intentional.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Morgan Smith]

Ihor Radchenko <yantar92@posteo.net> writes:

> So, in the message I linked, Nicolas (the major Org mode contributor)
> was not right. I hence need to fix the parser and update Org syntax
> page. This includes fixing `org-element-clock-line-re' to account for
> CLOCK: => 1:00 syntax.

Cool.  I guess ping this thread when that's done so I can give you
another version of the patch.  Or if you'd like help with that stuff let
me know.  I'm here to help.

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

[Ihor Radchenko]

Morgan Smith <morgan.j.smith@outlook.com> writes:

> Ihor Radchenko <yantar92@posteo.net> writes:
>
>> So, in the message I linked, Nicolas (the major Org mode contributor)
>> was not right. I hence need to fix the parser and update Org syntax
>> page. This includes fixing `org-element-clock-line-re' to account for
>> CLOCK: => 1:00 syntax.
>
> Cool.  I guess ping this thread when that's done so I can give you
> another version of the patch.  Or if you'd like help with that stuff let
> me know.  I'm here to help.

Ping ;)

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Morgan Smith]

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

Ihor Radchenko <yantar92@posteo.net> writes:

> Ping ;)

So I gave up on this specific patch because I wrote a patch to just
rewrite the entire `org-clock-sum' function using org-element API.
Attached is the `org-clock-sum' rewrite patch which I've been using for
a while with no issues.  I have half finished patches locally to add
more clocktable tests and to add clocktable benchmarks which is why I
hadn't submitted this yet.

This probably belongs in this email thread instead:
https://list.orgmode.org/87y18vxgjs.fsf@localhost/

I believe I fixed all the points you brought up in that thread.


While this patch is probably ready to merge, doing so might cause
regressions to the fix applied in commit
fd8ddf2874ca00505aa096c6172ea750cd5e9eaa.

Ideally the fix in that commit should be ported to the org-element API.
Notably, the malformed clock from the email thread from that commit is
parsed a little strangely by org-element.  I'm not sure what effect this
has on my rewrite patch but regardless, we should probably fix this.
Notice how ":day-end" and ":minute-end" are set but not ":hour-start" or
":minute-start".

I have attached a rough patch adding a test for this case but my brain
is currently melting from a heatwave so it might take me a while to make
it into something good.  Feel free to work on this yourself if you have
time.

"CLOCK: [2012-01-01 sun. 00rr:01]--[2012-01-01 sun. 00:02] =>  0:01"

(clock
 (:standard-properties ...
  :status closed :value
  (timestamp
   (:standard-properties ...
    :type inactive-range :range-type daterange :raw-value
    "[2012-01-01 sun. 00rr:01]--[2012-01-01 sun. 00:02]" :year-start 2012
    :month-start 1 :day-start 1 :hour-start nil :minute-start nil :year-end
    2012 :month-end 1 :day-end 1 :hour-end 0 :minute-end 2))
  :duration "0:01"))


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-lisp-org-clock.el-org-clock-sum-Rewrite-using-elemen.patch --]
[-- Type: text/x-patch, Size: 11413 bytes --]

From d407b357ba4285acc3c2548e38f034b5665b40bb Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Thu, 11 Apr 2024 12:23:21 -0400
Subject: [PATCH 1/2] lisp/org-clock.el (org-clock-sum): Rewrite using element
 api

---
 lisp/org-clock.el | 200 +++++++++++++++++++++++-----------------------
 1 file changed, 99 insertions(+), 101 deletions(-)

diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index c6fd507b0..5842c1cc7 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -33,15 +33,13 @@
 
 (require 'cl-lib)
 (require 'org)
+(require 'org-element)
 
 (declare-function calendar-iso-to-absolute "cal-iso" (date))
 (declare-function notifications-notify "notifications" (&rest params))
 (declare-function org-element-property "org-element-ast" (property node))
-(declare-function org-element-contents-end "org-element" (node))
-(declare-function org-element-end "org-element" (node))
 (declare-function org-element-type "org-element-ast" (node &optional anonymous))
 (declare-function org-element-type-p "org-element-ast" (node types))
-(defvar org-element-use-cache)
 (declare-function org-inlinetask-at-task-p "org-inlinetask" ())
 (declare-function org-inlinetask-goto-beginning "org-inlinetask" ())
 (declare-function org-inlinetask-goto-end "org-inlinetask" ())
@@ -2021,105 +2019,68 @@ TSTART and TEND can mark a time range to be considered.
 HEADLINE-FILTER is a zero-arg function that, if specified, is called for
 each headline in the time range with point at the headline.  Headlines for
 which HEADLINE-FILTER returns nil are excluded from the clock summation.
-PROPNAME lets you set a custom text property instead of :org-clock-minutes."
+PROPNAME lets you set a custom text property instead of :org-clock-minutes.
+
+Clocking entries that are open (as in don't have an end time) that are
+not the current clocking entry will be ignored."
   (with-silent-modifications
-    (let* ((re (concat "^\\(\\*+\\)[ \t]\\|^[ \t]*"
-		       org-clock-string
-		       "[ \t]*\\(?:\\(\\[.*?\\]\\)-+\\(\\[.*?\\]\\)\\|=>[ \t]+\\([0-9]+\\):\\([0-9]+\\)\\)"))
-	   (lmax 30)
-	   (ltimes (make-vector lmax 0))
-	   (level 0)
-	   (tstart (cond ((stringp tstart) (org-time-string-to-seconds tstart))
-			 ((consp tstart) (float-time tstart))
-			 (t tstart)))
-	   (tend (cond ((stringp tend) (org-time-string-to-seconds tend))
-		       ((consp tend) (float-time tend))
-		       (t tend)))
-	   (t1 0)
-	   time)
-      (remove-text-properties (point-min) (point-max)
-			      `(,(or propname :org-clock-minutes) t
-				:org-clock-force-headline-inclusion t))
-      (save-excursion
-	(goto-char (point-max))
-	(while (re-search-backward re nil t)
-          (let* ((element (save-match-data (org-element-at-point)))
-                 (element-type (org-element-type element)))
-	    (cond
-	     ((and (eq element-type 'clock) (match-end 2))
-	      ;; Two time stamps.
-              (condition-case nil
-	          (let* ((timestamp (org-element-property :value element))
-		         (ts (float-time
-                              (org-encode-time
-                               (list 0
-                                     (org-element-property :minute-start timestamp)
-                                     (org-element-property :hour-start timestamp)
-                                     (org-element-property :day-start timestamp)
-                                     (org-element-property :month-start timestamp)
-                                     (org-element-property :year-start timestamp)
-                                     nil -1 nil))))
-		         (te (float-time
-                              (org-encode-time
-                               (list 0
-                                     (org-element-property :minute-end timestamp)
-                                     (org-element-property :hour-end timestamp)
-                                     (org-element-property :day-end timestamp)
-                                     (org-element-property :month-end timestamp)
-                                     (org-element-property :year-end timestamp)
-                                     nil -1 nil))))
-		         (dt (- (if tend (min te tend) te)
-			        (if tstart (max ts tstart) ts))))
-	            (when (> dt 0) (cl-incf t1 (floor dt 60))))
-                (error
-                 (org-display-warning (format "org-clock-sum: Ignoring invalid %s" (org-current-line-string))))))
-	     ((match-end 4)
-	      ;; A naked time.
-	      (setq t1 (+ t1 (string-to-number (match-string 5))
-			  (* 60 (string-to-number (match-string 4))))))
-	     ((memq element-type '(headline inlinetask)) ;A headline
-	      ;; Add the currently clocking item time to the total.
-	      (when (and org-clock-report-include-clocking-task
-		         (eq (org-clocking-buffer) (current-buffer))
-		         (eq (marker-position org-clock-hd-marker) (point))
-		         tstart
-		         tend
-		         (>= (float-time org-clock-start-time) tstart)
-		         (<= (float-time org-clock-start-time) tend))
-	        (let ((time (floor (org-time-convert-to-integer
-				    (time-since org-clock-start-time))
-				   60)))
-		  (setq t1 (+ t1 time))))
-	      (let* ((headline-forced
-		      (get-text-property (point)
-				         :org-clock-force-headline-inclusion))
-		     (headline-included
-		      (or (null headline-filter)
-			  (save-excursion
-			    (save-match-data (funcall headline-filter))))))
-	        (setq level (- (match-end 1) (match-beginning 1)))
-	        (when (>= level lmax)
-		  (setq ltimes (vconcat ltimes (make-vector lmax 0)) lmax (* 2 lmax)))
-	        (when (or (> t1 0) (> (aref ltimes level) 0))
-		  (when (or headline-included headline-forced)
-		    (if headline-included
-		        (cl-loop for l from 0 to level do
-			         (aset ltimes l (+ (aref ltimes l) t1))))
-		    (setq time (aref ltimes level))
-		    (goto-char (match-beginning 0))
-                    (put-text-property (point) (line-end-position)
-				       (or propname :org-clock-minutes) time)
-		    (when headline-filter
-		      (save-excursion
-		        (save-match-data
-			  (while (org-up-heading-safe)
-			    (put-text-property
-			     (point) (line-end-position)
-			     :org-clock-force-headline-inclusion t))))))
-		  (setq t1 0)
-		  (cl-loop for l from level to (1- lmax) do
-			   (aset ltimes l 0))))))))
-	(setq org-clock-file-total-minutes (aref ltimes 0))))))
+    (let ((tstart (cond ((stringp tstart) (org-time-string-to-seconds tstart))
+                        ((consp tstart) (float-time tstart))
+                        (t tstart)))
+          (tend (cond ((stringp tend) (org-time-string-to-seconds tend))
+                      ((consp tend) (float-time tend))
+                      (t tend)))
+          (propname (or propname :org-clock-minutes))
+          (t1 0)
+          (total 0)
+          time)
+      (remove-text-properties (point-min) (point-max) `(,propname t))
+      (org-element-cache-map
+       (lambda (headline-or-inlinetask)
+         (when (or (null headline-filter)
+                   (save-excursion
+                     (funcall headline-filter)))
+           (mapc
+            (lambda (range)
+              (setq time
+                    (pcase range
+                      (`(,_ . open)
+                       (when (and org-clock-report-include-clocking-task
+                                  (eq (org-clocking-buffer) (current-buffer))
+                                  (eq (marker-position org-clock-hd-marker)
+                                      (org-element-begin headline-or-inlinetask))
+                                  (or (not tstart)
+                                      (>= (float-time org-clock-start-time) tstart))
+                                  (or (not tend)
+                                      (<= (float-time org-clock-start-time) tend)))
+                         (floor (org-time-convert-to-integer
+                                 (time-since org-clock-start-time))
+                                60)))
+                      ((pred floatp) range)
+                      (`(,time1 . ,time2)
+                       (let* ((ts (float-time time1))
+                              (te (float-time time2))
+                              (dt (- (if tend (min te tend) te)
+                                     (if tstart (max ts tstart) ts))))
+                         (floor dt 60)))))
+              (when (and time (> time 0)) (cl-incf t1 time)))
+            (org--clock-ranges headline-or-inlinetask))
+           (when (> t1 0)
+             (setq total (+ total t1))
+             (org-element-lineage-map headline-or-inlinetask
+                 (lambda (parent)
+                   (put-text-property
+                    (org-element-begin parent) (1- (org-element-contents-begin parent))
+                    propname
+                    (+ t1 (or (get-text-property
+                               (org-element-begin parent)
+                               propname)
+                              0))))
+               ;; TODO: can inlinetasks contain inlinetasks?
+               '(headline) t))
+           (setq t1 0)))
+       :narrow t)
+      (setq org-clock-file-total-minutes total))))
 
 (defun org-clock-sum-current-item (&optional tstart)
   "Return time, clocked on current item in total."
@@ -2134,6 +2095,43 @@ PROPNAME lets you set a custom text property instead of :org-clock-minutes."
       (org-clock-sum tstart)
       org-clock-file-total-minutes)))
 
+(defun org--clock-ranges (headline)
+  "Return a list of clock ranges of HEADLINE.
+Does not recurse into subheadings.
+Ranges are in one of these formats:
+   (cons time . time)
+   (cons time . \\='open) The clock does not have an end time
+   float               The number of minutes as a float"
+  (unless (org-element-type-p headline '(headline inlinetask))
+    (error "Argument must be a headline"))
+  (and
+   (org-element-contents-begin headline) ;; empty headline
+   (or
+    (org-element-cache-get-key headline :clock-ranges)
+    (let ((clock-ranges
+           (org-element-cache-map
+            (lambda (elem)
+              (when (org-element-type-p elem 'clock)
+                (if-let ((timestamp (org-element-property :value elem)))
+                    (cons (org-timestamp-to-time timestamp)
+                          (if (eq 'running (org-element-property :status elem))
+                              'open
+                            (org-timestamp-to-time timestamp t)))
+                  (org-duration-to-minutes (org-element-property :duration elem)))))
+            ;; XXX: using these arguments would be more intuitive
+            ;; but don't seem to work due to bugs in
+            ;; `org-element-cache-map'
+            ;; :restrict-elements '(clock)
+            ;; :after-element headline
+            :granularity 'element
+            :next-re org-element-clock-line-re
+            :from-pos (org-element-contents-begin headline)
+            :to-pos (save-excursion
+                      (goto-char (org-element-begin headline))
+                      (org-entry-end-position)))))
+      (org-element-cache-store-key headline :clock-ranges clock-ranges)
+      clock-ranges))))
+
 ;;;###autoload
 (defun org-clock-display (&optional arg)
   "Show subtree times in the entire buffer.
-- 
2.45.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-malformed-clock-tests.patch --]
[-- Type: text/x-patch, Size: 1846 bytes --]

From 475c8a75c09efb36be9b918520c99d9ab8c374a1 Mon Sep 17 00:00:00 2001
From: Morgan Smith <Morgan.J.Smith@outlook.com>
Date: Fri, 14 Jun 2024 10:08:19 -0400
Subject: [PATCH 2/2] malformed clock tests

---
 testing/lisp/test-org-element.el | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/testing/lisp/test-org-element.el b/testing/lisp/test-org-element.el
index 6a4ec6c22..12d694226 100644
--- a/testing/lisp/test-org-element.el
+++ b/testing/lisp/test-org-element.el
@@ -1148,6 +1148,7 @@ CLOCK: [2023-10-13 Fri 14:40]--[2023-10-13 Fri 14:51] =>  0:11"
 
 (ert-deftest test-org-element/clock-parser ()
   "Test `clock' parser."
+  ;; TODO: does not check time information like :year-start etc!!!
   ;; Running clock.
   (let ((clock (org-test-with-temp-text "CLOCK: [2012-01-01 sun. 00:01]"
 		 (org-element-at-point))))
@@ -1183,7 +1184,18 @@ CLOCK: [2023-10-13 Fri 14:40]--[2023-10-13 Fri 14:51] =>  0:11"
 	   (org-element-at-point))))
     (should (eq (org-element-property :status clock) 'closed))
     (should-not (org-element-property :value clock))
-    (should (equal (org-element-property :duration clock) "0:11"))))
+    (should (equal (org-element-property :duration clock) "0:11")))
+  ;; malformed clocks
+  ;; TODO: should probably emit warning or something!!
+  (let ((clock
+         (org-test-with-temp-text
+          "CLOCK: [2012-01-01 sun. 00rr:01]--[2012-01-01 sun. 00:02] =>  0:01"
+          (org-element-at-point))))
+    (should (eq (org-element-property :status clock) 'closed))
+    (should (equal (org-element-property :raw-value
+                                         (org-element-property :value clock))
+                   "[2012-01-01 sun. 00rr:01]--[2012-01-01 sun. 00:02]"))
+    (should (equal (org-element-property :duration clock) "0:01"))))
 
 
 ;;;; Code
-- 
2.45.1

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

[Ihor Radchenko]

Morgan Smith <morgan.j.smith@outlook.com> writes:

> So I gave up on this specific patch because I wrote a patch to just
> rewrite the entire `org-clock-sum' function using org-element API.
> Attached is the `org-clock-sum' rewrite patch which I've been using for
> a while with no issues.  I have half finished patches locally to add
> more clocktable tests and to add clocktable benchmarks which is why I
> hadn't submitted this yet.
>
> This probably belongs in this email thread instead:
> https://list.orgmode.org/87y18vxgjs.fsf@localhost/

Yup. But my "to-followup" reminder came up earlier for this thread ;)
I do not remember every single thread, unfortunately.

> Ideally the fix in that commit should be ported to the org-element API.
> Notably, the malformed clock from the email thread from that commit is
> parsed a little strangely by org-element.  I'm not sure what effect this
> has on my rewrite patch but regardless, we should probably fix this.
> Notice how ":day-end" and ":minute-end" are set but not ":hour-start" or
> ":minute-start".

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

And [2012-01-01 sun. 00rr:01] is a perfectly valid timestamp without
time part. Org does allow arbitrary additional text in the timestamps.
(it is by design, to allow future extensions of the syntax, like the
planned timezone support)

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Morgan Smith]

Ihor Radchenko <yantar92@posteo.net> writes:

>> Ideally the fix in that commit should be ported to the org-element API.
>> Notably, the malformed clock from the email thread from that commit is
>> parsed a little strangely by org-element.  I'm not sure what effect this
>> has on my rewrite patch but regardless, we should probably fix this.
>> Notice how ":day-end" and ":minute-end" are set but not ":hour-start" or
>> ":minute-start".
>
> 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
>
> And [2012-01-01 sun. 00rr:01] is a perfectly valid timestamp without
> time part. Org does allow arbitrary additional text in the timestamps.
> (it is by design, to allow future extensions of the syntax, like the
> planned timezone support)

My specific issue is that the ":*-end" stuff can be set when the
"*-start" stuff is not.

Running the following snippet on the following file results in this:

======= snippet
(org-element-cache-map
 (lambda (clock)
   "Calculate end-time - start-time"
   (let ((timestamp (org-element-property :value clock)))
     (- (float-time (org-timestamp-to-time timestamp t))
        (float-time (org-timestamp-to-time timestamp)))))
 :granularity 'element
 :restrict-elements 'clock)
========


======== file
* Test
CLOCK: [2023-11-15 Wed 15:26]--[2023-11-15 Wed 16:12] =>  0:46
calculated time: 2760.0
CLOCK: [2023-11-15 Wed 15rr:26]--[2023-11-15 Wed 16:12] =>  0:46
calculated time: 58320.0
CLOCK: [2023-11-15 Wed 15:26]--[2023-11-15 Wed 16rr:12] =>  0:46
calculated time: 0.0
CLOCK: [2023-11-15 Wed 15rr:26]--[2023-11-15 Wed 16rr:12] =>  0:46
calculated time: 0.0
=======

======= results
(2760.0 58320.0 0.0 0.0)
======

What this shows is that an invalid end will cause the entry to be
ignored, but an invalid start will not.

We can totally claim that this is a feature, not a bug, if you would
like.  As this essentially treats
"CLOCK: [2023-11-15 Wed 15rr:26]--[2023-11-15 Wed 16:12] =>  0:46"
as
"CLOCK: [2023-11-15 Wed]--[2023-11-15 Wed 16:12] => 16:12"


If this is a feature then my previous patch is actually ready to merge.

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

[Ihor Radchenko]

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.

> We can totally claim that this is a feature, not a bug, if you would
> like.  As this essentially treats
> "CLOCK: [2023-11-15 Wed 15rr:26]--[2023-11-15 Wed 16:12] =>  0:46"
> as
> "CLOCK: [2023-11-15 Wed]--[2023-11-15 Wed 16:12] => 16:12"

I consider this as a problem that should be addressed in
`org-clock-sum' - if opening/closing time is not specified, there is
likely some typo in clock lines. So, as you saw in
fd8ddf2874ca00505aa096c6172ea750cd5e9eaa, I want to display a warning if
something is sus, so that the user is notified that time calculations
might be off.

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.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Morgan Smith]

[-- 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

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

[Ihor Radchenko]

Morgan Smith <morgan.j.smith@outlook.com> writes:

>> 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.

> -	    (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)))))

This is harmless, but also does nothing - if there is no year, month,
and date specified, `date-end' would never match.

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

Clock lines and timestamps are not only used to calculate clock
sums. They can, for example, be exported. Or they can be used manually,
by reading them as text. So, any kind of change in the parser should be
considered extremely carefully.

> 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.

> 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)

This is not acceptable.
org-element is a _parser_. It has nothing to do with interpretation of
timestamps. Moreover, there is no notion of "invalid" syntax in Org mode
- any text is a valid Org syntax, although it may be interpreted as
plain text if it does not fit any existing markup.

I still insist that it is a job of the Elisp code that interprets the
clock lines (the clock sum function) to display warnings and otherwise
analyze the timestamp components. We may also hint potential problems in
org-lint.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

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

[Ihor Radchenko]

Ihor Radchenko <yantar92@posteo.net> writes:

> So, in the message I linked, Nicolas (the major Org mode contributor)
> was not right. I hence need to fix the parser and update Org syntax
> page. This includes fixing `org-element-clock-line-re' to account for
> CLOCK: => 1:00 syntax.

I changed the parser on main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=17072a469
and updated the syntax ref
https://git.sr.ht/~bzg/worg/commit/1c56837d

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>