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

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

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> 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

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> 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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Tue, 28 Jul 2020 00:24:23 +0200
> Cc: Michael Albinus <michael.albinus@gmx.de>
> 
> (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?

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> 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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  42431@debbugs.gnu.org
> 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.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Lars Ingebrigtsen <larsi@gnus.org>
>> Cc: michael.albinus@gmx.de,  42431@debbugs.gnu.org
>> 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

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> 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 <eggert@cs.ucla.edu>
AuthorDate: Thu Oct 31 23:31:17 2019 -0700
Commit:     Paul Eggert <eggert@cs.ucla.edu>
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!eggert@cs.ucla.edu.

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: michael.albinus@gmx.de,  42431@debbugs.gnu.org, Paul Eggert
>  <eggert@cs.ucla.edu>
> Date: Sun, 09 Aug 2020 11:45:05 +0200
> 
> Lars Ingebrigtsen <larsi@gnus.org> 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?

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> 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

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> @@ -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);
>      }

I've now fixed this in Emacs 29.

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

[Paul Eggert]

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

On 2022-10-13 00:00, Lars Ingebrigtsen wrote:

> I've now fixed this in Emacs 29.

Thanks, I simplified the fileio.c fix by installing the attached further 
patch.

[-- Attachment #2: 0001-Simplify-recent-File-exists-fix.patch --]
[-- Type: text/x-patch, Size: 882 bytes --]

From 97de273dca8d97039131f9d3f29b2820b5497805 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Thu, 13 Oct 2022 01:48:10 -0700
Subject: [PATCH] Simplify recent "File exists" fix

* src/fileio.c (Finsert_file_contents):
Simplify previous change that fixed bug#42431.
---
 src/fileio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/fileio.c b/src/fileio.c
index a238889803..49553f3c91 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -5000,7 +5000,7 @@ because (1) it preserves some marker positions (in unchanged portions
       unbind_to (count1, Qnil);
     }
 
-  if (NILP (handler) && !NILP (visit) && current_buffer->modtime.tv_nsec < 0)
+  if (save_errno != 0)
     {
       /* Signal an error if visiting a file that could not be opened.  */
       report_file_errno ("Opening input file", orig_filename, save_errno);
-- 
2.34.1

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

[Eli Zaretskii]

> Date: Thu, 13 Oct 2022 01:51:00 -0700
> Cc: michael.albinus@gmx.de, 42431@debbugs.gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -5000,7 +5000,7 @@ because (1) it preserves some marker positions (in unchanged portions
>        unbind_to (count1, Qnil);
>      }
>  
> -  if (NILP (handler) && !NILP (visit) && current_buffer->modtime.tv_nsec < 0)
> +  if (save_errno != 0)
>      {
>        /* Signal an error if visiting a file that could not be opened.  */
>        report_file_errno ("Opening input file", orig_filename, save_errno);

Why would we want to signal an error when VISIT is nil?  We didn't
before.