Package: emacs;
Reported by: Stephen Berman <stephen.berman <at> gmx.net>
Date: Thu, 10 Apr 2025 22:23:02 UTC
Severity: normal
Found in version 31.0.50
Done: Stefan Monnier <monnier <at> iro.umontreal.ca>
View this message in rfc822 format
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: 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))))))
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.