Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.

[Max Nikulin <manikulin@gmail.com>]

On 30/04/2022 01:10, Paul Eggert wrote:
> On 4/29/22 07:22, Max Nikulin wrote:
> 
>>  From my point of view, it is better to rewrite `org-compile-time' to 
>> treat the case when there were no file prior to the call as that the 
>> file has been updated without comparison of timestamps
> 
> Yes, that sounds simpler and better. How about the attached patch?

Thank you, Paul. I am glad to see that you agree with my idea.

> From: Paul Eggert
> Date: Fri, 29 Apr 2022 11:06:00 -0700
> Subject: [PATCH] Improve org-compile-file timestamp handling

> diff --git a/lisp/org/org-macs.el b/lisp/org/org-macs.el
> index bb0562dde0..907043580a 100644
> --- a/lisp/org/org-macs.el
> +++ b/lisp/org/org-macs.el

It should be committed to the "main" branch of the Org repository as 
well. I am unsure if all Emacs developers have commit permission however.

> @@ -260,14 +260,8 @@ org-file-newer-than-p
>    "Non-nil if FILE is newer than TIME.
>  FILE is a filename, as a string, TIME is a Lisp time value, as
>  returned by, e.g., `current-time'."

It is not true any more, it is expected that TIME is obtained using 
`file-attributes'.

I would consider replacing docstring with something like:

Non-nil if FILE modification time is greater than or equal to TIME.
TIME should be obtained from the return value of the `file-attributes'
function.  If TIME is nil (file did not exist)
then any existing FILE is considered as a newer one.
This function may give false positive for file systems with coarse
timestamp resolution such as 1 second for HFS+ or 2 seconds for FAT.
File timestamp and system clock may have different resolution,
so attempts to compare them may give unexpected results.

> -  (and (file-exists-p file)
> -       ;; Only compare times up to whole seconds as some file-systems
> -       ;; (e.g. HFS+) do not retain any finer granularity.  As
> -       ;; a consequence, make sure we return non-nil when the two
> -       ;; times are equal.
> -       (not (time-less-p (org-time-convert-to-integer
> -			  (nth 5 (file-attributes file)))
> -			 (org-time-convert-to-integer time)))))
> +  (when-let ((mtime (file-attribute-modification-time (file-attributes file))))
> +    (time-less-p time mtime)))

org-macs.el does not contain (require 'subr-x) thus `when-let' will 
cause a warning on Emacs-26.

`file-attribute-modification-time' makes code clearer, but it causes 
some complications. Formally compatibility with Emacs-25 (e.g. 
ubuntu-18.04 LTS bionic) is not required for the "main" branch. Emacs 
sources have the "bugfix" Org branch of the stable release though. The 
latter still supports Emacs-25, so either the Emacs source tree and the 
Org bugfix branch will diverge at this point or it is safer to avoid 
`file-attribute-modification-time' till the next major Org release. 
Maybe Org maintainers and developers will correct me.

Likely you already guessed from the suggested docscring that I would prefer

     (and mtime (not (and time (time-less-p mtime time))))

to keep existing behavior on HFS+ and to allow nil for TIME.

>  (defun org-compile-file (source process ext &optional err-msg log-buf spec)
>    "Compile a SOURCE file using PROCESS.
> @@ -301,7 +295,7 @@ org-compile-file
>  	 (full-name (file-truename source))
>  	 (out-dir (or (file-name-directory source) "./"))
>  	 (output (expand-file-name (concat base-name "." ext) out-dir))
> -	 (time (current-time))
> +	 (time (file-attribute-modification-time (file-attributes output)))
>  	 (err-msg (if (stringp err-msg) (concat ".  " err-msg) "")))
>      (save-window-excursion
>        (pcase process
> @@ -320,7 +314,7 @@ org-compile-file
>  	(_ (error "No valid command to process %S%s" source err-msg))))
>      ;; Check for process failure.  Output file is expected to be
>      ;; located in the same directory as SOURCE.
> -    (unless (org-file-newer-than-p output time)
> +    (unless (or (not time) (org-file-newer-than-p output time))
>        (error (format "File %S wasn't produced%s" output err-msg)))
>      output))

I am considering handling of (not time) case inside 
`org-file-newer-than-p` as a more robust approach.