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

[Eli Zaretskii <eliz@gnu.org>]

> Date: Mon, 01 Nov 2021 12:26:03 +0000
> From: Gregory Heytings <gregory@heytings.org>
> Cc: 51523@debbugs.gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
> 
> I attach a better patch, it uses a file-has-change-p function instead of a 
> when-file-has-changed macro.

Thanks.

> +(defun file-has-changed-p (file)
> +  "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))))

Bother: I think this implementation might cause both false positives
and false negatives.

  . it uses the literal file name without even expanding it to an
    absolute file name, so the same FILE might mean different files if
    default-directory changes
  . file names are generally not reliable enough for unique
    identifiers of files (think symlinks on all systems, letter-case
    and numerical tails on Windows, etc.), so we should at least use
    file-truename
  . interning the file names could produce many unnecessary symbols in
    the global obarray

Can we make the implementation more robust by fixing these?

The name of the function also doesn't reflect what it does: it only
looks at the file's last modification time, so maybe at least "time"
should be in the name?

I also question the decision of testing modification time for
equality: why not check if the time stamp is newer, and disregard the
changes to an older time stamp?  When looking this way at this
function, I ask myself whether extending file-newer-than-file-p to do
this job would be a better idea?  Or maybe I don't understand the
general use case for this function.