GNU bug report logs - #77718
31.0.50; completion styles substring and flex are broken

Previous Next

To reply to this bug, email your comments to 77718 AT debbugs.gnu.org.
There is no need to reopen the bug first.

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

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 31.0.50; completion styles substring and flex are broken
Date: Fri, 11 Apr 2025 00:22:01 +0200

0. Start Emacs like this:

   emacs -Q --eval "(custom-set-variables '(completion-category-overrides '((file (styles substring)))))"

   or like this:

   emacs -Q --eval "(custom-set-variables '(completion-category-overrides '((file (styles flex)))))"

1. Type `C-x d' and at the prompt enter: `/usr/ TAB'.
=> Now the text following the prompt is this: "/usr//" and "/usr/" is
fontified with face file-name-shadow.

2. Type `TAB' again.
=> Now the text following the prompt is this: "//" and the first forward
slash is fontified with face file-name-shadow.

3. Type `TAB' again.
=> Now a *Completions* buffer pops up listing all the directories
directly under root (dev/, etc/, home/ and so on).

This only happens with the substring and flex completion styles; with
any of the other completion styles (basic, partial-completion, emacs22,
initials), or if Emacs is started just with -Q, then after step 1 as
expected a *Completions* buffer pops up listing the directories under
/usr (bin/, include/, lib/ and so on).

The broken behavior with the substring and flex styles is due to the
change in completion-pcm--merge-completions in this commit:

commit 0fbba16387513e7692b46885833e4a9c218251f0
Author:     Spencer Baugh <sbaugh <at> janestreet.com>
Commit:     Stefan Monnier <monnier <at> iro.umontreal.ca>
CommitDate: Tue Apr 8 14:36:30 2025 -0400

  Preserve an explicit * in pcm-try-completion
  [...]
  * lisp/minibuffer.el (completion-pcm--merge-completions): Don't drop
  important wildcards. (bug#74420)


In GNU Emacs 31.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version
 3.24.49, cairo version 1.18.4) of 2025-04-10 built on strobelfs2
Repository revision: c0ea954d0f650227dc518f02a292daeb27cf0c37
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12101016
System Description: Linux From Scratch r12.3-20

Configured using:
 'configure -C 'CFLAGS=-Og -g3' PKG_CONFIG_PATH=/opt/qt6/lib/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
LCMS2 LIBSYSTEMD LIBXML2 MODULES NATIVE_COMP NOTIFY INOTIFY PDUMPER PNG
RSVG SECCOMP SOUND SQLITE3 THREADS TIFF TOOLKIT_SCROLL_BARS TREE_SITTER
WEBP X11 XDBE XIM XINERAMA XINPUT2 XPM XRANDR GTK3 ZLIB

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Stephen Berman <stephen.berman <at> gmx.net>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org
Subject: Re: bug#77718: 31.0.50;
 completion styles substring and flex are broken
Date: Fri, 11 Apr 2025 10:22:46 +0300

> Date: Fri, 11 Apr 2025 00:22:01 +0200
> From:  Stephen Berman via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
> 
> 0. Start Emacs like this:
> 
>    emacs -Q --eval "(custom-set-variables '(completion-category-overrides '((file (styles substring)))))"
> 
>    or like this:
> 
>    emacs -Q --eval "(custom-set-variables '(completion-category-overrides '((file (styles flex)))))"
> 
> 1. Type `C-x d' and at the prompt enter: `/usr/ TAB'.
> => Now the text following the prompt is this: "/usr//" and "/usr/" is
> fontified with face file-name-shadow.
> 
> 2. Type `TAB' again.
> => Now the text following the prompt is this: "//" and the first forward
> slash is fontified with face file-name-shadow.
> 
> 3. Type `TAB' again.
> => Now a *Completions* buffer pops up listing all the directories
> directly under root (dev/, etc/, home/ and so on).
> 
> This only happens with the substring and flex completion styles; with
> any of the other completion styles (basic, partial-completion, emacs22,
> initials), or if Emacs is started just with -Q, then after step 1 as
> expected a *Completions* buffer pops up listing the directories under
> /usr (bin/, include/, lib/ and so on).
> 
> The broken behavior with the substring and flex styles is due to the
> change in completion-pcm--merge-completions in this commit:
> 
> commit 0fbba16387513e7692b46885833e4a9c218251f0
> Author:     Spencer Baugh <sbaugh <at> janestreet.com>
> Commit:     Stefan Monnier <monnier <at> iro.umontreal.ca>
> CommitDate: Tue Apr 8 14:36:30 2025 -0400
> 
>   Preserve an explicit * in pcm-try-completion
>   [...]
>   * lisp/minibuffer.el (completion-pcm--merge-completions): Don't drop
>   important wildcards. (bug#74420)

Thanks, adding the guilty parties to the discussion.

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77718 <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Fri, 11 Apr 2025 09:03:12 -0400

Eli Zaretskii <eliz <at> gnu.org> writes:
>> Date: Fri, 11 Apr 2025 00:22:01 +0200
>> From:  Stephen Berman via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs <at> gnu.org>
>> 
>> 0. Start Emacs like this:
>> 
>>    emacs -Q --eval "(custom-set-variables '(completion-category-overrides '((file (styles substring)))))"
>> 
>>    or like this:
>> 
>>    emacs -Q --eval "(custom-set-variables '(completion-category-overrides '((file (styles flex)))))"
>> 
>> 1. Type `C-x d' and at the prompt enter: `/usr/ TAB'.
>> => Now the text following the prompt is this: "/usr//" and "/usr/" is
>> fontified with face file-name-shadow.
>> 
>> 2. Type `TAB' again.
>> => Now the text following the prompt is this: "//" and the first forward
>> slash is fontified with face file-name-shadow.
>> 
>> 3. Type `TAB' again.
>> => Now a *Completions* buffer pops up listing all the directories
>> directly under root (dev/, etc/, home/ and so on).
>> 
>> This only happens with the substring and flex completion styles; with
>> any of the other completion styles (basic, partial-completion, emacs22,
>> initials), or if Emacs is started just with -Q, then after step 1 as
>> expected a *Completions* buffer pops up listing the directories under
>> /usr (bin/, include/, lib/ and so on).
>> 
>> The broken behavior with the substring and flex styles is due to the
>> change in completion-pcm--merge-completions in this commit:
>> 
>> commit 0fbba16387513e7692b46885833e4a9c218251f0
>> Author:     Spencer Baugh <sbaugh <at> janestreet.com>
>> Commit:     Stefan Monnier <monnier <at> iro.umontreal.ca>
>> CommitDate: Tue Apr 8 14:36:30 2025 -0400
>> 
>>   Preserve an explicit * in pcm-try-completion
>>   [...]
>>   * lisp/minibuffer.el (completion-pcm--merge-completions): Don't drop
>>   important wildcards. (bug#74420)

Thanks for the detailed bug report.

Reduced slightly, the issue is that
  (completion-pcm--merge-completions '("x/" "y/") '(prefix))
now returns 
  ("/" any prefix "" "")
when it used to return
  (any "" "")

I think this may be related to the implict "any" added by
completion-pcm--merge-completions.  I'll look into it and find a fix
(and add a test covering this case).

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77718 <at> debbugs.gnu.org, Stephen Berman <stephen.berman <at> gmx.net>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Fri, 11 Apr 2025 15:43:18 -0400

Spencer Baugh <sbaugh <at> janestreet.com> writes:
> Reduced slightly, the issue is that
>   (completion-pcm--merge-completions '("x/" "y/") '(prefix))
> now returns 
>   ("/" any prefix "" "")
> when it used to return
>   (any "" "")
>
> I think this may be related to the implict "any" added by
> completion-pcm--merge-completions.  I'll look into it and find a fix
> (and add a test covering this case).

Update: That's not the issue; this new return value for
completion-pcm--merge-completions is actually correct, it's a desirable
consequence of my bugfix patch.

The issue is actually in completion-table-with-quoting, with filename
completion and substitute-in-file-name; it doesn't handle // properly.

I'm trying to fix it; it may be a somewhat large change to
completion-table-with-quoting.

Stefan, a related question: do you think it would be reasonable to split
completion-table-with-quoting into two functions, separating the
substitute-in-file-name and comint-quote-filename cases?  It seems to me
that the comint-quote-filename case is way simpler than the
substitute-in-file-name case, and it would be a lot easier to understand
if the two cases were done separately, without the abstraction of
requote and unquote functions.  I can do that if that seems reasonable.
It would make fixing this issue a lot easier.

(I don't think we need to maintain the same interface for
completion-table-with-quoting - I see no usage in the 200 packages used
at my site - I strongly suspect, given the complexity, that the only
users are in Emacs core)

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Fri, 11 Apr 2025 17:37:52 -0400

> Stefan, a related question: do you think it would be reasonable to split
> completion-table-with-quoting into two functions, separating the
> substitute-in-file-name and comint-quote-filename cases?  It seems to me
> that the comint-quote-filename case is way simpler than the
> substitute-in-file-name case, and it would be a lot easier to understand
> if the two cases were done separately, without the abstraction of
> requote and unquote functions.  I can do that if that seems reasonable.
> It would make fixing this issue a lot easier.

It's worth a try.  The system we have is the best I could come up with
back then but is hideous, so I'm interested in a simpler solution.

> (I don't think we need to maintain the same interface for
> completion-table-with-quoting - I see no usage in the 200 packages used
> at my site - I strongly suspect, given the complexity, that the only
> users are in Emacs core)

It's definitely not worth worrying about that while designing
a replacement.  Maybe we'll want to preserve backward compatibility, but
let's first get a good replacement.


        Stefan

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Mon, 14 Apr 2025 16:30:09 -0400

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

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

>> Stefan, a related question: do you think it would be reasonable to split
>> completion-table-with-quoting into two functions, separating the
>> substitute-in-file-name and comint-quote-filename cases?  It seems to me
>> that the comint-quote-filename case is way simpler than the
>> substitute-in-file-name case, and it would be a lot easier to understand
>> if the two cases were done separately, without the abstraction of
>> requote and unquote functions.  I can do that if that seems reasonable.
>> It would make fixing this issue a lot easier.
>
> It's worth a try.  The system we have is the best I could come up with
> back then but is hideous, so I'm interested in a simpler solution.

OK, here is a rework of just the substitute-in-file-name side.  This
fixes this bug (by making completion continue to work when point is at
/usr/|/), and fixes another FIXME in a test (partial completion now
preserves environment variables!).  And I don't think it loses anything
important, but I may be missing something.

(completion--sifn-requote can now be deleted and there are probably
other cleanups possible after this patch, but I didn't do them here to
keep the patch smaller.  Also, rfn-eshadow needs to be updated, but
that's just a visual issue.)

[0001-Improve-env-var-handling-in-read-file-name.patch (text/x-patch, inline)]

From 48653fd0b22739f6d836bbe121989f5051502d81 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 14 Apr 2025 16:01:38 -0400
Subject: [PATCH] Improve env var handling in read-file-name

Fix various bugs, including bug#77718, by rewriting the way file
name completion handles environment variable expansion.  Instead
of using completion-table-with-quoting to manipulate the string
being completed on, simply make the completion table itself
understand substitute-in-file-name.

The completion table first tries to complete without
substituting environment variables; this allows completion to
work on file names which contain things which look like
environment variables.  If that fails, we call
substitute-in-file-name and then complete again.

Tests are updated: we no longer duplicate $s in file names,
since that's not necessary.  And partial-completion now
preserves unexpanded environment variables.  However,
partial-completion no longer works across environment variables
containing delimiters; that's an acceptable sacrifice.

* lisp/minibuffer.el (completion--replace-prefix): Add.
(completion--file-name-table): Rewrite to use
substitute-in-file-name explicitly. (bug#77718)
* test/lisp/minibuffer-tests.el (completion-table-test-quoting)
(minibuffer-next-completion): Update.
---
 lisp/minibuffer.el            | 43 +++++++++++++++++++++++++++++++----
 test/lisp/minibuffer-tests.el | 16 ++++++-------
 2 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c0ccfc00ce5..4ccf3a1d2f9 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3596,13 +3596,46 @@ completion--sifn-requote
           (setq qpos (1- qpos)))
         (cons qpos #'minibuffer-maybe-quote-filename)))))
 
-(defalias 'completion--file-name-table
-  (completion-table-with-quoting #'completion-file-name-table
-                                 #'substitute-in-file-name
-                                 #'completion--sifn-requote)
+(defun completion--replace-prefix (s old new)
+  "Replace prefix OLD of string S with prefix NEW."
+  (concat new (substring s (length old))))
+
+(defun completion--file-name-table (orig pred action)
   "Internal subroutine for `read-file-name'.  Do not call this.
 This is a completion table for file names, like `completion-file-name-table'
-except that it passes the file name through `substitute-in-file-name'.")
+except that it passes the file name through `substitute-in-file-name'."
+  (let ((table #'completion-file-name-table))
+    (cond
+     ((eq (car-safe action) 'boundaries)
+      ;; Boundaries are always computed on the original string.
+      (complete-with-action action table orig pred))
+     (;; Ensure we discard text before // by always using sifn in that case.
+      (unless (string-search "//" orig)
+        (complete-with-action action table orig pred)))
+     (t ;; On a nil result, complete again after sifn and adjust the result.
+      (let* ((expansion (substitute-in-file-name orig))
+             (result (complete-with-action action table expansion pred)))
+        (cond
+         ((null action)
+          (if (stringp result)
+              (completion--replace-prefix result expansion orig)
+            result))
+         ((eq action t)
+          ;; Find the part of EXPANSION which is inside the boundaries, and
+          ;; replace it with the part of ORIG which is inside the boundaires.
+          (let* ((expansion-bounds (completion-boundaries expansion table pred ""))
+                 (expansion-in-bounds (substring expansion (car expansion-bounds)))
+                 (orig-bounds (completion-boundaries orig table pred ""))
+                 (orig-in-bounds (substring orig (car orig-bounds))))
+            (if (and (string-empty-p expansion-in-bounds)
+                     (string-empty-p orig-in-bounds))
+                ;; Nothing to replace, so avoid doing unnecessary work.
+                result
+              (mapcar
+               (lambda (s)
+                 (completion--replace-prefix s expansion-in-bounds orig-in-bounds))
+               result))))
+         (t result)))))))
 
 (defalias 'read-file-name-internal
   (completion-table-in-turn #'completion--embedded-envvar-table
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 59ac5ab9578..38c005bd1b5 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -91,20 +91,18 @@ completion-table-subvert-test
 
 (ert-deftest completion-table-test-quoting ()
   (let ((process-environment
-         `("CTTQ1=ed" "CTTQ2=et/" ,@process-environment))
+         `("CTTQ1=ed" ,@process-environment))
         (default-directory (ert-resource-directory)))
     (pcase-dolist (`(,input ,output)
                    '(
-                     ;; Test that $ in files is properly $$ quoted.
-                     ("data/m-cttq" "data/minibuffer-test-cttq$$tion")
-                     ;; Test that $$ in input is properly unquoted.
-                     ("data/m-cttq$$t" "data/minibuffer-test-cttq$$tion")
+                     ;; Test that files containing $ can be completed
+                     ("data/m-cttq" "data/minibuffer-test-cttq$tion")
+                     ;; Test that $ can be completed
+                     ("data/m-cttq$" "data/minibuffer-test-cttq$tion")
                      ;; Test that env-vars are preserved.
                      ("lisp/c${CTTQ1}et/se-u" "lisp/c${CTTQ1}et/semantic-utest")
-                     ("lisp/ced${CTTQ2}se-u" "lisp/ced${CTTQ2}semantic-utest")
                      ;; Test that env-vars don't prevent partial-completion.
-                     ;; FIXME: Ideally we'd like to keep the ${CTTQ}!
-                     ("lis/c${CTTQ1}/se-u" "lisp/cedet/semantic-utest")
+                     ("lis/c${CTTQ1}/se-u" "lisp/c${CTTQ1}et/semantic-utest")
                      ))
       (should (equal (completion-try-completion input
                                                 #'completion--file-name-table
@@ -690,7 +688,7 @@ minibuffer-next-completion
     (completing-read-with-minibuffer-setup #'read-file-name-internal
       (insert "d/")
       (execute-kbd-macro (kbd "M-<down> M-<down> M-<down>"))
-      (should (equal "data/minibuffer-test-cttq$$tion" (minibuffer-contents))))))
+      (should (equal "data/minibuffer-test-cttq$tion" (minibuffer-contents))))))
 
 (provide 'minibuffer-tests)
 ;;; minibuffer-tests.el ends here
-- 
2.39.3

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, stephen.berman <at> gmx.net, monnier <at> iro.umontreal.ca
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 15 Apr 2025 09:25:39 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: 77718 <at> debbugs.gnu.org,  Eli Zaretskii <eliz <at> gnu.org>,  Stephen Berman
>   <stephen.berman <at> gmx.net>
> Date: Mon, 14 Apr 2025 16:30:09 -0400
> 
> +     (;; Ensure we discard text before // by always using sifn in that case.
> +      (unless (string-search "//" orig)
> +        (complete-with-action action table orig pred)))

AFAIU, this ignores the case of UNC file names, which begin with "//",
and other complications with file names on Windows.  Emacs on Windows
reacts differently to "//" than it does on Posix systems, and it does
so for very good reasons.

Please don't break file-name completion on Windows.  It took us many
iterations to get it right, and the result is fragile.  Be sure to
test each change on Windows as well before installing.

For the same reasons, please test thoroughly with remote file names of
different formats.

Given the fragility of this, I'd tend to suggest to define a new style
with these changes, and leave alone the existing styles, even if you
consider them "broken".

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 15 Apr 2025 10:22:15 +0200

On Mon, 14 Apr 2025 16:30:09 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>>> Stefan, a related question: do you think it would be reasonable to split
>>> completion-table-with-quoting into two functions, separating the
>>> substitute-in-file-name and comint-quote-filename cases?  It seems to me
>>> that the comint-quote-filename case is way simpler than the
>>> substitute-in-file-name case, and it would be a lot easier to understand
>>> if the two cases were done separately, without the abstraction of
>>> requote and unquote functions.  I can do that if that seems reasonable.
>>> It would make fixing this issue a lot easier.
>>
>> It's worth a try.  The system we have is the best I could come up with
>> back then but is hideous, so I'm interested in a simpler solution.
>
> OK, here is a rework of just the substitute-in-file-name side.  This
> fixes this bug (by making completion continue to work when point is at
> /usr/|/), and fixes another FIXME in a test (partial completion now
> preserves environment variables!).  And I don't think it loses anything
> important, but I may be missing something.
>
> (completion--sifn-requote can now be deleted and there are probably
> other cleanups possible after this patch, but I didn't do them here to
> keep the patch smaller.  Also, rfn-eshadow needs to be updated, but
> that's just a visual issue.)

Thanks, I applied your patch and rebuilt emacs, and now completion with
the substring style works, but it's still different from the behavior
before you previous change, as your comments above seem to acknowledge:
now when I enter "/us" and hit TAB, it completes to "/usr/" and when I
hit TAB again it becomes "/usr//" with point on (i.e. just before) the
last slash and "/usr/" displayed with the file-name-shadow face.
Hitting TAB again pops up a *Completions* buffer with the correct
completions (immediate subdirectories of /usr) and typing part of a
candidate in the minibuffer correctly expands the file name.  This
behavior obtains with any directory whose immediate children are all
directories; if at least one child is a non-directory file, the
*Completions* buffer up on hitting the TAB the second time (and the part
of the file name up to the final slash has the default face); so here
the completion works also visually as before.  So your patch only
partially fixes the regression.

Steve Berman

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

From: Ship Mints <shipmints <at> gmail.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: Spencer Baugh <sbaugh <at> janestreet.com>, stephen.berman <at> gmx.net,
 monnier <at> iro.umontreal.ca, 77718 <at> debbugs.gnu.org
Subject: Re: bug#77718: 31.0.50;
 completion styles substring and flex are broken
Date: Tue, 15 Apr 2025 07:16:58 -0400

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

On Tue, Apr 15, 2025 at 2:26 AM Eli Zaretskii <eliz <at> gnu.org> wrote:

> > From: Spencer Baugh <sbaugh <at> janestreet.com>
> > Cc: 77718 <at> debbugs.gnu.org,  Eli Zaretskii <eliz <at> gnu.org>,  Stephen
> Berman
> >   <stephen.berman <at> gmx.net>
> > Date: Mon, 14 Apr 2025 16:30:09 -0400
> >
> > +     (;; Ensure we discard text before // by always using sifn in that
> case.
> > +      (unless (string-search "//" orig)
> > +        (complete-with-action action table orig pred)))
>
> AFAIU, this ignores the case of UNC file names, which begin with "//",
> and other complications with file names on Windows.  Emacs on Windows
> reacts differently to "//" than it does on Posix systems, and it does
> so for very good reasons.
>
> Please don't break file-name completion on Windows.  It took us many
> iterations to get it right, and the result is fragile.  Be sure to
> test each change on Windows as well before installing.
>
> For the same reasons, please test thoroughly with remote file names of
> different formats.
>
> Given the fragility of this, I'd tend to suggest to define a new style
> with these changes, and leave alone the existing styles, even if you
> consider them "broken".
>

The POSIX standard allows "pathnames" that start with "//" and Emacs, for
sure, should respect file names that begin with //.

"A pathname that begins with two successive slashes may be interpreted in
an implementation-defined manner, although more than two leading slashes
shall be treated as a single slash."

"A pathname may optionally contain one or more trailing slashes."

"Multiple successive slashes are considered to be the same as one slash."

For mailing list posterity:

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_271:~:text=in%20Filename.-,3.273,-Path%20Prefix
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_271:~:text=in%20their%20execution.-,3.40,-Basename
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#:~:text=If%20a%20pathname%20begins%20with%20two%20successive%20%3Cslash%3E%20characters%2C%20the%20first%20component%20following%20the%20leading%20%3Cslash%3E%20characters%20may%20be%20interpreted%20in%20an%20implementation%2Ddefined%20manner%2C%20although%20more%20than%20two%20leading%20%3Cslash%3E%20characters%20shall%20be%20treated%20as%20a%20single%20%3Cslash%3E%20character.
https://pubs.opengroup.org/onlinepubs/007904875/basedefs/xbd_chap03.html#tag_03_266:~:text=A%20pathname%20may%20optionally%20contain%20one%20or%20more%20trailing%20slashes.%20Multiple%20successive%20slashes%20are%20considered%20to%20be%20the%20same%20as%20one%20slash.

-Stephane

[Message part 2 (text/html, inline)]

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 77718 <at> debbugs.gnu.org, stephen.berman <at> gmx.net, monnier <at> iro.umontreal.ca
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 15 Apr 2025 15:24:45 -0400

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

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

>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Cc: 77718 <at> debbugs.gnu.org,  Eli Zaretskii <eliz <at> gnu.org>,  Stephen Berman
>>   <stephen.berman <at> gmx.net>
>> Date: Mon, 14 Apr 2025 16:30:09 -0400
>> 
>> +     (;; Ensure we discard text before // by always using sifn in that case.
>> +      (unless (string-search "//" orig)
>> +        (complete-with-action action table orig pred)))
>
> AFAIU, this ignores the case of UNC file names, which begin with "//",
> and other complications with file names on Windows.  Emacs on Windows
> reacts differently to "//" than it does on Posix systems, and it does
> so for very good reasons.

Ah, thanks, good point.  I updated it to remove this hardcoded handling
of //, and instead defer to substitute-in-file-name, which will behave
right on Windows.  Updated patch attached.

> Please don't break file-name completion on Windows.  It took us many
> iterations to get it right, and the result is fragile.  Be sure to
> test each change on Windows as well before installing.

Can do, I'm working on setting up a test Windows system right now.

Is there any way to approximately test the behavior of Windows file name
completion while still on GNU/Linux?  It might be useful to have tests
which can run on GNU/Linux but test the Windows behavior.

> For the same reasons, please test thoroughly with remote file names of
> different formats.

Will do - I'll make some additions to tramp-tests as well.

[0001-Improve-env-var-handling-in-read-file-name.patch (text/x-patch, inline)]

From d35a5068cb9d9a75a19914fb624da5b0bd461937 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 14 Apr 2025 16:01:38 -0400
Subject: [PATCH] Improve env var handling in read-file-name

Fix various bugs, including bug#77718, by rewriting the way file
name completion handles environment variable expansion.  Instead
of using completion-table-with-quoting to manipulate the string
being completed on, simply make the completion table itself
understand substitute-in-file-name.

The completion table first tries to complete without
substituting environment variables; this allows completion to
work on file names which contain things which look like
environment variables.  If that fails, we call
substitute-in-file-name and then complete again.

Tests are updated: we no longer duplicate $s in file names,
since that's not necessary.  And partial-completion now
preserves unexpanded environment variables.  However,
partial-completion no longer works across environment variables
containing delimiters; that's an acceptable sacrifice.

* lisp/minibuffer.el (completion--replace-prefix)
(completion--complete-on-expansion): Add.
(completion--file-name-table): Rewrite to use
substitute-in-file-name explicitly. (bug#77718)
* test/lisp/minibuffer-tests.el (completion-table-test-quoting)
(minibuffer-next-completion): Update.
---
 lisp/minibuffer.el            | 57 ++++++++++++++++++++++++++++++++---
 test/lisp/minibuffer-tests.el | 16 +++++-----
 2 files changed, 59 insertions(+), 14 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ab00395a850..e53bd673df9 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3565,13 +3565,60 @@ completion--sifn-requote
           (setq qpos (1- qpos)))
         (cons qpos #'minibuffer-maybe-quote-filename)))))
 
-(defalias 'completion--file-name-table
-  (completion-table-with-quoting #'completion-file-name-table
-                                 #'substitute-in-file-name
-                                 #'completion--sifn-requote)
+(defun completion--replace-prefix (s old new)
+  "Replace prefix OLD of string S with prefix NEW."
+  (concat new (substring s (length old))))
+
+(defun completion--file-name-table (orig pred action)
   "Internal subroutine for `read-file-name'.  Do not call this.
 This is a completion table for file names, like `completion-file-name-table'
-except that it passes the file name through `substitute-in-file-name'.")
+except that it passes the file name through `substitute-in-file-name'."
+  (let ((table #'completion-file-name-table))
+    (or
+     ;; First try completing without expanding environment variables; we still
+     ;; call `substitute-in-file-name' to handle // and other constructs.
+     (let ((sifned (let ((process-environment nil)) (substitute-in-file-name orig))))
+       ;; Fall through to the second case if `substitute-in-file-name' expanded
+       ;; something anyway.
+       (when (string-suffix-p sifned orig)
+         (let ((discarded (string-remove-suffix sifned orig))
+               (result (complete-with-action action table sifned pred)))
+           (cond
+            ((eq (car-safe action) 'boundaries)
+             (cons 'boundaries (cons (+ (length discarded) (cadr result)) (cddr result))))
+            ((null action)
+             (if (stringp result) (concat discarded result) result))
+            (t result)))))
+     ;; Try again, this time expanding environment variables.
+     (cond
+      ((eq (car-safe action) 'boundaries)
+       ;; Compute the boundaries on ORIG; boundaries created by expanding
+       ;; environment variables can't be used.
+       (complete-with-action action table orig pred))
+      ((let* ((sifned (substitute-in-file-name orig))
+              (result (complete-with-action action table sifned pred)))
+         (cond
+          ((null action)
+           (if (stringp result)
+               (completion--replace-prefix result sifned orig)
+             result))
+          ((eq action t)
+           (when result
+             ;; Find the part of SIFNED which is inside the boundaries, and
+             ;; replace it with the part of ORIG which is inside the boundaries.
+             (let* ((expansion-bounds (completion-boundaries sifned table pred ""))
+                    (expansion-in-bounds (substring sifned (car expansion-bounds)))
+                    (orig-bounds (completion-boundaries orig table pred ""))
+                    (orig-in-bounds (substring orig (car orig-bounds))))
+               (if (and (string-empty-p expansion-in-bounds)
+                        (string-empty-p orig-in-bounds))
+                   ;; Nothing to replace, so avoid doing unnecessary work.
+                   result
+                 (mapcar
+                  (lambda (s)
+                    (completion--replace-prefix s expansion-in-bounds orig-in-bounds))
+                  result)))))
+          (t result))))))))
 
 (defalias 'read-file-name-internal
   (completion-table-in-turn #'completion--embedded-envvar-table
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 59ac5ab9578..38c005bd1b5 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -91,20 +91,18 @@ completion-table-subvert-test
 
 (ert-deftest completion-table-test-quoting ()
   (let ((process-environment
-         `("CTTQ1=ed" "CTTQ2=et/" ,@process-environment))
+         `("CTTQ1=ed" ,@process-environment))
         (default-directory (ert-resource-directory)))
     (pcase-dolist (`(,input ,output)
                    '(
-                     ;; Test that $ in files is properly $$ quoted.
-                     ("data/m-cttq" "data/minibuffer-test-cttq$$tion")
-                     ;; Test that $$ in input is properly unquoted.
-                     ("data/m-cttq$$t" "data/minibuffer-test-cttq$$tion")
+                     ;; Test that files containing $ can be completed
+                     ("data/m-cttq" "data/minibuffer-test-cttq$tion")
+                     ;; Test that $ can be completed
+                     ("data/m-cttq$" "data/minibuffer-test-cttq$tion")
                      ;; Test that env-vars are preserved.
                      ("lisp/c${CTTQ1}et/se-u" "lisp/c${CTTQ1}et/semantic-utest")
-                     ("lisp/ced${CTTQ2}se-u" "lisp/ced${CTTQ2}semantic-utest")
                      ;; Test that env-vars don't prevent partial-completion.
-                     ;; FIXME: Ideally we'd like to keep the ${CTTQ}!
-                     ("lis/c${CTTQ1}/se-u" "lisp/cedet/semantic-utest")
+                     ("lis/c${CTTQ1}/se-u" "lisp/c${CTTQ1}et/semantic-utest")
                      ))
       (should (equal (completion-try-completion input
                                                 #'completion--file-name-table
@@ -690,7 +688,7 @@ minibuffer-next-completion
     (completing-read-with-minibuffer-setup #'read-file-name-internal
       (insert "d/")
       (execute-kbd-macro (kbd "M-<down> M-<down> M-<down>"))
-      (should (equal "data/minibuffer-test-cttq$$tion" (minibuffer-contents))))))
+      (should (equal "data/minibuffer-test-cttq$tion" (minibuffer-contents))))))
 
 (provide 'minibuffer-tests)
 ;;; minibuffer-tests.el ends here
-- 
2.39.3

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 15 Apr 2025 17:04:43 -0400

> Fix various bugs, including bug#77718, by rewriting the way file
> name completion handles environment variable expansion.  Instead
> of using completion-table-with-quoting to manipulate the string
> being completed on, simply make the completion table itself
> understand substitute-in-file-name.

OK.

> The completion table first tries to complete without
> substituting environment variables; this allows completion to
> work on file names which contain things which look like
> environment variables.  If that fails, we call
> substitute-in-file-name and then complete again.

Why?  `substitute-in-file-name` presumably already takes care of that:

    (substitute-in-file-name "/foo/$NOTAVAR/bar")
    => "/foo/$NOTAVAR/bar"

> Tests are updated: we no longer duplicate $s in file names,
> since that's not necessary.

AFAIK it is still needed for the (null action) case.

> And partial-completion now
> preserves unexpanded environment variables.  However,
> partial-completion no longer works across environment variables
> containing delimiters; that's an acceptable sacrifice.

Agreed.

> * lisp/minibuffer.el (completion--replace-prefix)
> (completion--complete-on-expansion): Add.
> (completion--file-name-table): Rewrite to use
> substitute-in-file-name explicitly. (bug#77718)

I think you can also delete `completion--sifn-requote` while at it.


        Stefan

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 15 Apr 2025 17:21:11 -0400

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> The completion table first tries to complete without
>> substituting environment variables; this allows completion to
>> work on file names which contain things which look like
>> environment variables.  If that fails, we call
>> substitute-in-file-name and then complete again.
>
> Why?  `substitute-in-file-name` presumably already takes care of that:
>
>     (substitute-in-file-name "/foo/$NOTAVAR/bar")
>     => "/foo/$NOTAVAR/bar"

To support completing file names that look like environment variables,
without the need to quote $ by duplicating it.

>> Tests are updated: we no longer duplicate $s in file names,
>> since that's not necessary.
>
> AFAIK it is still needed for the (null action) case.

Why would that be?  I believe quoting $ should no longer be necessary.

>> * lisp/minibuffer.el (completion--replace-prefix)
>> (completion--complete-on-expansion): Add.
>> (completion--file-name-table): Rewrite to use
>> substitute-in-file-name explicitly. (bug#77718)
>
> I think you can also delete `completion--sifn-requote` while at it.

Indeed, I will, just avoiding that for now while iterating on the patch.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 15 Apr 2025 17:50:44 -0400

>> Why?  `substitute-in-file-name` presumably already takes care of that:
>>
>>     (substitute-in-file-name "/foo/$NOTAVAR/bar")
>>     => "/foo/$NOTAVAR/bar"
>
> To support completing file names that look like environment variables,
> without the need to quote $ by duplicating it.

I don't understand: the example I show above suggests you can do that
without doing anything special.

[ Also, I'm not sure it's worth the trouble anyway: dollars in file names
  are fairly rare, so having to double them in those rare cases where they
  occur is not a big deal.  In any case, it seems orthogonal to the
  problem at hand.  ]

>>> Tests are updated: we no longer duplicate $s in file names,
>>> since that's not necessary.
>> AFAIK it is still needed for the (null action) case.
> Why would that be?  I believe quoting $ should no longer be necessary.

Say I have a file

    ~/tmp/foo$HOME.txt

and I do

    C-x C-f ~/tmp/foo TAB

AFAIK it needs to complete to

    ~/tmp/foo$$HOME.txt

otherwise the file name returned by `read-file-name` will be

    ~/tmp/foo/home/monnier.txt

> Indeed, I will, just avoiding that for now while iterating on the patch.

+1


        Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, stephen.berman <at> gmx.net, monnier <at> iro.umontreal.ca
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Wed, 16 Apr 2025 10:31:25 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: 77718 <at> debbugs.gnu.org,  stephen.berman <at> gmx.net,  monnier <at> iro.umontreal.ca
> Date: Tue, 15 Apr 2025 15:24:45 -0400
> 
> Eli Zaretskii <eliz <at> gnu.org> writes:
> 
> >> From: Spencer Baugh <sbaugh <at> janestreet.com>
> >> Cc: 77718 <at> debbugs.gnu.org,  Eli Zaretskii <eliz <at> gnu.org>,  Stephen Berman
> >>   <stephen.berman <at> gmx.net>
> >> Date: Mon, 14 Apr 2025 16:30:09 -0400
> >> 
> >> +     (;; Ensure we discard text before // by always using sifn in that case.
> >> +      (unless (string-search "//" orig)
> >> +        (complete-with-action action table orig pred)))
> >
> > AFAIU, this ignores the case of UNC file names, which begin with "//",
> > and other complications with file names on Windows.  Emacs on Windows
> > reacts differently to "//" than it does on Posix systems, and it does
> > so for very good reasons.
> 
> Ah, thanks, good point.  I updated it to remove this hardcoded handling
> of //, and instead defer to substitute-in-file-name, which will behave
> right on Windows.  Updated patch attached.
> 
> > Please don't break file-name completion on Windows.  It took us many
> > iterations to get it right, and the result is fragile.  Be sure to
> > test each change on Windows as well before installing.
> 
> Can do, I'm working on setting up a test Windows system right now.

Thanks.

> Is there any way to approximately test the behavior of Windows file name
> completion while still on GNU/Linux?  It might be useful to have tests
> which can run on GNU/Linux but test the Windows behavior.

Unfortunately, this is not possible, because portions of the relevant
code in C are only compiled into the Windows build of Emacs.

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Wed, 16 Apr 2025 13:10:42 -0400

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> Why?  `substitute-in-file-name` presumably already takes care of that:
>>>
>>>     (substitute-in-file-name "/foo/$NOTAVAR/bar")
>>>     => "/foo/$NOTAVAR/bar"
>>
>> To support completing file names that look like environment variables,
>> without the need to quote $ by duplicating it.
>
> I don't understand: the example I show above suggests you can do that
> without doing anything special.

That only works for completing file names that contain $ and which
*aren't* currently environment variables.

My code supports completing on ~/$HOME/foo where "$HOME" is the literal
name of a directory.

> [ Also, I'm not sure it's worth the trouble anyway: dollars in file names
>   are fairly rare, so having to double them in those rare cases where they
>   occur is not a big deal.  In any case, it seems orthogonal to the
>   problem at hand.  ]

That's fair - so you think it might be OK to, basically, not support
completion on file names containing substrings like "$HOME"?  That could
make things simpler, if we think that's acceptable.

>>>> Tests are updated: we no longer duplicate $s in file names,
>>>> since that's not necessary.
>>> AFAIK it is still needed for the (null action) case.
>> Why would that be?  I believe quoting $ should no longer be necessary.
>
> Say I have a file
>
>     ~/tmp/foo$HOME.txt
>
> and I do
>
>     C-x C-f ~/tmp/foo TAB
>
> AFAIK it needs to complete to
>
>     ~/tmp/foo$$HOME.txt
>
> otherwise the file name returned by `read-file-name` will be
>
>     ~/tmp/foo/home/monnier.txt

Ah, I see.  I didn't realize that read-file-name calls
substitute-in-file-name on the result; so a file name containing
literally "$HOME" would be erroneously expanded by my code, right now.
I'll add some tests for this case in the next version of my patch.

This TAB behavior doesn't help if I manually type $HOME though, right?
The $ is only automatically duplicated if you tab-complete.  So, if the
user manually types foo$HOME.txt then $HOME would be incorrectly
expanded.

So here's one idea: Since this only works if I tab-complete, that means
it only works for files which actually exist.  So, instead, I could have
read-file-name check if the file (e.g. "~/tmp/foo$HOME.txt") exists
before expanding environment variables; if the file exists, don't expand
environment variables.  This avoids the need to duplicate $s.

Do you think that might suffice?

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Wed, 16 Apr 2025 14:29:56 -0400

>> [ Also, I'm not sure it's worth the trouble anyway: dollars in file names
>>   are fairly rare, so having to double them in those rare cases where they
>>   occur is not a big deal.  In any case, it seems orthogonal to the
>>   problem at hand.  ]
> That's fair - so you think it might be OK to, basically, not support
> completion on file names containing substrings like "$HOME"?

Of course we support it, but we require the use of `$$HOME` to state
clearly which interpretation you want.

> This TAB behavior doesn't help if I manually type $HOME though, right?

Presumably if you type `$HOME` it means you want the value of the HOME
envvar.  If you want the file named `$HOME` you need to type `$$HOME`.

When the typing is done by the completion code, it's up to the
completion code to apply that same rule.

> So here's one idea: Since this only works if I tab-complete, that means
> it only works for files which actually exist.  So, instead, I could have
> read-file-name check if the file (e.g. "~/tmp/foo$HOME.txt") exists
> before expanding environment variables; if the file exists, don't expand
> environment variables.  This avoids the need to duplicate $s.

But then we'd need to add some way for the user to say "I really want
this $HOME to be expanded even though there is also a file by that funny
name".

[ In any case, this is not a question of completion but of
  `read-file-name`.  ]


        Stefan

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Wed, 16 Apr 2025 15:15:44 -0400

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

>>> [ Also, I'm not sure it's worth the trouble anyway: dollars in file names
>>>   are fairly rare, so having to double them in those rare cases where they
>>>   occur is not a big deal.  In any case, it seems orthogonal to the
>>>   problem at hand.  ]
>> That's fair - so you think it might be OK to, basically, not support
>> completion on file names containing substrings like "$HOME"?
>
> Of course we support it, but we require the use of `$$HOME` to state
> clearly which interpretation you want.

Ah, sure.  But my code supports completion on file names containing
"$HOME" without the user of "$$HOME", which seems even nicer.

>> This TAB behavior doesn't help if I manually type $HOME though, right?
>
> Presumably if you type `$HOME` it means you want the value of the HOME
> envvar.  If you want the file named `$HOME` you need to type `$$HOME`.
>
> When the typing is done by the completion code, it's up to the
> completion code to apply that same rule.

Sure, that's fair.  Though, a user might not know that they can type
$$HOME to get the file named '$HOME'; it's not that obvious.  It seems
like it would be nice to automatically DTRT, where possible, without
requiring them to know that.

>> So here's one idea: Since this only works if I tab-complete, that means
>> it only works for files which actually exist.  So, instead, I could have
>> read-file-name check if the file (e.g. "~/tmp/foo$HOME.txt") exists
>> before expanding environment variables; if the file exists, don't expand
>> environment variables.  This avoids the need to duplicate $s.
>
> But then we'd need to add some way for the user to say "I really want
> this $HOME to be expanded even though there is also a file by that funny
> name".

True.  Although, this situation seems likely to be quite rare.  Perhaps
we could support it by adding some new command
minibuffer-substitute-in-file-name or something, which explicitly
expands environment variables found in the current minibuffer.

Then read-file-name does the right thing automatically in every case
except this one, without the user needing to insert $$; and in this
case, the user can just use minibuffer-substitute-in-file-name
explicitly.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Wed, 16 Apr 2025 16:00:09 -0400

I suggest we focus on the immediate completion issue.
If you want to refine the treatment of $$, let's do that in a separate
bug report.


        Stefan


Spencer Baugh [2025-04-16 15:15:44] wrote:

> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>
>>>> [ Also, I'm not sure it's worth the trouble anyway: dollars in file names
>>>>   are fairly rare, so having to double them in those rare cases where they
>>>>   occur is not a big deal.  In any case, it seems orthogonal to the
>>>>   problem at hand.  ]
>>> That's fair - so you think it might be OK to, basically, not support
>>> completion on file names containing substrings like "$HOME"?
>>
>> Of course we support it, but we require the use of `$$HOME` to state
>> clearly which interpretation you want.
>
> Ah, sure.  But my code supports completion on file names containing
> "$HOME" without the user of "$$HOME", which seems even nicer.
>
>>> This TAB behavior doesn't help if I manually type $HOME though, right?
>>
>> Presumably if you type `$HOME` it means you want the value of the HOME
>> envvar.  If you want the file named `$HOME` you need to type `$$HOME`.
>>
>> When the typing is done by the completion code, it's up to the
>> completion code to apply that same rule.
>
> Sure, that's fair.  Though, a user might not know that they can type
> $$HOME to get the file named '$HOME'; it's not that obvious.  It seems
> like it would be nice to automatically DTRT, where possible, without
> requiring them to know that.
>
>>> So here's one idea: Since this only works if I tab-complete, that means
>>> it only works for files which actually exist.  So, instead, I could have
>>> read-file-name check if the file (e.g. "~/tmp/foo$HOME.txt") exists
>>> before expanding environment variables; if the file exists, don't expand
>>> environment variables.  This avoids the need to duplicate $s.
>>
>> But then we'd need to add some way for the user to say "I really want
>> this $HOME to be expanded even though there is also a file by that funny
>> name".
>
> True.  Although, this situation seems likely to be quite rare.  Perhaps
> we could support it by adding some new command
> minibuffer-substitute-in-file-name or something, which explicitly
> expands environment variables found in the current minibuffer.
>
> Then read-file-name does the right thing automatically in every case
> except this one, without the user needing to insert $$; and in this
> case, the user can just use minibuffer-substitute-in-file-name
> explicitly.

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Wed, 16 Apr 2025 16:20:20 -0400

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> I suggest we focus on the immediate completion issue.
> If you want to refine the treatment of $$, let's do that in a separate
> bug report.

OK, that's fair.

But to be clear, the issue is in completion-table-with-quoting, so it's
quite tied together with the treatment of $$.

My new version of completion--file-name-table fixes the bug by taking a
much simpler approach than completion-table-with-quoting, but that
approach depends on avoiding the need for $$.

I will try to think of another approach which is more similar to how
completion-table-with-quoting does things, but it will be more
complicated.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Wed, 16 Apr 2025 16:37:55 -0400

>> I suggest we focus on the immediate completion issue.
>> If you want to refine the treatment of $$, let's do that in a separate
>> bug report.
>
> OK, that's fair.
>
> But to be clear, the issue is in completion-table-with-quoting, so it's
> quite tied together with the treatment of $$.

Yes, I know.

> My new version of completion--file-name-table fixes the bug by taking
> a much simpler approach than completion-table-with-quoting, but that
> approach depends on avoiding the need for $$.

I think I missed some of that, then.  Can you clarify how they interact?

> I will try to think of another approach which is more similar to how
> completion-table-with-quoting does things, but it will be more
> complicated.

I thought all that's missing is to double the $$ in the text we append
during completion.


        Stefan

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, stephen.berman <at> gmx.net, monnier <at> iro.umontreal.ca
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Thu, 17 Apr 2025 09:00:04 +0300

> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Cc: 77718 <at> debbugs.gnu.org,  Eli Zaretskii <eliz <at> gnu.org>,
>    stephen.berman <at> gmx.net
> Date: Wed, 16 Apr 2025 15:15:44 -0400
> 
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> 
> >>> [ Also, I'm not sure it's worth the trouble anyway: dollars in file names
> >>>   are fairly rare, so having to double them in those rare cases where they
> >>>   occur is not a big deal.  In any case, it seems orthogonal to the
> >>>   problem at hand.  ]
> >> That's fair - so you think it might be OK to, basically, not support
> >> completion on file names containing substrings like "$HOME"?
> >
> > Of course we support it, but we require the use of `$$HOME` to state
> > clearly which interpretation you want.
> 
> Ah, sure.  But my code supports completion on file names containing
> "$HOME" without the user of "$$HOME", which seems even nicer.

IMO, it's confusing, because it's inconsistent with the rest of
Emacs's behavior regarding file names with embedded "$".

> >> This TAB behavior doesn't help if I manually type $HOME though, right?
> >
> > Presumably if you type `$HOME` it means you want the value of the HOME
> > envvar.  If you want the file named `$HOME` you need to type `$$HOME`.
> >
> > When the typing is done by the completion code, it's up to the
> > completion code to apply that same rule.
> 
> Sure, that's fair.  Though, a user might not know that they can type
> $$HOME to get the file named '$HOME'; it's not that obvious.  It seems
> like it would be nice to automatically DTRT, where possible, without
> requiring them to know that.

What do you mean by "where possible"?  The behavior should be
consistent across all Emacs interfaces that accept file names, so
"where possible" should be common to all of them.  It would be wrong
for some UIs to use rules and exhibit behaviors that are different
from other UIs in this regard, since then the users will have much
harder time knowing what is right and what to expect.

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Thu, 17 Apr 2025 12:39:48 -0400

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

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> I will try to think of another approach which is more similar to how
>> completion-table-with-quoting does things, but it will be more
>> complicated.
>
> I thought all that's missing is to double the $$ in the text we append
> during completion.

Yes, actually you're right, that turns out to be relatively simple.  And
now the quoting behavior doesn't change at all.  Updated patch attached.

The $ doubling also needs to happen in all-completions, not just
try-completion.  But, with sufficiently robust logic for figuring out
the completion boundaries, that's straightforward: just double the $ in
the newly added text.

[0001-Improve-env-var-handling-in-read-file-name.patch (text/x-patch, inline)]

From 6a8685edc22cba617ddced1f2b2e1b6c4e1c9de2 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 14 Apr 2025 16:01:38 -0400
Subject: [PATCH] Improve env var handling in read-file-name

Fix various bugs, including bug#77718, by rewriting the way file
name completion handles environment variable expansion.  Instead
of using completion-table-with-quoting to manipulate the string
being completed on, simply make the completion table itself
understand substitute-in-file-name.

Tests are updated: partial-completion now preserves unexpanded
environment variables.  However, partial-completion no longer
works across environment variables containing delimiters; that's
an acceptable sacrifice.

* lisp/minibuffer.el (completion--sifn-boundaries): Add.
(completion--file-name-table): Rewrite to use
substitute-in-file-name explicitly. (bug#77718)
* test/lisp/minibuffer-tests.el (completion-table-test-quoting):
Update.
---
 lisp/minibuffer.el            | 59 ++++++++++++++++++++++++++++++++---
 test/lisp/minibuffer-tests.el |  6 ++--
 2 files changed, 56 insertions(+), 9 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c0ccfc00ce5..8edeba1fba9 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3596,13 +3596,62 @@ completion--sifn-requote
           (setq qpos (1- qpos)))
         (cons qpos #'minibuffer-maybe-quote-filename)))))
 
-(defalias 'completion--file-name-table
-  (completion-table-with-quoting #'completion-file-name-table
-                                 #'substitute-in-file-name
-                                 #'completion--sifn-requote)
+(defun completion--sifn-boundaries (string table pred suffix)
+  "Return completion boundaries on file name STRING.
+
+Runs `substitute-in-file-name' on STRING first, but returns completion
+boundaries for the original string."
+  (let* (;; Calculate the completion boundaries without expanding env vars.
+         (unquoted
+          (let ((process-environment nil)) (substitute-in-file-name string)))
+         (bounds (completion-boundaries unquoted table pred suffix))
+         ;; This is the part of UNQUOTED inside the completion boundaries.
+         (unquoted-suffix (substring unquoted (car bounds)))
+         ;; Each $ in UNQUOTED-SUFFIX is either $ or $$ in STRING.
+         (regex
+          (concat (replace-regexp-in-string "\\$" "\\$\\$?" unquoted-suffix t t) "$")))
+    (cl-assert (string-match regex string) t)
+    (cons (match-beginning 0) (cdr bounds))))
+
+(defun completion--file-name-table (orig pred action)
   "Internal subroutine for `read-file-name'.  Do not call this.
 This is a completion table for file names, like `completion-file-name-table'
-except that it passes the file name through `substitute-in-file-name'.")
+except that it passes the file name through `substitute-in-file-name'."
+  (let ((table #'completion-file-name-table))
+    (if (eq (car-safe action) 'boundaries)
+        (cons 'boundaries (completion--sifn-boundaries orig table pred (cdr action)))
+      (let* ((sifned (substitute-in-file-name orig))
+             (result
+              (let ((completion-regexp-list
+                     ;; Regexps are matched against the real file names after
+                     ;; expansion, so regexps containing $ won't work.  Drop
+                     ;; them; we'll return more completions, but callers need to
+                     ;; handle that anyway.
+                     (cl-remove-if (lambda (regexp) (string-search "$" regexp))
+                                   completion-regexp-list)))
+                (complete-with-action action table sifned pred))))
+        (cond
+         ((null action)                 ; try-completion
+          (if (stringp result)
+              ;; Extract the newly added text, quote any dollar signs, and
+              ;; append it to ORIG.
+              (let ((new-text (substring result (length sifned))))
+                (concat orig (minibuffer--double-dollars new-text)))
+            result))
+         ((eq action t)                 ; all-completions
+          (mapcar
+           (let ((orig-prefix
+                  (substring orig (car (completion--sifn-boundaries orig table pred ""))))
+                 (sifned-prefix-length
+                  (- (length sifned)
+                     (car (completion-boundaries sifned table pred "")))))
+             ;; Extract the newly added text, quote any dollar signs, and append
+             ;; it to the part of ORIG inside the completion boundaries.
+             (lambda (compl)
+               (let ((new-text (substring compl sifned-prefix-length)))
+                 (concat orig-prefix (minibuffer--double-dollars new-text)))))
+           result))
+         (t result))))))
 
 (defalias 'read-file-name-internal
   (completion-table-in-turn #'completion--embedded-envvar-table
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 59ac5ab9578..9e28077026c 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -91,7 +91,7 @@ completion-table-subvert-test
 
 (ert-deftest completion-table-test-quoting ()
   (let ((process-environment
-         `("CTTQ1=ed" "CTTQ2=et/" ,@process-environment))
+         `("CTTQ1=ed" ,@process-environment))
         (default-directory (ert-resource-directory)))
     (pcase-dolist (`(,input ,output)
                    '(
@@ -101,10 +101,8 @@ completion-table-test-quoting
                      ("data/m-cttq$$t" "data/minibuffer-test-cttq$$tion")
                      ;; Test that env-vars are preserved.
                      ("lisp/c${CTTQ1}et/se-u" "lisp/c${CTTQ1}et/semantic-utest")
-                     ("lisp/ced${CTTQ2}se-u" "lisp/ced${CTTQ2}semantic-utest")
                      ;; Test that env-vars don't prevent partial-completion.
-                     ;; FIXME: Ideally we'd like to keep the ${CTTQ}!
-                     ("lis/c${CTTQ1}/se-u" "lisp/cedet/semantic-utest")
+                     ("lis/c${CTTQ1}/se-u" "lisp/c${CTTQ1}et/semantic-utest")
                      ))
       (should (equal (completion-try-completion input
                                                 #'completion--file-name-table
-- 
2.39.3

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Fri, 18 Apr 2025 11:59:30 -0400

> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index c0ccfc00ce5..8edeba1fba9 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3596,13 +3596,62 @@ completion--sifn-requote
>            (setq qpos (1- qpos)))
>          (cons qpos #'minibuffer-maybe-quote-filename)))))
>  
> -(defalias 'completion--file-name-table
> -  (completion-table-with-quoting #'completion-file-name-table
> -                                 #'substitute-in-file-name
> -                                 #'completion--sifn-requote)
> +(defun completion--sifn-boundaries (string table pred suffix)
> +  "Return completion boundaries on file name STRING.
> +
> +Runs `substitute-in-file-name' on STRING first, but returns completion
> +boundaries for the original string."
> +  (let* (;; Calculate the completion boundaries without expanding env vars.
> +         (unquoted
> +          (let ((process-environment nil)) (substitute-in-file-name string)))
> +         (bounds (completion-boundaries unquoted table pred suffix))
> +         ;; This is the part of UNQUOTED inside the completion boundaries.
> +         (unquoted-suffix (substring unquoted (car bounds)))
> +         ;; Each $ in UNQUOTED-SUFFIX is either $ or $$ in STRING.
> +         (regex
> +          (concat (replace-regexp-in-string "\\$" "\\$\\$?" unquoted-suffix t t) "$")))
> +    (cl-assert (string-match regex string) t)
> +    (cons (match-beginning 0) (cdr bounds))))

What makes us think that the `cl-assert` will always be satisfied?
Won't it fail in `/blabla/foo-$BAR.c` when `BAR=bash` ?

> @@ -101,10 +101,8 @@ completion-table-test-quoting
>                       ("data/m-cttq$$t" "data/minibuffer-test-cttq$$tion")
>                       ;; Test that env-vars are preserved.
>                       ("lisp/c${CTTQ1}et/se-u" "lisp/c${CTTQ1}et/semantic-utest")
> -                     ("lisp/ced${CTTQ2}se-u" "lisp/ced${CTTQ2}semantic-utest")

Why do we remove this test?  Maybe it's OK that it doesn't return the
same value (e.g. that it behaves less well), but if so, we should
probably keep this commented out with an explanation of what's going on.


        Stefan

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>, stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 22 Apr 2025 14:52:16 -0400

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

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
>> index c0ccfc00ce5..8edeba1fba9 100644
>> --- a/lisp/minibuffer.el
>> +++ b/lisp/minibuffer.el
>> @@ -3596,13 +3596,62 @@ completion--sifn-requote
>>            (setq qpos (1- qpos)))
>>          (cons qpos #'minibuffer-maybe-quote-filename)))))
>>  
>> -(defalias 'completion--file-name-table
>> -  (completion-table-with-quoting #'completion-file-name-table
>> -                                 #'substitute-in-file-name
>> -                                 #'completion--sifn-requote)
>> +(defun completion--sifn-boundaries (string table pred suffix)
>> +  "Return completion boundaries on file name STRING.
>> +
>> +Runs `substitute-in-file-name' on STRING first, but returns completion
>> +boundaries for the original string."
>> +  (let* (;; Calculate the completion boundaries without expanding env vars.
>> +         (unquoted
>> +          (let ((process-environment nil)) (substitute-in-file-name string)))
>> +         (bounds (completion-boundaries unquoted table pred suffix))
>> +         ;; This is the part of UNQUOTED inside the completion boundaries.
>> +         (unquoted-suffix (substring unquoted (car bounds)))
>> +         ;; Each $ in UNQUOTED-SUFFIX is either $ or $$ in STRING.
>> +         (regex
>> +          (concat (replace-regexp-in-string "\\$" "\\$\\$?" unquoted-suffix t t) "$")))
>> +    (cl-assert (string-match regex string) t)
>> +    (cons (match-beginning 0) (cdr bounds))))
>
> What makes us think that the `cl-assert` will always be satisfied?
> Won't it fail in `/blabla/foo-$BAR.c` when `BAR=bash` ?

Because I bound process-environment to nil around the
substitute-in-file-name call, with the goal of preventing environment
variable expansion.

But actually there's a simpler and better way to do that: just duplicate
all the $s to $$, then substitute-in-file-name turns all the $$ back to
$ without expanding environment variables.  So I did that in the
attached updated patch.

>> @@ -101,10 +101,8 @@ completion-table-test-quoting
>>                       ("data/m-cttq$$t" "data/minibuffer-test-cttq$$tion")
>>                       ;; Test that env-vars are preserved.
>>                       ("lisp/c${CTTQ1}et/se-u" "lisp/c${CTTQ1}et/semantic-utest")
>> -                     ("lisp/ced${CTTQ2}se-u" "lisp/ced${CTTQ2}semantic-utest")
>
> Why do we remove this test?  Maybe it's OK that it doesn't return the
> same value (e.g. that it behaves less well), but if so, we should
> probably keep this commented out with an explanation of what's going on.

Fixed - actually, this test doesn't need to change at all, it still
works, just an oversight on my part.

[0001-Improve-env-var-handling-in-read-file-name.patch (text/x-patch, inline)]

From c2773fbc301ef0b6eb34b84da181e916d563c490 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 14 Apr 2025 16:01:38 -0400
Subject: [PATCH] Improve env var handling in read-file-name

Fix various bugs, including bug#77718, by rewriting the way file
name completion handles environment variable expansion.  Instead
of using completion-table-with-quoting to manipulate the string
being completed on, simply make the completion table itself
understand substitute-in-file-name.

Tests are updated: partial-completion now preserves unexpanded
environment variables.  However, partial-completion no longer
works across environment variables containing delimiters; that's
an acceptable sacrifice.

* lisp/minibuffer.el (completion--sifn-boundaries): Add.
(completion--file-name-table): Rewrite to use
substitute-in-file-name explicitly. (bug#77718)
* test/lisp/minibuffer-tests.el (completion-table-test-quoting):
Update.
---
 lisp/minibuffer.el            | 63 ++++++++++++++++++++++++++++++++---
 test/lisp/minibuffer-tests.el |  3 +-
 2 files changed, 59 insertions(+), 7 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index c0ccfc00ce5..efe2eba9539 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3596,13 +3596,66 @@ completion--sifn-requote
           (setq qpos (1- qpos)))
         (cons qpos #'minibuffer-maybe-quote-filename)))))
 
-(defalias 'completion--file-name-table
-  (completion-table-with-quoting #'completion-file-name-table
-                                 #'substitute-in-file-name
-                                 #'completion--sifn-requote)
+(defun completion--sifn-boundaries (string table pred suffix)
+  "Return completion boundaries on file name STRING.
+
+Runs `substitute-in-file-name' on STRING first, but returns completion
+boundaries for the original string."
+  ;; We want to compute the start boundary on the result of
+  ;; `substitute-in-file-name' (since that's what we use for actual completion),
+  ;; and then transform that into an offset in STRING instead.  We can't do this
+  ;; if we expand environment variables, so double the $s to prevent that.
+  (let* ((doubled-string (replace-regexp-in-string "\\$" "$$" string t t))
+         ;; sifn will change $$ back into $, so the result is a suffix of STRING
+         ;; (in fact, it's the last absolute file name in STRING).
+         (last-file-name (substitute-in-file-name doubled-string))
+         (bounds (completion-boundaries last-file-name table pred suffix)))
+    (cl-assert (string-suffix-p last-file-name string) t)
+    ;; BOUNDS contains the start boundary in LAST-FILE-NAME; adjust it to be an
+    ;; offset in STRING instead.
+    (cons (+ (- (length string) (length last-file-name)) (car bounds))
+          ;; No special processing happens on SUFFIX and the end boundary.
+          (cdr bounds))))
+
+(defun completion--file-name-table (orig pred action)
   "Internal subroutine for `read-file-name'.  Do not call this.
 This is a completion table for file names, like `completion-file-name-table'
-except that it passes the file name through `substitute-in-file-name'.")
+except that it passes the file name through `substitute-in-file-name'."
+  (let ((table #'completion-file-name-table))
+    (if (eq (car-safe action) 'boundaries)
+        (cons 'boundaries (completion--sifn-boundaries orig table pred (cdr action)))
+      (let* ((sifned (substitute-in-file-name orig))
+             (result
+              (let ((completion-regexp-list
+                     ;; Regexps are matched against the real file names after
+                     ;; expansion, so regexps containing $ won't work.  Drop
+                     ;; them; we'll return more completions, but callers need to
+                     ;; handle that anyway.
+                     (cl-remove-if (lambda (regexp) (string-search "$" regexp))
+                                   completion-regexp-list)))
+                (complete-with-action action table sifned pred))))
+        (cond
+         ((null action)                 ; try-completion
+          (if (stringp result)
+              ;; Extract the newly added text, quote any dollar signs, and
+              ;; append it to ORIG.
+              (let ((new-text (substring result (length sifned))))
+                (concat orig (minibuffer--double-dollars new-text)))
+            result))
+         ((eq action t)                 ; all-completions
+          (mapcar
+           (let ((orig-prefix
+                  (substring orig (car (completion--sifn-boundaries orig table pred ""))))
+                 (sifned-prefix-length
+                  (- (length sifned)
+                     (car (completion-boundaries sifned table pred "")))))
+             ;; Extract the newly added text, quote any dollar signs, and append
+             ;; it to the part of ORIG inside the completion boundaries.
+             (lambda (compl)
+               (let ((new-text (substring compl sifned-prefix-length)))
+                 (concat orig-prefix (minibuffer--double-dollars new-text)))))
+           result))
+         (t result))))))
 
 (defalias 'read-file-name-internal
   (completion-table-in-turn #'completion--embedded-envvar-table
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 57c7cc2ae1a..00a2055e602 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -103,8 +103,7 @@ completion-table-test-quoting
                      ("lisp/c${CTTQ1}et/se-u" "lisp/c${CTTQ1}et/semantic-utest")
                      ("lisp/ced${CTTQ2}se-u" "lisp/ced${CTTQ2}semantic-utest")
                      ;; Test that env-vars don't prevent partial-completion.
-                     ;; FIXME: Ideally we'd like to keep the ${CTTQ}!
-                     ("lis/c${CTTQ1}/se-u" "lisp/cedet/semantic-utest")
+                     ("lis/c${CTTQ1}/se-u" "lisp/c${CTTQ1}et/semantic-utest")
                      ))
       (should (equal (completion-try-completion input
                                                 #'completion--file-name-table
-- 
2.39.3

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: Eli Zaretskii <eliz <at> gnu.org>, 77718-done <at> debbugs.gnu.org,
 stephen.berman <at> gmx.net
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Mon, 28 Apr 2025 14:01:18 -0400

> But actually there's a simpler and better way to do that: just duplicate
> all the $s to $$, then substitute-in-file-name turns all the $$ back to
> $ without expanding environment variables.  So I did that in the
> attached updated patch.

LGTM, pushed to `master`.


        Stefan

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Spencer Baugh <sbaugh <at> janestreet.com>, Eli Zaretskii <eliz <at> gnu.org>,
 77718-done <at> debbugs.gnu.org
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 29 Apr 2025 12:19:11 +0200

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

On Mon, 28 Apr 2025 14:01:18 -0400 Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>> But actually there's a simpler and better way to do that: just duplicate
>> all the $s to $$, then substitute-in-file-name turns all the $$ back to
>> $ without expanding environment variables.  So I did that in the
>> attached updated patch.
>
> LGTM, pushed to `master`.

This patch causes two regressions wrt emacs-30.  One I pointed out
upthread (<87a58hal6w.fsf <at> gmx.net>,
<https://lists.gnu.org/archive/html/bug-gnu-emacs/2025-04/msg00884.html>).
For this, the following patch fixes the regression according to my
testing:

[Message part 2 (text/x-patch, attachment)]

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

For the other regression, here is a reproducer:

0. mkdir /tmp/{Test,retest}
1. emacs -Q --eval "(custom-set-variables                        \
   '(completion-category-overrides '((file (styles substring)))) \
   '(read-file-name-completion-ignore-case t))"
2. Type `C-x d /tmp/tes TAB': this completes to /tmp/test/
3. Type TAB again: Emacs dings and shows "[no match]"

If you repeat this recipe in emacs-30, then after the TAB in step 3, this
completion is shown: /tmp/Test/. and typing TAB again pops up the
*Completions* buffer displaying the two completions ../ and ./

While the result in emacs-30 seems suboptimal, since it omits the
possible completion /tmp/retest/, still it's better than "[no match]".

A workaround for both emacs-30 and master is to enter at step 2 `C-x d
/tmp/*tes TAB'. This shows the completion /tmp/*test/, then hitting TAB
again shows /tmp/*test/., and hitting TAB a third time pops up the
*Completions* buffer showing the four completions Test/../, Test/./,
retest/../, and retest/./

Steve Berman

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 77718-done <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Thu, 01 May 2025 13:59:48 -0400

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

Stephen Berman <stephen.berman <at> gmx.net> writes:
> For the other regression, here is a reproducer:
>
> 0. mkdir /tmp/{Test,retest}
> 1. emacs -Q --eval "(custom-set-variables                        \
>    '(completion-category-overrides '((file (styles substring)))) \
>    '(read-file-name-completion-ignore-case t))"
> 2. Type `C-x d /tmp/tes TAB': this completes to /tmp/test/
> 3. Type TAB again: Emacs dings and shows "[no match]"
>
> If you repeat this recipe in emacs-30, then after the TAB in step 3, this
> completion is shown: /tmp/Test/. and typing TAB again pops up the
> *Completions* buffer displaying the two completions ../ and ./
>
> While the result in emacs-30 seems suboptimal, since it omits the
> possible completion /tmp/retest/, still it's better than "[no match]".
>
> A workaround for both emacs-30 and master is to enter at step 2 `C-x d
> /tmp/*tes TAB'. This shows the completion /tmp/*test/, then hitting TAB
> again shows /tmp/*test/., and hitting TAB a third time pops up the
> *Completions* buffer showing the four completions Test/../, Test/./,
> retest/../, and retest/./
>
> Steve Berman

Thanks for the prompt and detailed report.  Does the attached patch fix
this issue for you?

[0001-Fix-completion-ignore-case-with-completion-file-name.patch (text/x-patch, inline)]

From 2619db58b2f3bdaeed0ea077b3292d93621e15d8 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Thu, 1 May 2025 13:56:37 -0400
Subject: [PATCH] Fix completion-ignore-case with completion--file-name-table

509cbe1c35b3d "Improve env var handling in read-file-name"
caused try-completion and all-completion operations with
completion--file-name-table to no longer update the case of text
which was already present in the input string.  That is,
completions would be returned ignoring case, but the completions
would have letter-casing which matched the input string rather
than matching the actual file names.

Fix this by always using text from the file name completions
whenever it might have changed case.  Also, fix a related bug
where mixing completion-ignore-case and expanding environment
variables would cause `read-file-name' to return the wrong
result; do this by suppressing completion-ignore-case in the
problematic case.

* lisp/minibuffer.el (completion--file-name-table): Use text
from the completions, not the input string.  (bug#77718)
* test/lisp/minibuffer-tests.el (completion-table-test-quoting):
Test with completion-ignore-case as well.
---
 lisp/minibuffer.el            | 42 +++++++++++++++++++++++------------
 test/lisp/minibuffer-tests.el | 18 ++++++++++++++-
 2 files changed, 45 insertions(+), 15 deletions(-)

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 7b2b986aa1d..e35db9dad64 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3597,8 +3597,18 @@ completion--file-name-table
     (if (eq (car-safe action) 'boundaries)
         (cons 'boundaries (completion--sifn-boundaries orig table pred (cdr action)))
       (let* ((sifned (substitute-in-file-name orig))
+             (orig-start (car (completion--sifn-boundaries orig table pred "")))
+             (sifned-start (car (completion-boundaries sifned table pred "")))
+             (orig-in-bounds (substring orig orig-start))
+             (sifned-in-bounds (substring sifned sifned-start))
+             (did-expansion-in-bounds (not (string-equal orig-in-bounds sifned-in-bounds)))
              (result
-              (let ((completion-regexp-list
+              (let ((completion-ignore-case
+                     ;; If we expanded an environment variable in the completion
+                     ;; bounds, don't ignore-case, otherwise `read-file-name'
+                     ;; can return the wrong filename.
+                     (and (not did-expansion-in-bounds) completion-ignore-case))
+                    (completion-regexp-list
                      ;; Regexps are matched against the real file names after
                      ;; expansion, so regexps containing $ won't work.  Drop
                      ;; them; we'll return more completions, but callers need to
@@ -3606,26 +3616,30 @@ completion--file-name-table
                      (seq-remove (lambda (regexp) (string-search "$" regexp))
                                  completion-regexp-list)))
                 (complete-with-action action table sifned pred))))
+        ;; For each completion, quote dollar signs in newly added text.  If sifn
+        ;; changed text within the completion boundaries, replace that with text
+        ;; from ORIG; otherwise, use text from the completion (it may differ if
+        ;; `completion-ignore-case' is non-nil)
         (cond
          ((null action)                 ; try-completion
           (if (stringp result)
-              ;; Extract the newly added text, quote any dollar signs, and
-              ;; append it to ORIG.
               (let ((new-text (substring result (length sifned))))
-                (concat orig (minibuffer--double-dollars new-text)))
+                ;; Unconditionally replace the text outside the
+                ;; completion boundaries with text from ORIG.
+                (concat (substring orig nil orig-start)
+                        (if did-expansion-in-bounds
+                            orig-in-bounds
+                          (substring result sifned-start (length sifned)))
+                        (minibuffer--double-dollars new-text)))
             result))
          ((eq action t)                 ; all-completions
           (mapcar
-           (let ((orig-prefix
-                  (substring orig (car (completion--sifn-boundaries orig table pred ""))))
-                 (sifned-prefix-length
-                  (- (length sifned)
-                     (car (completion-boundaries sifned table pred "")))))
-             ;; Extract the newly added text, quote any dollar signs, and append
-             ;; it to the part of ORIG inside the completion boundaries.
-             (lambda (compl)
-               (let ((new-text (substring compl sifned-prefix-length)))
-                 (concat orig-prefix (minibuffer--double-dollars new-text)))))
+           (lambda (compl)
+             (let ((new-text (substring compl (length sifned-in-bounds))))
+               (concat (if did-expansion-in-bounds
+                           orig-in-bounds
+                         (substring compl nil (length sifned-in-bounds)))
+                       (minibuffer--double-dollars new-text))))
            result))
          (t result))))))
 
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index bed797bdb14..5e013668a2e 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -108,7 +108,23 @@ completion-table-test-quoting
       (should (equal (completion-try-completion input
                                                 #'completion--file-name-table
                                                 nil (length input))
-                     (cons output (length output)))))))
+                     (cons output (length output)))))
+    ;; Everything also works with `completion-ignore-case'.
+    (let ((completion-ignore-case t))
+      (pcase-dolist (`(,input ,output)
+                     '(
+                       ("data/M-CTTQ" "data/minibuffer-test-cttq$$tion")
+                       ("data/M-CTTQ$$t" "data/minibuffer-test-cttq$$tion")
+                       ("lisp/c${CTTQ1}et/SE-U" "lisp/c${CTTQ1}et/semantic-utest")
+                       ;; `completion-ignore-case' is suppressed when an
+                       ;; environment variable is in the completion bounds.
+                       ("lisp/ced${CTTQ2}SE-U" nil)
+                       ("lis/c${CTTQ1}/SE-U" "lisp/c${CTTQ1}et/semantic-utest")
+                       ))
+        (should (equal (car (completion-try-completion input
+                                                       #'completion--file-name-table
+                                                       nil (length input)))
+                       output))))))
 
 (ert-deftest completion--insert-strings-faces ()
   (with-temp-buffer
-- 
2.39.3

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 77718-done <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Fri, 02 May 2025 10:30:15 +0200

On Thu, 01 May 2025 13:59:48 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stephen Berman <stephen.berman <at> gmx.net> writes:
>> For the other regression, here is a reproducer:
>>
>> 0. mkdir /tmp/{Test,retest}
>> 1. emacs -Q --eval "(custom-set-variables                        \
>>    '(completion-category-overrides '((file (styles substring)))) \
>>    '(read-file-name-completion-ignore-case t))"
>> 2. Type `C-x d /tmp/tes TAB': this completes to /tmp/test/
>> 3. Type TAB again: Emacs dings and shows "[no match]"
>>
>> If you repeat this recipe in emacs-30, then after the TAB in step 3, this
>> completion is shown: /tmp/Test/. and typing TAB again pops up the
>> *Completions* buffer displaying the two completions ../ and ./
>>
>> While the result in emacs-30 seems suboptimal, since it omits the
>> possible completion /tmp/retest/, still it's better than "[no match]".
>>
>> A workaround for both emacs-30 and master is to enter at step 2 `C-x d
>> /tmp/*tes TAB'. This shows the completion /tmp/*test/, then hitting TAB
>> again shows /tmp/*test/., and hitting TAB a third time pops up the
>> *Completions* buffer showing the four completions Test/../, Test/./,
>> retest/../, and retest/./
>>
>> Steve Berman
>
> Thanks for the prompt and detailed report.  Does the attached patch fix
> this issue for you?

Yes, with that patch my reproducer now gives the same results on master
as on emacs-30, i.e., the patch fixes the regression; thanks!

What about the other regression I mentioned?  Have you looked at that
and tried the fix for it I included?  If there are no objections to that
patch, I'd like to install it on master.  Or if there are problems with
it, I'd welcome a better fix.

Steve Berman

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 77718-done <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50;
 completion styles substring and flex are broken
Date: Fri, 2 May 2025 08:26:44 -0400

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

On Fri, May 2, 2025, 4:30 AM Stephen Berman <stephen.berman <at> gmx.net> wrote:

> On Thu, 01 May 2025 13:59:48 -0400 Spencer Baugh <sbaugh <at> janestreet.com>
> wrote:
>
> > Stephen Berman <stephen.berman <at> gmx.net> writes:
> >> For the other regression, here is a reproducer:
> >>
> >> 0. mkdir /tmp/{Test,retest}
> >> 1. emacs -Q --eval "(custom-set-variables                        \
> >>    '(completion-category-overrides '((file (styles substring)))) \
> >>    '(read-file-name-completion-ignore-case t))"
> >> 2. Type `C-x d /tmp/tes TAB': this completes to /tmp/test/
> >> 3. Type TAB again: Emacs dings and shows "[no match]"
> >>
> >> If you repeat this recipe in emacs-30, then after the TAB in step 3,
> this
> >> completion is shown: /tmp/Test/. and typing TAB again pops up the
> >> *Completions* buffer displaying the two completions ../ and ./
> >>
> >> While the result in emacs-30 seems suboptimal, since it omits the
> >> possible completion /tmp/retest/, still it's better than "[no match]".
> >>
> >> A workaround for both emacs-30 and master is to enter at step 2 `C-x d
> >> /tmp/*tes TAB'. This shows the completion /tmp/*test/, then hitting TAB
> >> again shows /tmp/*test/., and hitting TAB a third time pops up the
> >> *Completions* buffer showing the four completions Test/../, Test/./,
> >> retest/../, and retest/./
> >>
> >> Steve Berman
> >
> > Thanks for the prompt and detailed report.  Does the attached patch fix
> > this issue for you?
>
> Yes, with that patch my reproducer now gives the same results on master
> as on emacs-30, i.e., the patch fixes the regression; thanks!
>

Great! It should be good to install if no one has any objections


> What about the other regression I mentioned?  Have you looked at that
> and tried the fix for it I included?  If there are no objections to that
> patch, I'd like to install it on master.  Or if there are problems with
> it, I'd welcome a better fix.
>

That fix is not right and would cause other issues.  I will look at it
soon; it's lower priority since completion still works fine.  Part of what
is needed is just to update rfn-eshadow.

>

[Message part 2 (text/html, inline)]

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 77718-done <at> debbugs.gnu.org, Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Fri, 02 May 2025 14:44:04 +0200

On Fri, 2 May 2025 08:26:44 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> On Fri, May 2, 2025, 4:30 AM Stephen Berman <stephen.berman <at> gmx.net> wrote:

>> What about the other regression I mentioned?  Have you looked at that
>> and tried the fix for it I included?  If there are no objections to that
>> patch, I'd like to install it on master.  Or if there are problems with
>> it, I'd welcome a better fix.
>>
>
> That fix is not right and would cause other issues.

When you have the opportunity, please say what those issues are; I have
been using my patch since the first version of your changes that
introduced this regression and don't recall any issues with it with my
setup, but admittedly I haven't extensive testing with -Q or other
configurations.

Steve Berman

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stephen Berman <stephen.berman <at> gmx.net>, 77718-done <at> debbugs.gnu.org
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Fri, 02 May 2025 12:04:57 -0400

> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index 7b2b986aa1d..e35db9dad64 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -3597,8 +3597,18 @@ completion--file-name-table
>      (if (eq (car-safe action) 'boundaries)
>          (cons 'boundaries (completion--sifn-boundaries orig table pred (cdr action)))
>        (let* ((sifned (substitute-in-file-name orig))
> +             (orig-start (car (completion--sifn-boundaries orig table pred "")))
> +             (sifned-start (car (completion-boundaries sifned table pred "")))
> +             (orig-in-bounds (substring orig orig-start))
> +             (sifned-in-bounds (substring sifned sifned-start))
> +             (did-expansion-in-bounds (not (string-equal orig-in-bounds sifned-in-bounds)))

Doesn't this set `did-expansion-in-bounds` to non-nil if there was a $$
in `orig`?

>               (result
> -              (let ((completion-regexp-list
> +              (let ((completion-ignore-case
> +                     ;; If we expanded an environment variable in the completion
> +                     ;; bounds, don't ignore-case, otherwise `read-file-name'
> +                     ;; can return the wrong filename.
> +                     (and (not did-expansion-in-bounds) completion-ignore-case))

AFAICT we're not "doing The Right Thing" here but falling back to
a suboptimal behavior that favors not returning bogus completions at the
cost of missing some completions.  And we do this because doing TRT is
a lot more complicated.

I'm not opposed to this trade-off, but the comment should make it more
clear.  And it should come with a good example of the wrong thing we're
trying to avoid.

> +        ;; For each completion, quote dollar signs in newly added text.  If sifn
> +        ;; changed text within the completion boundaries, replace that with text
> +        ;; from ORIG; otherwise, use text from the completion (it may differ if
> +        ;; `completion-ignore-case' is non-nil)

Here also, clarify that we choose between those two because just don't
want to write the code that correctly merges the capitalization changes
back into the non-env-var part of the original string.  An example is
probably the easier way to illustrate the difficulty.


        Stefan

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stephen Berman <stephen.berman <at> gmx.net>, 77718-done <at> debbugs.gnu.org
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 06 May 2025 11:39:00 -0400

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
>> index 7b2b986aa1d..e35db9dad64 100644
>> --- a/lisp/minibuffer.el
>> +++ b/lisp/minibuffer.el
>> @@ -3597,8 +3597,18 @@ completion--file-name-table
>>      (if (eq (car-safe action) 'boundaries)
>>          (cons 'boundaries (completion--sifn-boundaries orig table pred (cdr action)))
>>        (let* ((sifned (substitute-in-file-name orig))
>> +             (orig-start (car (completion--sifn-boundaries orig table pred "")))
>> +             (sifned-start (car (completion-boundaries sifned table pred "")))
>> +             (orig-in-bounds (substring orig orig-start))
>> +             (sifned-in-bounds (substring sifned sifned-start))
>> +             (did-expansion-in-bounds (not (string-equal orig-in-bounds sifned-in-bounds)))
>
> Doesn't this set `did-expansion-in-bounds` to non-nil if there was a $$
> in `orig`?

Good point, I didn't think about that.  That's probably not
acceptable... fixed in the latest version at the end.

>>               (result
>> -              (let ((completion-regexp-list
>> +              (let ((completion-ignore-case
>> +                     ;; If we expanded an environment variable in the completion
>> +                     ;; bounds, don't ignore-case, otherwise `read-file-name'
>> +                     ;; can return the wrong filename.
>> +                     (and (not did-expansion-in-bounds) completion-ignore-case))
>
> AFAICT we're not "doing The Right Thing" here but falling back to
> a suboptimal behavior that favors not returning bogus completions at the
> cost of missing some completions.  And we do this because doing TRT is
> a lot more complicated.
>
> I'm not opposed to this trade-off, but the comment should make it more
> clear.  And it should come with a good example of the wrong thing we're
> trying to avoid.

Yes, that's true.  I'll try to include a nice example in later versions.

>> +        ;; For each completion, quote dollar signs in newly added text.  If sifn
>> +        ;; changed text within the completion boundaries, replace that with text
>> +        ;; from ORIG; otherwise, use text from the completion (it may differ if
>> +        ;; `completion-ignore-case' is non-nil)
>
> Here also, clarify that we choose between those two because just don't
> want to write the code that correctly merges the capitalization changes
> back into the non-env-var part of the original string.  An example is
> probably the easier way to illustrate the difficulty.

That's fair.

Though to be clear, at this point the issue isn't really the
capitalization changes.  I have what I think is a pretty much correct
approach for that, where we just give up on preserving the environment
variables if the capitalization changes can't be merged into the
original string.

That approach works great if the text inside the completion boundaries
in ORIG (the original string) "matches up" with the text inside the
completion boundaries in SIFNED (the string after s-i-f-n).

But, when the last component of ORIG contains an environment variable
which contains a slash, then the completion boundaries on ORIG and
SIFNED don't "match up".  The completions returned from the underlying
table will be for the boundaries on SIFNED, and need to be adjusted for
the boundaries on ORIG (by adding additional text in front), since those
are the boundaries our table returns.

e.g., supposing SLASH_B=/b
orig: /foo/bar${SLASH_B}az
orig split on the completion boundaries we returned: /foo/ bar${SLASH_B}az
sifned: /foo/bar/baz
sifned split on the underlying table's completion boundaries: /foo/bar/ baz

underlying table returns completions like:
baz1 baz2

we need to return completions like:
bar${SLASH_B}az1 bar${SLASH_B}az2

This is easy if we ignore case completely, and that's what we do in the
version currently in the master branch: Simply replace "baz" (the text
in the underlying table's completion boundaries) with "bar${SLASH_B}az"
(the text in our completion in each completion, and we're good.

But if we want to preserve case, then even if we don't care about
preserving environment variables at all, this becomes difficult.

e.g. if the underlying table returns completions like:
baz1 Baz2

Suppose we're okay with returning completions like:
bar/baz1 bar/Baz2

It's still unclear how to compute that "bar/" is the text which we
should prefer in front of each completion.

It's hard to even figure out that this ${SLASH_B} scenario is happening; I
haven't yet thought of a robust way to figure out that there's a /
inside the completion boundaries in ORIG.

So instead I'm trying to side-step this problem by identifying the
simple common scenario where the text inside the completion boundaries
doesn't contain any environment variables, and then behaving right in
that scenario, and just blindly preserving environment variables
heedless of case when not in that scenario.  Here's an attempt at that,
do you think it might be fine?

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 7b2b986aa1d..6f9e6c67541 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -3597,6 +3597,16 @@ completion--file-name-table
     (if (eq (car-safe action) 'boundaries)
         (cons 'boundaries (completion--sifn-boundaries orig table pred (cdr action)))
       (let* ((sifned (substitute-in-file-name orig))
+             (orig-start (car (completion--sifn-boundaries orig table pred "")))
+             (sifned-start (car (completion-boundaries sifned table pred "")))
+             (orig-in-bounds (substring orig orig-start))
+             (sifned-in-bounds (substring sifned sifned-start))
+             (only-need-double-dollars
+              ;; If true, sifn only un-doubled $s in ORIG, so we can fix a
+              ;; completion to match ORIG by just doubling $s again.  This
+              ;; preserves more text from the completion, behaving better with
+              ;; non-nil `completion-ignore-case'.
+              (string-equal orig-in-bounds (minibuffer--double-dollars sifned-in-bounds)))
              (result
               (let ((completion-regexp-list
                      ;; Regexps are matched against the real file names after
@@ -3611,21 +3621,21 @@ completion--file-name-table
           (if (stringp result)
               ;; Extract the newly added text, quote any dollar signs, and
               ;; append it to ORIG.
-              (let ((new-text (substring result (length sifned))))
-                (concat orig (minibuffer--double-dollars new-text)))
+              (if only-need-double-dollars
+                  (concat (substring orig nil orig-start)
+                          (minibuffer--double-dollars (substring result sifned-start)))
+                (let ((new-text (substring result (length sifned))))
+                  (concat orig (minibuffer--double-dollars new-text))))
             result))
          ((eq action t)                 ; all-completions
           (mapcar
-           (let ((orig-prefix
-                  (substring orig (car (completion--sifn-boundaries orig table pred ""))))
-                 (sifned-prefix-length
-                  (- (length sifned)
-                     (car (completion-boundaries sifned table pred "")))))
+           (if only-need-double-dollars
+               #'minibuffer--double-dollars
              ;; Extract the newly added text, quote any dollar signs, and append
              ;; it to the part of ORIG inside the completion boundaries.
              (lambda (compl)
-               (let ((new-text (substring compl sifned-prefix-length)))
-                 (concat orig-prefix (minibuffer--double-dollars new-text)))))
+               (let ((new-text (substring compl (length sifned-in-bounds))))
+                 (concat orig-in-bounds (minibuffer--double-dollars new-text)))))
            result))
          (t result))))))
 

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Mon, 02 Jun 2025 12:38:06 +0200

On Fri, 02 May 2025 14:44:04 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:

> On Fri, 2 May 2025 08:26:44 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>
>> On Fri, May 2, 2025, 4:30 AM Stephen Berman <stephen.berman <at> gmx.net> wrote:
>
>>> What about the other regression I mentioned?  Have you looked at that
>>> and tried the fix for it I included?  If there are no objections to that
>>> patch, I'd like to install it on master.  Or if there are problems with
>>> it, I'd welcome a better fix.
>>>
>>
>> That fix is not right and would cause other issues.
>
> When you have the opportunity, please say what those issues are; I have
> been using my patch since the first version of your changes that
> introduced this regression and don't recall any issues with it with my
> setup, but admittedly I haven't extensive testing with -Q or other
> configurations.

Do you have a rough ETA for fixing this regression?  And again, can you
please explain what's wrong with my fix?

Thanks.

Steve Berman

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 17 Jun 2025 09:50:15 +0200

On Mon, 02 Jun 2025 12:38:06 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:

> On Fri, 02 May 2025 14:44:04 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:
>
>> On Fri, 2 May 2025 08:26:44 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>>
>>> On Fri, May 2, 2025, 4:30 AM Stephen Berman <stephen.berman <at> gmx.net> wrote:
>>
>>>> What about the other regression I mentioned?  Have you looked at that
>>>> and tried the fix for it I included?  If there are no objections to that
>>>> patch, I'd like to install it on master.  Or if there are problems with
>>>> it, I'd welcome a better fix.
>>>>
>>>
>>> That fix is not right and would cause other issues.
>>
>> When you have the opportunity, please say what those issues are; I have
>> been using my patch since the first version of your changes that
>> introduced this regression and don't recall any issues with it with my
>> setup, but admittedly I haven't extensive testing with -Q or other
>> configurations.
>
> Do you have a rough ETA for fixing this regression?  And again, can you
> please explain what's wrong with my fix?

Progress on this issue seems to be stalled.  I have no reason to doubt
my fix may be problematic for use cases I haven't encountered, but I
don't know what these are, so I cannot reasonably investigate further.
If my patch is installed, those who encounter any problems due to it can
report them, which hopefully will facilitate finding a better fix.
Therefore, I request permission to install my patch.  If it causes
serious problems for which there is no quick fix, it can be reverted.

Steve Berman

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 17 Jun 2025 10:59:56 -0400

Stephen Berman <stephen.berman <at> gmx.net> writes:
> On Mon, 02 Jun 2025 12:38:06 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:
>
>> On Fri, 02 May 2025 14:44:04 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:
>>
>>> On Fri, 2 May 2025 08:26:44 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>>>
>>>> On Fri, May 2, 2025, 4:30 AM Stephen Berman <stephen.berman <at> gmx.net> wrote:
>>>
>>>>> What about the other regression I mentioned?  Have you looked at that
>>>>> and tried the fix for it I included?  If there are no objections to that
>>>>> patch, I'd like to install it on master.  Or if there are problems with
>>>>> it, I'd welcome a better fix.
>>>>>
>>>>
>>>> That fix is not right and would cause other issues.
>>>
>>> When you have the opportunity, please say what those issues are; I have
>>> been using my patch since the first version of your changes that
>>> introduced this regression and don't recall any issues with it with my
>>> setup, but admittedly I haven't extensive testing with -Q or other
>>> configurations.
>>
>> Do you have a rough ETA for fixing this regression?  And again, can you
>> please explain what's wrong with my fix?
>
> Progress on this issue seems to be stalled.  I have no reason to doubt
> my fix may be problematic for use cases I haven't encountered, but I
> don't know what these are, so I cannot reasonably investigate further.
> If my patch is installed, those who encounter any problems due to it can
> report them, which hopefully will facilitate finding a better fix.
> Therefore, I request permission to install my patch.  If it causes
> serious problems for which there is no quick fix, it can be reverted.

There is no other regression.  rfn-eshadow works correctly, graying out
the section of the minibuffer text which will be removed by
substitute-in-file-name when read-file-name returns, even if point is in
that section of the text.  This has always been the case.  Likewise, the
pcm-based completion styles sometimes move point into that text on
try-completion.  This has also always been the case.  So there is no
other regression.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 17 Jun 2025 11:43:03 -0400

> There is no other regression.  rfn-eshadow works correctly, graying out
> the section of the minibuffer text which will be removed by
> substitute-in-file-name when read-file-name returns, even if point is in
> that section of the text.  This has always been the case.  Likewise, the
> pcm-based completion styles sometimes move point into that text on
> try-completion.  This has also always been the case.  So there is no
> other regression.

So, can we close this?


        Stefan

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stephen Berman <stephen.berman <at> gmx.net>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 17 Jun 2025 11:49:12 -0400

Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> There is no other regression.  rfn-eshadow works correctly, graying out
>> the section of the minibuffer text which will be removed by
>> substitute-in-file-name when read-file-name returns, even if point is in
>> that section of the text.  This has always been the case.  Likewise, the
>> pcm-based completion styles sometimes move point into that text on
>> try-completion.  This has also always been the case.  So there is no
>> other regression.
>
> So, can we close this?

I believe it is already closed :)

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 17 Jun 2025 17:52:16 +0200

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

On Tue, 17 Jun 2025 10:59:56 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stephen Berman <stephen.berman <at> gmx.net> writes:
>> On Mon, 02 Jun 2025 12:38:06 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:
>>
>>> On Fri, 02 May 2025 14:44:04 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:
>>>
>>>> On Fri, 2 May 2025 08:26:44 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>>>>
>>>>> On Fri, May 2, 2025, 4:30 AM Stephen Berman <stephen.berman <at> gmx.net> wrote:
>>>>
>>>>>> What about the other regression I mentioned?  Have you looked at that
>>>>>> and tried the fix for it I included?  If there are no objections to that
>>>>>> patch, I'd like to install it on master.  Or if there are problems with
>>>>>> it, I'd welcome a better fix.
>>>>>>
>>>>>
>>>>> That fix is not right and would cause other issues.
>>>>
>>>> When you have the opportunity, please say what those issues are; I have
>>>> been using my patch since the first version of your changes that
>>>> introduced this regression and don't recall any issues with it with my
>>>> setup, but admittedly I haven't extensive testing with -Q or other
>>>> configurations.
>>>
>>> Do you have a rough ETA for fixing this regression?  And again, can you
>>> please explain what's wrong with my fix?
>>
>> Progress on this issue seems to be stalled.  I have no reason to doubt
>> my fix may be problematic for use cases I haven't encountered, but I
>> don't know what these are, so I cannot reasonably investigate further.
>> If my patch is installed, those who encounter any problems due to it can
>> report them, which hopefully will facilitate finding a better fix.
>> Therefore, I request permission to install my patch.  If it causes
>> serious problems for which there is no quick fix, it can be reverted.
>
> There is no other regression.  rfn-eshadow works correctly, graying out
> the section of the minibuffer text which will be removed by
> substitute-in-file-name when read-file-name returns, even if point is in
> that section of the text.  This has always been the case.  Likewise, the
> pcm-based completion styles sometimes move point into that text on
> try-completion.  This has also always been the case.  So there is no
> other regression.

I'm not sure what you mean by "no other regression", but what I reported
upstream still holds in current master.  That is, start Emacs like this:

emacs -Q --eval "(custom-set-variables '(completion-category-overrides '((file (styles substring)))))"

After typing `C-x d / TAB', I see this on master:

[master-1.png (image/png, attachment)]

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

and after continuing typing `us TAB TAB', I see this:

[master-2.png (image/png, attachment)]

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

In contrast, starting emacs-30 as above and making the same input, I see
the following:

[emacs-30-1.png (image/png, attachment)]

[emacs-30-2.png (image/png, attachment)]

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

And these last two results are also what I see when I repeat the test on
master with my patch applied.

Steve

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 17 Jun 2025 17:56:38 +0200

On Tue, 17 Jun 2025 11:49:12 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> There is no other regression.  rfn-eshadow works correctly, graying out
>>> the section of the minibuffer text which will be removed by
>>> substitute-in-file-name when read-file-name returns, even if point is in
>>> that section of the text.  This has always been the case.  Likewise, the
>>> pcm-based completion styles sometimes move point into that text on
>>> try-completion.  This has also always been the case.  So there is no
>>> other regression.
>>
>> So, can we close this?
>
> I believe it is already closed :)

Nevertheless, I hope the followup I just posted convinces you both that
the regression still obtains.

Steve Berman

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

From: Spencer Baugh <sbaugh <at> janestreet.com>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 17 Jun 2025 15:26:36 -0400

Stephen Berman <stephen.berman <at> gmx.net> writes:

> On Tue, 17 Jun 2025 10:59:56 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>
>> Stephen Berman <stephen.berman <at> gmx.net> writes:
>>> On Mon, 02 Jun 2025 12:38:06 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:
>>>
>>>> On Fri, 02 May 2025 14:44:04 +0200 Stephen Berman <stephen.berman <at> gmx.net> wrote:
>>>>
>>>>> On Fri, 2 May 2025 08:26:44 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:
>>>>>
>>>>>> On Fri, May 2, 2025, 4:30 AM Stephen Berman <stephen.berman <at> gmx.net> wrote:
>>>>>
>>>>>>> What about the other regression I mentioned?  Have you looked at that
>>>>>>> and tried the fix for it I included?  If there are no objections to that
>>>>>>> patch, I'd like to install it on master.  Or if there are problems with
>>>>>>> it, I'd welcome a better fix.
>>>>>>>
>>>>>>
>>>>>> That fix is not right and would cause other issues.
>>>>>
>>>>> When you have the opportunity, please say what those issues are; I have
>>>>> been using my patch since the first version of your changes that
>>>>> introduced this regression and don't recall any issues with it with my
>>>>> setup, but admittedly I haven't extensive testing with -Q or other
>>>>> configurations.
>>>>
>>>> Do you have a rough ETA for fixing this regression?  And again, can you
>>>> please explain what's wrong with my fix?
>>>
>>> Progress on this issue seems to be stalled.  I have no reason to doubt
>>> my fix may be problematic for use cases I haven't encountered, but I
>>> don't know what these are, so I cannot reasonably investigate further.
>>> If my patch is installed, those who encounter any problems due to it can
>>> report them, which hopefully will facilitate finding a better fix.
>>> Therefore, I request permission to install my patch.  If it causes
>>> serious problems for which there is no quick fix, it can be reverted.
>>
>> There is no other regression.  rfn-eshadow works correctly, graying out
>> the section of the minibuffer text which will be removed by
>> substitute-in-file-name when read-file-name returns, even if point is in
>> that section of the text.  This has always been the case.  Likewise, the
>> pcm-based completion styles sometimes move point into that text on
>> try-completion.  This has also always been the case.  So there is no
>> other regression.
>
> I'm not sure what you mean by "no other regression", but what I reported
> upstream still holds in current master.  That is, start Emacs like this:
>
> emacs -Q --eval "(custom-set-variables '(completion-category-overrides '((file (styles substring)))))"
>
> After typing `C-x d / TAB', I see this on master:
>
> x

More simply, even with -Q, 
(let ((file-name-handler-alist nil)
      (completion-styles '(substring))
      (default-directory "/"))
  (read-file-name ":"))

gives you a minibuffer containing "/"

Hitting ? will show all the possible completions, all of which (on your
system) contain a "/" as a suffix since they are directories.

Hitting TAB will make the minibuffer contain "//", with point in between
the two slashes.  You can then continue to type more characters
contained in the directory names to further narrow down the completions.

This is the behavior of the substring completion style.  When there's a
common substring between all the completions,
completion-substring-try-completion inserts that substring.  Since the
substring is a common suffix, it positions point before that common
suffix.

If there are any other completion styles before substring, this doesn't
happen.  e.g. with:

(let ((file-name-handler-alist nil)
      (completion-styles '(basic substring))
      (default-directory "/"))
  (read-file-name ":"))

Hitting TAB will do nothing.

To have only "substring" configured as a completion style implies you
want the behavior of the "substring" completion style, and only that
behavior.  You are indeed getting that behavior.  What's the problem?
substring has always behaved like this, so improved consistency in edge
cases like this one should not be a surprise.

Is there some problem with this behavior, some reason it doesn't work
right for you?  A more concrete complaint about what doesn't work would
be helpful.

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Stephen Berman <stephen.berman <at> gmx.net>
Cc: Spencer Baugh <sbaugh <at> janestreet.com>, Eli Zaretskii <eliz <at> gnu.org>,
 77718 <at> debbugs.gnu.org
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Tue, 17 Jun 2025 15:55:25 -0400

> I'm not sure what you mean by "no other regression", but what I reported
> upstream still holds in current master.  That is, start Emacs like this:
>
> emacs -Q --eval "(custom-set-variables '(completion-category-overrides
> '((file (styles substring)))))"
>
> After typing `C-x d / TAB', I see this on master:
>
>    Dired (directory): ~///
>
> and after continuing typing `us TAB TAB', I see this:
>
>    Dired (directory): ~//usr//

When I try your recipe I see something very similar, but my cursor is
positioned right *before* the last `/`, so in terms of completion, it
seems like an acceptable behavior (I'm not sure it's very useful, but
it's the result of the generic attempt to insert the longest common
substring, which happens to be just "/").

I do see one problem which is that the whole `~//usr/` prefix is greyed
out by the rfn-shadow.
It's not strictly incorrect since "/" is indeed the filename that would be
returned if I were to hit RET to exit the minibuffer, but given that
point is *before* it, we could argue that the `rfn-shadow` could/should
presume that I'm about to type something in there, which will
immediately bring the `/usr` back to life.


        Stefan

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: Spencer Baugh <sbaugh <at> janestreet.com>, Eli Zaretskii <eliz <at> gnu.org>,
 77718 <at> debbugs.gnu.org
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Wed, 18 Jun 2025 01:00:48 +0200

On Tue, 17 Jun 2025 15:55:25 -0400 Stefan Monnier <monnier <at> iro.umontreal.ca> wrote:

>> I'm not sure what you mean by "no other regression", but what I reported
>> upstream still holds in current master.  That is, start Emacs like this:
>>
>> emacs -Q --eval "(custom-set-variables '(completion-category-overrides
>> '((file (styles substring)))))"
>>
>> After typing `C-x d / TAB', I see this on master:
>>
>>    Dired (directory): ~///
>>
>> and after continuing typing `us TAB TAB', I see this:
>>
>>    Dired (directory): ~//usr//
>
> When I try your recipe I see something very similar, but my cursor is
> positioned right *before* the last `/`,

Yes, that's what I see, too; the screen shots failed to capture the
cursor and I failed to mention that.

>                                         so in terms of completion, it
> seems like an acceptable behavior (I'm not sure it's very useful, but
> it's the result of the generic attempt to insert the longest common
> substring, which happens to be just "/").

Yes, Spencer pointed this out too, and I can appreciate the desire to
strive for more consistency (whether this is an edge case or not), as
long as the resulting behavior is no worse than before; however...

> I do see one problem which is that the whole `~//usr/` prefix is greyed
> out by the rfn-shadow.
> It's not strictly incorrect since "/" is indeed the filename that would be
> returned if I were to hit RET to exit the minibuffer, but given that
> point is *before* it, we could argue that the `rfn-shadow` could/should
> presume that I'm about to type something in there, which will
> immediately bring the `/usr` back to life.

That, and also the points I mentioned in my reply to Spencer (extra TAB,
failure of `M-DEL').

Steve Berman

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

From: Stephen Berman <stephen.berman <at> gmx.net>
To: Spencer Baugh <sbaugh <at> janestreet.com>
Cc: 77718 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Subject: Re: bug#77718: 31.0.50; completion styles substring and flex are
 broken
Date: Wed, 18 Jun 2025 01:00:31 +0200

On Tue, 17 Jun 2025 15:26:36 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:

> Stephen Berman <stephen.berman <at> gmx.net> writes:
>
>> On Tue, 17 Jun 2025 10:59:56 -0400 Spencer Baugh <sbaugh <at> janestreet.com> wrote:
[...]
>>> There is no other regression.  rfn-eshadow works correctly, graying out
>>> the section of the minibuffer text which will be removed by
>>> substitute-in-file-name when read-file-name returns, even if point is in
>>> that section of the text.  This has always been the case.  Likewise, the
>>> pcm-based completion styles sometimes move point into that text on
>>> try-completion.  This has also always been the case.  So there is no
>>> other regression.
>>
>> I'm not sure what you mean by "no other regression", but what I reported
>> upstream still holds in current master.  That is, start Emacs like this:
>>
>> emacs -Q --eval "(custom-set-variables '(completion-category-overrides
>> '((file (styles substring)))))"
>>
>> After typing `C-x d / TAB', I see this on master:
>>
>> x
>
> More simply, even with -Q, 
> (let ((file-name-handler-alist nil)
>       (completion-styles '(substring))
>       (default-directory "/"))
>   (read-file-name ":"))
>
> gives you a minibuffer containing "/"
>
> Hitting ? will show all the possible completions, all of which (on your
> system) contain a "/" as a suffix since they are directories.
>
> Hitting TAB will make the minibuffer contain "//", with point in between
> the two slashes.  You can then continue to type more characters
> contained in the directory names to further narrow down the completions.

Yes, I noted this upthread, where I pointed out that this differs from
the previous behavior, which is why I called it a regression
(https://lists.gnu.org/archive/html/bug-gnu-emacs/2025-04/msg00884.html).

> This is the behavior of the substring completion style.  When there's a
> common substring between all the completions,
> completion-substring-try-completion inserts that substring.  Since the
> substring is a common suffix, it positions point before that common
> suffix.
>
> If there are any other completion styles before substring, this doesn't
> happen.  e.g. with:
>
> (let ((file-name-handler-alist nil)
>       (completion-styles '(basic substring))
>       (default-directory "/"))
>   (read-file-name ":"))
>
> Hitting TAB will do nothing.
>
> To have only "substring" configured as a completion style implies you
> want the behavior of the "substring" completion style, and only that
> behavior.  You are indeed getting that behavior.  What's the problem?
> substring has always behaved like this, so improved consistency in edge
> cases like this one should not be a surprise.

You may consider this an edge case, but it is proof that the substring
style has not "always behaved like this".  I don't recall that the goal
of "improved consistency in edge cases" was previously cited in this
thread; if it had been, I might have viewed the issue in a different
light, though I still find the current behavior problematic...

> Is there some problem with this behavior, some reason it doesn't work
> right for you?  A more concrete complaint about what doesn't work would
> be helpful.

...to wit:

Before your change, when using substring style, typing `C-x d /us TAB
TAB' completes to "/usr/" and pops up the *Completions* buffer.  If I
want to change the input, e.g. to "/var/", I can type `M-DEL va TAB'.

After your change, when using substring style, typing `C-x d /us TAB
TAB' completes to "/usr//" and only after typing TAB a third time does
the *Completions* buffer pop up.  If I want to change the input, typing
`M-DEL' has no effect, though e.g. `C-f / va TAB' works (the minibuffer
then displays "~//usr//var/" with "~//usr/" in file-name-shadow face).
It works, but it's a bit more cumbersome than previously (an extra TAB
to get the *Completions* buffer) and visually confusing (at least given
familiarity with the previous behavior).

And again, with my patch (posted in
https://lists.gnu.org/archive/html/bug-gnu-emacs/2025-04/msg01860.html)
I get the behavior and appearance I'm used to, and for my usage have
encountered no problems with it.  I ask you once again: please tell me
specifically what problems that patch causes.

Steve Berman

Previous Next

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