Package: emacs;
Reported by: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 18 Aug 2025 18:54:01 UTC
Severity: normal
Tags: patch
Done: Dmitry Gutov <dmitry <at> gutov.dev>
To reply to this bug, email your comments to 79265 AT debbugs.gnu.org.
There is no need to reopen the bug first.
Toggle the display of automated, internal messages from the tracker.
View this report as an mbox folder, status mbox, maintainer mbox
dmitry <at> gutov.dev, monnier <at> iro.umontreal.ca, bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Mon, 18 Aug 2025 18:54:02 GMT) Full text and rfc822 format available.Spencer Baugh <sbaugh <at> janestreet.com>:dmitry <at> gutov.dev, monnier <at> iro.umontreal.ca, bug-gnu-emacs <at> gnu.org.
(Mon, 18 Aug 2025 18:54:02 GMT) Full text and rfc822 format available.Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: bug-gnu-emacs <at> gnu.org Subject: [PATCH] Treat point more consistently in PCM completion Date: Mon, 18 Aug 2025 14:53:24 -0400
[Message part 1 (text/plain, inline)]
Tags: patch Properly fix bug#38458, which is fundamentally an issue with completion-ignore-case, by checking if the completions are unique ignoring case. When the completions are unique, the normal code to delete a wildcard naturally causes point to be moved to the end of the minibuffer, which is the correct behavior. Now that the bug is fixed properly, remove a hack which previously was used to "fix" it, which made point behave inconsistently if it was in the middle of the minibuffer versus at the end of the minibuffer. * lisp/minibuffer.el (completion-pcm--merge-completions): Respect completion-ignore-case when checking for completion uniqueness. (completion-pcm--string->pattern) (completion-pcm--optimize-pattern): Allow point at the end of the pattern. * test/lisp/minibuffer-tests.el (completion-table-test-quoting) (completion-test--pcm-bug38458, completion-pcm-test-8): Update tests for more correct behavior. In GNU Emacs 30.1.90 (build 9, x86_64-pc-linux-gnu, X toolkit, cairo version 1.15.12, Xaw scroll bars) of 2025-08-14 built on igm-qws-u22796a Repository revision: 6adc26ffa74aedbd1cfa9a1ee72073ebccea2b96 Repository branch: emacs-30 Windowing system distributor 'The X.Org Foundation', version 11.0.12011000 System Description: Rocky Linux 8.10 (Green Obsidian) Configured using: 'configure --with-x-toolkit=lucid --without-gpm --without-gconf --without-selinux --without-imagemagick --with-modules --with-gif=no --with-cairo --with-rsvg --without-compress-install --with-tree-sitter --with-native-compilation=aot PKG_CONFIG_PATH=/usr/local/home/garnish/libtree-sitter/0.22.6-1/lib/pkgconfig/'
[0001-Treat-point-more-consistently-in-PCM-completion.patch (text/patch, attachment)]
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Tue, 19 Aug 2025 04:05:02 GMT) Full text and rfc822 format available.Message #8 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dmitry <at> gutov.dev> To: Spencer Baugh <sbaugh <at> janestreet.com>, 79265 <at> debbugs.gnu.org Cc: Stefan Monnier <monnier <at> iro.umontreal.ca> Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion Date: Tue, 19 Aug 2025 07:04:02 +0300
On 18/08/2025 21:53, Spencer Baugh wrote: > Properly fix bug#38458, which is fundamentally an issue with > completion-ignore-case, by checking if the completions are > unique ignoring case. When the completions are unique, the > normal code to delete a wildcard naturally causes point to be > moved to the end of the minibuffer, which is the correct > behavior. > > Now that the bug is fixed properly, remove a hack which > previously was used to "fix" it, which made point behave > inconsistently if it was in the middle of the minibuffer versus > at the end of the minibuffer. Thanks, the description makes sense and the tests are great to have. I wonder if Stefan will want to comment though. Just one thing: > + ;; They're all the same string, ignoring case. > + (seq-every-p > + (lambda (elem) (string-equal-ignore-case elem prefix)) > + comps))))) Is this performance-sensitive? Using a less abstracted function like cl-every of even a (cl-loop for ... always ...) could be faster. There is a (dolist) above it which iterates across segments of the pattern, so whatever overhead is added could be multiplied; OT2H I haven't tested it myself yet, maybe all bottlenecks are somewhere else.
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Tue, 19 Aug 2025 16:04:02 GMT) Full text and rfc822 format available.Message #11 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Dmitry Gutov <dmitry <at> gutov.dev> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion Date: Tue, 19 Aug 2025 12:03:22 -0400
[Message part 1 (text/plain, inline)]
Dmitry Gutov <dmitry <at> gutov.dev> writes: > On 18/08/2025 21:53, Spencer Baugh wrote: >> Properly fix bug#38458, which is fundamentally an issue with >> completion-ignore-case, by checking if the completions are >> unique ignoring case. When the completions are unique, the >> normal code to delete a wildcard naturally causes point to be >> moved to the end of the minibuffer, which is the correct >> behavior. >> Now that the bug is fixed properly, remove a hack which >> previously was used to "fix" it, which made point behave >> inconsistently if it was in the middle of the minibuffer versus >> at the end of the minibuffer. > > Thanks, the description makes sense and the tests are great to have. > > I wonder if Stefan will want to comment though. > > Just one thing: > >> + ;; They're all the same string, ignoring case. >> + (seq-every-p >> + (lambda (elem) (string-equal-ignore-case elem prefix)) >> + comps))))) > > Is this performance-sensitive? Using a less abstracted function like > cl-every of even a (cl-loop for ... always ...) could be faster. > > There is a (dolist) above it which iterates across segments of the > pattern, so whatever overhead is added could be multiplied; OT2H I > haven't tested it myself yet, maybe all bottlenecks are somewhere > else. Hm, good point. After thinking about it more, I realized that the second try-completion call above somewhat duplicates the check I added, and that I could refactor in general to improve performance here. So I reworked the change a bit to combine my check with the second try-completion call. This should have better performance even though we aren't using the C implementation of try-completion anymore, since we're now doing much less work than try-completion does. (I also refactored to use a cond instead of (or (and ...) (and ...)), which I think makes this logic a fair bit clearer)
[0001-Treat-point-more-consistently-in-PCM-completion.patch (text/x-patch, inline)]
From 9465045d19a8676847cc8932ca66a40aabe2af59 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 18 Aug 2025 14:50:44 -0400
Subject: [PATCH] Treat point more consistently in PCM completion
Properly fix bug#38458, which is fundamentally an issue with
completion-ignore-case, by checking if the completions are
unique ignoring case. When the completions are unique, the
normal code to delete a wildcard naturally causes point to be
moved to the end of the minibuffer, which is the correct
behavior.
Now that the bug is fixed properly, remove a hack which
previously was used to "fix" it, which made point behave
inconsistently if it was in the middle of the minibuffer versus
at the end of the minibuffer.
* lisp/minibuffer.el (completion-pcm--merge-completions):
Respect completion-ignore-case when checking for completion
uniqueness. (bug#79265)
(completion-pcm--string->pattern)
(completion-pcm--optimize-pattern): Allow point at the end of
the pattern.
* test/lisp/minibuffer-tests.el (completion-table-test-quoting)
(completion-test--pcm-bug38458, completion-pcm-test-8): Update
tests for more correct behavior.
---
lisp/minibuffer.el | 25 +++++++++++++++----------
test/lisp/minibuffer-tests.el | 28 +++++++++++++++++++++-------
2 files changed, 36 insertions(+), 17 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 689cda7edab..c7efba10fec 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4237,7 +4237,7 @@ completion-pcm--string->pattern
"Split STRING into a pattern.
A pattern is a list where each element is either a string
or a symbol, see `completion-pcm--merge-completions'."
- (if (and point (< point (length string)))
+ (if (and point (<= point (length string)))
(let ((prefix (substring string 0 point))
(suffix (substring string point)))
(append (completion-pcm--string->pattern prefix)
@@ -4298,12 +4298,6 @@ completion-pcm--optimize-pattern
(pcase p
(`(,(or 'any 'any-delim) ,(or 'any 'point) . ,_)
(setq p (cdr p)))
- ;; This is not just a performance improvement: it turns a
- ;; terminating `point' into an implicit `any', which affects
- ;; the final position of point (because `point' gets turned
- ;; into a non-greedy ".*?" regexp whereas we need it to be
- ;; greedy when it's at the end, see bug#38458).
- (`(point) (setq p nil)) ;Implicit terminating `any'.
(_ (push (pop p) n))))
(nreverse n)))
@@ -4755,9 +4749,20 @@ completion-pcm--merge-completions
;; In practice, it doesn't seem to make any difference.
(setq ccs (nreverse ccs))
(let* ((prefix (try-completion fixed comps))
- (unique (or (and (eq prefix t) (setq prefix fixed))
- (and (stringp prefix)
- (eq t (try-completion prefix comps))))))
+ (unique
+ (cond
+ ((eq prefix t)
+ (setq prefix fixed)
+ t)
+ ((stringp prefix)
+ ;; `try-completion' is almost this, but it doesn't
+ ;; handle `completion-ignore-case' the way we want.
+ (not (cl-some
+ (lambda (comp)
+ ;; PREFIX could complete to COMP, so it's not unique.
+ (and (string-prefix-p prefix comp completion-ignore-case)
+ (not (= (length prefix) (length comp)))))
+ comps))))))
;; If there's only one completion, `elem' is not useful
;; any more: it can only match the empty string.
;; FIXME: in some cases, it may be necessary to turn an
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index f9a26d17e58..59b72899e22 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -100,10 +100,10 @@ completion-table-test-quoting
;; Test that $$ in input is properly unquoted.
("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")
+ ("lisp/c${CTTQ1}et/se-u-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
+ ("lisp/ced${CTTQ2}se-u-c" "lisp/ced${CTTQ2}semantic-utest-c.test")
;; Test that env-vars don't prevent partial-completion.
- ("lis/c${CTTQ1}/se-u" "lisp/c${CTTQ1}et/semantic-utest")
+ ("lis/c${CTTQ1}/se-u-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
))
(should (equal (completion-try-completion input
#'completion--file-name-table
@@ -118,11 +118,11 @@ completion-table-test-quoting
;; When an env var is in the completion bounds, try-completion
;; won't change letter case.
("lisp/c${CTTQ1}E" "lisp/c${CTTQ1}Et/")
- ("lisp/ced${CTTQ2}SE-U" "lisp/ced${CTTQ2}SEmantic-utest")
+ ("lisp/ced${CTTQ2}SE-U-c" "lisp/ced${CTTQ2}SEmantic-utest-c.test")
;; If the env var is before the completion bounds, try-completion
;; *will* change letter case.
- ("lisp/c${CTTQ1}et/SE-U" "lisp/c${CTTQ1}et/semantic-utest")
- ("lis/c${CTTQ1}/SE-U" "lisp/c${CTTQ1}et/semantic-utest")
+ ("lisp/c${CTTQ1}et/SE-U-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
+ ("lis/c${CTTQ1}/SE-U-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
))
(should (equal (car (completion-try-completion input
#'completion--file-name-table
@@ -224,7 +224,11 @@ completion-test--pcm-bug38458
(completion-pcm--merge-try '("tes" point "ing")
'("Testing" "testing")
"" ""))
- '("testing" . 4))))
+ '("testing" . 7)))
+ (should (equal
+ (let ((completion-ignore-case t))
+ (completion-pcm-try-completion "tes" '("Testing" "testing") nil 3))
+ '("testing" . 7))))
(ert-deftest completion-pcm-test-1 ()
;; Point is at end, this does not match anything
@@ -318,6 +322,16 @@ completion-pcm-test-7
'(prefix any "bar" any) '("xbarxfoo" "ybaryfoo") "" "")
'("bar" . 3))))
+(ert-deftest completion-pcm-test-8 ()
+ ;; try-completion inserts the common prefix and suffix at point.
+ (should (equal (completion-pcm-try-completion
+ "r" '("fooxbar" "fooybar") nil 0)
+ '("foobar" . 3)))
+ ;; Even if point is at the end of the minibuffer.
+ (should (equal (completion-pcm-try-completion
+ "" '("fooxbar" "fooybar") nil 0)
+ '("foobar" . 3))))
+
(ert-deftest completion-substring-test-1 ()
;; One third of a match!
(should (equal
--
2.43.7
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Wed, 20 Aug 2025 12:13:02 GMT) Full text and rfc822 format available.Message #14 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dmitry <at> gutov.dev> To: Spencer Baugh <sbaugh <at> janestreet.com> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion Date: Wed, 20 Aug 2025 15:12:08 +0300
On 19/08/2025 19:03, Spencer Baugh wrote: > Hm, good point. After thinking about it more, I realized that the > second try-completion call above somewhat duplicates the check I added, > and that I could refactor in general to improve performance here. > > So I reworked the change a bit to combine my check with the second > try-completion call. This should have better performance even though we > aren't using the C implementation of try-completion anymore, since we're > now doing much less work than try-completion does. > > (I also refactored to use a cond instead of (or (and ...) (and ...)), > which I think makes this logic a fair bit clearer) Hi, this looks sensible. Just a couple of questions. > - (unique (or (and (eq prefix t) (setq prefix fixed)) > - (and (stringp prefix) > - (eq t (try-completion prefix comps)))))) > + (unique > + (cond > + ((eq prefix t) > + (setq prefix fixed) > + t) > + ((stringp prefix) > + ;; `try-completion' is almost this, but it doesn't > + ;; handle `completion-ignore-case' the way we want. > + (not (cl-some This gets a warning "the function ‘cl-some’ might not be defined at runtime" because cl-lib is only loaded for macros, in minibuffer.el. So IDK - maybe it's better to use the alien cl-loop, or maybe to go back to seq-some, if no measurements show a cause for worry. > + (lambda (comp) > + ;; PREFIX could complete to COMP, so it's not unique. > + (and (string-prefix-p prefix comp completion-ignore-case) > + (not (= (length prefix) (length comp))))) > + comps)))))) Just to check: in our limited test examples all elements of COMP match PREFIX already. And the beginning of 'completion-pcm--merge-completions' even 'error'-s if string-match comparison fails. So it's possible that we could omit this comparison from the lambda: (and (string-prefix-p prefix comp completion-ignore-case) But I suppose that would make it a bigger change, a bit riskier.
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Wed, 20 Aug 2025 14:36:02 GMT) Full text and rfc822 format available.Message #17 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Dmitry Gutov <dmitry <at> gutov.dev> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion Date: Wed, 20 Aug 2025 10:35:44 -0400
[Message part 1 (text/plain, inline)]
Dmitry Gutov <dmitry <at> gutov.dev> writes:
> On 19/08/2025 19:03, Spencer Baugh wrote:
>> Hm, good point. After thinking about it more, I realized that the
>> second try-completion call above somewhat duplicates the check I added,
>> and that I could refactor in general to improve performance here.
>> So I reworked the change a bit to combine my check with the second
>> try-completion call. This should have better performance even though we
>> aren't using the C implementation of try-completion anymore, since we're
>> now doing much less work than try-completion does.
>> (I also refactored to use a cond instead of (or (and ...) (and
>> ...)),
>> which I think makes this logic a fair bit clearer)
>
> Hi, this looks sensible. Just a couple of questions.
>
>> - (unique (or (and (eq prefix t) (setq prefix fixed))
>> - (and (stringp prefix)
>> - (eq t (try-completion prefix comps))))))
>> + (unique
>> + (cond
>> + ((eq prefix t)
>> + (setq prefix fixed)
>> + t)
>> + ((stringp prefix)
>> + ;; `try-completion' is almost this, but it doesn't
>> + ;; handle `completion-ignore-case' the way we want.
>> + (not (cl-some
>
> This gets a warning "the function ‘cl-some’ might not be defined at
> runtime" because cl-lib is only loaded for macros, in minibuffer.el.
Oops, sorry, just missed that.
> So IDK - maybe it's better to use the alien cl-loop, or maybe to go
> back to seq-some, if no measurements show a cause for worry.
Eh, I went back to seq-every-p for now. This shouldn't really cause a
performance problem, since it will usually just return nil quickly.
Also it's only run when the user hits TAB.
(I'd rather not use cl-loop, this code is already hard enough to
understand :) )
>> + (lambda (comp)
>> + ;; PREFIX could complete to COMP, so it's not unique.
>> + (and (string-prefix-p prefix comp completion-ignore-case)
>> + (not (= (length prefix) (length comp)))))
>> + comps))))))
>
> Just to check: in our limited test examples all elements of COMP match
> PREFIX already. And the beginning of
> 'completion-pcm--merge-completions' even 'error'-s if string-match
> comparison fails.
>
> So it's possible that we could omit this comparison from the lambda:
>
> (and (string-prefix-p prefix comp completion-ignore-case)
>
> But I suppose that would make it a bigger change, a bit riskier.
Ah, you're right. I missed that we already have this property
guaranteed, but indeed we do.
So thanks to that, the change can be further simplified. Removing my
cond refactoring, the patch is now just replacing
(eq t (try-completion prefix comps))
with
(seq-every-p
(lambda (comp) (= (length prefix) (length comp)))
comps)
which is a nice simple change. I added some comments to clarify why
this is correct, too.
[0001-Treat-point-more-consistently-in-PCM-completion.patch (text/x-patch, inline)]
From 292b02d873d99ec22385681a4622512fa3d8ce25 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 18 Aug 2025 14:50:44 -0400
Subject: [PATCH] Treat point more consistently in PCM completion
Properly fix bug#38458, which is fundamentally an issue with
completion-ignore-case, by checking if the completions are
unique ignoring case. When the completions are unique, the
normal code to delete a wildcard naturally causes point to be
moved to the end of the minibuffer, which is the correct
behavior.
Now that the bug is fixed properly, remove a hack which
previously was used to "fix" it, which made point behave
inconsistently if it was in the middle of the minibuffer versus
at the end of the minibuffer.
* lisp/minibuffer.el (completion-pcm--merge-completions):
Respect completion-ignore-case when checking for completion
uniqueness. (bug#79265)
(completion-pcm--string->pattern)
(completion-pcm--optimize-pattern): Allow point at the end of
the pattern.
* test/lisp/minibuffer-tests.el (completion-table-test-quoting)
(completion-test--pcm-bug38458, completion-pcm-test-8): Update
tests for more correct behavior.
---
lisp/minibuffer.el | 19 +++++++++++--------
test/lisp/minibuffer-tests.el | 28 +++++++++++++++++++++-------
2 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 689cda7edab..36202566136 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4237,7 +4237,7 @@ completion-pcm--string->pattern
"Split STRING into a pattern.
A pattern is a list where each element is either a string
or a symbol, see `completion-pcm--merge-completions'."
- (if (and point (< point (length string)))
+ (if (and point (<= point (length string)))
(let ((prefix (substring string 0 point))
(suffix (substring string point)))
(append (completion-pcm--string->pattern prefix)
@@ -4298,12 +4298,6 @@ completion-pcm--optimize-pattern
(pcase p
(`(,(or 'any 'any-delim) ,(or 'any 'point) . ,_)
(setq p (cdr p)))
- ;; This is not just a performance improvement: it turns a
- ;; terminating `point' into an implicit `any', which affects
- ;; the final position of point (because `point' gets turned
- ;; into a non-greedy ".*?" regexp whereas we need it to be
- ;; greedy when it's at the end, see bug#38458).
- (`(point) (setq p nil)) ;Implicit terminating `any'.
(_ (push (pop p) n))))
(nreverse n)))
@@ -4754,10 +4748,19 @@ completion-pcm--merge-completions
;; different capitalizations in different parts.
;; In practice, it doesn't seem to make any difference.
(setq ccs (nreverse ccs))
+ ;; FIXED is a prefix of all of COMPS. Try to grow that prefix.
(let* ((prefix (try-completion fixed comps))
(unique (or (and (eq prefix t) (setq prefix fixed))
(and (stringp prefix)
- (eq t (try-completion prefix comps))))))
+ ;; If PREFIX is equal to all of COMPS,
+ ;; then PREFIX is a unique completion.
+ (seq-every-p
+ ;; PREFIX is still a prefix of all of
+ ;; COMPS, so if COMP is the same length,
+ ;; they're equal.
+ (lambda (comp)
+ (= (length prefix) (length comp)))
+ comps)))))
;; If there's only one completion, `elem' is not useful
;; any more: it can only match the empty string.
;; FIXME: in some cases, it may be necessary to turn an
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index f9a26d17e58..59b72899e22 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -100,10 +100,10 @@ completion-table-test-quoting
;; Test that $$ in input is properly unquoted.
("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")
+ ("lisp/c${CTTQ1}et/se-u-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
+ ("lisp/ced${CTTQ2}se-u-c" "lisp/ced${CTTQ2}semantic-utest-c.test")
;; Test that env-vars don't prevent partial-completion.
- ("lis/c${CTTQ1}/se-u" "lisp/c${CTTQ1}et/semantic-utest")
+ ("lis/c${CTTQ1}/se-u-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
))
(should (equal (completion-try-completion input
#'completion--file-name-table
@@ -118,11 +118,11 @@ completion-table-test-quoting
;; When an env var is in the completion bounds, try-completion
;; won't change letter case.
("lisp/c${CTTQ1}E" "lisp/c${CTTQ1}Et/")
- ("lisp/ced${CTTQ2}SE-U" "lisp/ced${CTTQ2}SEmantic-utest")
+ ("lisp/ced${CTTQ2}SE-U-c" "lisp/ced${CTTQ2}SEmantic-utest-c.test")
;; If the env var is before the completion bounds, try-completion
;; *will* change letter case.
- ("lisp/c${CTTQ1}et/SE-U" "lisp/c${CTTQ1}et/semantic-utest")
- ("lis/c${CTTQ1}/SE-U" "lisp/c${CTTQ1}et/semantic-utest")
+ ("lisp/c${CTTQ1}et/SE-U-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
+ ("lis/c${CTTQ1}/SE-U-c" "lisp/c${CTTQ1}et/semantic-utest-c.test")
))
(should (equal (car (completion-try-completion input
#'completion--file-name-table
@@ -224,7 +224,11 @@ completion-test--pcm-bug38458
(completion-pcm--merge-try '("tes" point "ing")
'("Testing" "testing")
"" ""))
- '("testing" . 4))))
+ '("testing" . 7)))
+ (should (equal
+ (let ((completion-ignore-case t))
+ (completion-pcm-try-completion "tes" '("Testing" "testing") nil 3))
+ '("testing" . 7))))
(ert-deftest completion-pcm-test-1 ()
;; Point is at end, this does not match anything
@@ -318,6 +322,16 @@ completion-pcm-test-7
'(prefix any "bar" any) '("xbarxfoo" "ybaryfoo") "" "")
'("bar" . 3))))
+(ert-deftest completion-pcm-test-8 ()
+ ;; try-completion inserts the common prefix and suffix at point.
+ (should (equal (completion-pcm-try-completion
+ "r" '("fooxbar" "fooybar") nil 0)
+ '("foobar" . 3)))
+ ;; Even if point is at the end of the minibuffer.
+ (should (equal (completion-pcm-try-completion
+ "" '("fooxbar" "fooybar") nil 0)
+ '("foobar" . 3))))
+
(ert-deftest completion-substring-test-1 ()
;; One third of a match!
(should (equal
--
2.43.7
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Wed, 20 Aug 2025 17:27:01 GMT) Full text and rfc822 format available.Message #20 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Dmitry Gutov <dmitry <at> gutov.dev> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion Date: Wed, 20 Aug 2025 13:26:16 -0400
[Message part 1 (text/plain, inline)]
Spencer Baugh <sbaugh <at> janestreet.com> writes: > Dmitry Gutov <dmitry <at> gutov.dev> writes: > >> On 19/08/2025 19:03, Spencer Baugh wrote: >>> Hm, good point. After thinking about it more, I realized that the >>> second try-completion call above somewhat duplicates the check I added, >>> and that I could refactor in general to improve performance here. >>> So I reworked the change a bit to combine my check with the second >>> try-completion call. This should have better performance even though we >>> aren't using the C implementation of try-completion anymore, since we're >>> now doing much less work than try-completion does. >>> (I also refactored to use a cond instead of (or (and ...) (and >>> ...)), >>> which I think makes this logic a fair bit clearer) >> >> Hi, this looks sensible. Just a couple of questions. >> >>> - (unique (or (and (eq prefix t) (setq prefix fixed)) >>> - (and (stringp prefix) >>> - (eq t (try-completion prefix comps)))))) >>> + (unique >>> + (cond >>> + ((eq prefix t) >>> + (setq prefix fixed) >>> + t) >>> + ((stringp prefix) >>> + ;; `try-completion' is almost this, but it doesn't >>> + ;; handle `completion-ignore-case' the way we want. >>> + (not (cl-some >> >> This gets a warning "the function ‘cl-some’ might not be defined at >> runtime" because cl-lib is only loaded for macros, in minibuffer.el. > > Oops, sorry, just missed that. > >> So IDK - maybe it's better to use the alien cl-loop, or maybe to go >> back to seq-some, if no measurements show a cause for worry. > > Eh, I went back to seq-every-p for now. This shouldn't really cause a > performance problem, since it will usually just return nil quickly. > Also it's only run when the user hits TAB. > > (I'd rather not use cl-loop, this code is already hard enough to > understand :) ) > >>> + (lambda (comp) >>> + ;; PREFIX could complete to COMP, so it's not unique. >>> + (and (string-prefix-p prefix comp completion-ignore-case) >>> + (not (= (length prefix) (length comp))))) >>> + comps)))))) >> >> Just to check: in our limited test examples all elements of COMP match >> PREFIX already. And the beginning of >> 'completion-pcm--merge-completions' even 'error'-s if string-match >> comparison fails. >> >> So it's possible that we could omit this comparison from the lambda: >> >> (and (string-prefix-p prefix comp completion-ignore-case) >> >> But I suppose that would make it a bigger change, a bit riskier. > > Ah, you're right. I missed that we already have this property > guaranteed, but indeed we do. > > So thanks to that, the change can be further simplified. Removing my > cond refactoring, the patch is now just replacing > > (eq t (try-completion prefix comps)) > > with > > (seq-every-p > (lambda (comp) (= (length prefix) (length comp))) > comps) > > which is a nice simple change. I added some comments to clarify why > this is correct, too. And this followup patch makes it much clearer that this is correct, and also improves performance further. I think we should apply both.
[0001-Avoid-duplicating-strings-in-pcm-merge-completions.patch (text/x-patch, inline)]
From fcaab60e765961b2fffe064c1facf078bce1a101 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Wed, 20 Aug 2025 13:23:34 -0400
Subject: [PATCH] Avoid duplicating strings in pcm--merge-completions
Make completion-pcm--merge-completions operate only on the text
matched by the wildcards, instead of also the text in between
the wildcards. This improves performance and simplifies the
code by removing the need for the previous mutable variable
"fixed".
* lisp/minibuffer.el (completion-pcm--merge-completions):
Operate only on text matched by wildcards. (bug#79265)
---
lisp/minibuffer.el | 39 +++++++++++++++------------------------
1 file changed, 15 insertions(+), 24 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 36202566136..999d699c488 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4706,38 +4706,35 @@ completion-pcm--merge-completions
(cond
((null (cdr strs)) (list (car strs)))
(t
- (let ((re (completion-pcm--pattern->regex pattern 'group))
+ (let ((re (concat
+ (completion-pcm--pattern->regex pattern 'group)
+ ;; The implicit trailing `any' is greedy.
+ "\\([^z-a]*\\)"))
(ccs ())) ;Chopped completions.
- ;; First chop each string into the parts corresponding to each
- ;; non-constant element of `pattern', using regexp-matching.
+ ;; First match each string against PATTERN as a regex and extract
+ ;; the text matched by each wildcard.
(let ((case-fold-search completion-ignore-case))
(dolist (str strs)
(unless (string-match re str)
(error "Internal error: %s doesn't match %s" str re))
(let ((chopped ())
- (last 0)
(i 1)
next)
- (while (setq next (match-end i))
- (push (substring str last next) chopped)
- (setq last next)
+ (while (setq next (match-string i str))
+ (push next chopped)
(setq i (1+ i)))
- ;; Add the text corresponding to the implicit trailing `any'.
- (push (substring str last) chopped)
(push (nreverse chopped) ccs))))
- ;; Then for each of those non-constant elements, extract the
- ;; commonality between them.
+ ;; Then for each of those wildcards, extract the commonality between them.
(let ((res ())
- (fixed "")
;; Accumulate each stretch of wildcards, and process them as a unit.
(wildcards ()))
;; Make the implicit trailing `any' explicit.
(dolist (elem (append pattern '(any)))
(if (stringp elem)
(progn
- (setq fixed (concat fixed elem))
+ (push elem res)
(setq wildcards nil))
(let ((comps ()))
(push elem wildcards)
@@ -4748,18 +4745,13 @@ completion-pcm--merge-completions
;; different capitalizations in different parts.
;; In practice, it doesn't seem to make any difference.
(setq ccs (nreverse ccs))
- ;; FIXED is a prefix of all of COMPS. Try to grow that prefix.
- (let* ((prefix (try-completion fixed comps))
- (unique (or (and (eq prefix t) (setq prefix fixed))
+ (let* ((prefix (try-completion "" comps))
+ (unique (or (and (eq prefix t) (setq prefix ""))
(and (stringp prefix)
;; If PREFIX is equal to all of COMPS,
;; then PREFIX is a unique completion.
(seq-every-p
- ;; PREFIX is still a prefix of all of
- ;; COMPS, so if COMP is the same length,
- ;; they're equal.
- (lambda (comp)
- (= (length prefix) (length comp)))
+ (lambda (comp) (= (length prefix) (length comp)))
comps)))))
;; If there's only one completion, `elem' is not useful
;; any more: it can only match the empty string.
@@ -4774,7 +4766,7 @@ completion-pcm--merge-completions
;; `prefix' only wants to include the fixed part before the
;; wildcard, not the result of growing that fixed part.
(when (seq-some (lambda (elem) (eq elem 'prefix)) wildcards)
- (setq prefix fixed))
+ (setq prefix ""))
(push prefix res)
;; Push all the wildcards in this stretch, to preserve `point' and
;; `star' wildcards before ELEM.
@@ -4798,8 +4790,7 @@ completion-pcm--merge-completions
(unless (equal suffix "")
(push suffix res))))
;; We pushed these wildcards on RES, so we're done with them.
- (setq wildcards nil))
- (setq fixed "")))))
+ (setq wildcards nil))))))
;; We return it in reverse order.
res)))))
--
2.43.7
Dmitry Gutov <dmitry <at> gutov.dev>:Spencer Baugh <sbaugh <at> janestreet.com>:Message #25 received at 79265-done <at> debbugs.gnu.org (full text, mbox):
From: Dmitry Gutov <dmitry <at> gutov.dev> To: Spencer Baugh <sbaugh <at> janestreet.com> Cc: Stefan Monnier <monnier <at> iro.umontreal.ca>, 79265-done <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion Date: Thu, 21 Aug 2025 04:05:38 +0300
On 20/08/2025 17:35, Spencer Baugh wrote: >> So IDK - maybe it's better to use the alien cl-loop, or maybe to go >> back to seq-some, if no measurements show a cause for worry. > > Eh, I went back to seq-every-p for now. This shouldn't really cause a > performance problem, since it will usually just return nil quickly. > Also it's only run when the user hits TAB. Sounds good. Its overhead it two dynamic dispatches, so probably fine for most uses. > (I'd rather not use cl-loop, this code is already hard enough to > understand :) ) All right then. (I like cl-loop ;-( ) >> Just to check: in our limited test examples all elements of COMP match >> PREFIX already. And the beginning of >> 'completion-pcm--merge-completions' even 'error'-s if string-match >> comparison fails. >> >> So it's possible that we could omit this comparison from the lambda: >> >> (and (string-prefix-p prefix comp completion-ignore-case) >> >> But I suppose that would make it a bigger change, a bit riskier. > > Ah, you're right. I missed that we already have this property > guaranteed, but indeed we do. > > So thanks to that, the change can be further simplified. Removing my > cond refactoring, the patch is now just replacing > > (eq t (try-completion prefix comps)) > > with > > (seq-every-p > (lambda (comp) (= (length prefix) (length comp))) > comps) > > which is a nice simple change. I added some comments to clarify why > this is correct, too. Thanks, pushed to master, this and the the following patch (nice optimization!). And with that, time to close.
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Thu, 21 Aug 2025 18:59:02 GMT) Full text and rfc822 format available.Message #28 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Spencer Baugh <sbaugh <at> janestreet.com> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion Date: Thu, 21 Aug 2025 14:58:07 -0400
[ Sorry for the delay reviewing this patch (and others, still waiting).
I still have rather spotty access to my keyboard, tho things should
slowly return to normal over the next two weeks. ]
> From: Spencer Baugh <sbaugh <at> janestreet.com>
> Date: Wed, 20 Aug 2025 13:23:34 -0400
> Subject: [PATCH] Avoid duplicating strings in pcm--merge-completions
>
> Make completion-pcm--merge-completions operate only on the text
> matched by the wildcards, instead of also the text in between
> the wildcards. This improves performance and simplifies the
> code by removing the need for the previous mutable variable
> "fixed".
>
> * lisp/minibuffer.el (completion-pcm--merge-completions):
> Operate only on text matched by wildcards. (bug#79265)
Could we have a regression test for this change?
Or is it supposed to make no difference to behavior?
> - (let ((re (completion-pcm--pattern->regex pattern 'group))
> + (let ((re (concat
> + (completion-pcm--pattern->regex pattern 'group)
> + ;; The implicit trailing `any' is greedy.
> + "\\([^z-a]*\\)"))
> (ccs ())) ;Chopped completions.
The comment makes sense only if you know that
`completion-pcm--pattern->regex` would use a non-greedy regexp for
something like `any`.
> - (while (setq next (match-end i))
> - (push (substring str last next) chopped)
> - (setq last next)
> + (while (setq next (match-string i str))
> + (push next chopped)
IIUC this means that when `completion-ignore-case` is t we will keep the
capitalization typed by the user rather than adjust it based on the
completion candidates.
E.g.
(let ((completion-ignore-case t))
(completion-pcm--merge-completions '("ABC" "ABD") '("a")))
will complete "a" to "aB" rather than to "AB".
See commit 681e0e7c02c4f8ff6d316c8c24001106cb847023 (which should have
come with a regression test, obviously).
Stefan
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Thu, 21 Aug 2025 19:30:02 GMT) Full text and rfc822 format available.Message #31 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion Date: Thu, 21 Aug 2025 15:29:21 -0400
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
> [ Sorry for the delay reviewing this patch (and others, still waiting).
> I still have rather spotty access to my keyboard, tho things should
> slowly return to normal over the next two weeks. ]
No worries, thanks for responding to this, to save me from my own buggy
change :)
>> From: Spencer Baugh <sbaugh <at> janestreet.com>
>> Date: Wed, 20 Aug 2025 13:23:34 -0400
>> Subject: [PATCH] Avoid duplicating strings in pcm--merge-completions
>>
>> Make completion-pcm--merge-completions operate only on the text
>> matched by the wildcards, instead of also the text in between
>> the wildcards. This improves performance and simplifies the
>> code by removing the need for the previous mutable variable
>> "fixed".
>>
>> * lisp/minibuffer.el (completion-pcm--merge-completions):
>> Operate only on text matched by wildcards. (bug#79265)
>
> Could we have a regression test for this change?
> Or is it supposed to make no difference to behavior?
It's supposed to make no difference to behavior...
>> - (while (setq next (match-end i))
>> - (push (substring str last next) chopped)
>> - (setq last next)
>> + (while (setq next (match-string i str))
>> + (push next chopped)
>
> IIUC this means that when `completion-ignore-case` is t we will keep the
> capitalization typed by the user rather than adjust it based on the
> completion candidates.
>
> E.g.
>
> (let ((completion-ignore-case t))
> (completion-pcm--merge-completions '("ABC" "ABD") '("a")))
>
> will complete "a" to "aB" rather than to "AB".
>
> See commit 681e0e7c02c4f8ff6d316c8c24001106cb847023 (which should have
> come with a regression test, obviously).
...except, you're right, of course it does cause this issue. Thanks for catching this.
I'll write a test for this and try to find a further change to the code
which retains the improvement in clarity while still fixing this issue.
It should work to just arbitrarily take the capitalization from the
first completion in the list, I think, right? That also will make
things more consistent, I think?
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Thu, 21 Aug 2025 20:45:02 GMT) Full text and rfc822 format available.Message #34 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion Date: Thu, 21 Aug 2025 16:44:40 -0400
[Message part 1 (text/plain, inline)]
Spencer Baugh <sbaugh <at> janestreet.com> writes:
> Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> - (while (setq next (match-end i))
>>> - (push (substring str last next) chopped)
>>> - (setq last next)
>>> + (while (setq next (match-string i str))
>>> + (push next chopped)
>>
>> IIUC this means that when `completion-ignore-case` is t we will keep the
>> capitalization typed by the user rather than adjust it based on the
>> completion candidates.
>>
>> E.g.
>>
>> (let ((completion-ignore-case t))
>> (completion-pcm--merge-completions '("ABC" "ABD") '("a")))
>>
>> will complete "a" to "aB" rather than to "AB".
>>
>> See commit 681e0e7c02c4f8ff6d316c8c24001106cb847023 (which should have
>> come with a regression test, obviously).
>
> ...except, you're right, of course it does cause this issue. Thanks for catching this.
>
> I'll write a test for this and try to find a further change to the code
> which retains the improvement in clarity while still fixing this issue.
>
> It should work to just arbitrarily take the capitalization from the
> first completion in the list, I think, right? That also will make
> things more consistent, I think?
No, that's not correct, actually. I need to use try-completion to merge
them to get nice behavior.
So how about this? This mostly reverts my earlier patch, so it may be
easier to review it relative to the old version before my earlier patch.
[0001-Use-capitalization-from-completions-in-pcm-try-again.patch (text/x-patch, inline)]
From 85a4b867646d563fc6e05edb2f6782073df77df8 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Thu, 21 Aug 2025 16:42:00 -0400
Subject: [PATCH] Use capitalization from completions in pcm-try, again
In b511c38bba5354ff21c697e4d27279bf73e4d3cf I tried to simplify
and optimize completion-pcm--merge-completions, but I broke
earlier behavior added by
681e0e7c02c4f8ff6d316c8c24001106cb847023 which made
completion-pcm-try-completion would change the case of text to
match the completions. Support this behavior again and add a
test checking this.
* lisp/minibuffer.el (completion-pcm--pattern->regex): When
GROUP is group-all, also group strings in PATTERN.
(completion-pcm--merge-completions): Preserve case from parts of
completions matching fixed strings in PATTERN. (bug#79265)
* test/lisp/minibuffer-tests.el (completion-pcm-bug4219): Add.
---
lisp/minibuffer.el | 55 +++++++++++++++++++++--------------
test/lisp/minibuffer-tests.el | 8 +++++
2 files changed, 41 insertions(+), 22 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 467ef4591db..67f3f924ffd 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4226,7 +4226,11 @@ completion-pcm--pattern->regex
(mapconcat
(lambda (x)
(cond
- ((stringp x) (regexp-quote x))
+ ((stringp x)
+ (let ((re (regexp-quote x)))
+ (if (eq group 'group-all)
+ (concat "\\(" re "\\)")
+ re)))
(t
(let ((re (if (eq x 'any-delim)
(concat completion-pcm--delim-wild-regex "*?")
@@ -4625,45 +4629,52 @@ completion-pcm--merge-completions
(cond
((null (cdr strs)) (list (car strs)))
(t
- (let ((re (concat
- (completion-pcm--pattern->regex pattern 'group)
- ;; The implicit trailing `any' is greedy.
- "\\([^z-a]*\\)"))
+ (let ((re (completion-pcm--pattern->regex pattern 'group-all))
(ccs ())) ;Chopped completions.
- ;; First match each string against PATTERN as a regex and extract
- ;; the text matched by each wildcard.
+ ;; First chop each string into the parts corresponding to each
+ ;; non-constant element of `pattern', using regexp-matching.
(let ((case-fold-search completion-ignore-case))
(dolist (str strs)
(unless (string-match re str)
(error "Internal error: %s doesn't match %s" str re))
(let ((chopped ())
+ (last 0)
(i 1)
next)
- (while (setq next (match-string i str))
- (push next chopped)
+ (while (setq next (match-end i))
+ (push (substring str last next) chopped)
+ (setq last next)
(setq i (1+ i)))
+ ;; Add the text corresponding to the implicit trailing `any'.
+ (push (substring str last) chopped)
(push (nreverse chopped) ccs))))
- ;; Then for each of those wildcards, extract the commonality between them.
+ ;; Then for each of those non-constant elements, extract the
+ ;; commonality between them.
(let ((res ())
;; Accumulate each stretch of wildcards, and process them as a unit.
(wildcards ()))
;; Make the implicit trailing `any' explicit.
(dolist (elem (append pattern '(any)))
- (if (stringp elem)
- (progn
- (push elem res)
- (setq wildcards nil))
- (let ((comps ()))
+ (let ((comps ()))
+ (dolist (cc (prog1 ccs (setq ccs nil)))
+ (push (car cc) comps)
+ (push (cdr cc) ccs))
+ ;; Might improve the likelihood to avoid choosing
+ ;; different capitalizations in different parts.
+ ;; In practice, it doesn't seem to make any difference.
+ (setq ccs (nreverse ccs))
+ (if (stringp elem)
+ (let ((shared (try-completion elem comps)))
+ ;; Use `try-completion' to merge the strings from each
+ ;; completion, which may differ in case.
+ (push (cond ((eq shared t) elem)
+ ((stringp shared) shared)
+ (t ""))
+ res)
+ (setq wildcards nil))
(push elem wildcards)
- (dolist (cc (prog1 ccs (setq ccs nil)))
- (push (car cc) comps)
- (push (cdr cc) ccs))
- ;; Might improve the likelihood to avoid choosing
- ;; different capitalizations in different parts.
- ;; In practice, it doesn't seem to make any difference.
- (setq ccs (nreverse ccs))
(let* ((prefix (try-completion "" comps))
(unique (or (and (eq prefix t) (setq prefix ""))
(and (stringp prefix)
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index c2c37e63012..e5868351619 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -332,6 +332,14 @@ completion-pcm-test-8
"" '("fooxbar" "fooybar") nil 0)
'("foobar" . 3))))
+(ert-deftest completion-pcm-bug4219 ()
+ ;; With `completion-ignore-case', try-completion should change the
+ ;; case of existing text when the completions have different casing.
+ (should (equal
+ (let ((completion-ignore-case t))
+ (completion-pcm-try-completion "a" '("ABC" "ABD") nil 1))
+ '("AB" . 2))))
+
(ert-deftest completion-substring-test-1 ()
;; One third of a match!
(should (equal
--
2.43.7
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Sat, 23 Aug 2025 07:00:03 GMT) Full text and rfc822 format available.Message #37 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Spencer Baugh <sbaugh <at> janestreet.com> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion Date: Sat, 23 Aug 2025 02:58:55 -0400
> So how about this? This mostly reverts my earlier patch, so it may be
> easier to review it relative to the old version before my earlier patch.
Now I'm wondering what is the advantage of this new code compared to the
old code.
> * lisp/minibuffer.el (completion-pcm--pattern->regex): When
> GROUP is group-all, also group strings in PATTERN.
Why do we need that?
> diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
> index c2c37e63012..e5868351619 100644
> --- a/test/lisp/minibuffer-tests.el
> +++ b/test/lisp/minibuffer-tests.el
> @@ -332,6 +332,14 @@ completion-pcm-test-8
> "" '("fooxbar" "fooybar") nil 0)
> '("foobar" . 3))))
>
> +(ert-deftest completion-pcm-bug4219 ()
> + ;; With `completion-ignore-case', try-completion should change the
> + ;; case of existing text when the completions have different casing.
> + (should (equal
> + (let ((completion-ignore-case t))
> + (completion-pcm-try-completion "a" '("ABC" "ABD") nil 1))
> + '("AB" . 2))))
> +
> (ert-deftest completion-substring-test-1 ()
> ;; One third of a match!
> (should (equal
Thanks,
Stefan
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Sat, 23 Aug 2025 14:54:01 GMT) Full text and rfc822 format available.Message #40 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion Date: Sat, 23 Aug 2025 10:53:20 -0400
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>> So how about this? This mostly reverts my earlier patch, so it may be
>> easier to review it relative to the old version before my earlier patch.
>
> Now I'm wondering what is the advantage of this new code compared to the
> old code.
Two things:
- It should have somewhat better performance because we're running
try-completion on smaller strings and therefore doing less string
comparison and work.
- It gets rid of the mutable variable FIXED which is mutated across each
iteration of the loop over the pattern. IMO this makes the code much
easier to follow.
Below is the diff from the original version, before my already-installed
earlier patch; possibly this is easiest to review.
>> * lisp/minibuffer.el (completion-pcm--pattern->regex): When
>> GROUP is group-all, also group strings in PATTERN.
>
> Why do we need that?
It makes it possible to extract the parts of the completions that
correspond to each part of PATTERN, rather than just the parts
corresponding to wildcards.
CCS previously contained, for each completion, a list containing one
element for each wildcard element of PATTERN.
Now CCS contains, for each completion, a list containing one element for
each element of PATTERN, both fixed strings and wildcards.
(This lets us iterate over PATTERN and CCS with a more uniform loop -
there could probably even be some further simplifications, to zip the
two lists together one way or another)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index b5060614841..37bfbdc0765 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4187,7 +4207,11 @@ completion-pcm--pattern->regex
(mapconcat
(lambda (x)
(cond
- ((stringp x) (regexp-quote x))
+ ((stringp x)
+ (let ((re (regexp-quote x)))
+ (if (eq group 'group-all)
+ (concat "\\(" re "\\)")
+ re)))
(t
(let ((re (if (eq x 'any-delim)
(concat completion-pcm--delim-wild-regex "*?")
@@ -4586,7 +4610,7 @@ completion-pcm--merge-completions
(cond
((null (cdr strs)) (list (car strs)))
(t
- (let ((re (completion-pcm--pattern->regex pattern 'group))
+ (let ((re (completion-pcm--pattern->regex pattern 'group-all))
(ccs ())) ;Chopped completions.
;; First chop each string into the parts corresponding to each
@@ -4610,36 +4634,35 @@ completion-pcm--merge-completions
;; Then for each of those non-constant elements, extract the
;; commonality between them.
(let ((res ())
- (fixed "")
;; Accumulate each stretch of wildcards, and process them as a unit.
(wildcards ()))
;; Make the implicit trailing `any' explicit.
(dolist (elem (append pattern '(any)))
- (if (stringp elem)
- (progn
- (setq fixed (concat fixed elem))
- (setq wildcards nil))
- (let ((comps ()))
+ (let ((comps ()))
+ (dolist (cc (prog1 ccs (setq ccs nil)))
+ (push (car cc) comps)
+ (push (cdr cc) ccs))
+ ;; Might improve the likelihood to avoid choosing
+ ;; different capitalizations in different parts.
+ ;; In practice, it doesn't seem to make any difference.
+ (setq ccs (nreverse ccs))
+ (if (stringp elem)
+ (let ((shared (try-completion elem comps)))
+ ;; Use `try-completion' to merge the strings from each
+ ;; completion, which may differ in case.
+ (push (cond ((eq shared t) elem)
+ ((stringp shared) shared)
+ (t ""))
+ res)
+ (setq wildcards nil))
(push elem wildcards)
- (dolist (cc (prog1 ccs (setq ccs nil)))
- (push (car cc) comps)
- (push (cdr cc) ccs))
- ;; Might improve the likelihood to avoid choosing
- ;; different capitalizations in different parts.
- ;; In practice, it doesn't seem to make any difference.
- (setq ccs (nreverse ccs))
- ;; FIXED is a prefix of all of COMPS. Try to grow that prefix.
- (let* ((prefix (try-completion fixed comps))
- (unique (or (and (eq prefix t) (setq prefix fixed))
+ (let* ((prefix (try-completion "" comps))
+ (unique (or (and (eq prefix t) (setq prefix ""))
(and (stringp prefix)
;; If PREFIX is equal to all of COMPS,
;; then PREFIX is a unique completion.
(seq-every-p
- ;; PREFIX is still a prefix of all of
- ;; COMPS, so if COMP is the same length,
- ;; they're equal.
- (lambda (comp)
- (= (length prefix) (length comp)))
+ (lambda (comp) (= (length prefix) (length comp)))
comps)))))
;; If there's only one completion, `elem' is not useful
;; any more: it can only match the empty string.
@@ -4654,7 +4677,7 @@ completion-pcm--merge-completions
;; `prefix' only wants to include the fixed part before the
;; wildcard, not the result of growing that fixed part.
(when (seq-some (lambda (elem) (eq elem 'prefix)) wildcards)
- (setq prefix fixed))
+ (setq prefix ""))
(push prefix res)
;; Push all the wildcards in this stretch, to preserve `point' and
;; `star' wildcards before ELEM.
@@ -4678,8 +4701,7 @@ completion-pcm--merge-completions
(unless (equal suffix "")
(push suffix res))))
;; We pushed these wildcards on RES, so we're done with them.
- (setq wildcards nil))
- (setq fixed "")))))
+ (setq wildcards nil))))))
;; We return it in reverse order.
res)))))
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Sun, 24 Aug 2025 07:28:02 GMT) Full text and rfc822 format available.Message #43 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Spencer Baugh <sbaugh <at> janestreet.com> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion Date: Sun, 24 Aug 2025 03:27:12 -0400
> Two things:
>
> - It should have somewhat better performance because we're running
> try-completion on smaller strings and therefore doing less string
> comparison and work.
Do we have concrete evidence to believe this?
> CCS previously contained, for each completion, a list containing one
> element for each wildcard element of PATTERN.
>
> Now CCS contains, for each completion, a list containing one element for
> each element of PATTERN, both fixed strings and wildcards.
Exactly: that means CCS is more costly to build and AFAICT it also
implies correspondingly more calls to `try-completion`.
I don't find it obvious that the new code will be more efficient.
Barring any clear evidence that one of the two versions is noticeably
more efficient than the other, I'd recommend we stick to the code we
happen to have.
From where I stand, my impression is that neither version is terribly
better nor terribly worse than the other, in terms of clarity,
simplicity, and efficiency.
Stefan
PS: BTW, there's currently a limit of 255 subgroups in regexps, which
for `flex` currently implies a limit of 254 chars and with your patch
that limit would be halved.
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Mon, 25 Aug 2025 18:41:02 GMT) Full text and rfc822 format available.Message #46 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion Date: Mon, 25 Aug 2025 14:39:55 -0400
[Message part 1 (text/plain, inline)]
Stefan Monnier <monnier <at> iro.umontreal.ca> writes: >> Two things: >> >> - It should have somewhat better performance because we're running >> try-completion on smaller strings and therefore doing less string >> comparison and work. > > Do we have concrete evidence to believe this? No evidence. So nevermind the performance argument, I suppose. >> CCS previously contained, for each completion, a list containing one >> element for each wildcard element of PATTERN. >> >> Now CCS contains, for each completion, a list containing one element for >> each element of PATTERN, both fixed strings and wildcards. > > Exactly: that means CCS is more costly to build and AFAICT it also > implies correspondingly more calls to `try-completion`. > > I don't find it obvious that the new code will be more efficient. > Barring any clear evidence that one of the two versions is noticeably > more efficient than the other, I'd recommend we stick to the code we > happen to have. > > From where I stand, my impression is that neither version is terribly > better nor terribly worse than the other, in terms of clarity, > simplicity, and efficiency. That's fair, that patch alone is not worth much. Attached is a patch (which applies on master) which does most of the rest of the refactoring that I wanted to do; it makes the dolist over the pattern into a simple mapcan over the pattern, with no state preserved between elements of the pattern. IMO, this is much easier to follow (and I think it would make the various bugs I've fixed in the last couple years in merge-completions, easier to catch). What do you think? > PS: BTW, there's currently a limit of 255 subgroups in regexps, which > for `flex` currently implies a limit of 254 chars and with your patch > that limit would be halved. Oh, I didn't realize that. I could definitely lift that limit on flex with my design, though, so that it's not limited to 254 chars. We can just do repeated string-searches instead for each of the fixed elements, which won't have this limitation.
[0001-Refactor-completion-pcm-merge-completions.patch (text/x-patch, inline)]
From 54857fe2fb0ed033afcba231f920f8f7fa185333 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 25 Aug 2025 14:27:19 -0400
Subject: [PATCH] Refactor completion-pcm--merge-completions
Further simplify completion-pcm--merge-completions. Also fix a
bug I introduced in b511c38bba5354ff21c697e4d27279bf73e4d3cf
which made it no longer change the case of text to match the
completions.
* lisp/minibuffer.el (completion-pcm--pattern->regex): When
GROUP is group-all, also group strings in PATTERN.
(completion-pcm--merge-completions): Preserve case from parts of
completions matching fixed strings in PATTERN. (bug#79265)
* test/lisp/minibuffer-tests.el (completion-pcm-bug4219): Add.
---
lisp/minibuffer.el | 165 +++++++++++++++++-----------------
test/lisp/minibuffer-tests.el | 8 ++
2 files changed, 90 insertions(+), 83 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 0221af5426e..462280233f1 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4249,7 +4249,11 @@ completion-pcm--pattern->regex
(mapconcat
(lambda (x)
(cond
- ((stringp x) (regexp-quote x))
+ ((stringp x)
+ (let ((re (regexp-quote x)))
+ (if (eq group 'group-all)
+ (concat "\\(" re "\\)")
+ re)))
(t
(let ((re (if (eq x 'any-delim)
(concat completion-pcm--delim-wild-regex "*?")
@@ -4622,6 +4626,19 @@ completion--common-suffix
"Return the common suffix of the strings STRS."
(nreverse (try-completion "" (mapcar #'reverse strs))))
+(defun completion-pcm--group-pattern (pattern)
+ "Group together adjacent wildcards in PATTERN."
+ (let (ret)
+ (dolist (elem pattern)
+ (cond
+ ((or (stringp elem) (eq elem 'any-delim))
+ (push elem ret))
+ ((consp (car-safe ret))
+ (setf (car ret) (append (car ret) (list elem))))
+ (t
+ (push (list elem) ret))))
+ (nreverse ret)))
+
(defun completion-pcm--merge-completions (strs pattern)
"Extract the commonality in STRS, with the help of PATTERN.
PATTERN can contain strings and symbols chosen among `star', `any', `point',
@@ -4648,93 +4665,75 @@ completion-pcm--merge-completions
(cond
((null (cdr strs)) (list (car strs)))
(t
- (let ((re (concat
- (completion-pcm--pattern->regex pattern 'group)
- ;; The implicit trailing `any' is greedy.
- "\\([^z-a]*\\)"))
- (ccs ())) ;Chopped completions.
-
+ (let* ((pattern (completion-pcm--group-pattern (append pattern '(any))))
+ (re (concat
+ ;; Force matching the entire string.
+ (completion-pcm--pattern->regex pattern 'group-all) (rx eos)))
+ (pattern-and-comps (mapcar (lambda (elem) (cons elem nil)) pattern)))
;; First match each string against PATTERN as a regex and extract
;; the text matched by each wildcard.
(let ((case-fold-search completion-ignore-case))
(dolist (str strs)
(unless (string-match re str)
(error "Internal error: %s doesn't match %s" str re))
- (let ((chopped ())
- (i 1)
- next)
- (while (setq next (match-string i str))
- (push next chopped)
- (setq i (1+ i)))
- (push (nreverse chopped) ccs))))
-
- ;; Then for each of those wildcards, extract the commonality between them.
- (let ((res ())
- ;; Accumulate each stretch of wildcards, and process them as a unit.
- (wildcards ()))
- ;; Make the implicit trailing `any' explicit.
- (dolist (elem (append pattern '(any)))
- (if (stringp elem)
- (progn
- (push elem res)
- (setq wildcards nil))
- (let ((comps ()))
- (push elem wildcards)
- (dolist (cc (prog1 ccs (setq ccs nil)))
- (push (car cc) comps)
- (push (cdr cc) ccs))
- ;; Might improve the likelihood to avoid choosing
- ;; different capitalizations in different parts.
- ;; In practice, it doesn't seem to make any difference.
- (setq ccs (nreverse ccs))
- (let* ((prefix (try-completion "" comps))
- (unique (or (and (eq prefix t) (setq prefix ""))
- (and (stringp prefix)
- ;; If PREFIX is equal to all of COMPS,
- ;; then PREFIX is a unique completion.
- (seq-every-p
- (lambda (comp) (= (length prefix) (length comp)))
- comps)))))
- ;; If there's only one completion, `elem' is not useful
- ;; any more: it can only match the empty string.
- ;; FIXME: in some cases, it may be necessary to turn an
- ;; `any' into a `star' because the surrounding context has
- ;; changed such that string->pattern wouldn't add an `any'
- ;; here any more.
- (if unique
- ;; If the common prefix is unique, it also is a common
- ;; suffix, so we should add it for `prefix' elements.
- (push prefix res)
- ;; `prefix' only wants to include the fixed part before the
- ;; wildcard, not the result of growing that fixed part.
- (when (seq-some (lambda (elem) (eq elem 'prefix)) wildcards)
- (setq prefix ""))
- (push prefix res)
- ;; Push all the wildcards in this stretch, to preserve `point' and
- ;; `star' wildcards before ELEM.
- (setq res (append wildcards res))
- ;; Extract common suffix additionally to common prefix.
- ;; Don't do it for `any' since it could lead to a merged
- ;; completion that doesn't itself match the candidates.
- (when (and (seq-some (lambda (elem) (memq elem '(star point prefix))) wildcards)
- ;; If prefix is one of the completions, there's no
- ;; suffix left to find.
- (not (assoc-string prefix comps t)))
- (let ((suffix
- (completion--common-suffix
- (if (zerop (length prefix)) comps
- ;; Ignore the chars in the common prefix, so we
- ;; don't merge '("abc" "abbc") as "ab*bc".
- (let ((skip (length prefix)))
- (mapcar (lambda (str) (substring str skip))
- comps))))))
- (cl-assert (stringp suffix))
- (unless (equal suffix "")
- (push suffix res))))
- ;; We pushed these wildcards on RES, so we're done with them.
- (setq wildcards nil))))))
- ;; We return it in reverse order.
- res)))))
+ (seq-do-indexed
+ (lambda (elem i)
+ (push (match-string (1+ i) str) (cdr elem)))
+ pattern-and-comps)))
+
+ ;; Then for each of those non-constant elements, extract the
+ ;; commonality between them.
+ (mapcan
+ (lambda (cons)
+ (let ((elem (car cons))
+ (comps (cdr cons)))
+ (if (stringp elem)
+ (let ((shared (try-completion elem comps)))
+ ;; Use `try-completion' to merge the strings from each
+ ;; completion, which may differ in case.
+ (cond ((eq shared t) (list elem))
+ ((stringp shared) (list shared))))
+ (let* ((wildcards elem)
+ (prefix (try-completion "" comps))
+ (unique (or (and (eq prefix t) (setq prefix ""))
+ (and (stringp prefix)
+ ;; If PREFIX is equal to all of COMPS,
+ ;; then PREFIX is a unique completion.
+ (seq-every-p
+ (lambda (comp) (= (length prefix) (length comp)))
+ comps)))))
+ ;; If there's only one completion, `elem' is not useful
+ ;; any more: it can only match the empty string.
+ ;; FIXME: in some cases, it may be necessary to turn an
+ ;; `any' into a `star' because the surrounding context has
+ ;; changed such that string->pattern wouldn't add an `any'
+ ;; here any more.
+ (if unique
+ ;; If the common prefix is unique, it also is a common
+ ;; suffix, so we should add it for `prefix' elements.
+ (list prefix)
+ (delq nil
+ (append
+ ;; `prefix' only wants to include the fixed part before the
+ ;; wildcard, not the result of growing that fixed part.
+ (unless (seq-some (lambda (elem) (eq elem 'prefix)) wildcards)
+ (list prefix))
+ wildcards
+ ;; Extract common suffix additionally to common prefix.
+ ;; Don't do it for `any' since it could lead to a merged
+ ;; completion that doesn't itself match the candidates.
+ (when (and (seq-some (lambda (elem) (memq elem '(star point prefix))) wildcards)
+ ;; If prefix is one of the completions, there's no
+ ;; suffix left to find.
+ (not (assoc-string prefix comps t)))
+ (list (completion--common-suffix
+ (if (zerop (length prefix)) comps
+ ;; Ignore the chars in the common prefix, so we
+ ;; don't merge '("abc" "abbc") as "ab*bc".
+ (let ((skip (length prefix)))
+ (mapcar (lambda (str) (substring str skip))
+ comps)))))))))))))
+ pattern-and-comps)))))
(defun completion-pcm--pattern->string (pattern)
(mapconcat (lambda (x) (cond
@@ -4774,7 +4773,7 @@ completion-pcm--merge-try
(equal (completion-pcm--pattern->string pattern) (car all)))
t)
(t
- (let* ((mergedpat (completion-pcm--merge-completions all pattern))
+ (let* ((mergedpat (nreverse (completion-pcm--merge-completions all pattern)))
;; `mergedpat' is in reverse order. Place new point (by
;; order of preference) either at the old point, or at
;; the last place where there's something to choose, or
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 123ca849f3b..5b2e6f96af0 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -329,6 +329,14 @@ completion-pcm-test-8
"" '("fooxbar" "fooybar") nil 0)
'("foobar" . 3))))
+(ert-deftest completion-pcm-bug4219 ()
+ ;; With `completion-ignore-case', try-completion should change the
+ ;; case of existing text when the completions have different casing.
+ (should (equal
+ (let ((completion-ignore-case t))
+ (completion-pcm-try-completion "a" '("ABC" "ABD") nil 1))
+ '("AB" . 2))))
+
(ert-deftest completion-substring-test-1 ()
;; One third of a match!
(should (equal
--
2.43.7
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Thu, 28 Aug 2025 18:43:02 GMT) Full text and rfc822 format available.Message #49 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion, [PATCH] Refactor completion-pcm--merge-completions Date: Thu, 28 Aug 2025 14:42:46 -0400
[Message part 1 (text/plain, inline)]
Spencer Baugh <sbaugh <at> janestreet.com> writes: > Stefan Monnier <monnier <at> iro.umontreal.ca> writes: > >>> Two things: >>> >>> - It should have somewhat better performance because we're running >>> try-completion on smaller strings and therefore doing less string >>> comparison and work. >> >> Do we have concrete evidence to believe this? > > No evidence. So nevermind the performance argument, I suppose. > >>> CCS previously contained, for each completion, a list containing one >>> element for each wildcard element of PATTERN. >>> >>> Now CCS contains, for each completion, a list containing one element for >>> each element of PATTERN, both fixed strings and wildcards. >> >> Exactly: that means CCS is more costly to build and AFAICT it also >> implies correspondingly more calls to `try-completion`. >> >> I don't find it obvious that the new code will be more efficient. >> Barring any clear evidence that one of the two versions is noticeably >> more efficient than the other, I'd recommend we stick to the code we >> happen to have. >> >> From where I stand, my impression is that neither version is terribly >> better nor terribly worse than the other, in terms of clarity, >> simplicity, and efficiency. > > That's fair, that patch alone is not worth much. > > Attached is a patch (which applies on master) which does most of the > rest of the refactoring that I wanted to do; it makes the dolist over > the pattern into a simple mapcan over the pattern, with no state > preserved between elements of the pattern. IMO, this is much easier to > follow (and I think it would make the various bugs I've fixed in the > last couple years in merge-completions, easier to catch). What do you > think? This patch has a small bug with any-delim, here's a fixed version, now with a test for that case.
[0001-Refactor-completion-pcm-merge-completions.patch (text/x-patch, inline)]
From 8f33ad72f43f40c6613cac5d1466747c7df796c2 Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 25 Aug 2025 14:27:19 -0400
Subject: [PATCH] Refactor completion-pcm--merge-completions
Further simplify completion-pcm--merge-completions. Also fix a
bug I introduced in b511c38bba5354ff21c697e4d27279bf73e4d3cf
which made it no longer change the case of text to match the
completions.
* lisp/minibuffer.el (completion-pcm--pattern->regex): When
GROUP is group-all, also group strings in PATTERN.
(completion-pcm--merge-completions): Preserve case from parts of
completions matching fixed strings in PATTERN. (bug#79265)
* test/lisp/minibuffer-tests.el (completion-pcm-bug4219): Add.
---
lisp/minibuffer.el | 165 +++++++++++++++++-----------------
test/lisp/minibuffer-tests.el | 15 ++++
2 files changed, 97 insertions(+), 83 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 0221af5426e..7921d3ddf84 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4249,7 +4249,11 @@ completion-pcm--pattern->regex
(mapconcat
(lambda (x)
(cond
- ((stringp x) (regexp-quote x))
+ ((stringp x)
+ (let ((re (regexp-quote x)))
+ (if (eq group 'group-all)
+ (concat "\\(" re "\\)")
+ re)))
(t
(let ((re (if (eq x 'any-delim)
(concat completion-pcm--delim-wild-regex "*?")
@@ -4622,6 +4626,19 @@ completion--common-suffix
"Return the common suffix of the strings STRS."
(nreverse (try-completion "" (mapcar #'reverse strs))))
+(defun completion-pcm--group-pattern (pattern)
+ "Group together adjacent wildcards in PATTERN."
+ (let (ret)
+ (dolist (elem pattern)
+ (cond
+ ((or (stringp elem) (eq elem 'any-delim))
+ (push elem ret))
+ ((consp (car-safe ret))
+ (setf (car ret) (append (car ret) (list elem))))
+ (t
+ (push (list elem) ret))))
+ (nreverse ret)))
+
(defun completion-pcm--merge-completions (strs pattern)
"Extract the commonality in STRS, with the help of PATTERN.
PATTERN can contain strings and symbols chosen among `star', `any', `point',
@@ -4648,93 +4665,75 @@ completion-pcm--merge-completions
(cond
((null (cdr strs)) (list (car strs)))
(t
- (let ((re (concat
- (completion-pcm--pattern->regex pattern 'group)
- ;; The implicit trailing `any' is greedy.
- "\\([^z-a]*\\)"))
- (ccs ())) ;Chopped completions.
-
+ (let* ((pattern (completion-pcm--group-pattern (append pattern '(any))))
+ (re (concat
+ ;; Force matching the entire string.
+ (completion-pcm--pattern->regex pattern 'group-all) (rx eos)))
+ (pattern-and-comps (mapcar (lambda (elem) (cons elem nil)) pattern)))
;; First match each string against PATTERN as a regex and extract
;; the text matched by each wildcard.
(let ((case-fold-search completion-ignore-case))
(dolist (str strs)
(unless (string-match re str)
(error "Internal error: %s doesn't match %s" str re))
- (let ((chopped ())
- (i 1)
- next)
- (while (setq next (match-string i str))
- (push next chopped)
- (setq i (1+ i)))
- (push (nreverse chopped) ccs))))
-
- ;; Then for each of those wildcards, extract the commonality between them.
- (let ((res ())
- ;; Accumulate each stretch of wildcards, and process them as a unit.
- (wildcards ()))
- ;; Make the implicit trailing `any' explicit.
- (dolist (elem (append pattern '(any)))
- (if (stringp elem)
- (progn
- (push elem res)
- (setq wildcards nil))
- (let ((comps ()))
- (push elem wildcards)
- (dolist (cc (prog1 ccs (setq ccs nil)))
- (push (car cc) comps)
- (push (cdr cc) ccs))
- ;; Might improve the likelihood to avoid choosing
- ;; different capitalizations in different parts.
- ;; In practice, it doesn't seem to make any difference.
- (setq ccs (nreverse ccs))
- (let* ((prefix (try-completion "" comps))
- (unique (or (and (eq prefix t) (setq prefix ""))
- (and (stringp prefix)
- ;; If PREFIX is equal to all of COMPS,
- ;; then PREFIX is a unique completion.
- (seq-every-p
- (lambda (comp) (= (length prefix) (length comp)))
- comps)))))
- ;; If there's only one completion, `elem' is not useful
- ;; any more: it can only match the empty string.
- ;; FIXME: in some cases, it may be necessary to turn an
- ;; `any' into a `star' because the surrounding context has
- ;; changed such that string->pattern wouldn't add an `any'
- ;; here any more.
- (if unique
- ;; If the common prefix is unique, it also is a common
- ;; suffix, so we should add it for `prefix' elements.
- (push prefix res)
- ;; `prefix' only wants to include the fixed part before the
- ;; wildcard, not the result of growing that fixed part.
- (when (seq-some (lambda (elem) (eq elem 'prefix)) wildcards)
- (setq prefix ""))
- (push prefix res)
- ;; Push all the wildcards in this stretch, to preserve `point' and
- ;; `star' wildcards before ELEM.
- (setq res (append wildcards res))
- ;; Extract common suffix additionally to common prefix.
- ;; Don't do it for `any' since it could lead to a merged
- ;; completion that doesn't itself match the candidates.
- (when (and (seq-some (lambda (elem) (memq elem '(star point prefix))) wildcards)
- ;; If prefix is one of the completions, there's no
- ;; suffix left to find.
- (not (assoc-string prefix comps t)))
- (let ((suffix
- (completion--common-suffix
- (if (zerop (length prefix)) comps
- ;; Ignore the chars in the common prefix, so we
- ;; don't merge '("abc" "abbc") as "ab*bc".
- (let ((skip (length prefix)))
- (mapcar (lambda (str) (substring str skip))
- comps))))))
- (cl-assert (stringp suffix))
- (unless (equal suffix "")
- (push suffix res))))
- ;; We pushed these wildcards on RES, so we're done with them.
- (setq wildcards nil))))))
- ;; We return it in reverse order.
- res)))))
+ (seq-do-indexed
+ (lambda (elem i)
+ (push (match-string (1+ i) str) (cdr elem)))
+ pattern-and-comps)))
+
+ ;; Then for each of those non-constant elements, extract the
+ ;; commonality between them.
+ (mapcan
+ (lambda (cons)
+ (let ((elem (car cons))
+ (comps (cdr cons)))
+ (if (stringp elem)
+ (let ((shared (try-completion elem comps)))
+ ;; Use `try-completion' to merge the strings from each
+ ;; completion, which may differ in case.
+ (cond ((eq shared t) (list elem))
+ ((stringp shared) (list shared))))
+ (let* ((wildcards (ensure-list elem))
+ (prefix (try-completion "" comps))
+ (unique (or (and (eq prefix t) (setq prefix ""))
+ (and (stringp prefix)
+ ;; If PREFIX is equal to all of COMPS,
+ ;; then PREFIX is a unique completion.
+ (seq-every-p
+ (lambda (comp) (= (length prefix) (length comp)))
+ comps)))))
+ ;; If there's only one completion, `elem' is not useful
+ ;; any more: it can only match the empty string.
+ ;; FIXME: in some cases, it may be necessary to turn an
+ ;; `any' into a `star' because the surrounding context has
+ ;; changed such that string->pattern wouldn't add an `any'
+ ;; here any more.
+ (if unique
+ ;; If the common prefix is unique, it also is a common
+ ;; suffix, so we should add it for `prefix' elements.
+ (list prefix)
+ (delq nil
+ (append
+ ;; `prefix' only wants to include the fixed part before the
+ ;; wildcard, not the result of growing that fixed part.
+ (unless (seq-some (lambda (elem) (eq elem 'prefix)) wildcards)
+ (list prefix))
+ wildcards
+ ;; Extract common suffix additionally to common prefix.
+ ;; Don't do it for `any' since it could lead to a merged
+ ;; completion that doesn't itself match the candidates.
+ (when (and (seq-some (lambda (elem) (memq elem '(star point prefix))) wildcards)
+ ;; If prefix is one of the completions, there's no
+ ;; suffix left to find.
+ (not (assoc-string prefix comps t)))
+ (list (completion--common-suffix
+ (if (zerop (length prefix)) comps
+ ;; Ignore the chars in the common prefix, so we
+ ;; don't merge '("abc" "abbc") as "ab*bc".
+ (let ((skip (length prefix)))
+ (mapcar (lambda (str) (substring str skip))
+ comps)))))))))))))
+ pattern-and-comps)))))
(defun completion-pcm--pattern->string (pattern)
(mapconcat (lambda (x) (cond
@@ -4774,7 +4773,7 @@ completion-pcm--merge-try
(equal (completion-pcm--pattern->string pattern) (car all)))
t)
(t
- (let* ((mergedpat (completion-pcm--merge-completions all pattern))
+ (let* ((mergedpat (nreverse (completion-pcm--merge-completions all pattern)))
;; `mergedpat' is in reverse order. Place new point (by
;; order of preference) either at the old point, or at
;; the last place where there's something to choose, or
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 123ca849f3b..bede9713aa1 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -329,6 +329,21 @@ completion-pcm-test-8
"" '("fooxbar" "fooybar") nil 0)
'("foobar" . 3))))
+(ert-deftest completion-pcm-test-anydelim ()
+ ;; After each delimiter is a special wildcard which matches any
+ ;; sequence of delimiters.
+ (should (equal (completion-pcm-try-completion
+ "-x" '("-_.x" "-__x") nil 2)
+ '("-_x" . 3))))
+
+(ert-deftest completion-pcm-bug4219 ()
+ ;; With `completion-ignore-case', try-completion should change the
+ ;; case of existing text when the completions have different casing.
+ (should (equal
+ (let ((completion-ignore-case t))
+ (completion-pcm-try-completion "a" '("ABC" "ABD") nil 1))
+ '("AB" . 2))))
+
(ert-deftest completion-substring-test-1 ()
;; One third of a match!
(should (equal
--
2.43.7
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Mon, 01 Sep 2025 05:42:02 GMT) Full text and rfc822 format available.Message #52 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Spencer Baugh <sbaugh <at> janestreet.com> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion, [PATCH] Refactor completion-pcm--merge-completions Date: Mon, 01 Sep 2025 01:41:37 -0400
>> Attached is a patch (which applies on master) which does most of the
>> rest of the refactoring that I wanted to do; it makes the dolist over
>> the pattern into a simple mapcan over the pattern, with no state
>> preserved between elements of the pattern. IMO, this is much easier to
>> follow (and I think it would make the various bugs I've fixed in the
>> last couple years in merge-completions, easier to catch). What do you
>> think?
I still don't understand what this is trying to do. It still seems more
costly since it allocates more strings and conses onto
`pattern-and-comps` than the previous code did with `ccs` (the
"corresponding" variable).
Also, can we just revert the last patch (and add the test(s) that we
found in the mean time) so as to make it easier to review patches?
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -4249,7 +4249,11 @@ completion-pcm--pattern->regex
> (mapconcat
> (lambda (x)
> (cond
> - ((stringp x) (regexp-quote x))
> + ((stringp x)
> + (let ((re (regexp-quote x)))
> + (if (eq group 'group-all)
> + (concat "\\(" re "\\)")
> + re)))
> (t
> (let ((re (if (eq x 'any-delim)
> (concat completion-pcm--delim-wild-regex "*?")
I think we should aim for a patch which doesn't use that many subgroups.
> @@ -4622,6 +4626,19 @@ completion--common-suffix
> "Return the common suffix of the strings STRS."
> (nreverse (try-completion "" (mapcar #'reverse strs))))
>
> +(defun completion-pcm--group-pattern (pattern)
> + "Group together adjacent wildcards in PATTERN."
> + (let (ret)
> + (dolist (elem pattern)
> + (cond
> + ((or (stringp elem) (eq elem 'any-delim))
> + (push elem ret))
> + ((consp (car-safe ret))
> + (setf (car ret) (append (car ret) (list elem))))
> + (t
> + (push (list elem) ret))))
> + (nreverse ret)))
Why is `any-delim` not considered a "wildcard"?
Maybe a better grouping would be to turn
("a" "c" any "b" point)
into something like
(("a" "c" any) ("b" point))
or
(("ac" any) ("b" point))
where each sublist will get its own regexp subgroup and will have its
corresponding `try-completion`.
> + ;; Then for each of those non-constant elements, extract the
> + ;; commonality between them.
AFAICT this includes "constant" elements, so either I'm missing
something or the comment is off.
> @@ -4774,7 +4773,7 @@ completion-pcm--merge-try
> (equal (completion-pcm--pattern->string pattern) (car all)))
> t)
> (t
> - (let* ((mergedpat (completion-pcm--merge-completions all pattern))
> + (let* ((mergedpat (nreverse (completion-pcm--merge-completions all pattern)))
> ;; `mergedpat' is in reverse order. Place new point (by
> ;; order of preference) either at the old point, or at
> ;; the last place where there's something to choose, or
Shouldn't that require an update to the subsequent comment?
Or is it that `completion-pcm--merge-completions` now doesn't return the
merged pat in reverse order, so we need an explicit `nreverse` to
recover the previous reserve-order merged pat?
Stefan
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Mon, 01 Sep 2025 15:57:02 GMT) Full text and rfc822 format available.Message #55 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Spencer Baugh <sbaugh <at> janestreet.com> To: Stefan Monnier <monnier <at> iro.umontreal.ca> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion, [PATCH] Refactor completion-pcm--merge-completions Date: Mon, 01 Sep 2025 11:56:30 -0400
[Message part 1 (text/plain, inline)]
Stefan Monnier <monnier <at> iro.umontreal.ca> writes:
>>> Attached is a patch (which applies on master) which does most of the
>>> rest of the refactoring that I wanted to do; it makes the dolist over
>>> the pattern into a simple mapcan over the pattern, with no state
>>> preserved between elements of the pattern. IMO, this is much easier to
>>> follow (and I think it would make the various bugs I've fixed in the
>>> last couple years in merge-completions, easier to catch). What do you
>>> think?
>
> I still don't understand what this is trying to do. It still seems more
> costly since it allocates more strings and conses onto
> `pattern-and-comps` than the previous code did with `ccs` (the
> "corresponding" variable).
My goal at this point is just to make this code simpler and easier to
understand, since I keep finding bugs in it. But that certainly should
not be done in a way that makes performance worse, and you're right that
it may currently do that.
> Also, can we just revert the last patch (and add the test(s) that we
> found in the mean time) so as to make it easier to review patches?
Certainly, the attached patch does that.
(I would have just done the revert directly but I don't have the ability
to push to the emacs repo)
>> --- a/lisp/minibuffer.el
>> +++ b/lisp/minibuffer.el
>> @@ -4249,7 +4249,11 @@ completion-pcm--pattern->regex
>> (mapconcat
>> (lambda (x)
>> (cond
>> - ((stringp x) (regexp-quote x))
>> + ((stringp x)
>> + (let ((re (regexp-quote x)))
>> + (if (eq group 'group-all)
>> + (concat "\\(" re "\\)")
>> + re)))
>> (t
>> (let ((re (if (eq x 'any-delim)
>> (concat completion-pcm--delim-wild-regex "*?")
>
> I think we should aim for a patch which doesn't use that many subgroups.
Well - just as a note, this patch uses only one subgroup for each
sequence of wildcards, which reduces the number of subgroups for some
kinds of pattern.
>> @@ -4622,6 +4626,19 @@ completion--common-suffix
>> "Return the common suffix of the strings STRS."
>> (nreverse (try-completion "" (mapcar #'reverse strs))))
>>
>> +(defun completion-pcm--group-pattern (pattern)
>> + "Group together adjacent wildcards in PATTERN."
>> + (let (ret)
>> + (dolist (elem pattern)
>> + (cond
>> + ((or (stringp elem) (eq elem 'any-delim))
>> + (push elem ret))
>> + ((consp (car-safe ret))
>> + (setf (car ret) (append (car ret) (list elem))))
>> + (t
>> + (push (list elem) ret))))
>> + (nreverse ret)))
>
> Why is `any-delim` not considered a "wildcard"?
Because it's matched by a different regex which only matches delimiters.
The idea here is to have one regex subgroup for each stretch of
wildcards; i.e., for the pattern '("foo" any star point) we have the
regex "foo(.*)" instead of "foo(.*)(.*)(.*)". But any-delim needs its
own regex subgroup since its matched by a different regex.
> Maybe a better grouping would be to turn
>
> ("a" "c" any "b" point)
>
> into something like
>
> (("a" "c" any) ("b" point))
>
> or
>
> (("ac" any) ("b" point))
>
> where each sublist will get its own regexp subgroup and will have its
> corresponding `try-completion`.
That's a great idea, and it matches up with things I was already
thinking of doing. I'll try that in the next version of this patch.
>> + ;; Then for each of those non-constant elements, extract the
>> + ;; commonality between them.
>
> AFAICT this includes "constant" elements, so either I'm missing
> something or the comment is off.
>
>> @@ -4774,7 +4773,7 @@ completion-pcm--merge-try
>> (equal (completion-pcm--pattern->string pattern) (car all)))
>> t)
>> (t
>> - (let* ((mergedpat (completion-pcm--merge-completions all pattern))
>> + (let* ((mergedpat (nreverse (completion-pcm--merge-completions all pattern)))
>> ;; `mergedpat' is in reverse order. Place new point (by
>> ;; order of preference) either at the old point, or at
>> ;; the last place where there's something to choose, or
>
> Shouldn't that require an update to the subsequent comment?
> Or is it that `completion-pcm--merge-completions` now doesn't return the
> merged pat in reverse order, so we need an explicit `nreverse` to
> recover the previous reserve-order merged pat?
Yes, the return value of completion-pcm--merge-completions is no longer
in reverse order.
The attached patch just reverts my original broken change and adds
tests. After landing that, I'll open a new bug if I want to further
work on a patch like this.
[0001-Revert-Avoid-duplicating-strings-in-pcm-merge-comple.patch (text/x-patch, inline)]
From 2bbb3e31467449be25286af5f03078c3fbc5ac8b Mon Sep 17 00:00:00 2001
From: Spencer Baugh <sbaugh <at> janestreet.com>
Date: Mon, 1 Sep 2025 11:43:25 -0400
Subject: [PATCH] Revert "Avoid duplicating strings in pcm--merge-completions"
Revert "Avoid duplicating strings in pcm--merge-completions",
commit b511c38bba5354ff21c697e4d27279bf73e4d3cf. It broke
existing behavior, now covered by tests adding in this commit.
* lisp/minibuffer.el (completion-pcm--merge-completions):
* test/lisp/minibuffer-tests.el (completion-pcm-test-anydelim):
(completion-pcm-bug4219):
---
lisp/minibuffer.el | 39 +++++++++++++++++++++--------------
test/lisp/minibuffer-tests.el | 15 ++++++++++++++
2 files changed, 39 insertions(+), 15 deletions(-)
diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index 1c3c4bd0681..6617aee2d5a 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4639,35 +4639,38 @@ completion-pcm--merge-completions
(cond
((null (cdr strs)) (list (car strs)))
(t
- (let ((re (concat
- (completion-pcm--pattern->regex pattern 'group)
- ;; The implicit trailing `any' is greedy.
- "\\([^z-a]*\\)"))
+ (let ((re (completion-pcm--pattern->regex pattern 'group))
(ccs ())) ;Chopped completions.
- ;; First match each string against PATTERN as a regex and extract
- ;; the text matched by each wildcard.
+ ;; First chop each string into the parts corresponding to each
+ ;; non-constant element of `pattern', using regexp-matching.
(let ((case-fold-search completion-ignore-case))
(dolist (str strs)
(unless (string-match re str)
(error "Internal error: %s doesn't match %s" str re))
(let ((chopped ())
+ (last 0)
(i 1)
next)
- (while (setq next (match-string i str))
- (push next chopped)
+ (while (setq next (match-end i))
+ (push (substring str last next) chopped)
+ (setq last next)
(setq i (1+ i)))
+ ;; Add the text corresponding to the implicit trailing `any'.
+ (push (substring str last) chopped)
(push (nreverse chopped) ccs))))
- ;; Then for each of those wildcards, extract the commonality between them.
+ ;; Then for each of those non-constant elements, extract the
+ ;; commonality between them.
(let ((res ())
+ (fixed "")
;; Accumulate each stretch of wildcards, and process them as a unit.
(wildcards ()))
;; Make the implicit trailing `any' explicit.
(dolist (elem (append pattern '(any)))
(if (stringp elem)
(progn
- (push elem res)
+ (setq fixed (concat fixed elem))
(setq wildcards nil))
(let ((comps ()))
(push elem wildcards)
@@ -4678,13 +4681,18 @@ completion-pcm--merge-completions
;; different capitalizations in different parts.
;; In practice, it doesn't seem to make any difference.
(setq ccs (nreverse ccs))
- (let* ((prefix (try-completion "" comps))
- (unique (or (and (eq prefix t) (setq prefix ""))
+ ;; FIXED is a prefix of all of COMPS. Try to grow that prefix.
+ (let* ((prefix (try-completion fixed comps))
+ (unique (or (and (eq prefix t) (setq prefix fixed))
(and (stringp prefix)
;; If PREFIX is equal to all of COMPS,
;; then PREFIX is a unique completion.
(seq-every-p
- (lambda (comp) (= (length prefix) (length comp)))
+ ;; PREFIX is still a prefix of all of
+ ;; COMPS, so if COMP is the same length,
+ ;; they're equal.
+ (lambda (comp)
+ (= (length prefix) (length comp)))
comps)))))
;; If there's only one completion, `elem' is not useful
;; any more: it can only match the empty string.
@@ -4699,7 +4707,7 @@ completion-pcm--merge-completions
;; `prefix' only wants to include the fixed part before the
;; wildcard, not the result of growing that fixed part.
(when (seq-some (lambda (elem) (eq elem 'prefix)) wildcards)
- (setq prefix ""))
+ (setq prefix fixed))
(push prefix res)
;; Push all the wildcards in this stretch, to preserve `point' and
;; `star' wildcards before ELEM.
@@ -4723,7 +4731,8 @@ completion-pcm--merge-completions
(unless (equal suffix "")
(push suffix res))))
;; We pushed these wildcards on RES, so we're done with them.
- (setq wildcards nil))))))
+ (setq wildcards nil))
+ (setq fixed "")))))
;; We return it in reverse order.
res)))))
diff --git a/test/lisp/minibuffer-tests.el b/test/lisp/minibuffer-tests.el
index 99753f31330..de1a98c8189 100644
--- a/test/lisp/minibuffer-tests.el
+++ b/test/lisp/minibuffer-tests.el
@@ -332,6 +332,21 @@ completion-pcm-test-8
"" '("fooxbar" "fooybar") nil 0)
'("foobar" . 3))))
+(ert-deftest completion-pcm-test-anydelim ()
+ ;; After each delimiter is a special wildcard which matches any
+ ;; sequence of delimiters.
+ (should (equal (completion-pcm-try-completion
+ "-x" '("-_.x" "-__x") nil 2)
+ '("-_x" . 3))))
+
+(ert-deftest completion-pcm-bug4219 ()
+ ;; With `completion-ignore-case', try-completion should change the
+ ;; case of existing text when the completions have different casing.
+ (should (equal
+ (let ((completion-ignore-case t))
+ (completion-pcm-try-completion "a" '("ABC" "ABD") nil 1))
+ '("AB" . 2))))
+
(ert-deftest completion-substring-test-1 ()
;; One third of a match!
(should (equal
--
2.43.7
bug-gnu-emacs <at> gnu.org:bug#79265; Package emacs.
(Mon, 01 Sep 2025 21:26:02 GMT) Full text and rfc822 format available.Message #58 received at 79265 <at> debbugs.gnu.org (full text, mbox):
From: Stefan Monnier <monnier <at> iro.umontreal.ca> To: Spencer Baugh <sbaugh <at> janestreet.com> Cc: Dmitry Gutov <dmitry <at> gutov.dev>, 79265 <at> debbugs.gnu.org Subject: Re: bug#79265: [PATCH] Treat point more consistently in PCM completion, [PATCH] Treat point more consistently in PCM completion, [PATCH] Refactor completion-pcm--merge-completions Date: Mon, 01 Sep 2025 17:24:45 -0400
>> Also, can we just revert the last patch (and add the test(s) that we
>> found in the mean time) so as to make it easier to review patches?
>
> Certainly, the attached patch does that.
>
> (I would have just done the revert directly but I don't have the ability
> to push to the emacs repo)
Sorry, forgot about that. Pushed, thanks.
>>> +(defun completion-pcm--group-pattern (pattern)
>>> + "Group together adjacent wildcards in PATTERN."
>>> + (let (ret)
>>> + (dolist (elem pattern)
>>> + (cond
>>> + ((or (stringp elem) (eq elem 'any-delim))
>>> + (push elem ret))
>>> + ((consp (car-safe ret))
>>> + (setf (car ret) (append (car ret) (list elem))))
>>> + (t
>>> + (push (list elem) ret))))
>>> + (nreverse ret)))
>>
>> Why is `any-delim` not considered a "wildcard"?
>
> Because it's matched by a different regex which only matches
> delimiters. The idea here is to have one regex subgroup for each
> stretch of wildcards; i.e., for the pattern '("foo" any star point) we
> have the regex "foo(.*)" instead of "foo(.*)(.*)(.*)". But any-delim
> needs its own regex subgroup since its matched by a different regex.
Ah, I see, thanks. I was thinking in terms of "needs a subgroup and
a `try-completion`", which `any-delim` does need. But yeah, it's also
good to collapse any/star/point (also because it can avoid some
pathological regexp matching performance issues, tho I haven't bumped
into them).
> Yes, the return value of completion-pcm--merge-completions is no
> longer in reverse order.
Thanks, makes a lot more sense now. 🙂
Stefan
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.