* 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).