bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

minibuffer.el: (read-file-name-default) has the following code:
```
                     (minibuffer-with-setup-hook
                         (lambda ()
                           (setq default-directory dir)
		   ...
			  (set-syntax-table minibuffer-local-filename-syntax))
```

This mutates the global binding of default-directory which is incorrect.

To demonstrate the problem, in emacs -Q

(insert "http://example.com")
(setq enable-recursive-minibuffers t)

position point at in the middle of the string  http://example.com
M-: (ffap)

;; while waiting for input
M-: default-directory
;; => default-directory is bound to "http://example.com"
M-: (shell-command "echo foo")
; ;=>
;; Debugger entered--Lisp error: (file-missing "Setting current directory" "No such file or directory" "http://example.com/")

I think this can be addressed by binding default-directory before
modifying it.

```
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3671,6 +3671,7 @@ read-file-name-default
                               (expand-file-name dir))))
                     (minibuffer-with-setup-hook
                         (lambda ()
+                         (let ((default-directory default-directory))
                           (setq default-directory dir)
                           ;; When the first default in `minibuffer-default'
                           ;; duplicates initial input `insdef',
@@ -3689,7 +3690,7 @@ read-file-name-default
                                  (with-current-buffer
                                      (window-buffer (minibuffer-selected-window))
 				   (read-file-name--defaults dir initial))))
-			  (set-syntax-table minibuffer-local-filename-syntax))
+			  (set-syntax-table minibuffer-local-filename-syntax)))
                       (completing-read prompt 'read-file-name-internal
                                        pred require-match insdef
                                        'file-name-history default-filename)))
```

[I'm sending this before I forget so it can be reviewed, but I can
send this patch as as attachment if needed, and it'll take some more
time until I figure out how to write a test for it and test it.]

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Date: Tue, 05 Nov 2024 07:39:24 +0530 (IST)
> From: Madhu <enometh@meer.net>
> 
> minibuffer.el: (read-file-name-default) has the following code:
> ```
>                      (minibuffer-with-setup-hook
>                          (lambda ()
>                            (setq default-directory dir)
> 		   ...
> 			  (set-syntax-table minibuffer-local-filename-syntax))
> ```
> 
> This mutates the global binding of default-directory which is incorrect.
> 
> To demonstrate the problem, in emacs -Q
> 
> (insert "http://example.com")
> (setq enable-recursive-minibuffers t)
> 
> position point at in the middle of the string  http://example.com
> M-: (ffap)
> 
> ;; while waiting for input
> M-: default-directory
> ;; => default-directory is bound to "http://example.com"
> M-: (shell-command "echo foo")
> ; ;=>
> ;; Debugger entered--Lisp error: (file-missing "Setting current directory" "No such file or directory" "http://example.com/")
> 
> I think this can be addressed by binding default-directory before
> modifying it.
> 
> ```
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3671,6 +3671,7 @@ read-file-name-default
>                                (expand-file-name dir))))
>                      (minibuffer-with-setup-hook
>                          (lambda ()
> +                         (let ((default-directory default-directory))
>                            (setq default-directory dir)
>                            ;; When the first default in `minibuffer-default'
>                            ;; duplicates initial input `insdef',
> @@ -3689,7 +3690,7 @@ read-file-name-default
>                                   (with-current-buffer
>                                       (window-buffer (minibuffer-selected-window))
>  				   (read-file-name--defaults dir initial))))
> -			  (set-syntax-table minibuffer-local-filename-syntax))
> +			  (set-syntax-table minibuffer-local-filename-syntax)))
>                        (completing-read prompt 'read-file-name-internal
>                                         pred require-match insdef
>                                         'file-name-history default-filename)))
> ```
> 
> [I'm sending this before I forget so it can be reviewed, but I can
> send this patch as as attachment if needed, and it'll take some more
> time until I figure out how to write a test for it and test it.]

Stefan, any comments?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> To demonstrate the problem, in emacs -Q
>
> (insert "http://example.com")
> (setq enable-recursive-minibuffers t)
>
> position point at in the middle of the string  http://example.com
> M-: (ffap)
>
> ;; while waiting for input
> M-: default-directory
> ;; => default-directory is bound to "http://example.com"
> M-: (shell-command "echo foo")
> ; ;=>
> ;; Debugger entered--Lisp error: (file-missing "Setting current directory" "No such file or directory" "http://example.com/")
>
> I think this can be addressed by binding default-directory before
> modifying it.

IIRC the `setq` is really there to set the value of `default-directory`
during the whole duration of the completion, not just during the setup hook.
This is needed/used if the user modifies the minibuffer's content to
hold only a relative file name, at which point we need that default
directory info in order to perform completions.

> ```
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3671,6 +3671,7 @@ read-file-name-default
>                                (expand-file-name dir))))
>                      (minibuffer-with-setup-hook
>                          (lambda ()
> +                         (let ((default-directory default-directory))
>                            (setq default-directory dir)
>                            ;; When the first default in `minibuffer-default'
>                            ;; duplicates initial input `insdef',
> @@ -3689,7 +3690,7 @@ read-file-name-default
>                                   (with-current-buffer
>                                       (window-buffer (minibuffer-selected-window))
>  				   (read-file-name--defaults dir initial))))
> -			  (set-syntax-table minibuffer-local-filename-syntax))
> +			  (set-syntax-table minibuffer-local-filename-syntax)))
>                        (completing-read prompt 'read-file-name-internal
>                                         pred require-match insdef
>                                         'file-name-history default-filename)))
> ```

The let+setq could be simplified to

                        (let ((default-directory dir))

but none of the code within this `let` uses `default-directory` since
there are only calls to `car/cdr`, `setq`, and `set-syntax-table`, none
of which touch file names, so we may as well remove the
(setq default-directory dir) instead.

I'm leaning towards declaring the `shell-command` behavior above as
a feature rather than a bug.


        Stefan

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

[-- Attachment #1: Type: Text/Plain, Size: 1664 bytes --]

[snip: I hope the context can be seen on the bug-list]
*  Stefan Monnier  <jwvjzdcz9fp.fsf-monnier+emacs@gnu.org>
> IIRC the `setq` is really there to set the value of `default-directory`
> during the whole duration of the completion, not just during the setup hook.
> This is needed/used if the user modifies the minibuffer's content to
> hold only a relative file name, at which point we need that default
> directory info in order to perform completions.

Ah yes I missed that, (though I haven't worked out yet how it works)

> The let+setq could be simplified to
>
>                         (let ((default-directory dir))
>
> but none of the code within this `let` uses `default-directory` since
> there are only calls to `car/cdr`, `setq`, and `set-syntax-table`, none
> of which touch file names, so we may as well remove the
> (setq default-directory dir) instead.

I still think the default-difectory should be let-bound so the binding
is undone when the dynamic scope exits.  I have not worked out where
the default-directory is being restored to the original value, but i
know it is possible for the global value to be set incorrectly
(e.g. after a M-: (debug), quit - -in the recursive minibuffer which
would do a non-local exit.)


> I'm leaning towards declaring the `shell-command` behavior above as
> a feature rather than a bug.

We could avoid still binding default-directory to a
non-directory. Using the logic of expand-file-name, how about the
following:

[shouldn't binding it before the call to `minibuffer-with-setup-hook'
be equivalent to a top level let in a hypothetical
(with-minibuffer-setup-hook FN &body BODY), or setting it via FN]


[-- Attachment #2: 0001-minibuffer.el-read-file-name-default-avoid-setting-i.patch --]
[-- Type: Text/X-Patch, Size: 1527 bytes --]

From ff4a2e046a66f7b364ad570c7dce11dab4987155 Mon Sep 17 00:00:00 2001
From: Madhu <enometh@net.meer>
Date: Tue, 5 Nov 2024 07:19:55 +0530
Subject: [PATCH] minibuffer.el: (read-file-name-default): avoid setting
 incorrect default-directory

* minibuffer.el: (read-file-name-default): bind default-directory
(in dynamic scope) instead of modifying it. try to bind it to a valid
directory. (e.g. don't set it to a url when calling (ffap) at a url).
---
 lisp/minibuffer.el | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c970752ec2a..672f7b2b318 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3668,10 +3668,11 @@ read-file-name-default
                   ;; changing `default-directory' in the current buffer,
                   ;; we don't let-bind it.
                   (let ((dir (file-name-as-directory
-                              (expand-file-name dir))))
+                              (expand-file-name dir)))
+                        (default-directory (if (file-name-absolute-p dir)
+                                               dir default-directory)))
                     (minibuffer-with-setup-hook
                         (lambda ()
-                          (setq default-directory dir)
                           ;; When the first default in `minibuffer-default'
                           ;; duplicates initial input `insdef',
                           ;; reset `minibuffer-default' to nil.
-- 
2.46.0.27.gfa3b914457

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Cc: 74208@debbugs.gnu.org
> Date: Sun, 10 Nov 2024 06:27:00 +0530 (IST)
> From: Madhu <enometh@meer.net>
> 
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3668,10 +3668,11 @@ read-file-name-default
>                    ;; changing `default-directory' in the current buffer,
>                    ;; we don't let-bind it.
>                    (let ((dir (file-name-as-directory
> -                              (expand-file-name dir))))
> +                              (expand-file-name dir)))
> +                        (default-directory (if (file-name-absolute-p dir)
> +                                               dir default-directory)))

You probably meant to use let*, right?

But anyway, I don't understand the logic: expand-file-name always
returns an absolute file name, so the test will always succeed.  What
did I miss?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Eli Zaretskii <eliz@gnu.org> <86msi7ljb0.fsf@gnu.org>
Wrote on Sun, 10 Nov 2024 08:00:35 +0200
>> --- a/lisp/minibuffer.el
>> +++ b/lisp/minibuffer.el
>> @@ -3668,10 +3668,11 @@ read-file-name-default
>>                    ;; changing `default-directory' in the current buffer,
>>                    ;; we don't let-bind it.
>>                    (let ((dir (file-name-as-directory
>> -                              (expand-file-name dir))))
>> +                              (expand-file-name dir)))
>> +                        (default-directory (if (file-name-absolute-p dir)
>> +                                               dir default-directory)))
> You probably meant to use let*, right?

Oops. yes, of course.

> But anyway, I don't understand the logic: expand-file-name always
> returns an absolute file name, so the test will always succeed.  What
> did I miss?

for the example in the thread,
ffap.el:(ffap-read-file-or-url) calls read-file-name-default, via
              (funcall #'read-file-name-default prompt guess guess) =>

  read-file-name-default("Find file or URL: " "http://example.com" "http://example.com")

(read-file-name-default PROMPT &optional DIR DEFAULT-FILENAME
 MUSTMATCH INITIAL PREDICATE)

The problem is that the parameter DIR == "http://example.com" was
getting bound to default-directory with undesirable results. DIR a url
not a file name but a url. and since expand-file-name behaves like
this:

(expand-file-name "http://example.com" "~") ->  "http://example.com"

it can probably be used to determine that DIR is not a legit filename
and default directory should not be bound to it. I don't think it has
any consequences for the completion-system completion via
read-file-name, which would not work anyway (and completing urls in
this context say matching against a history list of urls would not
make sense either)

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Date: Sun, 10 Nov 2024 12:30:07 +0530 (IST)
> Cc: monnier@iro.umontreal.ca, 74208@debbugs.gnu.org
> From: Madhu <enometh@meer.net>
> 
> >> -                              (expand-file-name dir))))
> >> +                              (expand-file-name dir)))
> >> +                        (default-directory (if (file-name-absolute-p dir)
> >> +                                               dir default-directory)))
> > You probably meant to use let*, right?
> 
> Oops. yes, of course.
> 
> > But anyway, I don't understand the logic: expand-file-name always
> > returns an absolute file name, so the test will always succeed.  What
> > did I miss?
> 
> for the example in the thread,
> ffap.el:(ffap-read-file-or-url) calls read-file-name-default, via
>               (funcall #'read-file-name-default prompt guess guess) =>
> 
>   read-file-name-default("Find file or URL: " "http://example.com" "http://example.com")
> 
> (read-file-name-default PROMPT &optional DIR DEFAULT-FILENAME
>  MUSTMATCH INITIAL PREDICATE)
> 
> The problem is that the parameter DIR == "http://example.com" was
> getting bound to default-directory with undesirable results. DIR a url
> not a file name but a url. and since expand-file-name behaves like
> this:
> 
> (expand-file-name "http://example.com" "~") ->  "http://example.com"

That's not what I see.  I see this:

  (expand-file-name "http://example.com" "~")
    => /my/home/directory/http:/example.com

In what version of Emacs do you see your result?
Is that in "emacs -Q"?

In any case, the code you propose calls

  (expand-file-name dir)

without the 2nd argument.

> it can probably be used to determine that DIR is not a legit filename
> and default directory should not be bound to it.

I don't see how expand-file-name could help here, see above.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Eli Zaretskii <eliz@gnu.org> <86h68fl86n.fsf@gnu.org>
Wrote on Sun, 10 Nov 2024 12:00:48 +0200
>   (expand-file-name "http://example.com" "~")
>     => /my/home/directory/http:/example.com
I get the same result.

> In what version of Emacs do you see your result?

I was evaluating it under edebug after calling edebug-defun on
read-file-name-default, and ivoking (ffap) on the url.

It returns the argument with or without the second parameter to
expand-file-name, and I was hoping I could count on this behaviour to
separate the urls from the files.

The behaviour of expand-file-name is apparenlty modified when it comes
to read-file-name-default, but I can't spot what's going on. ?????

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Date: Sun, 10 Nov 2024 16:11:41 +0530 (IST)
> Cc: monnier@iro.umontreal.ca, 74208@debbugs.gnu.org
> From: Madhu <enometh@meer.net>
> 
> *  Eli Zaretskii <eliz@gnu.org> <86h68fl86n.fsf@gnu.org>
> Wrote on Sun, 10 Nov 2024 12:00:48 +0200
> >   (expand-file-name "http://example.com" "~")
> >     => /my/home/directory/http:/example.com
> I get the same result.
> 
> > In what version of Emacs do you see your result?
> 
> I was evaluating it under edebug after calling edebug-defun on
> read-file-name-default, and ivoking (ffap) on the url.
> 
> It returns the argument with or without the second parameter to
> expand-file-name, and I was hoping I could count on this behaviour to
> separate the urls from the files.
> 
> The behaviour of expand-file-name is apparenlty modified when it comes
> to read-file-name-default, but I can't spot what's going on. ?????

Perhaps because TRAMP was loaded?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Eli Zaretskii <eliz@gnu.org> <86ed3jl63j.fsf@gnu.org>
Wrote on Sun, 10 Nov 2024 12:45:52 +0200
>> I was evaluating it under edebug after calling edebug-defun on
>> read-file-name-default, and ivoking (ffap) on the url.
>>
>> It returns the argument with or without the second parameter to
>> expand-file-name, and I was hoping I could count on this behaviour to
>> separate the urls from the files.
>>
>> The behaviour of expand-file-name is apparenlty modified when it comes
>> to read-file-name-default, but I can't spot what's going on. ?????
>
> Perhaps because TRAMP was loaded?
It's because of the call to
 (ffap-read-file-or-url "foo" "https://example.com/")

which roughly does the equivalent of

(let ((file-name-handler-alist
       (cl-adjoin (cons ffap-url-regexp #'ffap--url-file-handler)
		  file-name-handler-alist)))
  (expand-file-name "https://example.com" "~"))

Maybe the idea will still work?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> Date: Sun, 10 Nov 2024 16:47:17 +0530 (IST)
> Cc: monnier@iro.umontreal.ca, 74208@debbugs.gnu.org
> From: Madhu <enometh@meer.net>
> 
> *  Eli Zaretskii <eliz@gnu.org> <86ed3jl63j.fsf@gnu.org>
> Wrote on Sun, 10 Nov 2024 12:45:52 +0200
> >> I was evaluating it under edebug after calling edebug-defun on
> >> read-file-name-default, and ivoking (ffap) on the url.
> >>
> >> It returns the argument with or without the second parameter to
> >> expand-file-name, and I was hoping I could count on this behaviour to
> >> separate the urls from the files.
> >>
> >> The behaviour of expand-file-name is apparenlty modified when it comes
> >> to read-file-name-default, but I can't spot what's going on. ?????
> >
> > Perhaps because TRAMP was loaded?
> It's because of the call to
>  (ffap-read-file-or-url "foo" "https://example.com/")
> 
> which roughly does the equivalent of
> 
> (let ((file-name-handler-alist
>        (cl-adjoin (cons ffap-url-regexp #'ffap--url-file-handler)
> 		  file-name-handler-alist)))
>   (expand-file-name "https://example.com" "~"))
> 
> Maybe the idea will still work?

The idea being not to bind default-directory to the URL?  Doesn't ffap
need that?  If not, why does it override the file handlers?

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> for the example in the thread,
> ffap.el:(ffap-read-file-or-url) calls read-file-name-default, via
>               (funcall #'read-file-name-default prompt guess guess) =>
>
>   read-file-name-default("Find file or URL: " "http://example.com" "http://example.com")
>
> (read-file-name-default PROMPT &optional DIR DEFAULT-FILENAME
>  MUSTMATCH INITIAL PREDICATE)
>
> The problem is that the parameter DIR == "http://example.com" was
> getting bound to default-directory with undesirable results.

It can also have desirable results sometimes (depends on the URL at point
and what kind of support for URLs you have in your Emacs, admittedly).
Do you happen to have a more realistic scenario than your "echo foo"
that lets us better judge the severity of the problem, and maybe other
ways to solve the actual problem?


        Stefan

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

>> The problem is that the parameter DIR == "http://example.com" was
>> getting bound to default-directory with undesirable results.

> It can also have desirable results sometimes (depends on the URL at point
> and what kind of support for URLs you have in your Emacs, admittedly).
> Do you happen to have a more realistic scenario than your "echo foo"
> that lets us better judge the severity of the problem, and maybe other
> ways to solve the actual problem?

Are you saying it is ok for emacs to make all calls to `call-prcoess'
to unconditionally fail --- when emacs is waiting for input at the
minibuffer?  the calls to call-process can come from anywhere, from
timers, or from outside via emacsclient, etc.

I already posted that any hypothetical completion facilites are
singularly useless for the task of ffap (url) -- to get the url at
point into the minibuffer and maybe manually edit it.  If these
facilities exist and you are using them, I'd like to see an example.

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>>> The problem is that the parameter DIR == "http://example.com" was
>>> getting bound to default-directory with undesirable results.
>
>> It can also have desirable results sometimes (depends on the URL at point
>> and what kind of support for URLs you have in your Emacs, admittedly).
>> Do you happen to have a more realistic scenario than your "echo foo"
>> that lets us better judge the severity of the problem, and maybe other
>> ways to solve the actual problem?
>
> Are you saying it is ok for emacs to make all calls to `call-prcoess'
> to unconditionally fail --- when emacs is waiting for input at the
> minibuffer?  the calls to call-process can come from anywhere, from
> timers, or from outside via emacsclient, etc.

`default-directory` can point to random places, including non-existing
directories, HTTP places (e.g. if you enable `url-handler-mode`), so
code like timers need to take this possibility into account, yes.
The ffap minibuffer discussed here is just one of many other ways to get
into this situation.

> I already posted that any hypothetical completion facilites are
> singularly useless for the task of ffap (url) -- to get the url at
> point into the minibuffer and maybe manually edit it.  If these
> facilities exist and you are using them, I'd like to see an example.

diff --git a/lisp/url/url-handlers.el b/lisp/url/url-handlers.el
index 9edc7865a74..17013345c25 100644
--- a/lisp/url/url-handlers.el
+++ b/lisp/url/url-handlers.el
@@ -375,18 +375,48 @@ url-insert-file-contents-literally
     (kill-buffer buffer)
     nil))
 
-(defun url-file-name-completion (url _directory &optional _predicate)
-  ;; Even if it's not implemented, it's not an error to ask for completion,
-  ;; in case it's available (bug#14806).
-  ;; (error "Unimplemented")
-  url)
+(defun url-file-name-completion (url directory &optional predicate)
+  (let ((all (url-file-name-all-completions url directory)))
+    (if (null all)
+        ;; If `url' is the empty string, don't return nil, so as to prevent
+        ;; partial-completion from recursing into the parent directory.
+        (if (equal url "") url)
+      (try-completion url all predicate))))
 (put 'file-name-completion 'url-file-handlers #'url-file-name-completion)
 
-(defun url-file-name-all-completions (_file _directory)
-  ;; Even if it's not implemented, it's not an error to ask for completion,
-  ;; in case it's available (bug#14806).
-  ;; (error "Unimplemented")
-  nil)
+(defvar url-handler-temp-buf)
+
+(defun url-file-name-all-completions (file directory)
+  ;; FIXME: Cache the "directory" buffers between completion requests.
+  (let ((buf (get-file-buffer directory)))
+    (unless buf
+      (setq buf (ignore-errors (find-file-noselect directory)))
+      (when buf
+        (with-current-buffer buf
+          (set (make-local-variable 'url-handler-temp-buf) t))))
+    (when buf
+      (unwind-protect
+          (with-current-buffer buf
+            (save-excursion
+              (let ((all ())
+                    (case-fold-search t)
+                    ;; FIXME: Handle URL-quoting.
+                    (regexp (format "<a href=\"\\(%s[^\"]+\\)\"" file)))
+                (goto-char (point-min))
+                (while (re-search-forward regexp nil t)
+                  (let ((url (match-string 1)))
+                    (unless (string-match
+                             "\\`\\(?:\\.\\.\\|[#?/]\\|[-a-z]+:/\\)\\|" url)
+                      ;; It's a relative URL.
+                      (when (string-match "[#?]\\|/\\(.\\)" url)
+                        (setq url (substring url (or (match-beginning 1)
+                                                     (match-beginning 0)))))
+                      ;; FIXME: Handle URL-unquoting.
+                      (push url all))))
+                all)))
+        (and (buffer-live-p buf)
+             (buffer-local-value 'url-handler-temp-buf buf)
+             (kill-buffer buf))))))
 (put 'file-name-all-completions
      'url-file-handlers #'url-file-name-all-completions)
 

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

Ping! How should we make progress with this issue?

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: eliz@gnu.org,  74208@debbugs.gnu.org
> Date: Sun, 10 Nov 2024 22:40:27 -0500
> 
> >>> The problem is that the parameter DIR == "http://example.com" was
> >>> getting bound to default-directory with undesirable results.
> >
> >> It can also have desirable results sometimes (depends on the URL at point
> >> and what kind of support for URLs you have in your Emacs, admittedly).
> >> Do you happen to have a more realistic scenario than your "echo foo"
> >> that lets us better judge the severity of the problem, and maybe other
> >> ways to solve the actual problem?
> >
> > Are you saying it is ok for emacs to make all calls to `call-prcoess'
> > to unconditionally fail --- when emacs is waiting for input at the
> > minibuffer?  the calls to call-process can come from anywhere, from
> > timers, or from outside via emacsclient, etc.
> 
> `default-directory` can point to random places, including non-existing
> directories, HTTP places (e.g. if you enable `url-handler-mode`), so
> code like timers need to take this possibility into account, yes.
> The ffap minibuffer discussed here is just one of many other ways to get
> into this situation.
> 
> > I already posted that any hypothetical completion facilites are
> > singularly useless for the task of ffap (url) -- to get the url at
> > point into the minibuffer and maybe manually edit it.  If these
> > facilities exist and you are using them, I'd like to see an example.
> 
> diff --git a/lisp/url/url-handlers.el b/lisp/url/url-handlers.el
> index 9edc7865a74..17013345c25 100644
> --- a/lisp/url/url-handlers.el
> +++ b/lisp/url/url-handlers.el
> @@ -375,18 +375,48 @@ url-insert-file-contents-literally
>      (kill-buffer buffer)
>      nil))
>  
> -(defun url-file-name-completion (url _directory &optional _predicate)
> -  ;; Even if it's not implemented, it's not an error to ask for completion,
> -  ;; in case it's available (bug#14806).
> -  ;; (error "Unimplemented")
> -  url)
> +(defun url-file-name-completion (url directory &optional predicate)
> +  (let ((all (url-file-name-all-completions url directory)))
> +    (if (null all)
> +        ;; If `url' is the empty string, don't return nil, so as to prevent
> +        ;; partial-completion from recursing into the parent directory.
> +        (if (equal url "") url)
> +      (try-completion url all predicate))))
>  (put 'file-name-completion 'url-file-handlers #'url-file-name-completion)
>  
> -(defun url-file-name-all-completions (_file _directory)
> -  ;; Even if it's not implemented, it's not an error to ask for completion,
> -  ;; in case it's available (bug#14806).
> -  ;; (error "Unimplemented")
> -  nil)
> +(defvar url-handler-temp-buf)
> +
> +(defun url-file-name-all-completions (file directory)
> +  ;; FIXME: Cache the "directory" buffers between completion requests.
> +  (let ((buf (get-file-buffer directory)))
> +    (unless buf
> +      (setq buf (ignore-errors (find-file-noselect directory)))
> +      (when buf
> +        (with-current-buffer buf
> +          (set (make-local-variable 'url-handler-temp-buf) t))))
> +    (when buf
> +      (unwind-protect
> +          (with-current-buffer buf
> +            (save-excursion
> +              (let ((all ())
> +                    (case-fold-search t)
> +                    ;; FIXME: Handle URL-quoting.
> +                    (regexp (format "<a href=\"\\(%s[^\"]+\\)\"" file)))
> +                (goto-char (point-min))
> +                (while (re-search-forward regexp nil t)
> +                  (let ((url (match-string 1)))
> +                    (unless (string-match
> +                             "\\`\\(?:\\.\\.\\|[#?/]\\|[-a-z]+:/\\)\\|" url)
> +                      ;; It's a relative URL.
> +                      (when (string-match "[#?]\\|/\\(.\\)" url)
> +                        (setq url (substring url (or (match-beginning 1)
> +                                                     (match-beginning 0)))))
> +                      ;; FIXME: Handle URL-unquoting.
> +                      (push url all))))
> +                all)))
> +        (and (buffer-live-p buf)
> +             (buffer-local-value 'url-handler-temp-buf buf)
> +             (kill-buffer buf))))))
>  (put 'file-name-all-completions
>       'url-file-handlers #'url-file-name-all-completions)
>  
> 
> 

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Madhu]

*  Eli Zaretskii <eliz@gnu.org> <864j3ym915.fsf@gnu.org>
Wrote on Sat, 23 Nov 2024 14:25:42 +0200

> Ping! How should we make progress with this issue?

I haven't had a chance to use Stefan's url history code yet to see how
it works, but hopefully i'll get around to it soon.  But I don't think
it is acceptable for call process to fail just because
default-directory is bound to a url. (There are other problems like
bogus invalid entries in file-name-history -- resulting from a merging
a relative pathname to a url) which also argue against setting the
default-url in general (it may make sense in a specific use case)

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Ping! How should we make progress with this issue?

I think the core of this bug report is that `shell-command` can fail
when `default-directory` is set to a value that doesn't correspond to
a local directory.

At least for things like `(shell-command "date")` this is undesirable.

Maybe `call-process` should try to "look for" a valid directory if
`default-directory` isn't good enough?


        Stefan

bug#74208: 31.0.50; minibuffer read-file-name-default mutates global value of default-directory incorrectly

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: enometh@meer.net,  74208@debbugs.gnu.org
> Date: Sat, 23 Nov 2024 22:00:16 -0500
> 
> > Ping! How should we make progress with this issue?
> 
> I think the core of this bug report is that `shell-command` can fail
> when `default-directory` is set to a value that doesn't correspond to
> a local directory.
> 
> At least for things like `(shell-command "date")` this is undesirable.
> 
> Maybe `call-process` should try to "look for" a valid directory if
> `default-directory` isn't good enough?

It already does:

  Lisp_Object
  get_current_directory (bool encode)
  {
    Lisp_Object curdir = BVAR (current_buffer, directory);
    Lisp_Object dir = Funhandled_file_name_directory (curdir);

    /* If the file name handler says that dir is unreachable, use
       a sensible default. */
    if (NILP (dir))
      dir = build_string ("~");

I guess this case somehow evades the test?