From: Max Nikulin <firstname.lastname@example.org> To: Paul Eggert <email@example.com> Cc: emacs-orgmode <firstname.lastname@example.org> Subject: Re: [PATCH] org-macs.el: Do not compare wall time and file modification time Date: Thu, 12 May 2022 23:55:38 +0700 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> [-- 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 <email@example.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 <firstname.lastname@example.org> https://list.orgmode.org/BF5B9308-3FEF-4DC6-98C9-BFF36F19D36C@gmail.com During discussion of the issue Paul Eggert <email@example.com> 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
next prev parent reply other threads:[~2022-05-12 16:57 UTC|newest] Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top 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 2022-05-11 16:24 ` Paul Eggert 2022-05-12 16:55 ` Max Nikulin [this message] 2022-05-12 22:52 ` Paul Eggert 2022-05-13 12:28 ` Max Nikulin 2022-05-13 18:00 ` Paul Eggert
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: https://www.orgmode.org/ * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH] org-macs.el: Do not compare wall time and file modification time' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
Code repositories for project(s) associated with this inbox: https://git.savannah.gnu.org/cgit/emacs/org-mode.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).