GNU bug report logs - #61221
30.0.50; [PATCH] Support completion of quoted variable refs in Eshell

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 61221 in the body.
You can then email your comments to 61221 AT debbugs.gnu.org in the normal way.

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

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 30.0.50; [PATCH] Support completion of quoted variable refs in Eshell
Date: Wed, 1 Feb 2023 18:28:08 -0800

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

X-Debbugs-Cc: monnier <at> iro.umontreal.ca

Eshell lets you put quotes around variable names so that the parser can 
tell where the name ends, sort of like ${var} in other shells:

  ~ $ echo $'user-login-name'-suffix
  user-suffix

  ~ $ echo $"user-login-name"-suffix
  user-suffix

However, you can't tab-complete variable names when you do this. Here's 
a fix. I also fixed a couple small issues with completing directory 
names where it would sometimes complete to "whatever/ ". That extra 
trailing space isn't helpful, since you'd have to delete it before 
typing in a subdir.

Probably the most controversial part of this patch is in #0002, where I 
added another dynamic variable 'pcomplete-exit-function' that Pcomplete 
handlers can set to tell Pcomplete what to do after exiting a 
completion. Maybe it would be better to have handlers throw some special 
value for 'pcomplete-completions' that contains this info (sort of like 
the value that a 'completion-at-point-function' returns). I'm not sure 
what the best (and most-compatible) way to do this would be...

[0001-Throw-strings-as-the-values-for-eshell-incomplete.patch (text/plain, attachment)]

[0002-Add-support-for-completing-quoted-variables-in-Eshel.patch (text/plain, attachment)]

[0003-Don-t-add-a-space-after-the-trailing-slash-when-comp.patch (text/plain, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 61221 <at> debbugs.gnu.org
Subject: Re: bug#61221: 30.0.50; [PATCH] Support completion of quoted
 variable refs in Eshell
Date: Sun, 05 Feb 2023 10:33:42 -0500

> Probably the most controversial part of this patch is in #0002, where
> I added another dynamic variable 'pcomplete-exit-function' that Pcomplete
> handlers can set to tell Pcomplete what to do after exiting
> a completion. Maybe it would be better to have handlers throw some special
> value for 'pcomplete-completions' that contains this info (sort of like the
> value that a 'completion-at-point-function' returns). I'm not sure what the
> best (and most-compatible) way to do this would be...

BTW, a low-tech way to get similar results is to change your completion
table.  Instead of having entries like

   user-login-name

make it contains entries like:

   user-login-name"

or

   user-login-name'

or even

   'user-login-name'

[ assuming you change the `pcomplete-stub` so that it also contains the
  leading ' of course.  ]

Regarding `pcomplete-exit-function`:
- It's ugly and I don't like it, but given the general design of the
  current Pcomplete API, I think it's OK.
- Please add a good docstring to that variable (the fact that the other
  vars nearby don't have any is no excuse, instead it's a(nother)
  problem that should be fixed).
- Maybe `pcomplete-exit-function` should override the default exit
  function (i.e. the handing of `pcomplete-termination-string`) rather
  than being added to it?
  E.g. we could start with `pcomplete-exit-function` bound to the
  default (the one that handles `pcomplete-termination-string`) and let
  the rest of the code modify it either with `setq` (to completely
  override) or `add-function`.


        Stefan

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 61221 <at> debbugs.gnu.org
Subject: Re: bug#61221: 30.0.50; [PATCH] Support completion of quoted variable
 refs in Eshell
Date: Wed, 22 Feb 2023 22:42:07 -0800

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

On 2/5/2023 7:33 AM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
> BTW, a low-tech way to get similar results is to change your completion
> table.  Instead of having entries like
> 
>     user-login-name
> 
> make it contains entries like:
> 
>     user-login-name" ...

I'd thought about this, but it gets a little uglier for more-complex 
cases like bug#53371, which wants to complete things like the "foo" in 
"#<buffer foo>".

> Regarding `pcomplete-exit-function`:
> - It's ugly and I don't like it, but given the general design of the
>    current Pcomplete API, I think it's OK.

Yeah, I'd rather do it by returning some structured object, but that 
would probably require some significant incompatible changes to pcomplete...

> - Please add a good docstring to that variable (the fact that the other
>    vars nearby don't have any is no excuse, instead it's a(nother)
>    problem that should be fixed).

Done.

> - Maybe `pcomplete-exit-function` should override the default exit
>    function (i.e. the handing of `pcomplete-termination-string`) rather
>    than being added to it?
>    E.g. we could start with `pcomplete-exit-function` bound to the
>    default (the one that handles `pcomplete-termination-string`) and let
>    the rest of the code modify it either with `setq` (to completely
>    override) or `add-function`.

I like this a lot better than my previous solution. Done.

I also changed the last patch to return a programmed completion function 
that mimics "~user" completion in 'completion-file-name-table'. That 
seems like a more-consistent way to handle that case, since now Eshell 
"~user" completion works the same as completing any file name.

[0001-Throw-strings-as-the-values-for-eshell-incomplete.patch (text/plain, attachment)]

[0002-Add-support-for-completing-quoted-variables-in-Eshel.patch (text/plain, attachment)]

[0003-Don-t-add-a-space-after-the-trailing-slash-when-comp.patch (text/plain, attachment)]

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 61221 <at> debbugs.gnu.org
Subject: Re: bug#61221: 30.0.50; [PATCH] Support completion of quoted
 variable refs in Eshell
Date: Thu, 23 Feb 2023 13:02:06 -0500

The patches look good, feel free to push, AFAIC.
Some minor comments below.


        Stefan


> diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
> index a5bfbf4254d..e8e8cfb39b4 100644
> --- a/lisp/eshell/esh-var.el
> +++ b/lisp/eshell/esh-var.el
> @@ -434,9 +434,15 @@ eshell-insert-envvar
>  
>  (defun eshell-envvar-names (&optional environment)
>    "Return a list of currently visible environment variable names."
> -  (mapcar (lambda (x)
> -            (substring x 0 (string-search "=" x)))
> -	  (or environment process-environment)))
> +  (delete-dups
> +   (append
> +    ;; Real environment variables
> +    (mapcar (lambda (x)
> +              (substring x 0 (string-search "=" x)))
> +	    (or environment process-environment))
> +    ;; Eshell variable aliases
> +    (mapcar (lambda (x) (car x))

Aka (mapcar #'car

> @@ -817,36 +823,43 @@ eshell-index-value
>  
>  (defun eshell-complete-variable-reference ()
>    "If there is a variable reference, complete it."
> -  (let ((arg (pcomplete-actual-arg)))
> +  (let ((arg (pcomplete-actual-arg))
> +        delimiter)
>      (when (string-match
>             (rx "$" (? (or "#" "@"))
> -               (? (group (regexp eshell-variable-name-regexp)))
> -               string-end)
> +               (? (or (group-n 1 (regexp eshell-variable-name-regexp)
> +                               string-end)
> +                      (seq (group-n 2 (or "'" "\""))
> +                           (group-n 1 (+ anychar))))))
>             arg)
> -      (setq pcomplete-stub (substring arg (match-beginning 1)))
> +      (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).

> +     (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 :-)

> +                   ('lambda               ; test-completion
> +                     (let ((result (test-completion string names pred)))
> +                       (if (eq result t) string result)))

Hmm... why not just always return `result`?


        Stefan

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 61221-done <at> debbugs.gnu.org
Subject: Re: bug#61221: 30.0.50; [PATCH] Support completion of quoted variable
 refs in Eshell
Date: Thu, 23 Feb 2023 14:11:54 -0800

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

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

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 61221-done <at> debbugs.gnu.org
Subject: Re: bug#61221: 30.0.50; [PATCH] Support completion of quoted
 variable refs in Eshell
Date: Thu, 23 Feb 2023 17:39:02 -0500

>>> +                   ('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).

What you describe is for `try-completion`, not `test-completion`.

> 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

The `action` arg is not `lambda` is these examples.


        Stefan

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 61221-done <at> debbugs.gnu.org
Subject: Re: bug#61221: 30.0.50; [PATCH] Support completion of quoted variable
 refs in Eshell
Date: Thu, 23 Feb 2023 15:17:35 -0800

On 2/23/2023 2:39 PM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
>> 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).
> 
> What you describe is for `try-completion`, not `test-completion`.
> 
>> 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
> 
> The `action` arg is not `lambda` is these examples.

Ohhh. I totally missed that part. Pushed a fix as 0a361fd91a. Thanks 
again for finding my mistakes.

Previous Next

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