Subject: Re: bug#61221: 30.0.50; [PATCH] Support completion of quoted variable refs in Eshell
From: Jim Porter <[email-masked]>
To: Stefan Monnier <[email-masked]>
Cc: [email-masked]
Date: Thu, 23 Feb 2023 14:11:54 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

On 2/23/2023 10:02 AM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> The patches look good, feel free to push, AFAIC.

Thanks, merged as 9d48c9844b.

>> +    (mapcar (lambda (x) (car x))
> 
> Aka (mapcar #'car

Fixed.

>> +      (setq pcomplete-stub (substring arg (match-beginning 1))
>> +            delimiter (match-string 2 arg))
> 
> You could let-bind `delimiter` here instead of let-binding it earlier
> and then `setq`ing it here.  Better for karma and marginally more
> efficient (avoids the creation of a `cons` cell to contain the value of
> the var).

Done.

>> +     (append (eshell-envvar-names)
>> +             (all-completions argname obarray 'boundp))
>> +     #'string-lessp)))
> 
> Since you use #' for `string-lessp`, it would make sense to use #' for
> `boundp` as well :-)

Fixed.

>> +                   ('lambda               ; test-completion
>> +                     (let ((result (test-completion string names pred)))
>> +                       (if (eq result t) string result)))
> 
> Hmm... why not just always return `result`?

As I understand it, returning 't' means "there is just one matching 
completion, and the match is exact"[1], but in this case, that's not 
really true: after completing "~user/" there are still more matching 
completions (the contents of the user's home directory).

This is really just trying to match what happens when calling 
'completion-file-name-table':

   (completion-file-name-table "~user/" nil nil)
     => "~user/"

   (try-completion "~user/" '("~user/") nil)
     => t

So if we get 't' from 'try-completion', we "downgrade" that to the 
original string.

[1] 
https://www.gnu.org/software/emacs/manual/html_node/elisp/Basic-Completion.html