Re: bug#54764: encode-time: make DST and TIMEZONE fields of the list argument optional ones

[Paul Eggert <eggert@cs.ucla.edu>]

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

On 4/7/22 05:37, Max Nikulin wrote:

>      (encode-time '(0 30 20 07 04 2022 nil nil nil)) ; wrong! 

Yes, and I see a couple of places (org-parse-time-string, 
org-read-date-analyze) where Org mode returns the wrong decoded 
timestamps, ending in (nil nil nil) even though the DST flag is unknown 
so they should end in (nil -1 nil). Please see attached proposed patch 
which fixes this (also see below).


> Since Emacs-27 time fields as separated arguments are considered 
> obsolete for calls of `encode-time'.

Obsolescent, not obsolete. The old form still works and it's not going 
away any time soon. If the efficiency concerns you mention are 
significant, we should keep the old form indefinitely.


> t is inconvenient to add 3 extra mandatory components at 
> the each place.

Then let's keep using the obsolescent calling convention for places 
where that's convenient. Perhaps we should change the documentation to 
say "older" instead of "obsolescent".


>  From my point of view it is better to change implementation of 
> `encode-time' so that it may accept 6-component list SECOND...YEAR. It 
> should not add noticeable performance penalty but makes the function 
> more convenient in use.

Unfortunately it makes the function more convenient to use incorrectly. 
This was part of the motivation for the API change. The obsolescent 
calling convention has no way to deal with ambiguous timestamps like 
2022-11-06 01:30 when TZ="America/Los_Angeles". Org mode surely has bugs 
in this area, although I don't have time to scout them out.


> Daylight saving 
> time field matters only as a list component and ignored as a separate 
> argument (by the way, it should be stressed in the docstring).

Do you have a wording suggestion? (The doc string already covers the 
topic concisely; however, conciseness is not always a virtue. :-)

The reason -1 is the default in the obsolete API is backward 
compatibility. If we could have designed the API from scratch it would 
have been different.


> In the Org code it is unsure which way to call `encode-time' is more 
> convenient. In a half of the cases a list is obtained from another 
> function, but another half is timestamp built from computed components. 
> Unless the inconsistency with DST I would say that both ways to call the 
> function should be supported.

Yes, that's the idea.


> So my proposal is to not force Org mode to use new calling convention 
> for `encode-time' till DST and ZONE list components will became optional 
> ones in a released Emacs version.

This would delay things for ten years or so, no? We'd have to wait until 
Org mode supported only Emacs 29 and later.

Instead, I suggest that we stick with what we have when that's cleaner. 
That is, Org mode can use the obsolescent encode-time API when it's 
cleaner to do that.

It would be helpful for Org mode to use the new encode-time form in some 
cases, when the new form is cleaner. It's easy to support the new form 
efficiently even in older Emacs, using a compatibility shim. This is 
also in the attached proposed patch.

This patch has a few other minor cleanups in the area.

I haven't installed the patch, or tested it other than via 'make check'.


PS. Org mode usually uses encode-time for calendrical calculations. This 
is dicey, as some days don't exist (for example, December 30, 2011 does 
not exist if TZ="Pacific/Apia", because Samoa moved across the 
International Date Line that day). And it's also dicey when Org mode 
uses 00:00:00 (midnight at the start of the day) as a timestamp 
representing the entire day, as it's all too common for midnight to not 
exist (or to be duplicated) due to a DST transition. Generally speaking, 
when Org mode is doing calendrical calculations it should use 
calendrical functions rather than encode-time+decode-time, which are 
best used for time calculations not calendar calculations. (I realize 
that fixing this in Org would be nontrivial; perhaps I should file this 
"PS" as an Org bug report for whoever has time to fix it....)

[-- Attachment #2: 0001-Improve-Org-usage-of-timestamps.patch --]
[-- Type: text/x-patch, Size: 10568 bytes --]

From 094345e10ad45e06f7b32e2f8017592210f43463 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Sat, 9 Apr 2022 00:17:09 -0700
Subject: [PATCH] Improve Org usage of timestamps
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The main thing is to follow the (encode-time X) convention where
X’s DST component of nil means standard time, -1 means unknown.
* lisp/org/ol.el (org-store-link): Prefer plain (encode-time ...)
to (apply 'encode-time ...), for speed.
* lisp/org/org-clock.el (org-clock-sum)
(org-clock-update-time-maybe):
Prefer org-time-string-to-seconds to doing it by hand.
* lisp/org/org-colview.el (org-colview-construct-allowed-dates):
* lisp/org/org.el (org-time-string-to-time, org-timestamp-change):
Prefer (org-encode-time-1 X) to (apply #'encode-time X) when X is
the output of org-parse-time-string.  They're equivalent and
the former is a bit cleaner.
* lisp/org/org-compat.el (org-encode-time-1): New function.
* lisp/org/org-macro.el (org-macro--vc-modified-time):
Prefer (org-encode-time-1 X) to (apply #'encode-time X) when X is
the output of parse-time-string.  They're equivalent and the
former is a bit cleaner.
* lisp/org/org-macs.el (org-2ft):
Prefer org-time-string-to-seconds to doing it by hand.
(org-parse-time-string): Return unknown DST, not standard time.
(org-matcher-time): Omit an unnecessary call to ‘append’.
* lisp/org/org-table.el (org-table-eval-formula):
Prefer org-time-string-to-time to doing it by hand.
* lisp/org/org.el (org-add-planning-info, org-read-date)
(org-read-date-display, org-display-custom-time):
Prefer (org-encode-time-1 X) to (apply #'encode-time X) when X is
the output of org-read-date-analyze.  They're equivalent and
the former is a bit cleaner.
(org-read-date-analyze): Return a timestamp with a DST flag
of -1 (unknown) rather than nil (standard time).
---
 lisp/org/ol.el          |  4 +---
 lisp/org/org-clock.el   | 17 ++++++-----------
 lisp/org/org-colview.el |  2 +-
 lisp/org/org-compat.el  | 21 +++++++++++++++++++++
 lisp/org/org-macro.el   |  2 +-
 lisp/org/org-macs.el    |  8 ++++----
 lisp/org/org-table.el   |  3 +--
 lisp/org/org.el         | 14 +++++++-------
 8 files changed, 42 insertions(+), 29 deletions(-)

diff --git a/lisp/org/ol.el b/lisp/org/ol.el
index a03d85f618..fe6e97e928 100644
--- a/lisp/org/ol.el
+++ b/lisp/org/ol.el
@@ -1575,9 +1575,7 @@ org-store-link
 	  (setq link
 		(format-time-string
 		 (car org-time-stamp-formats)
-		 (apply 'encode-time
-			(list 0 0 0 (nth 1 cd) (nth 0 cd) (nth 2 cd)
-			      nil nil nil))))
+		 (encode-time 0 0 0 (nth 1 cd) (nth 0 cd) (nth 2 cd))))
 	  (org-link-store-props :type "calendar" :date cd)))
 
        ((eq major-mode 'w3-mode)
diff --git a/lisp/org/org-clock.el b/lisp/org/org-clock.el
index 7395669109..24d2b61b48 100644
--- a/lisp/org/org-clock.el
+++ b/lisp/org/org-clock.el
@@ -1903,13 +1903,10 @@ org-clock-sum
 	  (cond
 	   ((match-end 2)
 	    ;; Two time stamps.
-	    (let* ((ts (float-time
-			(apply #'encode-time
-			       (save-match-data
-				 (org-parse-time-string (match-string 2))))))
-		   (te (float-time
-			(apply #'encode-time
-			       (org-parse-time-string (match-string 3)))))
+	    (let* ((ss (match-string 2))
+		   (se (match-string 3))
+		   (ts (org-time-string-to-seconds ss))
+		   (te (org-time-string-to-seconds se))
 		   (dt (- (if tend (min te tend) te)
 			  (if tstart (max ts tstart) ts))))
 	      (when (> dt 0) (cl-incf t1 (floor dt 60)))))
@@ -3041,10 +3038,8 @@ org-clock-update-time-maybe
 	  (end-of-line 1)
 	  (setq ts (match-string 1)
 		te (match-string 3))
-	  (setq s (- (float-time
-		      (apply #'encode-time (org-parse-time-string te)))
-		     (float-time
-		      (apply #'encode-time (org-parse-time-string ts))))
+	  (setq s (- (org-time-string-to-seconds te)
+		     (org-time-string-to-seconds ts))
 		neg (< s 0)
 		s (abs s)
 		h (floor (/ s 3600))
diff --git a/lisp/org/org-colview.el b/lisp/org/org-colview.el
index 829fcbbe3f..190c2d5a4f 100644
--- a/lisp/org/org-colview.el
+++ b/lisp/org/org-colview.el
@@ -782,7 +782,7 @@ org-colview-construct-allowed-dates
       (setq time-after (copy-sequence time))
       (setf (nth 3 time-before) (1- (nth 3 time)))
       (setf (nth 3 time-after) (1+ (nth 3 time)))
-      (mapcar (lambda (x) (format-time-string fmt (apply #'encode-time x)))
+      (mapcar (lambda (x) (format-time-string fmt (org-encode-time-1 x)))
 	      (list time-before time time-after)))))
 
 (defun org-columns-open-link (&optional arg)
diff --git a/lisp/org/org-compat.el b/lisp/org/org-compat.el
index 819ce74d93..247373d6b9 100644
--- a/lisp/org/org-compat.el
+++ b/lisp/org/org-compat.el
@@ -115,6 +115,27 @@ org-table1-hline-regexp
   (defun org-time-convert-to-list (time)
     (seconds-to-time (float-time time))))
 
+;; Like Emacs 27+ `encode-time' with one argument.
+(if (ignore-errors (encode-time (decode-time)))
+    (defsubst org-encode-time-1 (time)
+      (encode-time time))
+  (defun org-encode-time-1 (time)
+    (let ((dst-zone (nthcdr 7 time)))
+      (unless (consp (cdr dst-zone))
+	(signal wrong-type-argument (list time)))
+      (let ((etime (apply #'encode-time time))
+	    (dst (car dst-zone))
+	    (zone (cadr dst-zone)))
+	(when (and (symbolp dst) (not (integerp zone)) (not (consp zone)))
+	  (let* ((detime (decode-time etime))
+		 (dedst (nth 7 detime)))
+	    (when (and (not (eq dedst dst)) (symbolp dedst))
+	      ;; Assume one-hour DST and adjust the timestamp.
+	      (setq etime (time-add etime (seconds-to-time
+					   (- (if dedst 3600 0)
+					      (if dst 3600 0))))))))
+	etime))))
+
 ;; `newline-and-indent' did not take a numeric argument before 27.1.
 (if (version< emacs-version "27")
     (defsubst org-newline-and-indent (&optional _arg)
diff --git a/lisp/org/org-macro.el b/lisp/org/org-macro.el
index 0921f3aa27..b8e4346002 100644
--- a/lisp/org/org-macro.el
+++ b/lisp/org/org-macro.el
@@ -378,7 +378,7 @@ org-macro--vc-modified-time
 				  (buffer-substring
 				   (point) (line-end-position)))))
 		       (when (cl-some #'identity time)
-			 (setq date (apply #'encode-time time))))))))
+			 (setq date (org-encode-time-1 time))))))))
 	      (let ((proc (get-buffer-process buf)))
 		(while (and proc (accept-process-output proc .5 nil t)))))
 	  (kill-buffer buf))
diff --git a/lisp/org/org-macs.el b/lisp/org/org-macs.el
index b10725bd52..0916da89ac 100644
--- a/lisp/org/org-macs.el
+++ b/lisp/org/org-macs.el
@@ -1185,7 +1185,7 @@ org-2ft
    ((numberp s) s)
    ((stringp s)
     (condition-case nil
-	(float-time (apply #'encode-time (org-parse-time-string s)))
+	(org-time-string-to-seconds s)
       (error 0)))
    (t 0)))
 
@@ -1242,7 +1242,7 @@ org-parse-time-string
 	(string-to-number (match-string 4 s))
 	(string-to-number (match-string 3 s))
 	(string-to-number (match-string 2 s))
-	nil nil nil))
+	nil -1 nil))
 
 (defun org-matcher-time (s)
   "Interpret a time comparison value S as a floating point time.
@@ -1252,8 +1252,8 @@ org-matcher-time
 \"<tomorrow>\", and \"<yesterday>\".
 
 Return 0. if S is not recognized as a valid value."
-  (let ((today (float-time (apply #'encode-time
-				  (append '(0 0 0) (nthcdr 3 (decode-time)))))))
+  (let ((today (float-time (apply #'encode-time 0 0 0
+				  (nthcdr 3 (decode-time))))))
     (save-match-data
       (cond
        ((string= s "<now>") (float-time))
diff --git a/lisp/org/org-table.el b/lisp/org/org-table.el
index c4daed1665..a192fb991b 100644
--- a/lisp/org/org-table.el
+++ b/lisp/org/org-table.el
@@ -2606,8 +2606,7 @@ org-table-eval-formula
 		     (format-time-string
 		      (org-time-stamp-format
 		       (string-match-p "[0-9]\\{1,2\\}:[0-9]\\{2\\}" ts))
-		      (apply #'encode-time
-			     (save-match-data (org-parse-time-string ts))))))
+		      (save-match-data (org-time-string-to-time ts)))))
 		 form t t))
 
 	  (setq ev (if (and duration (string-match "^[0-9]+:[0-9]+\\(?::[0-9]+\\)?$" form))
diff --git a/lisp/org/org.el b/lisp/org/org.el
index d656a51591..1bceb0f53a 100644
--- a/lisp/org/org.el
+++ b/lisp/org/org.el
@@ -10792,7 +10792,7 @@ org-add-planning-info
 	    (if (stringp time)
 		;; This is a string (relative or absolute), set
 		;; proper date.
-		(apply #'encode-time
+		(org-encode-time-1
 		       (org-read-date-analyze
 			time default-time (decode-time default-time)))
 	      ;; If necessary, get the time from the user
@@ -14059,7 +14059,7 @@ org-read-date
 		 "range representable on this machine"))
       (ding))
 
-    (setq final (apply #'encode-time final))
+    (setq final (org-encode-time-1 final))
 
     (setq org-read-date-final-answer ans)
 
@@ -14096,7 +14096,7 @@ org-read-date-display
 			  (and (boundp 'org-time-was-given) org-time-was-given))
 		      (cdr fmts)
 		    (car fmts)))
-	     (txt (format-time-string fmt (apply #'encode-time f)))
+	     (txt (format-time-string fmt (org-encode-time-1 f)))
 	     (txt (if org-read-date-inactive (concat "[" (substring txt 1 -1) "]") txt))
 	     (txt (concat "=> " txt)))
 	(when (and org-end-time-was-given
@@ -14334,7 +14334,7 @@ org-read-date-analyze
 	 (setq year (nth 5 org-defdecode))
 	 (setq org-read-date-analyze-forced-year t))))
     (setq org-read-date-analyze-futurep futurep)
-    (list second minute hour day month year)))
+    (list second minute hour day month year nil -1 nil)))
 
 (defvar parse-time-weekdays)
 (defun org-read-date-get-relative (s today default)
@@ -14470,7 +14470,7 @@ org-display-custom-time
 	  time (org-fix-decoded-time t1)
 	  str (org-add-props
 		  (format-time-string
-		   (substring tf 1 -1) (apply 'encode-time time))
+		   (substring tf 1 -1) (org-encode-time-1 time))
 		  nil 'mouse-face 'highlight))
     (put-text-property beg end 'display str)))
 
@@ -14725,7 +14725,7 @@ org-make-tdiff-string
 
 (defun org-time-string-to-time (s)
   "Convert timestamp string S into internal time."
-  (apply #'encode-time (org-parse-time-string s)))
+  (org-encode-time-1 (org-parse-time-string s)))
 
 (defun org-time-string-to-seconds (s)
   "Convert a timestamp string S into a number of seconds."
@@ -15155,7 +15155,7 @@ org-timestamp-change
 	  (setcar time0 (or (car time0) 0))
 	  (setcar (nthcdr 1 time0) (or (nth 1 time0) 0))
 	  (setcar (nthcdr 2 time0) (or (nth 2 time0) 0))
-	  (setq time (apply 'encode-time time0))))
+	  (setq time (org-encode-time-1 time0))))
       ;; Insert the new time-stamp, and ensure point stays in the same
       ;; category as before (i.e. not after the last position in that
       ;; category).
-- 
2.35.1