* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
@ 2022-04-27 6:37 Mark Barton
2022-04-27 7:20 ` Po Lu
2022-04-27 7:39 ` Paul Eggert
0 siblings, 2 replies; 10+ messages in thread
From: Mark Barton @ 2022-04-27 6:37 UTC (permalink / raw)
To: eggert, emacs-devel, emacs-orgmode
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)))))
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
2022-04-27 6:37 master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc Mark Barton
@ 2022-04-27 7:20 ` Po Lu
2022-04-27 7:39 ` Paul Eggert
1 sibling, 0 replies; 10+ messages in thread
From: Po Lu @ 2022-04-27 7:20 UTC (permalink / raw)
To: Mark Barton; +Cc: eggert, emacs-devel, emacs-orgmode
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?
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
2022-04-27 6:37 master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc Mark Barton
2022-04-27 7:20 ` Po Lu
@ 2022-04-27 7:39 ` Paul Eggert
2022-04-27 16:55 ` Stefan Monnier
1 sibling, 1 reply; 10+ messages in thread
From: Paul Eggert @ 2022-04-27 7:39 UTC (permalink / raw)
To: Mark Barton, emacs-devel, emacs-orgmode
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
2022-04-27 7:39 ` Paul Eggert
@ 2022-04-27 16:55 ` Stefan Monnier
2022-04-28 22:27 ` Paul Eggert
0 siblings, 1 reply; 10+ messages in thread
From: Stefan Monnier @ 2022-04-27 16:55 UTC (permalink / raw)
To: Paul Eggert; +Cc: Mark Barton, emacs-devel, emacs-orgmode
> - (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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
2022-04-27 16:55 ` Stefan Monnier
@ 2022-04-28 22:27 ` Paul Eggert
2022-04-29 14:22 ` Max Nikulin
0 siblings, 1 reply; 10+ messages in thread
From: Paul Eggert @ 2022-04-28 22:27 UTC (permalink / raw)
To: Stefan Monnier; +Cc: Mark Barton, emacs-orgmode, emacs-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
2022-04-28 22:27 ` Paul Eggert
@ 2022-04-29 14:22 ` Max Nikulin
2022-04-29 18:10 ` Paul Eggert
0 siblings, 1 reply; 10+ messages in thread
From: Max Nikulin @ 2022-04-29 14:22 UTC (permalink / raw)
To: Paul Eggert; +Cc: emacs-orgmode, emacs-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
2022-04-29 14:22 ` Max Nikulin
@ 2022-04-29 18:10 ` Paul Eggert
2022-04-30 10:56 ` Max Nikulin
0 siblings, 1 reply; 10+ messages in thread
From: Paul Eggert @ 2022-04-29 18:10 UTC (permalink / raw)
To: Max Nikulin; +Cc: emacs-orgmode, emacs-devel
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
2022-04-29 18:10 ` Paul Eggert
@ 2022-04-30 10:56 ` Max Nikulin
2022-05-06 16:56 ` [PATCH] org-macs.el: Do not compare wall time and file modification time Max Nikulin
0 siblings, 1 reply; 10+ messages in thread
From: Max Nikulin @ 2022-04-30 10:56 UTC (permalink / raw)
To: Paul Eggert; +Cc: emacs-orgmode, emacs-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] org-macs.el: Do not compare wall time and file modification time
2022-04-30 10:56 ` Max Nikulin
@ 2022-05-06 16:56 ` Max Nikulin
2022-05-11 12:28 ` Max Nikulin
0 siblings, 1 reply; 10+ messages in thread
From: Max Nikulin @ 2022-05-06 16:56 UTC (permalink / raw)
To: Paul Eggert, Bastien; +Cc: emacs-orgmode, emacs-devel
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] org-macs.el: Do not compare wall time and file modification time
2022-05-06 16:56 ` [PATCH] org-macs.el: Do not compare wall time and file modification time Max Nikulin
@ 2022-05-11 12:28 ` Max Nikulin
0 siblings, 0 replies; 10+ messages in thread
From: Max Nikulin @ 2022-05-11 12:28 UTC (permalink / raw)
To: Paul Eggert, Bastien; +Cc: emacs-orgmode, emacs-devel
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2022-05-11 12:28 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-27 6:37 master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc Mark Barton
2022-04-27 7:20 ` Po Lu
2022-04-27 7:39 ` Paul Eggert
2022-04-27 16:55 ` Stefan Monnier
2022-04-28 22:27 ` Paul Eggert
2022-04-29 14:22 ` Max Nikulin
2022-04-29 18:10 ` Paul Eggert
2022-04-30 10:56 ` Max Nikulin
2022-05-06 16:56 ` [PATCH] org-macs.el: Do not compare wall time and file modification time Max Nikulin
2022-05-11 12:28 ` Max Nikulin
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).