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

[Max Nikulin <manikulin@gmail.com>]

I am sorry, I have realized that debbugs.gnu.org does not send copies to 
the addresses specified in the X-Debbugs-CC header of the original 
report. Now I am sending a copy of my earlier message to emacs-orgmode.

Please, add 54764@debbugs.gnu.org to replies to this message.

Paul,

I am sorry that I forced you to make more changes in the Org code. While 
I was filing this bug, my intention was to add something like "if 
(!NILP(...)) { .... }" around several lines in src/timefns.c.

Now we should decide whether we leave this bug for possible changes in 
the `encode-time' implementation and moving the discussion related to 
further changes in Org to the emacs-orgmode mail list or we change the 
title of this bug and maybe create another one for `encode-time'.

On 09/04/2022 14:52, Paul Eggert wrote:
> 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)

I see you even change `org-store-link' that has the same problem.

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

I am against preserving the old form because it ignores the DST field. 
It is confusing and error prone to be a part of a well designed interface.

Actually I have a draft of `org-encode-time' macro that transforms 6 
elements list to 9 elements one at load or compile time, so it should 
not hurt runtime performance. I have not tried to replace all calls of 
the `encode-time' function yet however. But I still prefer to drop 6/9 
elements branch even if it may happen a decade later.
>>  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.

I do not see your point here. Old calling convention did not allow to 
specify DST flag at all and it was a problem. With the list argument 
even if last 3 fields are optional, it will not prevent adding DST value 
when it is known from some source. I think, requiring 3 extra fields 
when DST value is unknown is too hing price just to make a developer 
aware that it may be ambiguous (moreover it will not work without a 
clear statement in the docstring).

Org timestamps does not allow to specify timezone abbreviation such as 
PDT/PST to distinguish DST. If it were added then users would have false 
impression of full time zones support in Org. It would require huge 
amount of work. So guessed DST is the best that often can be offered.

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

My point is that subtle breaking changes must be prominent and hard to 
ignore. I do not have yet ready phases and you highlighted another issue 
missed in the docstring.

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

I do not think it is a problem.

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

I considered such approach to defense against "aggressive" modernizing 
of Emacs code. Then I decided it is better to allow to deprecate one of 
the styles of calling `encode-time'. I tried to express it in the 
original report and above.

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

Org has its own repository and changes should be committed there at first.

Org has unit tests, see
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/tree/testing/README
you changes should be accompanied by some adjustments in test expectations.

I believe that at least in some cases Org test suite should be run for 
the bundled version of Org after Emacs builds. There are may be 
copyright issues with including the tests into the Emacs source tree. 
Maybe some changes in tests were accepted without paperwork.

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

I agree with you that dates should not be represented as timestamps and 
date modifications (day, week, month, year increments and decrements) 
should be performed by dedicated functions. I even had 2011-12-30 
example in my notes. However I expect that underlying normalization of 
date-time fields may mitigate such issues to some extent.

> 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

I do not have complicated agenda setup to profile performance after such 
change. I will post my simple macro when we decide to continue 
discussion on debbugs or in a dedicated thread of the emacs-orgmode list.

In my opinion, the code obtaining DST value requires unit tests.

I like you idea to reuse `org-time-string-to-seconds' in more places. My 
original plan was to use `org-time-string-to-time' there.