Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
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 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.
Re: master 4a1f69ebca 2/2: Use (TICKS . HZ) for current-time etc.
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?From fbd6561952acf359236afcf7957a197376a18c66 Mon Sep 17 00:00:00 2001 From: Paul Eggert 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 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.
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.
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.
> - (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.
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.From 3abb3681b57d7c8ca7fa808addb0a10b6b109cab Mon Sep 17 00:00:00 2001 From: Paul Eggert 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 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.
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)
