GNU bug report logs - #68815
Unexpected behavior with read-file-name and functional REQUIRE-MATCH argument

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 68815 in the body.
You can then email your comments to 68815 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Emacs Bugs Mailing List <bug-gnu-emacs <at> gnu.org>
Subject: Unexpected behavior with read-file-name and functional
 REQUIRE-MATCH argument
Date: Tue, 30 Jan 2024 01:00:47 -0800

Hello!

With #66187 resolved (<https://yhetil.org/emacs-bugs/87cytlqmqh.fsf <at> breatheoutbreathe.in/>),
I reexamined #66114 (<https://yhetil.org/emacs-bugs/87v8bzi7iz.fsf <at> 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

Message #8 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: michael_heerdegen <at> web.de, Philip Kaludercic <philipk <at> posteo.net>,
 68815 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
Date: Tue, 30 Jan 2024 08:08:11 -0500

> 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

Message #11 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: michael_heerdegen <at> web.de, Philip Kaludercic <philipk <at> posteo.net>,
 68815 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
Date: Tue, 30 Jan 2024 22:11:37 -0800

[Message part 1 (text/plain, inline)]

Hi Stefan!

Stefan Monnier <monnier <at> 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

[0001-Pass-unquoted-filename-to-user-supplied-MUSTMATCH-pr.patch (text/x-diff, attachment)]

Message #14 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: michael_heerdegen <at> web.de, Philip Kaludercic <philipk <at> posteo.net>,
 68815 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
Date: Wed, 31 Jan 2024 08:28:01 -0500

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

Message #17 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: michael_heerdegen <at> web.de, Philip Kaludercic <philipk <at> posteo.net>,
 68815 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
Date: Wed, 31 Jan 2024 11:33:31 -0800

Stefan Monnier <monnier <at> 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

Message #20 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: michael_heerdegen <at> web.de, Philip Kaludercic <philipk <at> posteo.net>,
 68815 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
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?

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

Message #23 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: michael_heerdegen <at> web.de, Philip Kaludercic <philipk <at> posteo.net>,
 68815 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stefankangas <at> gmail.com
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
Date: Wed, 31 Jan 2024 14:34:22 -0800

[Message part 1 (text/plain, inline)]

Stefan Monnier <monnier <at> 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

[0001-Pass-unquoted-filename-to-user-supplied-MUSTMATCH-pr.patch (text/x-diff, attachment)]

Message #26 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: michael_heerdegen <at> web.de, Philip Kaludercic <philipk <at> posteo.net>,
 68815 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: bug#68815: Unexpected behavior with read-file-name and functional
 REQUIRE-MATCH argument
Date: Wed, 31 Jan 2024 17:44:41 -0500

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

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

None here.

Message #29 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: michael_heerdegen <at> web.de, philipk <at> posteo.net, 68815 <at> debbugs.gnu.org,
 joseph <at> breatheoutbreathe.in
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
Date: Thu, 01 Feb 2024 08:59:24 +0200

> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> Cc: 68815 <at> debbugs.gnu.org,  Philip Kaludercic <philipk <at> posteo.net>,
>   michael_heerdegen <at> web.de,  Eli Zaretskii <eliz <at> 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.

Message #32 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: michael_heerdegen <at> web.de, philipk <at> posteo.net, 68815 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
Date: Wed, 31 Jan 2024 23:04:59 -0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>> Cc: 68815 <at> debbugs.gnu.org,  Philip Kaludercic <philipk <at> posteo.net>,
>>   michael_heerdegen <at> web.de,  Eli Zaretskii <eliz <at> 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.

Message #35 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>, Eli Zaretskii <eliz <at> gnu.org>
Cc: michael_heerdegen <at> web.de, philipk <at> posteo.net, 68815 <at> debbugs.gnu.org,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#68815: Unexpected behavior with read-file-name and functional
 REQUIRE-MATCH argument
Date: Wed, 31 Jan 2024 23:39:52 -0800

Joseph Turner via "Bug reports for GNU Emacs, the Swiss army knife of
text editors" <bug-gnu-emacs <at> 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.

Message #38 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: michael_heerdegen <at> web.de, philipk <at> posteo.net, 68815 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
Date: Thu, 01 Feb 2024 09:59:57 +0200

> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 68815 <at> debbugs.gnu.org,
>  philipk <at> posteo.net, michael_heerdegen <at> web.de
> Date: Wed, 31 Jan 2024 23:04:59 -0800
> 
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
> >> Cc: 68815 <at> debbugs.gnu.org,  Philip Kaludercic <philipk <at> posteo.net>,
> >>   michael_heerdegen <at> web.de,  Eli Zaretskii <eliz <at> 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.

Message #41 received at 68815 <at> debbugs.gnu.org (full text, mbox):

From: Joseph Turner <joseph <at> breatheoutbreathe.in>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: michael_heerdegen <at> web.de, philipk <at> posteo.net, 68815 <at> debbugs.gnu.org,
 monnier <at> iro.umontreal.ca
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
Date: Thu, 01 Feb 2024 14:26:18 -0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> From: Joseph Turner <joseph <at> breatheoutbreathe.in>
>> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 68815 <at> debbugs.gnu.org,
>>  philipk <at> posteo.net, michael_heerdegen <at> web.de
>> Date: Wed, 31 Jan 2024 23:04:59 -0800
>>
>>
>> Eli Zaretskii <eliz <at> gnu.org> writes:
>>
>> >> From: Stefan Monnier <monnier <at> iro.umontreal.ca>
>> >> Cc: 68815 <at> debbugs.gnu.org,  Philip Kaludercic <philipk <at> posteo.net>,
>> >>   michael_heerdegen <at> web.de,  Eli Zaretskii <eliz <at> 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.

Message #46 received at 68815-done <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Joseph Turner <joseph <at> breatheoutbreathe.in>
Cc: michael_heerdegen <at> web.de, Philip Kaludercic <philipk <at> posteo.net>,
 Eli Zaretskii <eliz <at> gnu.org>, 68815-done <at> debbugs.gnu.org,
 stefankangas <at> gmail.com
Subject: Re: bug#68815: Unexpected behavior with read-file-name and
 functional REQUIRE-MATCH argument
Date: Tue, 06 Feb 2024 16:08:47 -0500

> Please see the attached patch.

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


        Stefan

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.