bug#51523: 29.0.50; gnus-mime-view-part-externally very slow

[Gregory Heytings <gregory@heytings.org>]

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


I attach an improved version of file-has-changed-p.  Following Eli's 
suggestion, it records the file size together with its timestamp.

Stefan:
>
> This API doesn't seem safe.
>
> If two packages read&parse the same file and both rely on this function 
> to decide when to reparse, the second package can get a nil value 
> (because the first has caused the mtime to be reset in the hash table) 
> even though the file has changed since it last parsed it.
>

I agree with you, and added a second optional "tag" argument to the 
function, in which one can for example pass the name of the calling 
function, or the name of the library.  It's the responsibility of the 
caller to use the tag they want.

Eli:
>
> What would be the purpose of replacing with an older file, but keeping 
> the old time stamp?  And why is Gnus obligated to support such strange 
> use cases?  We can always tell the users to bump the file's time stamp 
> by 'touch'ing it, no?
>

It's not a strange use case at all.  For example, you make a backup of 
your ~/.mailcap file, make some changes in the original file, decide that 
after all they're not what you want, and do mv ~/.mailcap.bak ~/.mailcap.

>
> But the real answer to that question is to compare the contents, not 
> file's attributes.  Testing attributes is an approximation, and once we 
> are using an approximation, it is legitimate to ask when it's okay for 
> the approximation to fail.
>

It's an approximation indeed, but it's a very safe one.  It's what rsync 
uses, and I haven't seen a single case in which it failed to do TRT in 
twenty years or so.  The likelihood that a file with the exact same 
filename, same size and same timestamp have different contents is, in 
practice, zero.  It is of course possible to cheat that detection 
mechanism, but only if you do it on purpose.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-diff; name=Improve-file-has-changed-p.patch, Size: 4288 bytes --]

From f8b969b1aced58e74e942a09335ba3f3c752eba1 Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Mon, 1 Nov 2021 21:10:49 +0000
Subject: [PATCH] Improve file-has-changed-p.

* lisp/files (file-has-changed-p): Add a second argument, and use it.
Update the docstring.

* doc/lispref/files.texi: Update the documentation.

* lisp/net/mailcap.el: Add a second argument to the call to
file-has-changed-p.
---
 doc/lispref/files.texi |  8 +++++---
 lisp/files.el          | 28 ++++++++++++++++------------
 lisp/net/mailcap.el    |  4 +++-
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/doc/lispref/files.texi b/doc/lispref/files.texi
index 250f7a3f9f..ce967ee381 100644
--- a/doc/lispref/files.texi
+++ b/doc/lispref/files.texi
@@ -1314,13 +1314,15 @@ File Attributes
 @end example
 @end defun
 
-@defun file-has-changed-p filename
+@defun file-has-changed-p filename tag
 This convenience function is useful when, for instance, parsing files
 run-time, and you typically want to re-read a file when it has
 changed.  This function returns non-@code{nil} the first time it's
 called on @var{filename} in an Emacs session, but will return
-@code{nil} on subsequent calls in that session (unless the file
-changes its modification time).
+@code{nil} on subsequent calls in that session (unless the file size
+or modification time has changed in the meantime).  With an optional
+argument @var{tag}, the size and modification time comparisons are
+limited to calls with the same tag.
 @end defun
 
 @defun file-attributes filename &optional id-format
diff --git a/lisp/files.el b/lisp/files.el
index 5e7be3844e..d7dfa9399e 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6181,21 +6181,25 @@ file-in-directory-p
 	  (unless mismatch
 	    (file-equal-p root dir)))))))
 
-(defvar file-has-changed-p--hash-table (make-hash-table)
+(defvar file-has-changed-p--hash-table (make-hash-table :test #'equal)
   "Internal variable used by `file-has-changed-p'.")
 
-(defun file-has-changed-p (file)
+(defun file-has-changed-p (file &optional tag)
   "Return non-nil if FILE has changed.
-The modification time of FILE is compared to the modification
-time of FILE during a previous invocation of `file-has-changed-p'.
-Therefore the first invocation of `file-has-changed-p' always
-returns non-nil."
-  (let* ((attr (file-attributes file 'integer))
-	  (mtime (file-attribute-modification-time attr))
-	  (saved-mtime (gethash (intern file)
-				file-has-changed-p--hash-table)))
-     (when (not (equal mtime saved-mtime))
-       (puthash (intern file) mtime file-has-changed-p--hash-table))))
+The size and modification time of FILE is compared to the size
+and modification time of FILE during a previous invocation of
+`file-has-changed-p'.  Therefore the first invocation of
+`file-has-changed-p' always returns non-nil.
+The optional argument TAG can be used to limit the comparison to
+invocations with identical tags; it can for example be the symbol
+of the calling function."
+  (let* ((fileattr (file-attributes file 'integer))
+	 (attr (cons (file-attribute-size fileattr)
+		     (file-attribute-modification-time fileattr)))
+	 (sym (concat (symbol-name tag) "@" file))
+	 (cachedattr (gethash sym file-has-changed-p--hash-table)))
+     (when (not (equal attr cachedattr))
+       (puthash sym attr file-has-changed-p--hash-table))))
 
 (defun copy-directory (directory newname &optional keep-time parents copy-contents)
   "Copy DIRECTORY to NEWNAME.  Both args must be strings.
diff --git a/lisp/net/mailcap.el b/lisp/net/mailcap.el
index 4dedd38c22..e40cf2a336 100644
--- a/lisp/net/mailcap.el
+++ b/lisp/net/mailcap.el
@@ -447,7 +447,9 @@ mailcap-parse-mailcaps
               ("/etc/mailcap" system)
               ("/usr/etc/mailcap" system)
 	      ("/usr/local/etc/mailcap" system)))))
-    (when (seq-some (lambda (f) (file-has-changed-p (car f))) path)
+    (when (seq-some (lambda (f)
+                      (file-has-changed-p (car f) 'mail-parse-mailcaps))
+                    path)
       ;; The ~/.mailcap entries will end up first in the resulting data.
       (dolist (spec (reverse
 		     (if (stringp path)
-- 
2.33.0