bug#41705: 28.0.50; minibuffer completion of ".../*/*" shouldn't be "...//"

bug#41705: 28.0.50; minibuffer completion of ".../*/*" shouldn't be "...//"

[Pip Cet]

Recipe:

./emacs -Q
enter: C-x C-f * / * <tab>

expected result: a list of sub-subdirectories of the emacs src dir. A
"Find file: .../emacs/src/*/" prompt.

actual result: prompt says "Find file: .../emacs/src//", further tab
completion uses the root directory.

I'd noticed this for a while now, but thought it was a local
configuration issue. It happens in emacs -Q, too, though.

For master, we should fix minibuffer.el properly, but I'm not sure what
to do on emacs-27. It's a regression (from Emacs 26) that might annoy
many users of tab completion in the minibuffer.

bug#41705: 28.0.50; minibuffer completion of ".../*/*" shouldn't be "...//"

[Eli Zaretskii]

> From: Pip Cet <pipcet@gmail.com>
> Date: Thu, 04 Jun 2020 09:35:12 +0000
> 
> ./emacs -Q
> enter: C-x C-f * / * <tab>
> 
> expected result: a list of sub-subdirectories of the emacs src dir. A
> "Find file: .../emacs/src/*/" prompt.
> 
> actual result: prompt says "Find file: .../emacs/src//", further tab
> completion uses the root directory.
> 
> I'd noticed this for a while now, but thought it was a local
> configuration issue. It happens in emacs -Q, too, though.
> 
> For master, we should fix minibuffer.el properly, but I'm not sure what
> to do on emacs-27. It's a regression (from Emacs 26) that might annoy
> many users of tab completion in the minibuffer.

Can you bisect to find which commit broke this?

Thanks.

bug#41705: 28.0.50; minibuffer completion of ".../*/*" shouldn't be "...//"

[Basil L. Contovounesios]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Pip Cet <pipcet@gmail.com>
>> Date: Thu, 04 Jun 2020 09:35:12 +0000
>> 
>> ./emacs -Q
>> enter: C-x C-f * / * <tab>
>> 
>> expected result: a list of sub-subdirectories of the emacs src dir. A
>> "Find file: .../emacs/src/*/" prompt.
>> 
>> actual result: prompt says "Find file: .../emacs/src//", further tab
>> completion uses the root directory.
>> 
>> I'd noticed this for a while now, but thought it was a local
>> configuration issue. It happens in emacs -Q, too, though.
>> 
>> For master, we should fix minibuffer.el properly, but I'm not sure what
>> to do on emacs-27. It's a regression (from Emacs 26) that might annoy
>> many users of tab completion in the minibuffer.
>
> Can you bisect to find which commit broke this?

I selectively applied commits onto emacs-26 rather than bisecting, and
it seems to point to [1].  Paging Stefan.

[1]: * lisp/minibuffer.el (completion-pcm--optimize-pattern): New function
8bea7e9ab4 2019-12-03 09:45:48 -0500
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=8bea7e9ab4453da71d9766d582089154f31de907

-- 
Basil

bug#41705: 28.0.50; minibuffer completion of ".../*/*" shouldn't be "...//"

[Pip Cet]

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

"Basil L. Contovounesios" <contovob@tcd.ie> writes:
> Eli Zaretskii <eliz@gnu.org> writes:
>>> From: Pip Cet <pipcet@gmail.com>
>>> Date: Thu, 04 Jun 2020 09:35:12 +0000
>>> 
>>> ./emacs -Q
>>> enter: C-x C-f * / * <tab>
>>> 
>>> expected result: a list of sub-subdirectories of the emacs src dir. A
>>> "Find file: .../emacs/src/*/" prompt.
>>> 
>>> actual result: prompt says "Find file: .../emacs/src//", further tab
>>> completion uses the root directory.
>>> 
>>> I'd noticed this for a while now, but thought it was a local
>>> configuration issue. It happens in emacs -Q, too, though.
>>> 
>>> For master, we should fix minibuffer.el properly, but I'm not sure what
>>> to do on emacs-27. It's a regression (from Emacs 26) that might annoy
>>> many users of tab completion in the minibuffer.
>>
>> Can you bisect to find which commit broke this?
>
> I selectively applied commits onto emacs-26 rather than bisecting, and
> it seems to point to [1].  Paging Stefan.

Thanks for that!

I'd actually suspected the same file as well, and started playing around
with the code a little. I'm suspicious of the code in
completion-pcm--optimize-pattern which turns a '(star) pattern into
nil. That's correct as far as which strings qualify as matches, but it's
incorrect because it doesn't survive the completion-pcm--pattern->string
round trip.

I think we get something closer to Emacs-26 behavior back with this
patch.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Don-t-optimize-away-star-patterns-in-minibuffer-file.patch --]
[-- Type: text/x-diff, Size: 1592 bytes --]

From b21734a42860b153a381ac92e5c17f863da4c8da Mon Sep 17 00:00:00 2001
From: Pip Cet <pipcet@gmail.com>
Date: Thu, 4 Jun 2020 18:21:42 +0000
Subject: [PATCH] Don't optimize away star patterns in minibuffer file name
 completion

* lisp/minibuffer.el (completion-pcm--optimize-pattern): Keep
'star in the pattern.  (Bug#41705)
---
 lisp/minibuffer.el | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index d2c3f9045e..15deccc1c3 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3112,12 +3112,12 @@ completion-pcm--optimize-pattern
     (while p
       (pcase p
         (`(,(or 'any 'any-delim) point . ,rest) (setq p `(point . ,rest)))
-        ;; This is not just a performance improvement: it also turns
-        ;; a terminating `point' into an implicit `any', which
-        ;; affects the final position of point (because `point' gets
-        ;; turned into a non-greedy ".*?" regexp whereas we need
-        ;; it the be greedy when it's at the end, see bug#38458).
-        (`(,(pred symbolp)) (setq p nil)) ;Implicit terminating `any'.
+        ;; This is not just a performance improvement: it turns a
+        ;; terminating `point' into an implicit `any', which affects
+        ;; the final position of point (because `point' gets turned
+        ;; into a non-greedy ".*?" regexp whereas we need it to be
+        ;; greedy when it's at the end, see bug#38458).
+        (`(point) (setq p nil)) ;Implicit terminating `any'.
         (_ (push (pop p) n))))
     (nreverse n)))
 
-- 
2.27.0.rc0


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


But it's fairly obvious this code is both tricky and should be worked on
some more, which probably precludes inclusion in emacs-27 (without
further testing) and master, respectively.

bug#41705: 28.0.50; minibuffer completion of ".../*/*" shouldn't be "...//"

[Lars Ingebrigtsen]

Pip Cet <pipcet@gmail.com> writes:

>> I selectively applied commits onto emacs-26 rather than bisecting, and
>> it seems to point to [1].  Paging Stefan.
>
> Thanks for that!
>
> I'd actually suspected the same file as well, and started playing around
> with the code a little. I'm suspicious of the code in
> completion-pcm--optimize-pattern which turns a '(star) pattern into
> nil. That's correct as far as which strings qualify as matches, but it's
> incorrect because it doesn't survive the completion-pcm--pattern->string
> round trip.
>
> I think we get something closer to Emacs-26 behavior back with this
> patch.

This was a couple of months ago, but was apparently never applied?

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#41705: 28.0.50; minibuffer completion of ".../*/*" shouldn't be "...//"

[Pip Cet]

On Fri, Aug 14, 2020 at 5:18 PM Lars Ingebrigtsen <larsi@gnus.org> wrote:
> Pip Cet <pipcet@gmail.com> writes:
> > I think we get something closer to Emacs-26 behavior back with this
> > patch.
>
> This was a couple of months ago, but was apparently never applied?

I was hoping Stefan would chime in.

bug#41705: 28.0.50; minibuffer completion of ".../*/*" shouldn't be "...//"

[Lars Ingebrigtsen]

Pip Cet <pipcet@gmail.com> writes:

>> > I think we get something closer to Emacs-26 behavior back with this
>> > patch.
>>
>> This was a couple of months ago, but was apparently never applied?
>
> I was hoping Stefan would chime in.

I can't hear any chimes, and your patch fixes the bug, so I went ahead
and applied it to Emacs 28.  

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no