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

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

[Mark Barton]

The change also breaks org-file-newer-than-p function that triggered the debugger while loading my init that uses org babel. I was able to use the example of the patch that Paul Eggert provided earlier for the desktop-save to add the time-convert to “fix” org-file-newer-than-p as shown below. Not positive that I needed to change it in both places, but it works for me now on macOS Monterey.

modified   lisp/org/org-macs.el
@@ -264,8 +264,8 @@ org-file-newer-than-p
        ;; (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)))))
+       (not (time-less-p (cl-subseq (time-convert (nth 5 (file-attributes file)) 'list) 0 2)
+			 (cl-subseq (time-convert time 'list) 0 2)))))

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

[Po Lu]

Mark Barton <mbarton98@gmail.com> writes:

> The change also breaks org-file-newer-than-p function that triggered
> the debugger while loading my init that uses org babel. I was able to
> use the example of the patch that Paul Eggert provided earlier for the
> desktop-save to add the time-convert to “fix” org-file-newer-than-p as
> shown below. Not positive that I needed to change it in both places,
> but it works for me now on macOS Monterey.
>
> modified   lisp/org/org-macs.el
> @@ -264,8 +264,8 @@ org-file-newer-than-p
>         ;; (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)))))
> +       (not (time-less-p (cl-subseq (time-convert (nth 5 (file-attributes file)) 'list) 0 2)
> +			 (cl-subseq (time-convert time 'list) 0 2)))))

Isn't the fact that changes are needed in multiple pieces of in-tree
code a wake-up call demonstrating it's too early to change the default
time format?

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

[Paul Eggert]

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

Thanks for reporting that. Fixed in Emacs master via the attached.

For the more general issue I'm planning to add a builtin boolean 
variable current-time-list soon, that is t for (HIGH LOW MICROSEC 
PICOSEC) format, nil for (TICKS . HZ) format.

[-- Attachment #2: 0001-Use-org-time-convert-to-integer-instead-of-by-hand.patch --]
[-- Type: text/x-patch, Size: 1623 bytes --]

From 3abb3681b57d7c8ca7fa808addb0a10b6b109cab Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 27 Apr 2022 00:29:26 -0700
Subject: [PATCH] Use org-time-convert-to-integer instead of by hand
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/org/org-macs.el (org-file-newer-than-p):
Don’t assume list-format timestamps, by using
org-time-convert-to-integer instead of doing it by hand.
---
 lisp/org/org-macs.el | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lisp/org/org-macs.el b/lisp/org/org-macs.el
index b10725bd52..92591b5bb7 100644
--- a/lisp/org/org-macs.el
+++ b/lisp/org/org-macs.el
@@ -257,15 +257,16 @@ org-fit-window-to-buffer
 
 (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
+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 (cl-subseq (nth 5 (file-attributes file)) 0 2)
-			 (cl-subseq time 0 2)))))
+       (not (time-less-p (org-time-convert-to-integer
+			  (nth 5 (file-attributes file)))
+			 (org-time-convert-to-integer time)))))
 
 (defun org-compile-file (source process ext &optional err-msg log-buf spec)
   "Compile a SOURCE file using PROCESS.
-- 
2.32.0

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

[Stefan Monnier]

> -       (not (time-less-p (cl-subseq (nth 5 (file-attributes file)) 0 2)
> -			 (cl-subseq time 0 2)))))
> +       (not (time-less-p (org-time-convert-to-integer
> +			  (nth 5 (file-attributes file)))
> +			 (org-time-convert-to-integer time)))))

Instead of rounding the times to whole seconds, wouldn't it make more
sense to check that the difference is larger than 1s?


        Stefan

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

[Paul Eggert]

On 4/27/22 09:55, Stefan Monnier wrote:
> Instead of rounding the times to whole seconds, wouldn't it make more
> sense to check that the difference is larger than 1s?

org-file-newer-than-p is intended to work on filesystems like HFS+ that 
store just the seconds part of the last-modified time. Since these 
filesystems take the floor of the system time, taking the floor should 
be the most-accurate way to work around timestamp truncation issues, 
where comparing one timestamp that comes from an HFS+ filesystem to 
another timestamp coming from some other source (which is how 
org-file-newer-than-p is used).

The code won't work as desired on filesystems like FAT where the 
last-modified time has only 2-second resolution. Ideally Emacs Lisp code 
would have access to file timestamp resolution but that's not something 
it has now, so I merely preserved org-file-newer-than-p's assumption 
that taking the floor is good enough.

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

[Max Nikulin]

On 29/04/2022 05:27, Paul Eggert wrote:
> On 4/27/22 09:55, Stefan Monnier wrote:
>> Instead of rounding the times to whole seconds, wouldn't it make more
>> sense to check that the difference is larger than 1s?
> 
> org-file-newer-than-p is intended to work on filesystems like HFS+ that 
> store just the seconds part of the last-modified time.

I have found just 2 calls of `org-file-newer-than-p' in the Org code and 
in both cases the intention is to check whether particular file has been 
updated. I have not checked Org extensions for usage of this function. I 
would rather assume that the code was written without any considerations 
concerning filesystem timestamps precision and its difference from 
`current-time' representation. It was still working in most real-life cases.

 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, so 
`current-time' can be dropped to eliminate comparison of timestamp from 
different sources. With such modification it is better to compare file 
timestamps without truncation to whole seconds, however I have not tried 
to create an example where fractional seconds may change behavior.

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

[Paul Eggert]

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

On 4/29/22 07:22, Max Nikulin wrote:

> It was still working in most real-life cases.

Yes, the current code breaks only in fine-grained cases. Most of the 
time it'll work fine since people rarely compile the same file twice in 
the same second.

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

[-- Attachment #2: 0001-Improve-org-compile-file-timestamp-handling.patch --]
[-- Type: text/x-patch, Size: 2512 bytes --]

From fbd6561952acf359236afcf7957a197376a18c66 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Fri, 29 Apr 2022 11:06:00 -0700
Subject: [PATCH] Improve org-compile-file timestamp handling
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

* lisp/org/org-macs.el (org-file-newer-than-p):
Don’t lose timestamp information in an attempt to work around
problems on filesystems with coarse-grained timestamps.
(org-compile-file): Use only filesystem timestamps;
don’t try to compare them to the current time, as
the filesystem clock may be different from our clock.
---
 lisp/org/org-macs.el | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

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
@@ -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'."
-  (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)))
 
 (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))
 
-- 
2.34.1

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

[Max Nikulin]

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.

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

[Max Nikulin]

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

Mark Barton to emacs-orgmode, emacs-devel. master 4a1f69ebca 2/2: Use 
(TICKS . HZ) for current-time etc. Tue, 26 Apr 2022 23:37:50 -0700. 
https://list.orgmode.org/BF5B9308-3FEF-4DC6-98C9-BFF36F19D36C@gmail.com
 >
> The change also breaks org-file-newer-than-p function that triggered the
> debugger while loading my init that uses org babel.

I think, it should be fixed in the bugfix Org branch.  The attached 
patch is a compromise to some degree, but I do not see a robust solution.

I do not consider current behavior as reliable, however if you would 
prefer to keep it, the following patch may be used instead:

Paul Eggert to emacs-orgmode. Re: master 4a1f69ebca 2/2: Use (TICKS . 
HZ) for current-time etc. Wed, 27 Apr 2022 00:39:01 -0700. 
https://list.orgmode.org/f200c9ab-d1d4-d5a8-24cf-4e1082528fe7@cs.ucla.edu

The changes are not covered by unit tests at least when most babel 
languages are disabled.

On 30/04/2022 17:56, Max Nikulin wrote:
> 
>      (and mtime (not (and time (time-less-p mtime time))))

Treating equality as "newer" would break `org-compile-file', so I 
changed the condition. Previously it was not a case since file 
modification time is usually in the past in comparison to current time.

> On 30/04/2022 01:10, Paul Eggert wrote: 
>> +  (when-let ((mtime (file-attribute-modification-time (file-attributes file))))
>> +    (time-less-p time mtime))) 

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

I have found `file-attribute-modification-time' in org-compat.el.

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

From d37b5bb295c69572d1615e7fb2c0bcce05cb2b58 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'.

It changes behavior of `org-babel-load-file' for the case of equal
modification time of source and tangled files that may happen on
filesystems with coarse timestamps, for example HFS+.  The file will be
tangled again.  Treating equal timestamps as up to date state would
break `org-compile-file' however.  Anyway time comparison is not really
reliable since precision of filesystem timestamps is unknown and it
differs from system clock precision.

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 | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index b10725bd5..556bf658d 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -256,16 +256,26 @@ 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.
+
+Attempt to check whether a derived file has been updated in
+response to modification of its source file may give unreliable
+result.  Equal timestamps in such case may mean that the derived
+file is up to date however this function returns nil assuming
+that the FILE is not modified."
+  (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 +309,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
-- 
2.25.1

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

[Max Nikulin]

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

On 06/05/2022 23:56, Max Nikulin wrote:
> Mark Barton to emacs-orgmode, emacs-devel. master 4a1f69ebca 2/2: Use 
> (TICKS . HZ) for current-time etc. Tue, 26 Apr 2022 23:37:50 -0700. 
> https://list.orgmode.org/BF5B9308-3FEF-4DC6-98C9-BFF36F19D36C@gmail.com
>>
>> The change also breaks org-file-newer-than-p function that triggered the
>> debugger while loading my init that uses org babel.
> 
> I think, it should be fixed in the bugfix Org branch.  The attached 
> patch is a compromise to some degree, but I do not see a robust solution.

Thinking more I realized that `org-file-newer-than-p' should not be 
reused for `org-babel-load-file'. I am attaching an updated patch for 
which I do not see any real drawback.

The only change in behavior is that if a file had modification time in 
future and it is overwritten by `org-compile-time' to current time than 
the function reports failure. I consider such case as a rare and 
peculiar one.

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

From e5f98dbc729904297bef529009ade96361dd4dd2 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): Do not use `org-file-newer-than-p'
to consider the .el file as up to date when its modification time is the
same as for the source .org file.

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

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 | 32 +++++++++++++++++++++-----------
 lisp/org.el      | 18 ++++++++++++------
 2 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/lisp/org-macs.el b/lisp/org-macs.el
index b10725bd5..556bf658d 100644
--- a/lisp/org-macs.el
+++ b/lisp/org-macs.el
@@ -256,16 +256,26 @@ 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.
+
+Attempt to check whether a derived file has been updated in
+response to modification of its source file may give unreliable
+result.  Equal timestamps in such case may mean that the derived
+file is up to date however this function returns nil assuming
+that the FILE is not modified."
+  (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 +309,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..c1ce57c4d 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -232,12 +232,18 @@ and then loads the resulting file using `load-file'.  With
 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))))
+  (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)))
       (org-babel-tangle-file file
                              tangled-file
                              (rx string-start
-- 
2.25.1

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

[Paul Eggert]

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.

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

[Max Nikulin]

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

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

[Paul Eggert]

On 5/12/22 09:55, Max Nikulin wrote:

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

file-newer-than-file-p succeeds only if FILE exists, so in that case 
it'd be a bit more efficient to avoid testing FILE's existence again, e.g.:

    (cond
      ((file-newer-than-file-p file tangled-file)
       (org-bable-tangle-file file ...))
      ((not (file-exists-p file))
       (error "File to tangle does not exist: %s" file)))

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

[Max Nikulin]

On 13/05/2022 05:52, Paul Eggert wrote:
> On 5/12/22 09:55, Max Nikulin wrote:
> 
>> +    (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 ...
> 
> file-newer-than-file-p succeeds only if FILE exists, so in that case 
> it'd be a bit more efficient to avoid testing FILE's existence again, e.g.:
> 
>     (cond
>       ((file-newer-than-file-p file tangled-file)
>        (org-bable-tangle-file file ...))
>       ((not (file-exists-p file))
>        (error "File to tangle does not exist: %s" file)))

My opinion is that performance improvement is negligible while negative 
impact related to code readability is noticeable. Maybe it is just 
because elisp is not my favorite language.

Feel free to commit your variant though, I will not object, but I am not 
going to update my patch in this way as well.

P.S. Since I believe it should be fixed in the bugfix branch, I tried to 
keep changes as minimal as possible.

I am not sure which kind of code I would prefer to see. I do not like 
`cond' with 2 branches. Even nested ifs are a bit better while still to 
"heavy"

     (if (not (file-newer-than-file-p file tangled-file))
         (unless (file-exists-p file)
           (error ...))
       (org-babel-tangle-file ...)) ; intentionally put below to the 
else branch

Maybe I would prefer something like file-target-is-up-to-date-p 
predicate that returns nil if target does not exist. For some reason 
`org-babel-tangle-file' silently ignores missed file neither signalling 
a error nor returning a special value. Delegating error handling to 
`org-babel-tangle-file' would allow to get consistent error messages.

In the previous version of the patch (with inline implementation of file 
modification time comparison) the suggested optimization was quite 
natural, with `file-newer-than-file-p' it is trade off with code 
readability.

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

[Paul Eggert]

On 5/13/22 05:28, Max Nikulin wrote:
> 
> Feel free to commit your variant though, I will not object, but I am not 
> going to update my patch in this way as well.

I'll leave it up to you; it's not a big deal either way.

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

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> writes:

> 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

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

-- 
Ihor Radchenko,
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

[Max Nikulin]

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

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

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

From cb03cdf1e7e727d596526690435c86fa5299f1ee Mon Sep 17 00:00:00 2001
In-Reply-To: <87sfk628ca.fsf@localhost>
References: <87sfk628ca.fsf@localhost>
From: Max Nikulin <manikulin@gmail.com>
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
<mbarton98@gmail.com> in
https://list.orgmode.org/BF5B9308-3FEF-4DC6-98C9-BFF36F19D36C@gmail.com
Paul Eggert <eggert@cs.ucla.edu> 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 &optional 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) "")))
     (save-window-excursion
       (pcase process
diff --git a/lisp/org.el b/lisp/org.el
index 6f4e04306..fa73e2976 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -254,22 +254,22 @@ 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
+                                 (or "emacs-lisp" "elisp")
+                                 string-end))
       ;; Make sure that tangled file modification time is
       ;; updated even when `org-babel-tangle-file' does not make changes.
       ;; This avoids re-tangling changed FILE where the changes did
       ;; not affect the tangled code.
       (when (file-exists-p tangled-file)
-        (set-file-times tangled-file))
-      (org-babel-tangle-file file
-                             tangled-file
-                             (rx string-start
-                                 (or "emacs-lisp" "elisp")
-                                 string-end)))
+        (set-file-times tangled-file)))
     (if compile
 	(progn
 	  (byte-compile-file tangled-file)
-- 
2.25.1

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

[Ihor Radchenko]

Max Nikulin <manikulin@gmail.com> 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>