Ihor Radchenko <[email protected]> writes: > Morgan Smith <[email protected]> writes: > >> The rest of the patches are the same except the last patch where I now >> limit the day to be 27 instead of 28 as I was getting test failures for >> the following string which includes a day of 29 since I add an hour for >> the second timestamp: >> >> "CLOCK: [2026-03-28 Sat 23:33]--[2026-03-29 Sun 00:33] => 1:00" > > Could you explain in more details?
I had assumed the issue was perfectly normal day rollover stuff but that assumption obviously came from not thinking about it very hard at all. You are right to question that case as it really should pass the test. Well, it turns out the code in `org-clock-timestamps-change' does a lot of sketchy point shenanigians and I'm actually still not certain why that case fails. While investigating I found slightly different cases that failed so I decided to just rewrite it as that felt easier. Also, I just want to point out something crazy sketchy that this function currently does (without my patch). Open a new org-mode buffer and put two timestamps in that buffer on seperate lines like this: [2026-03-29 Sun] [2026-03-29 Sun] Now put your point on the first one and try calling `org-clock-timestamps-up' and `org-clock-timestamps-down'. It modifies the one on the second line as well!!! That's a big nono!!! See attached for a better implementation. Tests pass on emacs 28, 29, 30.2, and a recent emacs/master build. Tests pass with TZ set to UTC, Europe/Istanbul, and America/New_York.
>From 38d030204cc0e6b66e80554971f4680ad05f2cbb Mon Sep 17 00:00:00 2001 From: Morgan Smith <[email protected]> Date: Sun, 29 Mar 2026 14:12:54 -0400 Subject: [PATCH] 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): 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 | 62 ++++++++++------------------------ testing/lisp/test-org-clock.el | 40 +++++++++++++++------- 2 files changed, 45 insertions(+), 57 deletions(-) diff --git a/lisp/org-clock.el b/lisp/org-clock.el index 995ee0a4d..a3dfe166e 100644 --- a/lisp/org-clock.el +++ b/lisp/org-clock.el @@ -1932,52 +1932,24 @@ 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) + (setq n (prefix-numeric-value n)) + (let ((original-point (point)) + (regex (rx (minimal-match (zero-or-more not-newline)) + (group-n 1 (regexp org-ts-regexp-both)))) + (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)))))))) + (move-beginning-of-line 1) + (looking-at regex) + (goto-char (1+ (match-beginning 1))) + (org-timestamp-change tschange timestamp? 'updown) + (when (looking-at regex) + (goto-char (1+ (match-beginning 1))) + (org-timestamp-change tschange timestamp? 'updown)) + ;; `save-excursion' won't work because + ;; `org-timestamp-change' deletes and re-inserts the + ;; timestamp. + (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 4d5cb055e..29e371621 100644 --- a/testing/lisp/test-org-clock.el +++ b/testing/lisp/test-org-clock.el @@ -120,6 +120,22 @@ test-org-clock/org-clock-timestamps-change (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 + (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 timestamps after a newline + (should + (equal + (format "[2026-03-29 %s 23:33]\n[2026-03-29 00:33]" sun) + (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) @@ -127,7 +143,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 @@ -147,19 +163,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)) base-commit: 7ebccd9505581bd13cd516e6bb6d16d8138086fa -- 2.52.0
