bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

Lars Ingebrigtsen

Create a file /tmp/a.txt, and visit /tmp.
M-x browse-url-of-dired-file on the file.

Emacs will say:

File exists, but cannot be read

and then display the file in a text-mode buffer.

If you then go to the real a.txt buffer, Emacs will then say

/tmp/a.txt and file:///tmp/a.txt are the same file

All this is very confusing, because the doc string of the command is

--
In Dired, ask a WWW browser to display the file named on this line.
--

So it should just call `browse-url' on the file:// URL, and not do
anything about these buffers?



In GNU Emacs 28.0.50 (build 8, x86_64-pc-linux-gnu, GTK+ Version 3.24.20, cairo version 1.16.0)
 of 2020-07-19 built on xo
Repository revision: 17f646128f04e9e8590f0371026a14d516f21c63
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Debian GNU/Linux bullseye/sid


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




Reply | Threaded
Open this post in threaded view
|

bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

Lars Ingebrigtsen
Lars Ingebrigtsen <[hidden email]> writes:

> All this is very confusing, because the doc string of the command is
>
> --
> In Dired, ask a WWW browser to display the file named on this line.
> --
>
> So it should just call `browse-url' on the file:// URL, and not do
> anything about these buffers?

Ah, it's because of this --

(defvar browse-url-default-handlers
  '(("\\`mailto:" . browse-url--mailto)
    ("\\`man:" . browse-url--man)
    (browse-url--non-html-file-url-p . browse-url-emacs))

browse-url on a file that doesn't have .html in it will now not open a
browser at all, but instead an Emacs buffer.

In any case, the messaging is misleading and the buffer name is
confusing.

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



Reply | Threaded
Open this post in threaded view
|

bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

Lars Ingebrigtsen
In reply to this post by Lars Ingebrigtsen
Lars Ingebrigtsen <[hidden email]> writes:

> Create a file /tmp/a.txt, and visit /tmp.
> M-x browse-url-of-dired-file on the file.
>
> Emacs will say:
>
> File exists, but cannot be read
>
> and then display the file in a text-mode buffer.

Here's a simpler reproduction.  Ensure that /tmp/a.txt exists, and then
eval:

(browse-url-of-file "/tmp/a.txt")

The message originates from here, sort of:

(defun find-file-noselect-1 (buf filename nowarn rawfile truename number)

[...]

          (condition-case ()
              (let ((inhibit-read-only t))
                (insert-file-contents-literally filename t))
            (file-error
             (when (and (file-exists-p filename)
                        (not (file-readable-p filename)))
               (kill-buffer buf)
               (signal 'file-error (list "File is not readable"
                                         filename)))

If we remove that condition-case, we get:

Debugger entered--Lisp error: (file-error "Opening input file" "Success")
  insert-file-contents("file:///tmp/a.txt" t)

So the insert-file-contents-literally signals a "Success" file-error
when using url-handler-mode.

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



Reply | Threaded
Open this post in threaded view
|

bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

Eli Zaretskii
> From: Lars Ingebrigtsen <[hidden email]>
> Date: Tue, 28 Jul 2020 00:24:23 +0200
> Cc: Michael Albinus <[hidden email]>
>
> (browse-url-of-file "/tmp/a.txt")
>
> The message originates from here, sort of:
>
> (defun find-file-noselect-1 (buf filename nowarn rawfile truename number)
>
> [...]
>
>  (condition-case ()
>      (let ((inhibit-read-only t))
> (insert-file-contents-literally filename t))
>    (file-error
>     (when (and (file-exists-p filename)
> (not (file-readable-p filename)))
>       (kill-buffer buf)
>       (signal 'file-error (list "File is not readable"
> filename)))

Is this because file-readable-p returns nil for file:// URLs?



Reply | Threaded
Open this post in threaded view
|

bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

Lars Ingebrigtsen
Eli Zaretskii <[hidden email]> writes:

>> (defun find-file-noselect-1 (buf filename nowarn rawfile truename number)
>>
>> [...]
>>
>>  (condition-case ()
>>      (let ((inhibit-read-only t))
>> (insert-file-contents-literally filename t))
>>    (file-error
>>     (when (and (file-exists-p filename)
>> (not (file-readable-p filename)))
>>       (kill-buffer buf)
>>       (signal 'file-error (list "File is not readable"
>> filename)))
>
> Is this because file-readable-p returns nil for file:// URLs?

That's the direct cause of the message, but the underlying reason is
that insert-file-contents-literally signalled a file-error here (after
inserting the contents).  I haven't yet chased down why.

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



Reply | Threaded
Open this post in threaded view
|

bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

Eli Zaretskii
> From: Lars Ingebrigtsen <[hidden email]>
> Cc: [hidden email],  [hidden email]
> Date: Sat, 08 Aug 2020 12:05:04 +0200
>
> >>  (condition-case ()
> >>      (let ((inhibit-read-only t))
> >> (insert-file-contents-literally filename t))
> >>    (file-error
> >>     (when (and (file-exists-p filename)
> >> (not (file-readable-p filename)))
> >>       (kill-buffer buf)
> >>       (signal 'file-error (list "File is not readable"
> >> filename)))
> >
> > Is this because file-readable-p returns nil for file:// URLs?
>
> That's the direct cause of the message, but the underlying reason is
> that insert-file-contents-literally signalled a file-error here (after
> inserting the contents).  I haven't yet chased down why.

I guess that's because expand-file-name doesn't convert file:// URLs
into local file names, and then insert-file-contents chokes on the
value produced by expand-file-name.  (insert-file-contents-literally
is just a thin wrapper around insert-file-contents.)

So one solution would be to convert file:// URLs into local file names
in the above snippet, before calling insert-file-contents-literally.



Reply | Threaded
Open this post in threaded view
|

bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

Lars Ingebrigtsen
Eli Zaretskii <[hidden email]> writes:

>> From: Lars Ingebrigtsen <[hidden email]>
>> Cc: [hidden email],  [hidden email]
>> Date: Sat, 08 Aug 2020 12:05:04 +0200
>>
>> >>  (condition-case ()
>> >>      (let ((inhibit-read-only t))
>> >> (insert-file-contents-literally filename t))
>> >>    (file-error
>> >>     (when (and (file-exists-p filename)
>> >> (not (file-readable-p filename)))
>> >>       (kill-buffer buf)
>> >>       (signal 'file-error (list "File is not readable"
>> >> filename)))
>> >
>> > Is this because file-readable-p returns nil for file:// URLs?
>>
>> That's the direct cause of the message, but the underlying reason is
>> that insert-file-contents-literally signalled a file-error here (after
>> inserting the contents).  I haven't yet chased down why.

My analysis here was kinda wrong -- the code above isn't what gives the
warning, because all those functions up there do the right thing, since
file-name-handler-alist is set:

  (let ((file-name-handler-alist
         (cons (cons url-handler-regexp 'url-file-handler)
               file-name-handler-alist)))
    (list (file-exists-p "file:///tmp/a.txt")
          (file-readable-p "file:///tmp/a.txt")))
=> (t t)

The problem is that insert-file-contents signals a file-error, and the
error string is "Success".  This makes things confused, because it knows
that it has an error, but when it tests for all things that could go
wrong, it doesn't find anything, and ends up here:

(defun after-find-file (&optional error warn noauto
                                  _after-find-file-from-revert-buffer
                                  nomodes)
[...]
             ((and error (file-attributes buffer-file-name))
              (setq buffer-read-only t)
              (if (and (file-symlink-p buffer-file-name)
                       (not (file-exists-p
                             (file-chase-links buffer-file-name))))
                  "Symbolic link that points to nonexistent file"
                "File exists, but cannot be read"))

Which is where the message itself comes from.

> So one solution would be to convert file:// URLs into local file names
> in the above snippet, before calling insert-file-contents-literally.

It would be, but I think this points to an error in insert-file-contents
itself.  I'll poke around some more...

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



Reply | Threaded
Open this post in threaded view
|

bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

Lars Ingebrigtsen
Lars Ingebrigtsen <[hidden email]> writes:

> It would be, but I think this points to an error in insert-file-contents
> itself.  I'll poke around some more...

Yup.  The error signalling comes from Finsert_file_contents.  If I make
this change, then the confusing messaging goes away:

diff --git a/src/fileio.c b/src/fileio.c
index 37072d9b6b..05e262b201 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -4826,7 +4826,6 @@ because (1) it preserves some marker positions and (2) it puts less data
   if (!NILP (visit) && current_buffer->modtime.tv_nsec < 0)
     {
       /* Signal an error if visiting a file that could not be opened.  */
-      report_file_errno ("Opening input file", orig_filename, save_errno);
     }
 
   /* We made a lot of deletions and insertions above, so invalidate

This was apparently introduced/changed in 2019 by this patch:

commit 3a1e7624ed234bb434cdafed59515cadd037cafa
Author:     Paul Eggert <[hidden email]>
AuthorDate: Thu Oct 31 23:31:17 2019 -0700
Commit:     Paul Eggert <[hidden email]>
CommitDate: Thu Oct 31 23:32:05 2019 -0700

    Fix insert-file-contents file error regression
   
    Problem reported for dired-view-file (Bug#37950).
    * src/fileio.c (Finsert_file_contents): When visiting,
    signal an error if the file could not be opened for any reason,
    rather than signaling an error only for nonexistent files, fixing
    a bug introduced in 2019-09-16T03:17:43![hidden email].

I've Cc'd Paul on this.  Paul, the test case is:

(browse-url-of-file "/tmp/a.txt")

This will open the file correctly (via the url-file-handler file name
handler), but Emacs will then message "File exists, but cannot be read"
because Finsert_file_contents signals an error with the error message
"Success".

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



Reply | Threaded
Open this post in threaded view
|

bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

Eli Zaretskii
> From: Lars Ingebrigtsen <[hidden email]>
> Cc: [hidden email],  [hidden email], Paul Eggert
>  <[hidden email]>
> Date: Sun, 09 Aug 2020 11:45:05 +0200
>
> Lars Ingebrigtsen <[hidden email]> writes:
>
> > It would be, but I think this points to an error in insert-file-contents
> > itself.  I'll poke around some more...
>
> Yup.  The error signalling comes from Finsert_file_contents.  If I make
> this change, then the confusing messaging goes away:
>
> diff --git a/src/fileio.c b/src/fileio.c
> index 37072d9b6b..05e262b201 100644
> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -4826,7 +4826,6 @@ because (1) it preserves some marker positions and (2) it puts less data
>    if (!NILP (visit) && current_buffer->modtime.tv_nsec < 0)
>      {
>        /* Signal an error if visiting a file that could not be opened.  */
> -      report_file_errno ("Opening input file", orig_filename, save_errno);
>      }
>  

Of course.  And that's exactly what I meant when I suggested to
convert the file:// URL to a local file name, before calling
insert-file-contents.  If we do that, the problem should go away.  Or
am I missing something?



Reply | Threaded
Open this post in threaded view
|

bug#42431: 28.0.50; browse-url-of-dired-file confusing messaging

Lars Ingebrigtsen
Eli Zaretskii <[hidden email]> writes:

> Of course.  And that's exactly what I meant when I suggested to
> convert the file:// URL to a local file name, before calling
> insert-file-contents.  If we do that, the problem should go away.  Or
> am I missing something?

I just thought all of this was supposed to work without doing any
mangling of the "file name" -- in this case it's an URL, but presumably
there are other file handlers for non-file stuff that Emacs treats are
files that have the same problem.

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