bug#41489: `package-dir-info' fails on a directory with a non-saved file

bug#41489: `package-dir-info' fails on a directory with a non-saved file

[Paul Pogonyshev]

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

To reproduce:

- edit any Elisp file, but don't save it;
- open its directory in Dired;
- evaluate `(package-dir-info)'.

Fails with: (file-missing "Opening input file" "No such file or directory"
".../.#blabla.el")

I.e. it tries to open the lock created for the unsaved file as a normal
file and fails. I'm not 100% sure this should be considered a bug, but from
my point of view it is so. Even if this is not a bug, the function should
probably fail with a more descriptive error.

Paul

[-- Attachment #2: Type: text/html, Size: 670 bytes --]

bug#41489: `package-dir-info' fails on a directory with a non-saved file

[Eli Zaretskii]

> From: Paul Pogonyshev <pogonyshev@gmail.com>
> Date: Sat, 23 May 2020 19:50:02 +0200
> 
> - edit any Elisp file, but don't save it;
> - open its directory in Dired;
> - evaluate `(package-dir-info)'.
> 
> Fails with: (file-missing "Opening input file" "No such file or directory" ".../.#blabla.el")

In what version of Emacs is that?  Please always supply the
information collected by report-emacs-bug, as that saves unnecessary
questions such as this one.

Thanks.

bug#41489: `package-dir-info' fails on a directory with a non-saved file

[Paul Pogonyshev]

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

Sorry. In any Emacs version. I tested with a fairly recently compiled
`master', but judging by the source code I checked out two minutes ago, the
bug is there currently too. The cause is this:

            (insert-file-contents (pop files))

This form is not inside `(ignore-errors ...)' and so any failure is
propagated out of `package-dir-info' too.

Paul

On Sat, 23 May 2020 at 20:23, Eli Zaretskii <eliz@gnu.org> wrote:

> > From: Paul Pogonyshev <pogonyshev@gmail.com>
> > Date: Sat, 23 May 2020 19:50:02 +0200
> >
> > - edit any Elisp file, but don't save it;
> > - open its directory in Dired;
> > - evaluate `(package-dir-info)'.
> >
> > Fails with: (file-missing "Opening input file" "No such file or
> directory" ".../.#blabla.el")
>
> In what version of Emacs is that?  Please always supply the
> information collected by report-emacs-bug, as that saves unnecessary
> questions such as this one.
>
> Thanks.
>
>

[-- Attachment #2: Type: text/html, Size: 1464 bytes --]

bug#41489: `package-dir-info' fails on a directory with a non-saved file

[Paul Pogonyshev]

[-- Attachment #1.1: Type: text/plain, Size: 1261 bytes --]

I also noticed that the error depends on the Moon phase, i.e. it is not
_always_ reproducible. The reason is that it depends on the random order of
elements in `(directory-files default-directory t "\\.el\\'" t)' result.

On Sat, 23 May 2020 at 20:37, Paul Pogonyshev <pogonyshev@gmail.com> wrote:

> Sorry. In any Emacs version. I tested with a fairly recently compiled
> `master', but judging by the source code I checked out two minutes ago, the
> bug is there currently too. The cause is this:
>
>             (insert-file-contents (pop files))
>
> This form is not inside `(ignore-errors ...)' and so any failure is
> propagated out of `package-dir-info' too.
>
> Paul
>
> On Sat, 23 May 2020 at 20:23, Eli Zaretskii <eliz@gnu.org> wrote:
>
>> > From: Paul Pogonyshev <pogonyshev@gmail.com>
>> > Date: Sat, 23 May 2020 19:50:02 +0200
>> >
>> > - edit any Elisp file, but don't save it;
>> > - open its directory in Dired;
>> > - evaluate `(package-dir-info)'.
>> >
>> > Fails with: (file-missing "Opening input file" "No such file or
>> directory" ".../.#blabla.el")
>>
>> In what version of Emacs is that?  Please always supply the
>> information collected by report-emacs-bug, as that saves unnecessary
>> questions such as this one.
>>
>> Thanks.
>>
>>

[-- Attachment #1.2: Type: text/html, Size: 2077 bytes --]

[-- Attachment #2: 0001-Fix-random-errors-in-package-dir-info-caused-by-unre.patch --]
[-- Type: text/x-patch, Size: 1005 bytes --]

From 5fd331e43e18e7b0fc11e93eb3b5fcd43a301d36 Mon Sep 17 00:00:00 2001
From: Paul Pogonyshev <pogonyshev@gmail.com>
Date: Tue, 26 May 2020 00:52:14 +0200
Subject: [PATCH] Fix random errors in `package-dir-info' caused by unreadable
 files (bug#39722)

---
 lisp/emacs-lisp/package.el | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 9a6d1d7319..99ba5d7107 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -1181,7 +1181,9 @@ package-dir-info
             info)
         (while files
           (with-temp-buffer
-            (insert-file-contents (pop files))
+            ;; Skip unreadable files, e.g. locks for unsaved `.el'
+            ;; buffers (bug#41489).
+            (ignore-errors (insert-file-contents (pop files)))
             ;; When we find the file with the data,
             (when (setq info (ignore-errors (package-buffer-info)))
               ;; stop looping,
-- 
2.20.1

bug#41489: `package-dir-info' fails on a directory with a non-saved file

[Stefan Kangas]

Paul Pogonyshev <pogonyshev@gmail.com> writes:

> diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> index 9a6d1d7319..99ba5d7107 100644
> --- a/lisp/emacs-lisp/package.el
> +++ b/lisp/emacs-lisp/package.el
> @@ -1181,7 +1181,9 @@ package-dir-info
>              info)
>          (while files
>            (with-temp-buffer
> -            (insert-file-contents (pop files))
> +            ;; Skip unreadable files, e.g. locks for unsaved `.el'
> +            ;; buffers (bug#41489).
> +            (ignore-errors (insert-file-contents (pop files)))
>              ;; When we find the file with the data,
>              (when (setq info (ignore-errors (package-buffer-info)))
>                ;; stop looping,

Do we really want to ignore *any* error from insert-file-contents here?

Should we really run package-buffer-info if inserting the file fails?
Won't that reach (error "Package lacks a file header") and signal an
error anyways, just a different and more cryptic one?

Best regards,
Stefan Kangas

bug#41489: `package-dir-info' fails on a directory with a non-saved file

[Paul Pogonyshev]

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

> Do we really want to ignore *any* error from insert-file-contents here?

Well, maybe that should be limited to `file-missing' instead (what actually
happens when it tries to read a lock file).

> Should we really run package-buffer-info if inserting the file fails?
> Won't that reach (error "Package lacks a file header") and signal an
> error anyways, just a different and more cryptic one?

`package-buffer-info' is already inside a different `ignore-errors', so it
will signal an error, but that error will be ignored and the file skipped.

I'm not attached to any particular way this bug is fixed. Please adjust it
yourself, the patch is only an example of how it could be done. This will
be faster than if we try to negotiate the best way and recreate the patch.

BTW, the bug being reproducible only in 50% of the cases makes it even more
important to be fixed from my point of view. Nothing is worse than
unspecified behavior when it's not justified by reasons like huge
performance gain in my opinion.

Paul

On Tue, 26 May 2020 at 04:07, Stefan Kangas <stefankangas@gmail.com> wrote:

> Paul Pogonyshev <pogonyshev@gmail.com> writes:
>
> > diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
> > index 9a6d1d7319..99ba5d7107 100644
> > --- a/lisp/emacs-lisp/package.el
> > +++ b/lisp/emacs-lisp/package.el
> > @@ -1181,7 +1181,9 @@ package-dir-info
> >              info)
> >          (while files
> >            (with-temp-buffer
> > -            (insert-file-contents (pop files))
> > +            ;; Skip unreadable files, e.g. locks for unsaved `.el'
> > +            ;; buffers (bug#41489).
> > +            (ignore-errors (insert-file-contents (pop files)))
> >              ;; When we find the file with the data,
> >              (when (setq info (ignore-errors (package-buffer-info)))
> >                ;; stop looping,
>
> Do we really want to ignore *any* error from insert-file-contents here?
>
> Should we really run package-buffer-info if inserting the file fails?
> Won't that reach (error "Package lacks a file header") and signal an
> error anyways, just a different and more cryptic one?
>
> Best regards,
> Stefan Kangas
>

[-- Attachment #2: Type: text/html, Size: 2929 bytes --]

bug#41489: `package-dir-info' fails on a directory with a non-saved file

[Stefan Kangas]

Paul Pogonyshev <pogonyshev@gmail.com> writes:

>> Do we really want to ignore *any* error from insert-file-contents here?
>
> Well, maybe that should be limited to `file-missing' instead (what actually
> happens when it tries to read a lock file).

Sounds good.

>> Should we really run package-buffer-info if inserting the file fails?
>> Won't that reach (error "Package lacks a file header") and signal an
>> error anyways, just a different and more cryptic one?
>
> `package-buffer-info' is already inside a different `ignore-errors', so it
> will signal an error, but that error will be ignored and the file skipped.

Thanks for clarifying.  I didn't study this code recently.

> I'm not attached to any particular way this bug is fixed. Please adjust it
> yourself, the patch is only an example of how it could be done. This will
> be faster than if we try to negotiate the best way and recreate the patch.

I don't mean to discourage you from working on this, on the contrary.
We appreciate that you're working on a fix.

Best regards,
Stefan Kangas

bug#41489: `package-dir-info' fails on a directory with a non-saved file

[Lars Ingebrigtsen]

Paul Pogonyshev <pogonyshev@gmail.com> writes:

> To reproduce:
>
> - edit any Elisp file, but don't save it;
> - open its directory in Dired;
> - evaluate `(package-dir-info)'.
>
> Fails with: (file-missing "Opening input file" "No such file or directory"
> ".../.#blabla.el")

I've now made it ignore nonexistent files in Emacs 29.

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