Re: [PATCH] em-cmpl: fix completion of command paths

[Stefan Monnier]

Hi Nicolas,

Terribly sorry for the long delay to answer.

> The use of `completion-table-dynamic' was introduced in 899055e to fix
> bug#48995. However the following issue remains: when completing a command
> path, absolute ("/usr/bin/") or relative ("./subdir/"), a space is
> automatically added at the end.
> Bypassing `completion-table-dynamic' for filenames containing a directory
> part fixes the final space bug and does not reintroduce bug#48995.

This looks very good.
IIUC this fixes a regression w.r.t Emacs-28, so it would be nice to
install it into `emacs-29`.

Eli, what do you think?


        Stefan


> As a result, `eshell--pcomplete-executables' is not needed anymore, it was
> only necessary to work around the way `completion-table-dynamic' works.
>
> Thanks to Stefan Monnier for his input on the problem.
> ---
>  lisp/eshell/em-cmpl.el | 127 ++++++++++++++++++-----------------------
>  1 file changed, 56 insertions(+), 71 deletions(-)
>
> diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
> index ca51cee2558..c5427fa1a6d 100644
> --- a/lisp/eshell/em-cmpl.el
> +++ b/lisp/eshell/em-cmpl.el
> @@ -378,31 +378,6 @@
>          args)
>         posns)))
>  
> -(defun eshell--pcomplete-executables ()
> -  "Complete amongst a list of directories and executables.
> -
> -Wrapper for `pcomplete-executables' or `pcomplete-dirs-or-entries',
> -depending on the value of `eshell-force-execution'.
> -
> -Adds path prefix to candidates independent of `action' value."
> -  ;; `pcomplete-entries' returns filenames without path on `action' to
> -  ;; use current string directory as done in `completion-file-name-table'
> -  ;; when `action' is nil to construct executable candidates.
> -  (let ((table (if eshell-force-execution
> -                   (pcomplete-dirs-or-entries nil #'file-readable-p)
> -                 (pcomplete-executables))))
> -    (lambda (string pred action)
> -      (let ((cands (funcall table string pred action)))
> -        (if (eq action t)
> -            (let ((specdir (file-name-directory string)))
> -              (mapcar
> -               (lambda (cand)
> -                 (if (stringp cand)
> -                     (file-name-concat specdir cand)
> -                   cand))
> -               cands))
> -          cands)))))
> -
>  (defun eshell--complete-commands-list ()
>    "Generate list of applicable, visible commands."
>    ;; Building the commands list can take quite a while, especially over Tramp
> @@ -413,11 +413,21 @@
>           ;; we complete.  Adjust `pcomplete-stub' accordingly!
>        (if (and (> (length pcomplete-stub) 0)
>                 (eq (aref pcomplete-stub 0) eshell-explicit-command-char))
> -          (setq pcomplete-stub (substring pcomplete-stub 1)))))
> +             (setq pcomplete-stub (substring pcomplete-stub 1))))
> +        (filename (pcomplete-arg)))
> +    ;; Do not use `completion-table-dynamic' when completing a command path
> +    ;; (absolute or relative): doing so assumes that the subpath in the input
> +    ;; string is always a command, and appends a space character, which is
> +    ;; incorrect (i.e. "/usr/bi" should yield "/usr/bin/" after completion,
> +    ;; not "/usr/bin/ ").
> +    ;;
> +    ;; If you work on this function, be careful not to reintroduce bug#48995.
> +    (if (file-name-directory filename)
> +        (if eshell-force-execution
> +            (pcomplete-dirs-or-entries nil #'file-readable-p)
> +          (pcomplete-executables))
>      (completion-table-dynamic
>       (lambda (filename)
> -       (if (file-name-directory filename)
> -           (eshell--pcomplete-executables)
>        (let* ((paths (eshell-get-path))
>               (cwd (file-name-as-directory
>                     (expand-file-name default-directory)))