Re: [PATCH] org-macs.el: Do not compare wall time and file modification time

[Max Nikulin <manikulin@gmail.com>]

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

On 11/05/2022 23:24, Paul Eggert wrote:
> The comments don't seem to match the code here.
> 
>> +  (let* ((tangled-file (concat (file-name-sans-extension file) ".el"))
>> +         (file-mtime (file-attribute-modification-time
>> +                      (file-attributes (file-truename file))))
>> +         (tangled-mtime (file-attribute-modification-time
>> +                         (file-attributes (file-truename 
>> tangled-file)))))
>> +    ;; Tangle only if the Elisp file is older than the Org file.
>> +    ;; Filesystem may have coarse timestamp resolution (HFS+, FAT)
>> +    ;; so no need to update if timestamps are equal and thus
>> +    ;; `org-file-newer-than-p' can not be used here.
>> +    (unless (and file-mtime
>> +                 tangled-mtime
>> +                 (not (time-less-p tangled-mtime file-mtime)))
> 
> Although this looks correct, there's no need to go to the work of 
> computing file-mtime in the common case where tangled-mtime is nil.

Thank you, I missed such case. I discovered that the code below 
recompiles an .el file even the .elc one is newer, moreover loading of 
compiled file is broken by another modernization of emacs code (see the 
dedicated thread). That is why I did not bother if the code may be 
optimized a bit.

Finally I have found `file-newer-than-file-p' that looks suitable for 
such case.

[-- Attachment #2: 0001-org-macs.el-Do-not-compare-wall-time-and-file-modifi.patch --]
[-- Type: text/x-patch, Size: 4747 bytes --]

From b2a546e239f32c78fb2dfaf92201a0b23eb76059 Mon Sep 17 00:00:00 2001
From: Max Nikulin <manikulin@gmail.com>
Date: Fri, 6 May 2022 23:34:52 +0700
Subject: [PATCH] org-macs.el: Do not compare wall time and file modification
 time

* lisp/org-macs.el (org-file-newer-than-p): Fix Emacs-29 problem with
changed representation of system clock timestamp.  Recommend passing
file modification time and do not truncate its precision.
(org-compile-file): Store file modification time instead of system clock
for later comparison by `org-file-newer-than-p'.
* lisp/org.el (org-babel-load-file): Use `file-newer-than-file-p'
instead of `org-file-newer-than-p' since the former is more suitable
for target-prerequisite relation in the case of equal timestamps.
Improve error reporting when source file does not exist.

Unchanged timestamp of a file means failure of `org-compile-file' but in
`org-babel-load-file' the target may be considered up to date if its
timestamp is equal to the one for prerequisite.
So `org-file-newer-than-p' is not suitable for both cases.  The
difference matter for filesystems with coarse timestamp resolution, for
example HFS+.

Before "Bad bounding indices: 0, 2" error was signalled in response to
call of `org-babel-load-file' for a non-existing .org file.

Reported by Mark Barton <mbarton98@gmail.com>
https://list.orgmode.org/BF5B9308-3FEF-4DC6-98C9-BFF36F19D36C@gmail.com

During discussion of the issue Paul Eggert <eggert@cs.ucla.edu>
suggested over variants of the changes in the same thread.
---
 lisp/org-macs.el | 29 ++++++++++++++++++-----------
 lisp/org.el      | 10 +++++-----
 2 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index b10725bd5..23e005e6f 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -256,16 +256,23 @@ ignored in this case."
 ;;; File
 
 (defun org-file-newer-than-p (file time)
-  "Non-nil if FILE is newer than TIME.
-FILE is a filename, as a string, TIME is a list of integers, as
-returned by, e.g., `current-time'."
-  (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 (cl-subseq (nth 5 (file-attributes file)) 0 2)
-			 (cl-subseq time 0 2)))))
+  "Non-nil if FILE modification time is greater than TIME.
+TIME should be obtained earlier for the same FILE name using
+
+  (file-attribute-modification-time (file-attributes file))
+
+If TIME is nil (file did not exist) then any existing FILE
+is considered as a newer one.  Some file systems have coarse
+timestamp resolution, for example 1 second on HFS+ or 2 seconds on FAT,
+so nil may be returned when file is updated twice within a short period
+of time.  File timestamp and system clock `current-time' may have
+different resolution, so attempts to compare them may give unexpected
+results.
+
+Consider `file-newer-than-file-p' to check up to date state
+in target-prerequisite files relation."
+  (let ((mtime (file-attribute-modification-time (file-attributes file))))
+    (and mtime (or (not time) (time-less-p time mtime)))))
 
 (defun org-compile-file (source process ext &optional err-msg log-buf spec)
   "Compile a SOURCE file using PROCESS.
@@ -299,7 +306,7 @@ it for output."
 	 (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
diff --git a/lisp/org.el b/lisp/org.el
index 54350faee..7096c5098 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -233,11 +233,11 @@ optional prefix argument COMPILE, the tangled Emacs Lisp file is
 byte-compiled before it is loaded."
   (interactive "fFile to load: \nP")
   (let ((tangled-file (concat (file-name-sans-extension file) ".el")))
-    ;; Tangle only if the Org file is newer than the Elisp file.
-    (unless (org-file-newer-than-p
-	     tangled-file
-	     (file-attribute-modification-time
-	      (file-attributes (file-truename file))))
+    ;; Tangle only if the Elisp file is older than the Org file.
+    ;; Catch the case when the .el file exists while the .org file is missing.
+    (unless (file-exists-p file)
+      (error "File to tangle does not exist: %s" file))
+    (when (file-newer-than-file-p file tangled-file)
       (org-babel-tangle-file file
                              tangled-file
                              (rx string-start
-- 
2.25.1