bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

Hello!

When the MUSTMATCH argument to read-file-name is a function, I'd expect
to see a message like "[Match required]" when the function returns nil.

For example, I'd expect the following to never match:

(read-file-name "Prompt: " nil nil #'ignore)

The behavior of read-file-name is unspecified when a MUSTMATCH function
returns nil:

- 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.

Would someone kindly explain the intended behavior here?

This issue originally came up in this thread about package-vc-checkout:
https://yhetil.org/emacs-bugs/87v8bzi7iz.fsf@breatheoutbreathe.in/T/#m224de986dcc97f23e17386fb0dd2db4a513726bf

Thanks!

Joseph

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Michael Heerdegen]

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

> Would someone kindly explain the intended behavior here?

I don't know, but I would expect that when the MUST-MATCH predicate
fails the behavior is the same as with MUST-MATCH t, and when it
succeeds, the behavior is like MUST-MATCH nil.  Quite simple I think.
Do you see something different?

Hmm - or, maybe the confusion is about the behavior for the members of
the collection?  With other words, whether the argument can be used to
restrict which existing files are matched, vs. whether it can only be
used to limit what additionally matches?  AFAIU the latter is the case -
existing files always match.

> This issue originally came up in this thread about package-vc-checkout:
> https://yhetil.org/emacs-bugs/87v8bzi7iz.fsf@breatheoutbreathe.in/T/#m224de986dcc97f23e17386fb0dd2db4a513726bf

I didn't understand the "this erroneously returns the default filename"
part.  What default filename is returned?

Michael.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

Thanks for your help!!

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs@gnu.org> writes:
>
>> Would someone kindly explain the intended behavior here?
>
> I don't know, but I would expect that when the MUST-MATCH predicate
> fails the behavior is the same as with MUST-MATCH t, and when it
> succeeds, the behavior is like MUST-MATCH nil.  Quite simple I think.
> Do you see something different?

Yes. In emacs -Q:

(mkdir "/tmp/foo" t)
(with-temp-buffer
  (write-file "/tmp/foo/bar"))
(setq default-directory "/tmp/foo/")
(read-file-name "Clone into new or empty directory: " nil nil
                (lambda (filename)
                  (or (not (file-exists-p filename))
                      (directory-empty-p filename)))
                nil
                (lambda (filename)
                  (file-directory-p filename)))

The default prompt is "/tmp/foo/". In my Emacs (29.0.92), when I
immediately press RET, read-file-name (erroneously?) returns "/tmp/foo/"
(which exists and is not empty) instead of saying "[Match required]".

Are you able to reproduce this on your machine?

> Hmm - or, maybe the confusion is about the behavior for the members of
> the collection?  With other words, whether the argument can be used to
> restrict which existing files are matched, vs. whether it can only be
> used to limit what additionally matches?  AFAIU the latter is the case -
> existing files always match.
>
>> This issue originally came up in this thread about package-vc-checkout:
>> https://yhetil.org/emacs-bugs/87v8bzi7iz.fsf@breatheoutbreathe.in/T/#m224de986dcc97f23e17386fb0dd2db4a513726bf
>
> I didn't understand the "this erroneously returns the default filename"
> part.  What default filename is returned?

Does my above comment answer this question?

Thank you!!

Joseph

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Michael Heerdegen]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> (mkdir "/tmp/foo" t)
> (with-temp-buffer
>   (write-file "/tmp/foo/bar"))
> (setq default-directory "/tmp/foo/")
> (read-file-name "Clone into new or empty directory: " nil nil
>                 (lambda (filename)
>                   (or (not (file-exists-p filename))
>                       (directory-empty-p filename)))
>                 nil
>                 (lambda (filename)
>                   (file-directory-p filename)))
>
> The default prompt is "/tmp/foo/". In my Emacs (29.0.92), when I
> immediately press RET, read-file-name (erroneously?) returns "/tmp/foo/"
> (which exists and is not empty) instead of saying "[Match required]".
>
> Are you able to reproduce this on your machine?

Yes, I see the same.  But I think it's expected.  AFAIU the algorithm
works like this: first it's checked whether a completion is allowed (by
checking whether it's the name of an existing file and consulting
PREDICATE when specified).  Only when it is not allowed we check whether
MUSTMATCH declares it acceptable.

This is not really explained in the docstring, however.

In your example, the default-directory is the name of an existing file,
so the MUSTMATCH argument is irrelevant, the specified PREDICATE is
fulfilled, so it's accepted as input.

If I guess correctly I think you want something like this:

#+begin_src emacs-lisp
(let ((acceptable-p (lambda (filename)
                      (and (file-directory-p filename)
                           (directory-empty-p filename)))))
  (read-file-name "Clone into new or empty directory: " nil nil
                  (lambda (filename)
                    (if (file-exists-p filename)
                        (funcall acceptable-p filename)
                      t))
                  nil
                  acceptable-p))
#+end_src

Does that come close?


Michael.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> AFAIU the algorithm
> works like this: first it's checked whether a completion is allowed (by
> checking whether it's the name of an existing file and consulting
> PREDICATE when specified).  Only when it is not allowed we check whether
> MUSTMATCH declares it acceptable.

Thank you!! That makes sense now.

> This is not really explained in the docstring, however.
>
> In your example, the default-directory is the name of an existing file,
> so the MUSTMATCH argument is irrelevant, the specified PREDICATE is
> fulfilled, so it's accepted as input.
>
> If I guess correctly I think you want something like this:
>
> #+begin_src emacs-lisp
> (let ((acceptable-p (lambda (filename)
>                       (and (file-directory-p filename)
>                            (directory-empty-p filename)))))
>   (read-file-name "Clone into new or empty directory: " nil nil
>                   (lambda (filename)
>                     (if (file-exists-p filename)
>                         (funcall acceptable-p filename)
>                       t))
>                   nil
>                   acceptable-p))
> #+end_src
>
> Does that come close?

Thank you! What I was hoping for may not have a clean solution:

- the completions buffer displays and allows tab-completion for all
  directories, empty or not, then
- upon pressing RET on a non-empty directory, the "[Match required]"
  message appears. Only upon pressing RET on an empty directory does
  completion succeed.

If this cannot be accomplished elegantly, I'll consider other designs
for the interactive prompt in package-vc-checkout.

Thanks!!

Joseph

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Michael Heerdegen]

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Thank you! What I was hoping for may not have a clean solution:
>
> - the completions buffer displays and allows tab-completion for all
>   directories, empty or not, then
> - upon pressing RET on a non-empty directory, the "[Match required]"
>   message appears. Only upon pressing RET on an empty directory does
>   completion succeed.

Ah - ok, now finally I've understood all the parts.

You want to prompt for a directory that is either empty or not yet
existing.  But with `read-file-name' you only get either (a) non-empty
directories accepted as input, or (b) failing completion of existing
non-empty directories, which makes it impossible to choose a directory
inside an existing directory.

Yes, looks like a bug that this is not possible.  It should be possible
to complete directory names that do not match.

Then the documentation needs to be improved: what exactly is accepted
when both MUSTMATCH and PREDICATE are specified?

Finally, I think the docstring of `read-directory-name' needs to be
updated: it fails to mention that MUSTMATCH can be a function (the
argument is passed directly to `read-file-name').


Michael.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

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


Michael Heerdegen <michael_heerdegen@web.de> writes:
> Ah - ok, now finally I've understood all the parts.
>
> You want to prompt for a directory that is either empty or not yet
> existing.  But with `read-file-name' you only get either (a) non-empty
> directories accepted as input, or (b) failing completion of existing
> non-empty directories, which makes it impossible to choose a directory
> inside an existing directory.
>
> Yes, looks like a bug that this is not possible.  It should be possible
> to complete directory names that do not match.

I'm happy to work on the bug, if others agree that this change is desired.

> Then the documentation needs to be improved: what exactly is accepted
> when both MUSTMATCH and PREDICATE are specified?

I can improve the docs too, once I have the answer that questions.

> Finally, I think the docstring of `read-directory-name' needs to be
> updated: it fails to mention that MUSTMATCH can be a function (the
> argument is passed directly to `read-file-name').

How about the attached patch?

> Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-MUSTMATCH-description-in-read-directory-name-doc.patch --]
[-- Type: text/x-diff, Size: 1069 bytes --]

From e0ea8501af4e0dfd9777879e44a07902f86d1a7e Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 26 Sep 2023 18:01:27 -0700
Subject: [PATCH] Fix MUSTMATCH description in read-directory-name docstring

* lisp/files.el (read-directory-name)
---
 lisp/files.el | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lisp/files.el b/lisp/files.el
index b72f141c0ee..1fe124848b9 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -817,7 +817,7 @@ non-empty string that was inserted by this function.
 If the user exits with an empty minibuffer, this function returns
 an empty string.  (This can happen only if the user erased the
 pre-inserted contents or if `insert-default-directory' is nil.)
-Fourth arg MUSTMATCH non-nil means require existing directory's name.
+Fourth arg MUSTMATCH is passed as-is to `read-file-name', which see.
  Non-nil and non-t means also require confirmation after completion.
 Fifth arg INITIAL specifies text to start with.
 DIR should be an absolute directory name.  It defaults to
-- 
2.41.0

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Michael Heerdegen]

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

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> > Yes, looks like a bug that this is not possible.  It should be possible
> > to complete directory names that do not match.
>
> I'm happy to work on the bug, if others agree that this change is
> desired.

I had a look and tried this:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Bug-66187.patch --]
[-- Type: text/x-diff, Size: 2765 bytes --]

From ad895d2df5c69e015c2c7eba5116ab2d440e1fbc Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Wed, 27 Sep 2023 03:32:37 +0200
Subject: [PATCH] WIP: Bug#66187

---
 lisp/minibuffer.el | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 2120e31775e..79a8786fbac 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3061,13 +3061,23 @@ completion-file-name-table
           (funcall (or pred 'file-exists-p) string)))

        (t
-        (let* ((name (file-name-nondirectory string))
+        (let* ((test-directory (lambda (s)
+                                 (let ((len (length s)))
+                                   (and (> len 0) (eq (aref s (1- len)) ?/)))))
+               (should-complete
+                (and pred
+                     (if (eq pred 'file-directory-p)
+                         test-directory
+                       (lambda (f)
+                         (or (funcall test-directory f)
+                             (funcall pred f))))))
+               (name (file-name-nondirectory string))
                (specdir (file-name-directory string))
                (realdir (or specdir default-directory)))

           (cond
            ((null action)
-            (let ((comp (file-name-completion name realdir pred)))
+            (let ((comp (file-name-completion name realdir should-complete)))
               (if (stringp comp)
                   (concat specdir comp)
                 comp)))
@@ -3078,18 +3088,9 @@ completion-file-name-table
               ;; Check the predicate, if necessary.
               (unless (memq pred '(nil file-exists-p))
                 (let ((comp ())
-                      (pred
-                       (if (eq pred 'file-directory-p)
-                           ;; Brute-force speed up for directory checking:
-                           ;; Discard strings which don't end in a slash.
-                           (lambda (s)
-                             (let ((len (length s)))
-                               (and (> len 0) (eq (aref s (1- len)) ?/))))
-                         ;; Must do it the hard (and slow) way.
-                         pred)))
-                  (let ((default-directory (expand-file-name realdir)))
-                    (dolist (tem all)
-                      (if (funcall pred tem) (push tem comp))))
+                      (default-directory (expand-file-name realdir)))
+                  (dolist (tem all)
+                    (if (funcall should-complete tem) (push tem comp)))
                   (setq all (nreverse comp))))

               all))))))
--
2.39.2


[-- Attachment #3: Type: text/plain, Size: 1179 bytes --]


I thought this would make Emacs complete as you want.  But: what files
should be shown when hitting TAB?  I decided to show only matching
files, plus all directories (seems logical, since these may contain
matching files).

But that gives a bad user experience: When I tried this I thought it
would not work because completion did not accept an existing empty
directory.  Then I saw that it actually was not empty.  But it contained only
plain files, no subdirectories, and TAB displayed nothing.  Quite confusing!


> How about the attached patch?

| diff --git a/lisp/files.el b/lisp/files.el
| index b72f141c0ee..1fe124848b9 100644
| --- a/lisp/files.el
| +++ b/lisp/files.el
| @@ -817,7 +817,7 @@ non-empty string that was inserted by this function.
|  If the user exits with an empty minibuffer, this function returns
|  an empty string.  (This can happen only if the user erased the
|  pre-inserted contents or if `insert-default-directory' is nil.)
| -Fourth arg MUSTMATCH non-nil means require existing directory's name.
| +Fourth arg MUSTMATCH is passed as-is to `read-file-name', which see.

Ok from my side.

What do others think about all of this?


Thx,

Michael.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Eli Zaretskii]

> Cc: philipk@posteo.net, 66187@debbugs.gnu.org
> Date: Tue, 26 Sep 2023 17:55:37 -0700
> From:  Joseph Turner via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> > Finally, I think the docstring of `read-directory-name' needs to be
> > updated: it fails to mention that MUSTMATCH can be a function (the
> > argument is passed directly to `read-file-name').
> 
> How about the attached patch?

This is okay, except that we do document the other arguments.  So why
not just copy/paste the description of MUSTMATCH from read-file-name
to read-directory-name instead?

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: philipk@posteo.net, 66187@debbugs.gnu.org
>> Date: Tue, 26 Sep 2023 17:55:37 -0700
>> From:  Joseph Turner via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> > Finally, I think the docstring of `read-directory-name' needs to be
>> > updated: it fails to mention that MUSTMATCH can be a function (the
>> > argument is passed directly to `read-file-name').
>>
>> How about the attached patch?
>
> This is okay, except that we do document the other arguments.  So why
> not just copy/paste the description of MUSTMATCH from read-file-name
> to read-directory-name instead?

The read-file-name docstring for MUSTMATCH is lengthy, and duplicating
it will mean that if read-file-name changes, we'll need to remember to
change the docstring of read-directory-name.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

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


Michael Heerdegen <michael_heerdegen@web.de> writes:

> I had a look and tried this:
>
> [2. text/x-diff; 0001-WIP-Bug-66187.patch]
> From ad895d2df5c69e015c2c7eba5116ab2d440e1fbc Mon Sep 17 00:00:00 2001
> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Wed, 27 Sep 2023 03:32:37 +0200
> Subject: [PATCH] WIP: Bug#66187
>
> ---
>  lisp/minibuffer.el | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index 2120e31775e..79a8786fbac 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3061,13 +3061,23 @@ completion-file-name-table
>            (funcall (or pred 'file-exists-p) string)))
>
>         (t
> -        (let* ((name (file-name-nondirectory string))
> +        (let* ((test-directory (lambda (s)
> +                                 (let ((len (length s)))
> +                                   (and (> len 0) (eq (aref s (1- len)) ?/)))))
> +               (should-complete
> +                (and pred
> +                     (if (eq pred 'file-directory-p)
> +                         test-directory
> +                       (lambda (f)
> +                         (or (funcall test-directory f)
> +                             (funcall pred f))))))
> +               (name (file-name-nondirectory string))
>                 (specdir (file-name-directory string))
>                 (realdir (or specdir default-directory)))
>
>            (cond
>             ((null action)
> -            (let ((comp (file-name-completion name realdir pred)))
> +            (let ((comp (file-name-completion name realdir should-complete)))
>                (if (stringp comp)
>                    (concat specdir comp)
>                  comp)))
> @@ -3078,18 +3088,9 @@ completion-file-name-table
>                ;; Check the predicate, if necessary.
>                (unless (memq pred '(nil file-exists-p))
>                  (let ((comp ())
> -                      (pred
> -                       (if (eq pred 'file-directory-p)
> -                           ;; Brute-force speed up for directory checking:
> -                           ;; Discard strings which don't end in a slash.
> -                           (lambda (s)
> -                             (let ((len (length s)))
> -                               (and (> len 0) (eq (aref s (1- len)) ?/))))
> -                         ;; Must do it the hard (and slow) way.
> -                         pred)))
> -                  (let ((default-directory (expand-file-name realdir)))
> -                    (dolist (tem all)
> -                      (if (funcall pred tem) (push tem comp))))
> +                      (default-directory (expand-file-name realdir)))
> +                  (dolist (tem all)
> +                    (if (funcall should-complete tem) (push tem comp)))
>                    (setq all (nreverse comp))))
>
>                all))))))

> I thought this would make Emacs complete as you want.  But: what files
> should be shown when hitting TAB?  I decided to show only matching
> files, plus all directories (seems logical, since these may contain
> matching files).

> But that gives a bad user experience: When I tried this I thought it
> would not work because completion did not accept an existing empty
> directory.  Then I saw that it actually was not empty.  But it contained only
> plain files, no subdirectories, and TAB displayed nothing.  Quite confusing!

I dug a little more and realized that the IMO unexpected behavior
resides in completing-read itself.  According to the completing-read
docstring, its REQUIRE-MATCH argument can be

- 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.

I incorrectly assumed that this function would prevent matching items
inside COLLECTION for which the function returns nil.  What it actually
does it accept input which is not part of COLLECTION if the
REQUIRE-MATCH function returns non-nil.  IOW, it doesn't narrow the
potential completion options; it widens them.

For example, given:

(completing-read "Prompt: " '("a" "b") nil
                 (lambda (input)
                   (string= "a" input)))

I expected that the prompt would refuse to complete "b". I was wrong.

Since completing-read is such a fundamental part of Emacs, I doubt it
will be possible to change this behavior.  What do you think about the
attached patch to clarify the completing-read docstring?

Thank you!!

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Clarify-completing-read-REQUIRE-MATCH-function-docst.patch --]
[-- Type: text/x-diff, Size: 1316 bytes --]

From 10fa6dd5c659bed2b818fb925445cab2cbbb5325 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 3 Oct 2023 14:40:22 -0700
Subject: [PATCH] Clarify completing-read REQUIRE-MATCH function docstring

* src/minibuf.c (Fcompleting_read): Explicitly note that REQUIRE-MATCH
function does not narrow the completion candidates.
---
 src/minibuf.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/src/minibuf.c b/src/minibuf.c
index 58adde1bf66..616be7091ee 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -1999,8 +1999,11 @@ REQUIRE-MATCH can take the following values:
   `minibuffer-complete' right before `minibuffer-complete-and-exit'
   and the input is not an element of COLLECTION.
 - 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.
+  argument.  If the function returns a non-nil value or if
+  the input is a member of COLLECTION, the minibuffer is
+  exited with that argument as the value.  If the function
+  returns nil and the input is not a member of COLLECTION,
+  then the user is not allowed to exit.
 - anything else behaves like t except that typing RET does not exit if it
   does non-null completion.
 
-- 
2.41.0

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Drew Adams]

> Since completing-read is such a fundamental part of Emacs,
> I doubt it will be possible to change this behavior.

Apparently it was possible for Someone (TM) to
change the behavior, in spite of the fact that
it's a longstanding "fundamental part of Emacs".
It _was_ a fundamental part of Emacs, but it
seems that somehow it's signature was changed
for Emacs 29.

The new behavior for a function value of
REQUIRE-MATCH is backward-incompatible.

A function as REQUIRE-MATCH value _should_ still
be treated as any other value that's not nil, t,
`confirm', or `confirm-after-completion'.  I
filed bug #66328 for this.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Michael Heerdegen]

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

> Since completing-read is such a fundamental part of Emacs, I doubt it
> will be possible to change this behavior.

This is only about `read-file-name' - and MUST-MATCH being allowed to be
a function is quite new (June 2022).  If we have good reasons to change
the behavior because we find that the case when both MUST-MATCH and
PREDICATE are specified does not work intuitively I think it would still
be possible to tune it a bit.

But we have to know which behavior is more expected than what we
currently have.

Michael.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

Michael Heerdegen <michael_heerdegen@web.de> writes:

> Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of
> text editors" <bug-gnu-emacs@gnu.org> writes:
>
>> Since completing-read is such a fundamental part of Emacs, I doubt it
>> will be possible to change this behavior.
>
> This is only about `read-file-name' - and MUST-MATCH being allowed to be
> a function is quite new (June 2022).  If we have good reasons to change
> the behavior because we find that the case when both MUST-MATCH and
> PREDICATE are specified does not work intuitively I think it would still
> be possible to tune it a bit.
>
> But we have to know which behavior is more expected than what we
> currently have.

My original expectations:

- PREDICATE narrows the list of completion candidates
- MUST-MATCH function determines whether to accept the input

I had thought that the two arguments would work independently of one
another.  IOW, the return value of MUST-MATCH would be the only factor
that determines whether input is accepted.  If MUST-MATCH returns
non-nil, the input is unconditionally accepted, and vice versa.

> This is only about `read-file-name'

Doesn't the unexpected behavior originate in completing-read?

Joseph

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Eli Zaretskii]

> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Cc: michael_heerdegen@web.de, philipk@posteo.net, 66187@debbugs.gnu.org
> Date: Tue, 03 Oct 2023 13:43:56 -0700
> 
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Cc: philipk@posteo.net, 66187@debbugs.gnu.org
> >> Date: Tue, 26 Sep 2023 17:55:37 -0700
> >> From:  Joseph Turner via "Bug reports for GNU Emacs,
> >>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> >>
> >> > Finally, I think the docstring of `read-directory-name' needs to be
> >> > updated: it fails to mention that MUSTMATCH can be a function (the
> >> > argument is passed directly to `read-file-name').
> >>
> >> How about the attached patch?
> >
> > This is okay, except that we do document the other arguments.  So why
> > not just copy/paste the description of MUSTMATCH from read-file-name
> > to read-directory-name instead?
> 
> The read-file-name docstring for MUSTMATCH is lengthy, and duplicating
> it will mean that if read-file-name changes, we'll need to remember to
> change the docstring of read-directory-name.

That's true, but it doesn't sound like a grave problem to me.  We have
the same duplication elsewhere, so this isn't a precedent.  Moreover,
when a doc string is long and complicated, asking the reader to read
part of it elsewhere is also quite a hassle.

So I still think duplication is the lesser evil here.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

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


Eli Zaretskii <eliz@gnu.org> writes:

>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Cc: michael_heerdegen@web.de, philipk@posteo.net, 66187@debbugs.gnu.org
>> Date: Tue, 03 Oct 2023 13:43:56 -0700
>>
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> > This is okay, except that we do document the other arguments.  So why
>> > not just copy/paste the description of MUSTMATCH from read-file-name
>> > to read-directory-name instead?
>>
>> The read-file-name docstring for MUSTMATCH is lengthy, and duplicating
>> it will mean that if read-file-name changes, we'll need to remember to
>> change the docstring of read-directory-name.
>
> That's true, but it doesn't sound like a grave problem to me.  We have
> the same duplication elsewhere, so this isn't a precedent.  Moreover,
> when a doc string is long and complicated, asking the reader to read
> part of it elsewhere is also quite a hassle.
>
> So I still think duplication is the lesser evil here.

Thanks for the explanation! See patch.

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-MUSTMATCH-description-in-read-directory-name-doc.patch --]
[-- Type: text/x-diff, Size: 2014 bytes --]

From 3a653d681083d8d14d2d2437cb3f405dd7f07f04 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Tue, 26 Sep 2023 18:01:27 -0700
Subject: [PATCH] Fix MUSTMATCH description in read-directory-name docstring

* lisp/files.el (read-directory-name): Copy from read-file-name
description, replacing "file" with "directory".
---
 lisp/files.el | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index b72f141c0ee..c31cb13653e 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -817,8 +817,23 @@ non-empty string that was inserted by this function.
 If the user exits with an empty minibuffer, this function returns
 an empty string.  (This can happen only if the user erased the
 pre-inserted contents or if `insert-default-directory' is nil.)
-Fourth arg MUSTMATCH non-nil means require existing directory's name.
- Non-nil and non-t means also require confirmation after completion.
+
+Fourth arg MUSTMATCH can take the following values:
+- nil means that the user can exit with any input.
+- t means that the user is not allowed to exit unless
+  the input is (or completes to) an existing directory.
+- `confirm' means that the user can exit with any input, but she needs
+  to confirm her choice if the input is not an existing directory.
+- `confirm-after-completion' means that the user can exit with any
+  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 directory.
+- 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.
+- anything else behaves like t except that typing RET does not exit if it
+  does non-null completion.
+
 Fifth arg INITIAL specifies text to start with.
 DIR should be an absolute directory name.  It defaults to
 the value of `default-directory'."
-- 
2.41.0

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Eli Zaretskii]

> Cc: philipk@posteo.net, 66187@debbugs.gnu.org
> Date: Tue, 03 Oct 2023 14:18:42 -0700
> From:  Joseph Turner via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> I dug a little more and realized that the IMO unexpected behavior
> resides in completing-read itself.  According to the completing-read
> docstring, its REQUIRE-MATCH argument can be
> 
> - 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.
> 
> I incorrectly assumed that this function would prevent matching items
> inside COLLECTION for which the function returns nil.  What it actually
> does it accept input which is not part of COLLECTION if the
> REQUIRE-MATCH function returns non-nil.  IOW, it doesn't narrow the
> potential completion options; it widens them.
> 
> For example, given:
> 
> (completing-read "Prompt: " '("a" "b") nil
>                  (lambda (input)
>                    (string= "a" input)))
> 
> I expected that the prompt would refuse to complete "b". I was wrong.
> 
> Since completing-read is such a fundamental part of Emacs, I doubt it
> will be possible to change this behavior.  What do you think about the
> attached patch to clarify the completing-read docstring?

I don't think I see how your proposed change clarifies this issue.

Adding Stefan for possibly more comments and ideas.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Michael Heerdegen]

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

> My original expectations:
>
> - PREDICATE narrows the list of completion candidates
> - MUST-MATCH function determines whether to accept the input
> [...]

> > This is only about `read-file-name'
>
> Doesn't the unexpected behavior originate in completing-read?

If that's the behavioral aspect you mean, then yes, the meaning of the
arguments is analogue, and they are passed to `completing-read'.

Michael.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

>> For example, given:
>> 
>> (completing-read "Prompt: " '("a" "b") nil
>>                  (lambda (input)
>>                    (string= "a" input)))
>> 
>> I expected that the prompt would refuse to complete "b". I was wrong.
>> 
>> Since completing-read is such a fundamental part of Emacs, I doubt it
>> will be possible to change this behavior.  What do you think about the
>> attached patch to clarify the completing-read docstring?

The use of a function as `require-match` is brand new in Emacs-29, so
I think it's not too late to fix it.  I think rather than fixing the doc
we should fix the behavior, e.g. with the patch below.


        Stefan


diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 2120e31775e..d4da2d0d19b 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1835,15 +1835,13 @@ completion--complete-and-exit
   (cond
    ;; Allow user to specify null string
    ((= beg end) (funcall exit-function))
-   ;; The CONFIRM argument is a predicate.
-   ((and (functionp minibuffer-completion-confirm)
-         (funcall minibuffer-completion-confirm
-                  (buffer-substring beg end)))
-    (funcall exit-function))
    ;; See if we have a completion from the table.
-   ((test-completion (buffer-substring beg end)
-                     minibuffer-completion-table
-                     minibuffer-completion-predicate)
+   ((if (functionp minibuffer-completion-confirm)
+        (funcall minibuffer-completion-confirm
+                 (buffer-substring beg end))
+      (test-completion (buffer-substring beg end)
+                       minibuffer-completion-table
+                       minibuffer-completion-predicate))
     ;; FIXME: completion-ignore-case has various slightly
     ;; incompatible meanings.  E.g. it can reflect whether the user
     ;; wants completion to pay attention to case, or whether the

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Drew Adams]

> The use of a function as `require-match` is
> brand new in Emacs-29, so I think it's not
> too late to fix it.  I think rather than
> fixing the doc we should fix the behavior 

If it's not too late to fix what was introduced
in Emacs 29, then how about not breaking the
behavior of REQUIRE-MATCH at all, and instead
just add a separate argument for what a function
value of REQUIRE-MATCH does in 29.1?

(As opposed to what a function value of
REQUIRE-MATCH has always done before Emacs 29,
i.e., act like any other non-nil, non-`confirm*',
non-`t' value.)

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Joseph Turner <joseph@breatheoutbreathe.in>,  michael_heerdegen@web.de,
>   philipk@posteo.net,  66187@debbugs.gnu.org
> Date: Thu, 05 Oct 2023 15:34:32 -0400
> 
> >> For example, given:
> >> 
> >> (completing-read "Prompt: " '("a" "b") nil
> >>                  (lambda (input)
> >>                    (string= "a" input)))
> >> 
> >> I expected that the prompt would refuse to complete "b". I was wrong.
> >> 
> >> Since completing-read is such a fundamental part of Emacs, I doubt it
> >> will be possible to change this behavior.  What do you think about the
> >> attached patch to clarify the completing-read docstring?
> 
> The use of a function as `require-match` is brand new in Emacs-29, so
> I think it's not too late to fix it.  I think rather than fixing the doc
> we should fix the behavior, e.g. with the patch below.

What change in behavior (wrt Emacs 29.1) does this patch cause?

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "michael_heerdegen@web.de" <michael_heerdegen@web.de>,
>         "philipk@posteo.net" <philipk@posteo.net>,
>         "66187@debbugs.gnu.org"
> 	<66187@debbugs.gnu.org>,
>         Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Thu, 5 Oct 2023 20:20:46 +0000
> 
> > The use of a function as `require-match` is
> > brand new in Emacs-29, so I think it's not
> > too late to fix it.  I think rather than
> > fixing the doc we should fix the behavior 
> 
> If it's not too late to fix what was introduced
> in Emacs 29

That is not yet established.

> then how about not breaking the
> behavior of REQUIRE-MATCH at all, and instead
> just add a separate argument for what a function
> value of REQUIRE-MATCH does in 29.1?

That is definitely not a change suitable for Emacs 29 at this point.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

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

>>> For example, given:
>>>
>>> (completing-read "Prompt: " '("a" "b") nil
>>>                  (lambda (input)
>>>                    (string= "a" input)))
>>>
>>> I expected that the prompt would refuse to complete "b". I was wrong.
>>>
>>> Since completing-read is such a fundamental part of Emacs, I doubt it
>>> will be possible to change this behavior.  What do you think about the
>>> attached patch to clarify the completing-read docstring?
>
> The use of a function as `require-match` is brand new in Emacs-29, so
> I think it's not too late to fix it.  I think rather than fixing the doc
> we should fix the behavior, e.g. with the patch below.
>
>
>         Stefan
>
>
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index 2120e31775e..d4da2d0d19b 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -1835,15 +1835,13 @@ completion--complete-and-exit
>    (cond
>     ;; Allow user to specify null string
>     ((= beg end) (funcall exit-function))
> -   ;; The CONFIRM argument is a predicate.
> -   ((and (functionp minibuffer-completion-confirm)
> -         (funcall minibuffer-completion-confirm
> -                  (buffer-substring beg end)))
> -    (funcall exit-function))
>     ;; See if we have a completion from the table.
> -   ((test-completion (buffer-substring beg end)
> -                     minibuffer-completion-table
> -                     minibuffer-completion-predicate)
> +   ((if (functionp minibuffer-completion-confirm)
> +        (funcall minibuffer-completion-confirm
> +                 (buffer-substring beg end))
> +      (test-completion (buffer-substring beg end)
> +                       minibuffer-completion-table
> +                       minibuffer-completion-predicate))
>      ;; FIXME: completion-ignore-case has various slightly
>      ;; incompatible meanings.  E.g. it can reflect whether the user
>      ;; wants completion to pay attention to case, or whether the

Thank you!! The above change fixes completion--complete-and-exit:
attempting to exit with "b" now runs the final catchall cond clause.

However, the unexpected behavior persists (it is possible to exit with
"b") because the completion-function that completion-complete-and-exit
passes to completion--complete-and-exit returns "b" anyway since "b"
exactly matches one of elements of COLLECTION.

Thank you!

Joseph

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

>> >> For example, given:
>> >> 
>> >> (completing-read "Prompt: " '("a" "b") nil
>> >>                  (lambda (input)
>> >>                    (string= "a" input)))
>> >> 
>> >> I expected that the prompt would refuse to complete "b". I was wrong.
>> >> 
>> >> Since completing-read is such a fundamental part of Emacs, I doubt it
>> >> will be possible to change this behavior.  What do you think about the
>> >> attached patch to clarify the completing-read docstring?
>> 
>> The use of a function as `require-match` is brand new in Emacs-29, so
>> I think it's not too late to fix it.  I think rather than fixing the doc
>> we should fix the behavior, e.g. with the patch below.
>
> What change in behavior (wrt Emacs 29.1) does this patch cause?

In the example he gave above it makes it so completion can be used to
insert "b" but when you try to exit it refuses to exit.

IOW it gives primacy to the `require-match` function over the
completion-table w.r.t deciding when the minibuffer's content is
acceptable or not.

Code which wants the 29.1 behavior can easily get it by modifying their
`require-match` function accordingly (and such code then works
correctly both with Emacs-29.1 and with the new behavior).


        Stefan

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

>> then how about not breaking the
>> behavior of REQUIRE-MATCH at all, and instead
>> just add a separate argument for what a function
>> value of REQUIRE-MATCH does in 29.1?
>
> That is definitely not a change suitable for Emacs 29 at this point.

Not only that but that would be hideous over-engineering (so not
suitable for `master` either).
The use of a function for `require-match` is already extremely rare
(we've had `require-match` for more than 30 years and it's only last
year that we decided it was worth generalizing it to a function), and
the new proposed behavior is strictly more general than the
29.1 behavior.


        Stefan

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: joseph@breatheoutbreathe.in,  michael_heerdegen@web.de,
>   philipk@posteo.net,  66187@debbugs.gnu.org
> Date: Fri, 06 Oct 2023 09:01:23 -0400
> 
> >> >> For example, given:
> >> >> 
> >> >> (completing-read "Prompt: " '("a" "b") nil
> >> >>                  (lambda (input)
> >> >>                    (string= "a" input)))
> >> >> 
> >> >> I expected that the prompt would refuse to complete "b". I was wrong.
> >> >> 
> >> >> Since completing-read is such a fundamental part of Emacs, I doubt it
> >> >> will be possible to change this behavior.  What do you think about the
> >> >> attached patch to clarify the completing-read docstring?
> >> 
> >> The use of a function as `require-match` is brand new in Emacs-29, so
> >> I think it's not too late to fix it.  I think rather than fixing the doc
> >> we should fix the behavior, e.g. with the patch below.
> >
> > What change in behavior (wrt Emacs 29.1) does this patch cause?
> 
> In the example he gave above it makes it so completion can be used to
> insert "b" but when you try to exit it refuses to exit.
> 
> IOW it gives primacy to the `require-match` function over the
> completion-table w.r.t deciding when the minibuffer's content is
> acceptable or not.
> 
> Code which wants the 29.1 behavior can easily get it by modifying their
> `require-match` function accordingly (and such code then works
> correctly both with Emacs-29.1 and with the new behavior).

Do we have any users of this feature in the tree?

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Drew Adams]

> (we've had `require-match` for more than 30 years
> and it's only last year that we decided it was
> worth generalizing it to a function)
        ^^^^^^^^^^^^

It was decided with very little discussion, FWIW.

The 29.1 behavior is not at all more general than
the longstanding previous behavior.  Just the
opposite.  It confuses different things, co-opting
the REQUIRE-MATCH behavior with completion behavior.

All code that previously might have passed a
function value for REQUIRE-MATCH expects that value
to be treated as any other non-`nil', non-`t',
non-`confirm*' value.  That's now broken.  That more
general behavior is lost - a function value being
treated differently imposes a limitation - the
behavior is more specific now (another case added),
not more general.

The new behavior that was cobbled together into the
REQUIRE-MATCH arg should have been provided in some
other way - whether an additional arg, or a non-arg
variable, or whatever.  It should not have been
jammed into REQUIRE-MATCH, changing its behavior in
an incompatible way.

(Just one opinion.)

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

>> Code which wants the 29.1 behavior can easily get it by modifying their
>> `require-match` function accordingly (and such code then works
>> correctly both with Emacs-29.1 and with the new behavior).
> Do we have any users of this feature in the tree?

We do, and it turns out to be a good argument in favor of the change.

The use of a function for `completing-read`s REQUIRE-MATCH and
`read-file-name`s MUSTMATCH was added in

    commit 49e06183f5972817d93dad6acf5351c204e61cc5
    Author: Lars Ingebrigtsen <larsi@gnus.org>
    Date:   Fri Jun 10 10:16:57 2022 +0200
    
        Allow REQUIRE-MATCH to be a function
        
        * doc/lispref/minibuf.texi (Minibuffer Completion): Document it.
        
        * lisp/minibuffer.el (completion--complete-and-exit): Allow
        REQUIRE-MATCH to be a function.
        (read-file-name): Mention it.
        
        * src/minibuf.c (Fcompleting_read): Mention it.

and the surrounding commits point to its motivation in:

    commit 7ee736a884766f2017a934d936bfbfa4c70b5099
    Author: Lars Ingebrigtsen <larsi@gnus.org>
    Date:   Fri Jun 10 10:19:15 2022 +0200
    
        Allow specifying a wildcard argument to list-directory again
        
        * lisp/files.el (list-directory): Allow specifying a wildcard
        argument interactively again (bug#55877).

But the code in that patch uses MUSTMATCH to *restrict* the possible inputs,
whereas Joseph's point is that the code currently does not allow using
MUSTMATCH to restrict the possible inputs.

And indeed we can currently do `M-x list-directory RET /etc/passwd RET`
and Emacs happily runs `list-directory` on that non-directory, contrary
to the intention of the code (IIUC):

     [...]
     (list (read-file-name
            (if pfx "List directory (verbose): "
	      "List directory (brief): ")
	    nil default-directory
            (lambda (file)
              (or (file-directory-p file)
                  (insert-directory-wildcard-in-dir-p
                   (file-name-as-directory (expand-file-name file))))))
     [...]


-- Stefan

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: joseph@breatheoutbreathe.in,  michael_heerdegen@web.de,
>   philipk@posteo.net,  66187@debbugs.gnu.org
> Date: Fri, 06 Oct 2023 12:43:51 -0400
> 
> >> Code which wants the 29.1 behavior can easily get it by modifying their
> >> `require-match` function accordingly (and such code then works
> >> correctly both with Emacs-29.1 and with the new behavior).
> > Do we have any users of this feature in the tree?
> 
> We do, and it turns out to be a good argument in favor of the change.
> 
> The use of a function for `completing-read`s REQUIRE-MATCH and
> `read-file-name`s MUSTMATCH was added in
> 
>     commit 49e06183f5972817d93dad6acf5351c204e61cc5
>     Author: Lars Ingebrigtsen <larsi@gnus.org>
>     Date:   Fri Jun 10 10:16:57 2022 +0200
>     
>         Allow REQUIRE-MATCH to be a function
>         
>         * doc/lispref/minibuf.texi (Minibuffer Completion): Document it.
>         
>         * lisp/minibuffer.el (completion--complete-and-exit): Allow
>         REQUIRE-MATCH to be a function.
>         (read-file-name): Mention it.
>         
>         * src/minibuf.c (Fcompleting_read): Mention it.
> 
> and the surrounding commits point to its motivation in:
> 
>     commit 7ee736a884766f2017a934d936bfbfa4c70b5099
>     Author: Lars Ingebrigtsen <larsi@gnus.org>
>     Date:   Fri Jun 10 10:19:15 2022 +0200
>     
>         Allow specifying a wildcard argument to list-directory again
>         
>         * lisp/files.el (list-directory): Allow specifying a wildcard
>         argument interactively again (bug#55877).
> 
> But the code in that patch uses MUSTMATCH to *restrict* the possible inputs,
> whereas Joseph's point is that the code currently does not allow using
> MUSTMATCH to restrict the possible inputs.
> 
> And indeed we can currently do `M-x list-directory RET /etc/passwd RET`
> and Emacs happily runs `list-directory` on that non-directory, contrary
> to the intention of the code (IIUC):
> 
>      [...]
>      (list (read-file-name
>             (if pfx "List directory (verbose): "
> 	      "List directory (brief): ")
> 	    nil default-directory
>             (lambda (file)
>               (or (file-directory-p file)
>                   (insert-directory-wildcard-in-dir-p
>                    (file-name-as-directory (expand-file-name file))))))
>      [...]
> 

So you are saying that the current uses of MUSTMATCH simply don't do
what the change was supposed to allow?

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

> So you are saying that the current uses of MUSTMATCH simply don't do
> what the change was supposed to allow?

I think so: the original use (don't know if there are others out there)
wants the behavior that is being discussed rather than the behavior that
is currently provided.


        Stefan

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: joseph@breatheoutbreathe.in,  michael_heerdegen@web.de,
>   philipk@posteo.net,  66187@debbugs.gnu.org
> Date: Sat, 07 Oct 2023 10:25:13 -0400
> 
> > So you are saying that the current uses of MUSTMATCH simply don't do
> > what the change was supposed to allow?
> 
> I think so: the original use (don't know if there are others out there)
> wants the behavior that is being discussed rather than the behavior that
> is currently provided.

Then I think we should install your patch on the emacs-29 branch,
thanks.

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

> Then I think we should install your patch on the emacs-29 branch,
> thanks.

Thanks for the vote of confidence, but as Joseph points out, the patch
isn't actually doing what it says on the tin, so we first have to fix
it :-)


        Stefan

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

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

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

>> Then I think we should install your patch on the emacs-29 branch,
>> thanks.
>
> Thanks for the vote of confidence, but as Joseph points out, the patch
> isn't actually doing what it says on the tin, so we first have to fix
> it :-)

Here's another potential solution.  While the attached patch seems to
work, I'm not sure that minibuffer-completion-confirm should be checked
in completion--do-completion instead of completion--complete-and-exit.

Thoughts?

Thanks!

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-completing-read-functional-REQUIRE-MATCH-behavio.patch --]
[-- Type: text/x-diff, Size: 1407 bytes --]

From 609bf4964f88b01f4843e29b2cf71ee1cd2f6125 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 12 Nov 2023 13:21:50 -0800
Subject: [PATCH] Fix completing-read functional REQUIRE-MATCH behavior

* lisp/minibuffer.el (completion--complete-and-exit): If
minibuffer-completion-confirm is a function which returns nil,
immediately fail to complete.

See bug#66187.
---
 lisp/minibuffer.el | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 3e30b68d5e9..9fad3e71fad 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1847,10 +1847,13 @@ appear to be a match."
    ;; Allow user to specify null string
    ((= beg end) (funcall exit-function))
    ;; The CONFIRM argument is a predicate.
-   ((and (functionp minibuffer-completion-confirm)
-         (funcall minibuffer-completion-confirm
-                  (buffer-substring beg end)))
-    (funcall exit-function))
+   ((functionp minibuffer-completion-confirm)
+    (if (funcall minibuffer-completion-confirm
+                 (buffer-substring beg end))
+        (funcall exit-function)
+      (unless completion-fail-discreetly
+	(ding)
+	(completion--message "No match"))))
    ;; See if we have a completion from the table.
    ((test-completion (buffer-substring beg end)
                      minibuffer-completion-table
-- 
2.41.0

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

Joseph Turner <joseph@breatheoutbreathe.in> writes:

> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>
>>> Then I think we should install your patch on the emacs-29 branch,
>>> thanks.
>>
>> Thanks for the vote of confidence, but as Joseph points out, the patch
>> isn't actually doing what it says on the tin, so we first have to fix
>> it :-)
>
> Here's another potential solution.  While the attached patch seems to
> work, I'm not sure that minibuffer-completion-confirm should be checked
> in completion--do-completion instead of completion--complete-and-exit.
>
> Thoughts?

I just ran the tests in /test/lisp/minibuffer-tests.el with this patch,
and they all still pass.

> Thanks!
>
> Joseph
>
> [2. text/x-diff; 0001-Fix-completing-read-functional-REQUIRE-MATCH-behavio.patch]...

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

> Here's another potential solution.  While the attached patch seems to
> work, I'm not sure that minibuffer-completion-confirm should be checked
> in completion--do-completion instead of completion--complete-and-exit.

It's definitely better to check it in `completion--complete-and-exit`
(which is about exiting and thus related to whether the content is
acceptable) than in `completion--do-completion` (which is just about
completion).

The patch looks OK, thanks.  We could make it call `completion-function`
instead of saying "no match" (after all, it has "complete" in its name),
but it comes with its own set of problems.


        Stefan

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

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

Hello!

For reference, here's the little snippet I'm testing with:

(completing-read "Prompt: " '("a" "b") nil
                 (lambda (input)
                   (string= "a" input)))

where the only expected valid input is "a".

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

>> Here's another potential solution.  While the attached patch seems to
>> work, I'm not sure that minibuffer-completion-confirm should be checked
>> in completion--do-completion instead of completion--complete-and-exit.
>
> It's definitely better to check it in `completion--complete-and-exit`
> (which is about exiting and thus related to whether the content is
> acceptable) than in `completion--do-completion` (which is just about
> completion).

I'm still not sure `completion--complete-and-exit' is the best place to
check that input is valid before exiting.

`completion--do-completion' contains the following cond clause, which
prevents the user from exiting with, e.g., "c".

((null comp)
 (minibuffer-hide-completions)
 (unless completion-fail-discreetly
   (ding)
   (completion--message "No match"))
 (minibuffer--bitset nil nil nil))

Perhaps we should also run that same body when REQUIRE-MATCH is a
function which returns nil?

> The patch looks OK, thanks.  We could make it call `completion-function`
> instead of saying "no match" (after all, it has "complete" in its name),
> but it comes with its own set of problems.

If I understand correctly, the attached patch does this.

I think this works inside of `completion-complete-and-exit', but maybe
not inside of `minibuffer-force-complete-and-exit' since the former
calls `completion--do-completion' internally but the latter does not.

Still exploring ideas...  Thanks!

Joseph


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-completing-read-functional-REQUIRE-MATCH-behavio-2.patch --]
[-- Type: text/x-diff, Size: 1803 bytes --]

From 7b779782504a7b94f64f100a1f8bb71c483873e5 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Mon, 18 Dec 2023 22:41:24 -0500
Subject: [PATCH] Fix completing-read functional REQUIRE-MATCH behavior

* lisp/minibuffer.el (completion--do-completion):  Refuse to exit
minibuffer when REQUIRE-MATCH is a function which returns nil.
(completion--complete-and-exit): Delegate handling of functional
REQUIRE-MATCH to completion-function.

See bug#66187.
---
 lisp/minibuffer.el | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 5c12d9fc914..5d26ff2423e 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1408,7 +1408,11 @@ when the buffer's text is already an exact match."
                         (- (point) beg)
                         md)))
     (cond
-     ((null comp)
+     ((or (null comp)
+          (and (eq t comp)
+               (functionp minibuffer-completion-confirm)
+               (not (funcall minibuffer-completion-confirm
+                             (buffer-substring beg end)))))
       (minibuffer-hide-completions)
       (unless completion-fail-discreetly
 	(ding)
@@ -1849,10 +1853,8 @@ appear to be a match."
    ;; Allow user to specify null string
    ((= beg end) (funcall exit-function))
    ;; The CONFIRM argument is a predicate.
-   ((and (functionp minibuffer-completion-confirm)
-         (funcall minibuffer-completion-confirm
-                  (buffer-substring beg end)))
-    (funcall exit-function))
+   ((functionp minibuffer-completion-confirm)
+    (funcall completion-function))
    ;; See if we have a completion from the table.
    ((test-completion (buffer-substring beg end)
                      minibuffer-completion-table
-- 
2.41.0

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

Friendly ping.  Happy New Year!

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

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

Ping!  I hope a fix like this can be merged into Emacs 29 still :)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-completing-read-functional-REQUIRE-MATCH-behavio.patch --]
[-- Type: text/x-diff, Size: 1407 bytes --]

From 609bf4964f88b01f4843e29b2cf71ee1cd2f6125 Mon Sep 17 00:00:00 2001
From: Joseph Turner <joseph@breatheoutbreathe.in>
Date: Sun, 12 Nov 2023 13:21:50 -0800
Subject: [PATCH] Fix completing-read functional REQUIRE-MATCH behavior

* lisp/minibuffer.el (completion--complete-and-exit): If
minibuffer-completion-confirm is a function which returns nil,
immediately fail to complete.

See bug#66187.
---
 lisp/minibuffer.el | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 3e30b68d5e9..9fad3e71fad 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -1847,10 +1847,13 @@ appear to be a match."
    ;; Allow user to specify null string
    ((= beg end) (funcall exit-function))
    ;; The CONFIRM argument is a predicate.
-   ((and (functionp minibuffer-completion-confirm)
-         (funcall minibuffer-completion-confirm
-                  (buffer-substring beg end)))
-    (funcall exit-function))
+   ((functionp minibuffer-completion-confirm)
+    (if (funcall minibuffer-completion-confirm
+                 (buffer-substring beg end))
+        (funcall exit-function)
+      (unless completion-fail-discreetly
+	(ding)
+	(completion--message "No match"))))
    ;; See if we have a completion from the table.
    ((test-completion (buffer-substring beg end)
                      minibuffer-completion-table
-- 
2.41.0


[-- Attachment #3: Type: text/plain, Size: 20 bytes --]


Thank you!

Joseph

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

> Ping!  I hope a fix like this can be merged into Emacs 29 still :)

Duh, I can't believe I left you waiting for so long, this is awful.
I just pushed this to `emacs-29`, thank you.
I hope there might still be a next time, so I get a chance to do better.


        Stefan


> From 609bf4964f88b01f4843e29b2cf71ee1cd2f6125 Mon Sep 17 00:00:00 2001
> From: Joseph Turner <joseph@breatheoutbreathe.in>
> Date: Sun, 12 Nov 2023 13:21:50 -0800
> Subject: [PATCH] Fix completing-read functional REQUIRE-MATCH behavior
>
> * lisp/minibuffer.el (completion--complete-and-exit): If
> minibuffer-completion-confirm is a function which returns nil,
> immediately fail to complete.
>
> See bug#66187.
> ---
>  lisp/minibuffer.el | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index 3e30b68d5e9..9fad3e71fad 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -1847,10 +1847,13 @@ appear to be a match."
>     ;; Allow user to specify null string
>     ((= beg end) (funcall exit-function))
>     ;; The CONFIRM argument is a predicate.
> -   ((and (functionp minibuffer-completion-confirm)
> -         (funcall minibuffer-completion-confirm
> -                  (buffer-substring beg end)))
> -    (funcall exit-function))
> +   ((functionp minibuffer-completion-confirm)
> +    (if (funcall minibuffer-completion-confirm
> +                 (buffer-substring beg end))
> +        (funcall exit-function)
> +      (unless completion-fail-discreetly
> +	(ding)
> +	(completion--message "No match"))))
>     ;; See if we have a completion from the table.
>     ((test-completion (buffer-substring beg end)
>                       minibuffer-completion-table

bug#66187: read-file-name unexpected behavior when MUSTMATCH is a function

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

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

>> Ping!  I hope a fix like this can be merged into Emacs 29 still :)
>
> Duh, I can't believe I left you waiting for so long, this is awful.
> I just pushed this to `emacs-29`, thank you.
> I hope there might still be a next time, so I get a chance to do better.

It's all good!!  Life is happening :)

Thank you!!

Joseph

>
>         Stefan
>
>
>> From 609bf4964f88b01f4843e29b2cf71ee1cd2f6125 Mon Sep 17 00:00:00 2001
>> From: Joseph Turner <joseph@breatheoutbreathe.in>
>> Date: Sun, 12 Nov 2023 13:21:50 -0800
>> Subject: [PATCH] Fix completing-read functional REQUIRE-MATCH behavior
>>
>> * lisp/minibuffer.el (completion--complete-and-exit): If
>> minibuffer-completion-confirm is a function which returns nil,
>> immediately fail to complete.
>>
>> See bug#66187.
>> ---
>>  lisp/minibuffer.el | 11 +++++++----
>>  1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
>> index 3e30b68d5e9..9fad3e71fad 100644
>> --- a/lisp/minibuffer.el
>> +++ b/lisp/minibuffer.el
>> @@ -1847,10 +1847,13 @@ appear to be a match."
>>     ;; Allow user to specify null string
>>     ((= beg end) (funcall exit-function))
>>     ;; The CONFIRM argument is a predicate.
>> -   ((and (functionp minibuffer-completion-confirm)
>> -         (funcall minibuffer-completion-confirm
>> -                  (buffer-substring beg end)))
>> -    (funcall exit-function))
>> +   ((functionp minibuffer-completion-confirm)
>> +    (if (funcall minibuffer-completion-confirm
>> +                 (buffer-substring beg end))
>> +        (funcall exit-function)
>> +      (unless completion-fail-discreetly
>> +	(ding)
>> +	(completion--message "No match"))))
>>     ;; See if we have a completion from the table.
>>     ((test-completion (buffer-substring beg end)
>>                       minibuffer-completion-table