bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

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

Hello!

With #66187 resolved (<https://yhetil.org/emacs-bugs/87cytlqmqh.fsf@breatheoutbreathe.in/>),
I reexamined #66114 (<https://yhetil.org/emacs-bugs/87v8bzi7iz.fsf@breatheoutbreathe.in/>),
only to discover that filename completion with a functional
REQUIRE-MATCH argument doesn't work as expected.

To reproduce:

emacs -Q on the tip of the emacs-29 branch (after commit 77f5d4d523a), run...

(let ((default-directory "~/"))
  (read-directory-name "Clone into new or empty directory: " nil nil
                       (lambda (dir) (or (not (file-exists-p dir))
                                    (directory-empty-p dir)))))

...then type "/tmp/" (the whole minibuffer now reads "~//tmp") then RET.

Expected: Completion does not exit, instead saying "[No match]".

Actual: Completion exits, returning "/tmp/".

If I delete the leading "~/" before typing "/tmp/", I get the expected result.

The issue appears to be inside completion--complete-and-exit:

((functionp minibuffer-completion-confirm)
    (if (funcall minibuffer-completion-confirm
                 (buffer-substring beg end))  ; Here, buffer-substring returns "~//tmp/"
        (funcall exit-function)
      (unless completion-fail-discreetly
	(ding)
	(completion--message "No match"))))

Since (file-exists-p "~//tmp/") returns nil, the whole predicate returns
t and the minibuffer completes "/tmp/".

In completion--complete-and-exit, should (buffer-substring beg end)
return only "/tmp/"?

Or maybe (file-exists-p "~//tmp/") should return t?

Thank you!!

Joseph

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

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

> emacs -Q on the tip of the emacs-29 branch (after commit 77f5d4d523a), run...
>
> (let ((default-directory "~/"))
>   (read-directory-name "Clone into new or empty directory: " nil nil
>                        (lambda (dir) (or (not (file-exists-p dir))
>                                     (directory-empty-p dir)))))
>
> ...then type "/tmp/" (the whole minibuffer now reads "~//tmp") then RET.
>
> Expected: Completion does not exit, instead saying "[No match]".

Ah, that good old problem about whether PRED should apply to the quoted
or to the unquoted string 🙁

I think here the bug is in `read-directory-name` (and `read-file-name`)
since their PRED arg is expected to apply to the (unquoted) file name
itself (i.e. the thing that would be returned by the function), rather
than to the quoted representation used in the minibuffer.

They should wrap PRED so as to pass the arg through
`substitute-in-file-name` (or otherwise arrange to make sure PRED is
called with an unquoted file name).


        Stefan

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

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

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

Hi Stefan!

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> emacs -Q on the tip of the emacs-29 branch (after commit 77f5d4d523a), run...
>>
>> (let ((default-directory "~/"))
>>   (read-directory-name "Clone into new or empty directory: " nil nil
>>                        (lambda (dir) (or (not (file-exists-p dir))
>>                                     (directory-empty-p dir)))))
>>
>> ...then type "/tmp/" (the whole minibuffer now reads "~//tmp") then RET.
>>
>> Expected: Completion does not exit, instead saying "[No match]".
>
> Ah, that good old problem about whether PRED should apply to the
> quoted
> or to the unquoted string 🙁
>
> I think here the bug is in `read-directory-name` (and
> `read-file-name`)
> since their PRED arg is expected to apply to the (unquoted) file name
> itself (i.e. the thing that would be returned by the function), rather
> than to the quoted representation used in the minibuffer.
>
> They should wrap PRED so as to pass the arg through
> `substitute-in-file-name` (or otherwise arrange to make sure PRED is
> called with an unquoted file name).

Thank you for the clear instructions!  Does the attached patch do the
right thing?

If so, may it be applied to emacs-29?

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Pass-unquoted-filename-to-user-supplied-MUSTMATCH-pr.patch --]
[-- Type: text/x-diff, Size: 1872 bytes --]

From 78c7232593cc12bdf3e772df602d75a31bc4fd2a Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 30 Jan 2024 22:08:50 -0800
Subject: [PATCH] Pass unquoted filename to user-supplied MUSTMATCH predicate

* lisp/minibuffer.el (read-file-name-default):

Resolves bug#68815.
---
 lisp/minibuffer.el | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index faa7f543ece..1f0f3bdade3 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3353,7 +3353,13 @@ See `read-file-name' for the meaning of the arguments."
     (let ((ignore-case read-file-name-completion-ignore-case)
           (minibuffer-completing-file-name t)
           (pred (or predicate 'file-exists-p))
-          (add-to-history nil))
+          (add-to-history nil)
+          (require-match (if (functionp mustmatch)
+                             (lambda (input)
+                               (funcall mustmatch
+                                        ;; User-supplied MUSTMATCH expects an unquoted filename
+                                        (substitute-in-file-name input)))
+                           mustmatch)))
 
       (let* ((val
               (if (or (not (next-read-file-uses-dialog-p))
@@ -3389,7 +3395,7 @@ See `read-file-name' for the meaning of the arguments."
 				   (read-file-name--defaults dir initial))))
 			  (set-syntax-table minibuffer-local-filename-syntax))
                       (completing-read prompt 'read-file-name-internal
-                                       pred mustmatch insdef
+                                       pred require-match insdef
                                        'file-name-history default-filename)))
                 ;; If DEFAULT-FILENAME not supplied and DIR contains
                 ;; a file name, split it.
-- 
2.41.0

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

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

>> They should wrap PRED so as to pass the arg through
>> `substitute-in-file-name` (or otherwise arrange to make sure PRED is
>> called with an unquoted file name).
> Thank you for the clear instructions!  Does the attached patch do the
> right thing?

I theory, I think it's correct, yes.

Whether the rest of the code handles it correctly OTOH is a different
question.  E.g. I have the impression that currently the
`read-file-name-internal` completion table presumes the PRED argument
takes an already-unquoted file name.

Also, performance can be a concern (in many cases it makes more sense
to make the caller pass the unquoted name rather than force it to quote
the name only for PRED to unquote it).

IOW, I think we have a bit of a mess in our hands (that's what I was
meant by "good old problem" 🙁).

> If so, may it be applied to emacs-29?

Definitely not material for `emacs-29` I'm afraid.


        Stefan

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> They should wrap PRED so as to pass the arg through
>>> `substitute-in-file-name` (or otherwise arrange to make sure PRED is
>>> called with an unquoted file name).
>> Thank you for the clear instructions!  Does the attached patch do the
>> right thing?
>
> I theory, I think it's correct, yes.
>
> Whether the rest of the code handles it correctly OTOH is a different
> question.  E.g. I have the impression that currently the
> `read-file-name-internal` completion table presumes the PRED argument
> takes an already-unquoted file name.

What other code would this patch affect?

I see that `completion--file-name-table' uses `substitute-in-file-name',
but I don't understand how this patch would affect completion table.

> Also, performance can be a concern (in many cases it makes more sense
> to make the caller pass the unquoted name rather than force it to quote
> the name only for PRED to unquote it).

The REQUIRE-MATCH function is only called once when the user attempts to
exit the minibuffer.  Would you please explain the performance concern?

Thank you!

Joseph

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

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

>> Also, performance can be a concern (in many cases it makes more sense
>> to make the caller pass the unquoted name rather than force it to quote
>> the name only for PRED to unquote it).
>
> The REQUIRE-MATCH function is only called once when the user attempts to
> exit the minibuffer.  Would you please explain the performance concern?

Oh, sorry, I got confused.  Indeed, you're wrapping the REQUIRE-MATCH
arg, not the PRED arg I was ranting about.  Duh!

It would be OK for `emacs-29`, indeed.  Eli?  Stefan?  Any objection?

In the mean time, could you update the docs to clarify the behavior.
E.g. currently the docstring of `read-file-name` says:

    - a function, which will be called with the input as the
      argument.  [...]

which could be understood to suggest it really receives the contents of
the minibuffer (i.e. the quoted file name) rather than the value we'd
extract from it (the unquoted file name).


        Stefan

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

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

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

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Also, performance can be a concern (in many cases it makes more sense
>>> to make the caller pass the unquoted name rather than force it to quote
>>> the name only for PRED to unquote it).
>>
>> The REQUIRE-MATCH function is only called once when the user attempts to
>> exit the minibuffer.  Would you please explain the performance concern?
>
> Oh, sorry, I got confused.  Indeed, you're wrapping the REQUIRE-MATCH
> arg, not the PRED arg I was ranting about.  Duh!
>
> It would be OK for `emacs-29`, indeed.  Eli?  Stefan?  Any objection?

Great!  I've CC'd Stefan as well in this email.

> In the mean time, could you update the docs to clarify the behavior.
> E.g. currently the docstring of `read-file-name` says:
>
>     - a function, which will be called with the input as the
>       argument.  [...]
>
> which could be understood to suggest it really receives the contents of
> the minibuffer (i.e. the quoted file name) rather than the value we'd
> extract from it (the unquoted file name).

Please see the attached patch.

Thanks!

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Pass-unquoted-filename-to-user-supplied-MUSTMATCH-pr.patch --]
[-- Type: text/x-diff, Size: 2753 bytes --]

From 208c7abff97f842057540fa2dfacd99808f17015 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 30 Jan 2024 22:08:50 -0800
Subject: [PATCH] Pass unquoted filename to user-supplied MUSTMATCH predicate

* lisp/minibuffer.el (read-file-name-default): Pass REQUIRE-MATCH
argument through substitute-in-file-name.
* lisp/minibuffer.el (read-file-name): Update docstring.

Resolves bug#68815.
---
 lisp/minibuffer.el | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index faa7f543ece..a9e3ec937f9 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3262,9 +3262,10 @@ Fourth arg MUSTMATCH can take the following values:
   input, but she needs to confirm her choice if she called
   `minibuffer-complete' right before `minibuffer-complete-and-exit'
   and the input is not an existing file.
-- a function, which will be called with the input as the
-  argument.  If the function returns a non-nil value, the
-  minibuffer is exited with that argument as the value.
+- a function, which will be called with a single argument, the
+  input unquoted by `substitute-in-file-name', which see.  If the
+  function returns a non-nil value, the minibuffer is exited with
+  that argument as the value.
 - anything else behaves like t except that typing RET does not exit if it
   does non-null completion.
 
@@ -3353,7 +3354,13 @@ See `read-file-name' for the meaning of the arguments."
     (let ((ignore-case read-file-name-completion-ignore-case)
           (minibuffer-completing-file-name t)
           (pred (or predicate 'file-exists-p))
-          (add-to-history nil))
+          (add-to-history nil)
+          (require-match (if (functionp mustmatch)
+                             (lambda (input)
+                               (funcall mustmatch
+                                        ;; User-supplied MUSTMATCH expects an unquoted filename
+                                        (substitute-in-file-name input)))
+                           mustmatch)))
 
       (let* ((val
               (if (or (not (next-read-file-uses-dialog-p))
@@ -3389,7 +3396,7 @@ See `read-file-name' for the meaning of the arguments."
 				   (read-file-name--defaults dir initial))))
 			  (set-syntax-table minibuffer-local-filename-syntax))
                       (completing-read prompt 'read-file-name-internal
-                                       pred mustmatch insdef
+                                       pred require-match insdef
                                        'file-name-history default-filename)))
                 ;; If DEFAULT-FILENAME not supplied and DIR contains
                 ;; a file name, split it.
-- 
2.41.0

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

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

> Please see the attached patch.

Thanks, Joseph, pushed to `emacs-29`,


        Stefan

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

[Stefan Kangas]

Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> It would be OK for `emacs-29`, indeed.  Eli?  Stefan?  Any objection?

None here.

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: 68815@debbugs.gnu.org,  Philip Kaludercic <philipk@posteo.net>,
>   michael_heerdegen@web.de,  Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 31 Jan 2024 17:05:30 -0500
> 
> >> Also, performance can be a concern (in many cases it makes more sense
> >> to make the caller pass the unquoted name rather than force it to quote
> >> the name only for PRED to unquote it).
> >
> > The REQUIRE-MATCH function is only called once when the user attempts to
> > exit the minibuffer.  Would you please explain the performance concern?
> 
> Oh, sorry, I got confused.  Indeed, you're wrapping the REQUIRE-MATCH
> arg, not the PRED arg I was ranting about.  Duh!
> 
> It would be OK for `emacs-29`, indeed.  Eli?  Stefan?  Any objection?

I don't mind, but please note that I'm not sure there will be any
further 29.x releases.

> In the mean time, could you update the docs to clarify the behavior.

Yes, please.

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> Cc: 68815@debbugs.gnu.org,  Philip Kaludercic <philipk@posteo.net>,
>>   michael_heerdegen@web.de,  Eli Zaretskii <eliz@gnu.org>
>> Date: Wed, 31 Jan 2024 17:05:30 -0500
>>
>> >> Also, performance can be a concern (in many cases it makes more sense
>> >> to make the caller pass the unquoted name rather than force it to quote
>> >> the name only for PRED to unquote it).
>> >
>> > The REQUIRE-MATCH function is only called once when the user attempts to
>> > exit the minibuffer.  Would you please explain the performance concern?
>>
>> Oh, sorry, I got confused.  Indeed, you're wrapping the REQUIRE-MATCH
>> arg, not the PRED arg I was ranting about.  Duh!
>>
>> It would be OK for `emacs-29`, indeed.  Eli?  Stefan?  Any objection?
>
> I don't mind, but please note that I'm not sure there will be any
> further 29.x releases.

Good to know.  What is the purpose of keeping the emacs-29 branch, then?

>> In the mean time, could you update the docs to clarify the behavior.
>
> Yes, please.

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

[Stefan Kangas]

Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs@gnu.org> writes:

> What is the purpose of keeping the emacs-29 branch, then?

It'll be useful in case we do decide to release 29.3.

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 68815@debbugs.gnu.org,
>  philipk@posteo.net, michael_heerdegen@web.de
> Date: Wed, 31 Jan 2024 23:04:59 -0800
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Stefan Monnier <monnier@iro.umontreal.ca>
> >> Cc: 68815@debbugs.gnu.org,  Philip Kaludercic <philipk@posteo.net>,
> >>   michael_heerdegen@web.de,  Eli Zaretskii <eliz@gnu.org>
> >> Date: Wed, 31 Jan 2024 17:05:30 -0500
> >>
> >> >> Also, performance can be a concern (in many cases it makes more sense
> >> >> to make the caller pass the unquoted name rather than force it to quote
> >> >> the name only for PRED to unquote it).
> >> >
> >> > The REQUIRE-MATCH function is only called once when the user attempts to
> >> > exit the minibuffer.  Would you please explain the performance concern?
> >>
> >> Oh, sorry, I got confused.  Indeed, you're wrapping the REQUIRE-MATCH
> >> arg, not the PRED arg I was ranting about.  Duh!
> >>
> >> It would be OK for `emacs-29`, indeed.  Eli?  Stefan?  Any objection?
> >
> > I don't mind, but please note that I'm not sure there will be any
> > further 29.x releases.
> 
> Good to know.  What is the purpose of keeping the emacs-29 branch, then?

I'm not sure we will NOT release further 29.x versions, either.  There
could be some urgent issue that justifies another release, for
example.

My point is that the motivation for backporting improvements and fixes
from master and for installing non-essential fixes on the release
branch is supposed to go down, since we keep the branch active only
for some unanticipated contingencies.

bug#68815: Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: Stefan Monnier <monnier@iro.umontreal.ca>, 68815@debbugs.gnu.org,
>>  philipk@posteo.net, michael_heerdegen@web.de
>> Date: Wed, 31 Jan 2024 23:04:59 -0800
>>
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> >> From: Stefan Monnier <monnier@iro.umontreal.ca>
>> >> Cc: 68815@debbugs.gnu.org,  Philip Kaludercic <philipk@posteo.net>,
>> >>   michael_heerdegen@web.de,  Eli Zaretskii <eliz@gnu.org>
>> >> Date: Wed, 31 Jan 2024 17:05:30 -0500
>> >>
>> >> >> Also, performance can be a concern (in many cases it makes more sense
>> >> >> to make the caller pass the unquoted name rather than force it to quote
>> >> >> the name only for PRED to unquote it).
>> >> >
>> >> > The REQUIRE-MATCH function is only called once when the user attempts to
>> >> > exit the minibuffer.  Would you please explain the performance concern?
>> >>
>> >> Oh, sorry, I got confused.  Indeed, you're wrapping the REQUIRE-MATCH
>> >> arg, not the PRED arg I was ranting about.  Duh!
>> >>
>> >> It would be OK for `emacs-29`, indeed.  Eli?  Stefan?  Any objection?
>> >
>> > I don't mind, but please note that I'm not sure there will be any
>> > further 29.x releases.
>>
>> Good to know.  What is the purpose of keeping the emacs-29 branch, then?
>
> I'm not sure we will NOT release further 29.x versions, either.  There
> could be some urgent issue that justifies another release, for
> example.
>
> My point is that the motivation for backporting improvements and fixes
> from master and for installing non-essential fixes on the release
> branch is supposed to go down, since we keep the branch active only
> for some unanticipated contingencies.

Thank you for explaining.