all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
From: Eli Zaretskii <eliz@gnu.org>
To: Michael Albinus <michael.albinus@gmx.de>
Cc: rpluim@gmail.com, emacs-devel@gnu.org, esr@thyrsus.com
Subject: Re: master 400df210ce0: Fix last change of 'delete-file'
Date: Fri, 11 Aug 2023 15:10:18 +0300	[thread overview]
Message-ID: <834jl5hj9x.fsf@gnu.org> (raw)
In-Reply-To: <87350pu7qq.fsf@gmx.de> (message from Michael Albinus on Fri, 11 Aug 2023 13:41:01 +0200)

> From: Michael Albinus <michael.albinus@gmx.de>
> Cc: rpluim@gmail.com,  emacs-devel@gnu.org,  esr@thyrsus.com
> Date: Fri, 11 Aug 2023 13:41:01 +0200
> 
> > Michael, this is my change, not Eric's.  My question is above: is
> > there a requirement that file handlers are called with absolute file
> > names?  If so, where is it documented, and why some places, which I
> > mentioned above, call file handlers with file names that were not
> > passed through expand-file-name?
> 
> Ffind_file_name_handler calls fast_string_match (string, filename).
> STRING is the car of an entry in file-name-handler-alist, FILENAME is
> the file name to be checked.
> 
> If the entry in file-name-handler-alist is a regexp for the head of an
> absolute file name, the check will fail when FILENAME is relative.

Shouldn't Ffind_file_name_handler also check the default-directory, if
FILENAME is not absolute and failed to match anything in
file-name-handler-alist?  If it doesn't do that, a file "foo" in a
remote directory will fail to match a handler, and that is probably
not expected (a.k.a. "bug")?

> >> > But the same would be true for substitute-in-file-name, for example,
> >> > and for directory-file-name, and file-name-as-directory, and several
> >> > other primitives, which call Ffind_file_name_handler without calling
> >> > expand-file-name before that.
> 
> substitute-in-file-name, directory-file-name, file-name-as-directory and
> likely more primitives do not support relative file names like
> "123". They support file names relative to the remote, like
> "/ssh::123". Is this a bug?  Don't know, nobody has protested so
> far. But at least it is undocumented.

These primitives should support relative file names, and they do for
local files.  So either they should call expand-file-name, or
find-file-name-handler should check default-directory.

> Other primitives handle this themselves if necessary. Fdelete_file and
> other function expand the file name. In Fexpand_file_name itself, we have
> 
> --8<---------------cut here---------------start------------->8---
>   handler = Ffind_file_name_handler (name, Qexpand_file_name);
>   ...
>   handler = Ffind_file_name_handler (default_directory, Qexpand_file_name);
> --8<---------------cut here---------------end--------------->8---

Primitives indeed _must_ call expand-file-name.  And expand-file-name
does that with default-directory because it accepts a directory as its
2nd argument, and that argument defaults to default-directory.

I've added back expand-file-name to delete-file.  But I find this
situation unsatisfactory: Lisp code should only call expand-file-name
when it needs to analyze absolute file names for its own purposes, it
is not supposed to call expand-file-name when invoking primitives.
The addition of expand-file-name to delete-file made that function a
bit slower, and for local files that is a net loss.  We should extend
instead find-file-name-handler to work for non-absolute file names to
avoid this overhead.

> I wouldn't touch this as it works. Improvements in documentation might
> be appreciated.

Indeed, the fact that find-file-name-handler needs an absolute file
name is never mentioned anywhere in the documentation.  It is strange
this didn't pop up earlier.



  reply	other threads:[~2023-08-11 12:10 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <169133064669.24990.11219399079845613336@vcs2.savannah.gnu.org>
     [not found] ` <20230806140407.09E6BC038BE@vcs2.savannah.gnu.org>
2023-08-10 13:12   ` master 400df210ce0: Fix last change of 'delete-file' Robert Pluim
2023-08-10 13:25     ` Eli Zaretskii
2023-08-10 13:44       ` Robert Pluim
2023-08-10 13:57         ` Eli Zaretskii
2023-08-10 14:41           ` Robert Pluim
2023-08-10 15:00             ` Eli Zaretskii
2023-08-11  7:33               ` Michael Albinus
2023-08-11 10:59                 ` Eli Zaretskii
2023-08-11 11:02                 ` Eli Zaretskii
2023-08-11 11:41                   ` Michael Albinus
2023-08-11 12:10                     ` Eli Zaretskii [this message]
2023-08-11 17:24                       ` Michael Albinus
2023-08-11 17:47                         ` Eli Zaretskii
2023-08-12  9:57                           ` Michael Albinus
2023-08-12 10:36                             ` Eli Zaretskii
2023-08-12 11:03                               ` Michael Albinus

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=834jl5hj9x.fsf@gnu.org \
    --to=eliz@gnu.org \
    --cc=emacs-devel@gnu.org \
    --cc=esr@thyrsus.com \
    --cc=michael.albinus@gmx.de \
    --cc=rpluim@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.