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

2022-10-20 Thread Ihor Radchenko
Max Nikulin  writes:

> Subject: [PATCH v2] org-macs.el: Do not compare wall time and file
>  modification time
> To: emacs-orgmode@gnu.org

Applied onto main.
https://git.savannah.gnu.org/cgit/emacs/org-mode.git/commit/?id=deb1517fe909fe9ba2b753c0a7fd6146a0550460

Thanks for the reminder.

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>



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

2022-10-09 Thread Max Nikulin

On 02/10/2022 10:49, Ihor Radchenko wrote:

Max Nikulin writes:


From: Max Nikulin
Date: Fri, 6 May 2022 23:34:52 +0700
Subject: [PATCH] org-macs.el: Do not compare wall time and file modification
  time


Max, the patch does not currently apply onto main. Can you please update
it?


When prepared, it was intended for the bugfix branch to avoid problems 
in the case of Emacs-29 and Org as an ELPA package. Changes in the main 
branch since that time caused conflicts: Kyle backported Paul's fix from 
Emacs to Org, you added `set-file-time' to `org-babel-load-file'.
From cb03cdf1e7e727d596526690435c86fa5299f1ee Mon Sep 17 00:00:00 2001
In-Reply-To: <87sfk628ca.fsf@localhost>
References: <87sfk628ca.fsf@localhost>
From: Max Nikulin 
Date: Fri, 6 May 2022 23:34:52 +0700
Subject: [PATCH v2] org-macs.el: Do not compare wall time and file
 modification time
To: emacs-orgmode@gnu.org

* lisp/org-macs.el (org-file-newer-than-p): Recommend passing file
modification time instead of wall time to avoid truncation of timestamp
precision for the sake of filesystems with coarse time resolution.
(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.
Update timestamp after tangling of an org file, not before it.

This is assumed to be a better fix of the problem with change of time
representation in Emacs-29. The problem was reported by Mark Barton
 in
https://list.orgmode.org/bf5b9308-3fef-4dc6-98c9-bff36f19d...@gmail.com
Paul Eggert  committed another variant to Emacs
as 3abb3681b5.  It was ported to Org as commit 56ba22b9df several months
later.

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

Earlier call of `org-babel-load-file' for a non-existing .org file
caused "Bad bounding indices: 0, 2" error.

Update file timestamp (introduced by the commit 1525a5a64e)
after tangling of the file.  Change caused by conflict resolution during
rebasing of the initial version of the patch.  See
https://list.orgmode.org/t75efi$9pv$1...@ciao.gmane.io for an argument
in support of such change.
---
 lisp/org-macs.el | 30 ++
 lisp/org.el  | 22 +++---
 2 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index 2666733d0..c858f3695 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -312,17 +312,23 @@ If EXCLUDE-TMP is non-nil, ignore temporary buffers."
 ;;; 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 Lisp time value, 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 (org-time-convert-to-integer
-			  (nth 5 (file-attributes file)))
-			 (org-time-convert-to-integer time)
+  "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  err-msg log-buf spec)
   "Compile a SOURCE file using PROCESS.
@@ -356,7 +362,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) "")))
 (s