Ihor Radchenko <[email protected]> writes:
> Morgan Smith <[email protected]> writes:
>
>> I have made the executive decision to allow negative durations in clock
>> lines. Personally I accidentally make negative durations all the time.
>> I'll edit a timestamp and hit "C-c C-c" and see a negative duration.
>> This lets me know I messed something up.
>>
>> The specific reason I need to allow negative durations for this change
>> is because I create negative duration in an intermediate step.
>
> Are you sure that we want to change Org syntax spec just to make the
> implementation simpler? That's going to be a breaking change.
Despite the time I've spent studying the parser, I honestly have no clue
what the implications of this change would be.
Anyways see attached for a version that doesn't need that change.
I previously got `org-clock-timestamps-change' confused with
`org-timestamp-{up|down}' and implemented the wrong logic. That logic
would've been tricky without negative durations. The correct logic
actually isn't that hard.
>From f09c5d438b95f214c3f31fc022fe9451ffb4a468 Mon Sep 17 00:00:00 2001
From: Morgan Smith <[email protected]>
Date: Sun, 5 Apr 2026 18:23:40 -0400
Subject: [PATCH 1/2] org-element: Add match groups to
`org-element-clock-line-re'
* lisp/org-element.el (org-element-clock-line-re): Add 3 match groups
and document them in the docstring.
(org-element-clock-line-re-no-group): New regex that has the old value
of 'org-element-clock-line-re'.
(org-element--set-regexps, org-element--current-element): Use
'org-element-clock-line-re-no-group'.
---
lisp/org-element.el | 56 +++++++++++++++++++++++++++------------------
1 file changed, 34 insertions(+), 22 deletions(-)
diff --git a/lisp/org-element.el b/lisp/org-element.el
index 313922ecc..fe78dfb2c 100644
--- a/lisp/org-element.el
+++ b/lisp/org-element.el
@@ -122,26 +122,38 @@ org-element-citation-prefix-re
"Regexp matching a citation prefix.
Style, if any, is located in match group 1.")
-(defconst org-element-clock-line-re
- (let ((duration ; "=> 212:12"
- '(seq
- (1+ (or ?\t ?\s)) "=>" (1+ (or ?\t ?\s))
- (1+ digit) ":" digit digit)))
- (rx-to-string
- `(seq
- line-start (0+ (or ?\t ?\s))
- "CLOCK:"
- (or
- (seq
- (1+ (or ?\t ?\s))
- (regexp ,org-ts-regexp-inactive)
- (opt "--"
- (regexp ,org-ts-regexp-inactive)
- ,duration))
- ,duration)
- (0+ (or ?\t ?\s))
- line-end)))
- "Regexp matching a clock line.")
+(rx-let
+ ((duration
+ (seq
+ (1+ (or ?\t ?\s)) "=>" (1+ (or ?\t ?\s))
+ (1+ digit) ":" digit digit))
+ (before (seq line-start (0+ (or ?\t ?\s)) "CLOCK:"))
+ (after (seq (0+ (or ?\t ?\s)) line-end)))
+ (defconst org-element-clock-line-re-no-group
+ (rx before
+ (or
+ (seq (1+ (or ?\t ?\s))
+ (regexp org-ts-regexp-inactive)
+ (opt "--"
+ (regexp org-ts-regexp-inactive)
+ duration))
+ duration)
+ after)
+ "Regexp matching a clock line.")
+ (defconst org-element-clock-line-re
+ (rx before
+ (or
+ (seq (1+ (or ?\t ?\s))
+ (group-n 1 (regexp org-ts-regexp-inactive))
+ (opt "--"
+ (group-n 2 (regexp org-ts-regexp-inactive))
+ (group-n 3 duration)))
+ (group-n 3 duration))
+ after)
+ "Regexp matching a clock line.
+The first timestamp is in match group 1.
+The second timestamp is in match group 2.
+The duration is in match group 3."))
(defconst org-element-comment-string "COMMENT"
"String marker for commented headlines.")
@@ -244,7 +256,7 @@ org-element--set-regexps
;; LaTeX environments.
"\\\\begin{\\([A-Za-z0-9*]+\\)}" "\\|"
;; Clock lines.
- org-element-clock-line-re "\\|"
+ org-element-clock-line-re-no-group "\\|"
;; Lists.
(let ((term (pcase org-plain-list-ordered-item-terminator
(?\) ")") (?. "\\.") (_ "[.)]")))
@@ -4823,7 +4835,7 @@ org-element--current-element
;; a footnote definition: next item is always a paragraph.
((not (bolp)) (org-element-paragraph-parser limit (list (point))))
;; Clock.
- ((looking-at-p org-element-clock-line-re) (org-element-clock-parser limit))
+ ((looking-at-p org-element-clock-line-re-no-group) (org-element-clock-parser limit))
;; Inlinetask.
(at-task? (org-element-inlinetask-parser limit raw-secondary-p))
;; From there, elements can have affiliated keywords.
base-commit: 4dc39c7eb3d481984fabfe2bfe578da51fb9c779
--
2.54.0
>From a0a1c96ae90f1b742414c2da47b1f3531061c958 Mon Sep 17 00:00:00 2001
From: Morgan Smith <[email protected]>
Date: Sun, 29 Mar 2026 14:12:54 -0400
Subject: [PATCH 2/2] lisp/org-clock.el: Rewrite 'org-clock-timestamps-change'
The previous implementation had a couple bugs.
1. It did strange things when the timestamps changed length
mid-operation (like if the weekday name got added during the call to
`org-timestamp-change').
2. In certain instances it double incremented the first timestamp,
leaving the second one unchanged. I assume this was also due to
managing the point wrong.
3. When used on a single timestamp, it would find the next timestamp
in the buffer and increment that one too.
* lisp/org-clock.el (org-clock-timestamps-change): Rewrite.
* testing/lisp/test-org-clock.el
(test-org-clock/org-clock-timestamps-change): Use 'string-equal'
instead of 'equal'. Add a test for a case that was previously failing.
Allow testing up to day 28 in month. Test that point does not change.
---
lisp/org-clock.el | 72 +++++++++++++---------------------
testing/lisp/test-org-clock.el | 46 +++++++++++++++-------
2 files changed, 58 insertions(+), 60 deletions(-)
diff --git a/lisp/org-clock.el b/lisp/org-clock.el
index a1de045fe..f3cdf017e 100644
--- a/lisp/org-clock.el
+++ b/lisp/org-clock.el
@@ -61,6 +61,7 @@ org-element-use-cache
(defvar org-frame-title-format-backup nil)
(defvar org-state)
(defvar org-link-bracket-re)
+(defvar org-element-clock-line-re)
(defgroup org-clock nil
"Options concerning clocking working time in Org mode."
@@ -1932,52 +1933,33 @@ org-clock-timestamps-change
"Change CLOCK timestamps synchronously at cursor.
UPDOWN tells whether to change `up' or `down'.
Optional argument N tells to change by that many units."
- (let ((tschange (if (eq updown 'up) 'org-timestamp-up
- 'org-timestamp-down))
- (timestamp? (org-at-timestamp-p 'lax))
- ts1 begts1 ts2 begts2 updatets1 tdiff)
+ ;; FIXME: this is the wrong way to deal with prefix arguments and is
+ ;; in conflict with the docstring
+ (setq n (prefix-numeric-value n))
+ (let ((original-point (point))
+ (tschange (if (eq updown 'up) n (- n)))
+ (timestamp? (org-at-timestamp-p 'lax)))
(when (not (memq timestamp? '(nil bracket after)))
- (save-excursion
- (move-beginning-of-line 1)
- (re-search-forward org-ts-regexp3 nil t)
- (setq ts1 (match-string 0) begts1 (match-beginning 0))
- (when (re-search-forward org-ts-regexp3 nil t)
- (setq ts2 (match-string 0) begts2 (match-beginning 0))))
- ;; Are we on the second timestamp?
- (if (<= begts2 (point)) (setq updatets1 t))
- (if (not ts2)
- ;; fall back on org-timestamp-up if there is only one
- (funcall tschange n)
- (funcall tschange n)
- (let ((ts (if updatets1 ts2 ts1))
- (begts (if updatets1 begts1 begts2)))
- (setq tdiff
- (time-subtract
- (org-time-string-to-time
- (save-excursion
- (goto-char (if updatets1 begts2 begts1))
- (looking-at org-ts-regexp3)
- (match-string 0)))
- (org-time-string-to-time ts)))
- ;; `save-excursion' won't work because
- ;; `org-timestamp-change' deletes and re-inserts the
- ;; timestamp.
- (let ((origin (point)))
- (save-excursion
- (goto-char begts)
- (org-timestamp-change
- (round (/ (float-time tdiff)
- (pcase-exhaustive timestamp?
- (`minute 60)
- (`hour 3600)
- (`day (* 24 3600))
- (`month (* 24 3600 31))
- (`year (* 24 3600 365.2)))))
- timestamp? 'updown))
- ;; Move back to initial position, but never beyond updated
- ;; clock.
- (unless (< (point) origin)
- (goto-char origin))))))))
+ (forward-line 0)
+ (when (looking-at org-element-clock-line-re)
+ ;; Negative durations break the clock parser so we need to
+ ;; decide the increment order carefully.
+ (let* ((first-stop (or (and (> tschange 0) 2) 1))
+ (second-stop (1+ (% first-stop 2))))
+ (goto-char (match-beginning first-stop))
+ (org-timestamp-change tschange timestamp? 'updown)
+ ;; Now that the first timestamp has been changed, the
+ ;; timestamp length and point might be different. So we have
+ ;; to run the search again from scratch.
+ (forward-line 0)
+ (looking-at org-element-clock-line-re)
+ (goto-char (match-beginning second-stop))
+ (org-timestamp-change tschange timestamp? 'updown)))
+ ;; `save-excursion' or markers won't work because
+ ;; `org-timestamp-change' deletes and re-inserts the
+ ;; timestamp. Jump back to the numerical point which will be
+ ;; wrong if the timestamp has changed length
+ (goto-char original-point))))
;;;###autoload
(defun org-clock-cancel ()
diff --git a/testing/lisp/test-org-clock.el b/testing/lisp/test-org-clock.el
index befd0e3e3..e336def5d 100644
--- a/testing/lisp/test-org-clock.el
+++ b/testing/lisp/test-org-clock.el
@@ -100,7 +100,7 @@ test-org-clock/org-clock-timestamps-change
(let ((sun (org-test-get-day-name "Sun"))
(mon (org-test-get-day-name "Mon")))
(should
- (equal
+ (string-equal
(format "CLOCK: [2023-02-19 %s 21:30]--[2023-02-19 %s 23:35] => 2:05"
sun sun)
(org-test-with-temp-text
@@ -108,7 +108,7 @@ test-org-clock/org-clock-timestamps-change
(org-clock-timestamps-change 'down 1)
(buffer-string))))
(should
- (equal
+ (string-equal
(format "CLOCK: [2023-02-20 %s 00:00]--[2023-02-20 %s 00:40] => 0:40"
mon mon)
(org-test-with-temp-text
@@ -116,12 +116,28 @@ test-org-clock/org-clock-timestamps-change
(org-clock-timestamps-change 'up 1)
(buffer-string))))
(should
- (equal
+ (string-equal
(format "CLOCK: [2023-02-20 %s 00:30]--[2023-02-20 %s 01:35] => 1:05"
mon mon)
(org-test-with-temp-text
"CLOCK: [2023-02-19 Sun 2<point>3:30]--[2023-02-20 Mon 00:35] => 1:05"
(org-clock-timestamps-change 'up 1)
+ (buffer-string))))
+ (should
+ (string-equal
+ (format "CLOCK: [2026-03-29 %s 23:33]--[2026-03-30 %s 00:33] => 1:00"
+ sun mon)
+ (org-test-with-temp-text
+ "CLOCK: [2026-03-2<point>8 23:33]--[2026-03-29 00:33] => 1:00"
+ (org-clock-timestamps-change 'up 1)
+ (buffer-string))))
+ ;; Don't touch things that aren't clocks
+ (should
+ (string-equal
+ "[2026-03-28 23:33]\n[2026-03-29 00:33]"
+ (org-test-with-temp-text
+ "[2026-03-2<point>8 23:33]\n[2026-03-29 00:33]"
+ (org-clock-timestamps-change 'up 1)
(buffer-string)))))
(let ((org-time-stamp-rounding-minutes '(1 1)) ;; No rounding!
(now (decode-time)) test-text point)
@@ -129,7 +145,7 @@ test-org-clock/org-clock-timestamps-change
;; 28th. This particular test is easier to write if the
;; days don't change when modifying the month.
(setf (decoded-time-day now)
- (min (decoded-time-day now) 27))
+ (min (decoded-time-day now) 28))
(setq now (encode-time now))
;; loop over regular timestamp formats and weekday-less timestamp
;; formats
@@ -149,19 +165,19 @@ test-org-clock/org-clock-timestamps-change
(while (not (eolp))
;; change the timestamp unit at point one down, two up,
;; one down, which should give us the original timestamp
- ;; again. However, point can move backward during that
- ;; operation, so take care of that. *Not* using
- ;; `save-excursion', which fails to restore point since
- ;; the timestamp gets completely replaced.
+ ;; again.
(setq point (point))
(org-clock-timestamps-down)
- (let ((current-prefix-arg '(2)))
- ;; We supply the prefix argument 2 to increment the
- ;; minutes by 2. We supply the function argument 2 for
- ;; everything else. Is this a bug? Probably. FIXME.
- (org-clock-timestamps-up 2))
- (org-clock-timestamps-down)
- (goto-char point)
+ (org-test-ignore-duplicate
+ (should (eq point (point)))
+ (let ((current-prefix-arg '(2)))
+ ;; We supply the prefix argument 2 to increment the
+ ;; minutes by 2. We supply the function argument 2 for
+ ;; everything else. Is this a bug? Probably. FIXME.
+ (org-clock-timestamps-up 2))
+ (should (eq point (point)))
+ (org-clock-timestamps-down)
+ (should (eq point (point))))
(should (string=
(buffer-substring (point-min) (point-max))
test-text))
--
2.54.0
