Re: master daea9b3 1/2: Read mailcaps again only when necessary

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Stefan Monnier]

> +(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))))

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.

So I think the function needs a second argument which represents the
"since when".  It could be the hash-table.

Side question: why use `intern`?
Why not just pass `:test #'equal` to the `make-hash-table`?


        Stefan

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> 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 think that's up to the higher level to decide whether it's a problem
or not, like with other memoizing functions.

(And relatedly, this function shouldn't use file-truename or anything on
the filename -- it's a string used as the key.)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 01 Nov 2021 18:01:56 +0100
> Cc: Gregory Heytings <gregory@heytings.org>, emacs-devel@gnu.org
> 
> (And relatedly, this function shouldn't use file-truename or anything on
> the filename -- it's a string used as the key.)

??? Then how do I tell the function which file do I want it to report
on?

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> (And relatedly, this function shouldn't use file-truename or anything on
>> the filename -- it's a string used as the key.)
>
> ??? Then how do I tell the function which file do I want it to report
> on?

???  You give it a filename, which is a string.  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: monnier@iro.umontreal.ca,  gregory@heytings.org,  emacs-devel@gnu.org
> Date: Mon, 01 Nov 2021 18:21:38 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> (And relatedly, this function shouldn't use file-truename or anything on
> >> the filename -- it's a string used as the key.)
> >
> > ??? Then how do I tell the function which file do I want it to report
> > on?
> 
> ???  You give it a filename, which is a string.  

What if we have the same file name in several different directories --
how will you record the files so they all get different hash slots?

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> What if we have the same file name in several different directories --
> how will you record the files so they all get different hash slots?

Oh, you mean relative file names?  I think it's up to the caller to
decide.  If they don't want that, then give an absolute file name, and
if they do want that, then don't.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Stefan Monnier]

Lars Ingebrigtsen [2021-11-01 18:01:56] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> 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 think that's up to the higher level to decide whether it's a problem
> or not, like with other memoizing functions.

What higher level?  I'm talking about two separate packages that don't
know each other, so there's no higher level magic that will save us.


        Stefan

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: monnier@iro.umontreal.ca,  gregory@heytings.org,  emacs-devel@gnu.org
> Date: Mon, 01 Nov 2021 18:29:12 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What if we have the same file name in several different directories --
> > how will you record the files so they all get different hash slots?
> 
> Oh, you mean relative file names?  I think it's up to the caller to
> decide.  If they don't want that, then give an absolute file name, and
> if they do want that, then don't.

But that's contrary to every other Emacs function that accepts file
names.  They all interpret the file names relative to the
default-directory.  And so do the primitives the current
implementation of file-has-changed-p: they expand the file name
internally.  But the file name that gets recorded in the hash table
does not follow that rule, and that is the problem.  because this way
it makes no sense to use relative file names, unless the caller asks
for trouble.  So in practice, every caller will have to pass an
absolute file name, which again is different from all the other APIs
we have that accept file names.

Once again, the key for recording the time stamp is the file name you
pass, so that key must be unique, and that means file-truename.

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Stefan Monnier]

Lars Ingebrigtsen [2021-11-01 18:29:12] wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
>> What if we have the same file name in several different directories --
>> how will you record the files so they all get different hash slots?
> Oh, you mean relative file names?  I think it's up to the caller to
> decide.  If they don't want that, then give an absolute file name, and
> if they do want that, then don't.

To avoid disappointment, it makes sense to pass the filename through
`expand-file-name` or to signal an error if the file name is not
absolute.


        Stefan

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  gregory@heytings.org,  emacs-devel@gnu.org
> Date: Mon, 01 Nov 2021 14:20:41 -0400
> 
> Lars Ingebrigtsen [2021-11-01 18:29:12] wrote:
> > Eli Zaretskii <eliz@gnu.org> writes:
> >> What if we have the same file name in several different directories --
> >> how will you record the files so they all get different hash slots?
> > Oh, you mean relative file names?  I think it's up to the caller to
> > decide.  If they don't want that, then give an absolute file name, and
> > if they do want that, then don't.
> 
> To avoid disappointment, it makes sense to pass the filename through
> `expand-file-name` or to signal an error if the file name is not
> absolute.

Why signal an error when the function can make the file name absolute
so easily?  It makes no sense.  No other API that accepts file names
does that, AFAIK.

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Stefan Monnier]

> Once again, the key for recording the time stamp is the file name you
> pass, so that key must be unique, and that means file-truename.

I'm not sure `file-truename` is indispensable or even desirable.
E.g.:

- read file ~/fileA which is a symlink to /foo/A,
  with `file-truename` when `file-changed-p` is called it records that
  /foo/A has timestamp T1.
- read ~/fileB which is a normal file.  When `file-changed-p` is called it
  records that ~/fileB has timestamp T2.
- user changes ~/fileB into a symlink to /foo/A
- we go and check whether ~/fileB has changed: when `file-changed-p` is
  called it passes it to `file-truename` before it looks up the
  hash-table; it then sees that it's still T1 and will say "everything
  good, ~/fileB hasn't changed".


-- Stefan

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> What higher level?  I'm talking about two separate packages that don't
> know each other, so there's no higher level magic that will save us.

That's true, and we could add a "namespace" parameter -- but...

If we really want to overengineer this, we'd make a file-cache object
that stores both the file name, the file metadata, and the cached data.

Well, come to think of it, perhaps that'd be nice as an interface?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Stefan Monnier]

Lars Ingebrigtsen [2021-11-01 19:48:16] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> What higher level?  I'm talking about two separate packages that don't
>> know each other, so there's no higher level magic that will save us.
> That's true, and we could add a "namespace" parameter -- but...
> If we really want to overengineer this, we'd make a file-cache object
> that stores both the file name, the file metadata, and the cached data.
> Well, come to think of it, perhaps that'd be nice as an interface?

That's pretty close to my suggestion to pass the hash-table as a second
arg, yes.


        Stefan

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Lars Ingebrigtsen <larsi@gnus.org>,  gregory@heytings.org,
>   emacs-devel@gnu.org
> Date: Mon, 01 Nov 2021 14:35:00 -0400
> 
> > Once again, the key for recording the time stamp is the file name you
> > pass, so that key must be unique, and that means file-truename.
> 
> I'm not sure `file-truename` is indispensable or even desirable.
> E.g.:
> 
> - read file ~/fileA which is a symlink to /foo/A,
>   with `file-truename` when `file-changed-p` is called it records that
>   /foo/A has timestamp T1.
> - read ~/fileB which is a normal file.  When `file-changed-p` is called it
>   records that ~/fileB has timestamp T2.
> - user changes ~/fileB into a symlink to /foo/A
> - we go and check whether ~/fileB has changed: when `file-changed-p` is
>   called it passes it to `file-truename` before it looks up the
>   hash-table; it then sees that it's still T1 and will say "everything
>   good, ~/fileB hasn't changed".

That's why I think we should also record the size and the inode, and
we probably should do that before taking file-truename.  The latter is
just for recording.

Of course, the situation you described is also highly improbable: why
should the same program care about two files in exactly that sequence
as to make your point for you?  What are the chances of that?

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> That's pretty close to my suggestion to pass the hash-table as a second
> arg, yes.

Sort of something like:

(setq mailcap-data
      (file-refresh-if-changed
       (or mailcap-data (make-instance 'file-data-cache
                                       :file "/etc/mailcap"
                                       :refresher #'mailcap-parse-file))))

and then the data would be in

(slot-value mailcap-data 'data)

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Stefan Monnier]

> That's why I think we should also record the size and the inode, and
> we probably should do that before taking file-truename.  The latter is
> just for recording.

There's a case to be made to just store most of the output of
`file-attributes`, indeed, instead of just the modification time.

> Of course, the situation you described is also highly improbable: why
> should the same program care about two files in exactly that sequence
> as to make your point for you?  What are the chances of that?

Maybe it's highly unlikely, indeed.

But I don't see what's the benefit of calling `file-truename`: I know it
makes it slower and introduces an (unlikely) broken corner case, but
what's the upside?


        Stefan

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Stefan Monnier]

Lars Ingebrigtsen [2021-11-01 20:04:33] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> That's pretty close to my suggestion to pass the hash-table as a second
>> arg, yes.
>
> Sort of something like:
>
> (setq mailcap-data
>       (file-refresh-if-changed
>        (or mailcap-data (make-instance 'file-data-cache
>                                        :file "/etc/mailcap"
>                                        :refresher #'mailcap-parse-file))))
>
> and then the data would be in
>
> (slot-value mailcap-data 'data)

I don't see any need to bring in EIEIO into this (remember that EIEIO
objects and operations are slow).

I was thinking more along the lines of

    (defun file-contents-attributes (file)
      (let ((a (file-attributes file)))
        ;; Stamp out data that will never reflect a change in the file.
        (setf (file-attribute-access-time a) nil)
        (setf (file-attribute-link-number a) nil)
        a))

    (defun file-unchanged-p (file cache)
      (unless (file-name-absolute-p file) (setq file (expand-file-name file)))
      (let ((old (gethash (cons file 'file-contents-attributes) cache))
            (new (file-contents-attributes file)))
        (if (equal old new)
            t
          (puthash (cons file 'file-contents-attributes) new cache)
          nil)))

    (defun mailcap-parse-file (file &optional cache)
      (unless (file-name-absolute-p file) (setq file (expand-file-name file)))
      (or (and cache
               (file-unchanged-p file cache)
               (gethash (cons file 'mailcap-parse-file) cache))
          (let ((data <do the actual parsing>))
            (when cache
              (puthash (cons file 'mailcap-parse-file) data cache))
            data)))


-- Stefan

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: larsi@gnus.org,  gregory@heytings.org,  emacs-devel@gnu.org
> Date: Mon, 01 Nov 2021 17:07:12 -0400
> 
> But I don't see what's the benefit of calling `file-truename`: I know it
> makes it slower and introduces an (unlikely) broken corner case, but
> what's the upside?

It will avoid several false responses, with symlinks and on
filesystems with special "features", like case-insensitivity.

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Stefan Monnier]

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: larsi@gnus.org,  gregory@heytings.org,  emacs-devel@gnu.org
>> Date: Mon, 01 Nov 2021 17:07:12 -0400
>> 
>> But I don't see what's the benefit of calling `file-truename`: I know it
>> makes it slower and introduces an (unlikely) broken corner case, but
>> what's the upside?
>
> It will avoid several false responses, with symlinks and on
> filesystems with special "features", like case-insensitivity.

I can see cases where using `file-truename` will return slightly less
pessimistic results, e.g. if we test "FooBar" after having tested
"foobar" it may avoid deciding it's changed even tho it wasn't.
But that seems like a very minor advantage (such pessimistic results
are largely unavoidable, so code should be prepared to work correctly
in their presence anyway).


        Stefan

Re: master daea9b3 1/2: Read mailcaps again only when necessary

[Lars Ingebrigtsen]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>     (defun mailcap-parse-file (file &optional cache)
>       (unless (file-name-absolute-p file) (setq file (expand-file-name file)))
>       (or (and cache
>                (file-unchanged-p file cache)
>                (gethash (cons file 'mailcap-parse-file) cache))
>           (let ((data <do the actual parsing>))
>             (when cache
>               (puthash (cons file 'mailcap-parse-file) data cache))
>             data)))

That does have the advantage that the package can store the data for
many files in the same place, which is nice.  But the interface doesn't
seem as clear as it could be.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no