Stefan Monnier <monnier@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.