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

[Eli Zaretskii]

> From: Nicolas Martyanoff <nicolas@n16f.net>
> Date: Wed, 25 Jan 2023 21:57:02 +0100
> 
> Well obviously I did not use git send-email correctly, so here is the
> modified patch attached manually.
> 
> Sorry about the noise.

Thanks, please use Reply All to make sure everyone in the discussion
gets the responses.

> >From c0eadf74e2d98df8a918b83a6d2a089772673aba Mon Sep 17 00:00:00 2001
> From: Nicolas Martyanoff <nicolas@n16f.net>
> Date: Sat, 7 Jan 2023 12:40:10 +0100
> Subject: [PATCH] em-cmpl: fix completion of command file names
> 
> Completion was originally broken by 82c76e3.
> 
> The use of `completion-table-dynamic' was introduced in 899055e to fix
> bug#48995. However the following issue remains: when completing a command file
> name, 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.
> 
> 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..f6636a48173 100644
> --- a/lisp/eshell/em-cmpl.el
> +++ b/lisp/eshell/em-cmpl.el
> @@ -378,88 +378,73 @@ eshell-complete-parse-arguments
>          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
>    ;; (bug#41423), so do it lazily.
>    (let ((glob-name
> -      ;; When a command is specified using `eshell-explicit-command-char',
> +         ;; When a command is specified using `eshell-explicit-command-char',
>           ;; that char is not part of the command and hence not part of what
>           ;; 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)))))
> -    (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)))
> -             (filepath "") (completions ()))
> -        ;; Go thru each path in the search path, finding completions.
> -        (dolist (path paths)
> -          (setq path (file-name-as-directory
> -                      (expand-file-name (or path "."))))
> -          ;; Go thru each completion found, to see whether it should
> -          ;; be used.
> -          (dolist (file (and (file-accessible-directory-p path)
> -                             (file-name-all-completions filename path)))
> -            (setq filepath (concat path file))
> -            (if (and (not (member file completions)) ;
> -                     (or (string-equal path cwd)
> -                         (not (file-directory-p filepath)))
> -                     ;; FIXME: Those repeated file tests end up
> -                     ;; very costly over Tramp, we should cache the result.
> -                     (if eshell-force-execution
> +         (if (and (> (length pcomplete-stub) 0)
> +                  (eq (aref pcomplete-stub 0) eshell-explicit-command-char))
> +             (setq pcomplete-stub (substring pcomplete-stub 1))))
> +        (filename (pcomplete-arg)))
> +    ;; Do not use `completion-table-dynamic' when completing a command file
> +    ;; name (absolute or relative): doing so assumes that the directory part
> +    ;; of the file name 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)
> +         (let* ((paths (eshell-get-path))
> +                (cwd (file-name-as-directory
> +                      (expand-file-name default-directory)))
> +                (filepath "") (completions ()))
> +           ;; Go thru each path in the search path, finding completions.
> +           (dolist (path paths)
> +             (setq path (file-name-as-directory
> +                         (expand-file-name (or path "."))))
> +             ;; Go thru each completion found, to see whether it should
> +             ;; be used.
> +             (dolist (file (and (file-accessible-directory-p path)
> +                                (file-name-all-completions filename path)))
> +               (setq filepath (concat path file))
> +               (if (and (not (member file completions)) ;
> +                        (or (string-equal path cwd)
> +                            (not (file-directory-p filepath)))
> +                        ;; FIXME: Those repeated file tests end up
> +                        ;; very costly over Tramp, we should cache the 
> result.
> +                        (if eshell-force-execution
>                              (file-readable-p filepath)
>                            (file-executable-p filepath)))
> -                (push file completions))))
> -        ;; Add aliases which are currently visible, and Lisp functions.
> -        (pcomplete-uniquify-list
> -         (if glob-name
> -             completions
> -           (setq completions
> -                 (append (if (fboundp 'eshell-alias-completions)
> -                             (eshell-alias-completions filename))
> -                         (eshell-winnow-list
> -                          (mapcar
> +                   (push file completions))))
> +           ;; Add aliases which are currently visible, and Lisp functions.
> +           (pcomplete-uniquify-list
> +            (if glob-name
> +                completions
> +              (setq completions
> +                    (append (if (fboundp 'eshell-alias-completions)
> +                                (eshell-alias-completions filename))
> +                            (eshell-winnow-list
> +                             (mapcar
>                                (lambda (name)
>                                  (substring name 7))
> -                           (all-completions (concat "eshell/" filename)
> -                                            obarray #'functionp))
> -                          nil '(eshell-find-alias-function))
> -                         completions))
> -           (append (and (or eshell-show-lisp-completions
> -                            (and eshell-show-lisp-alternatives
> -                                 (null completions)))
> -                        (all-completions filename obarray #'functionp))
> -                   completions)))))))))
> +                              (all-completions (concat "eshell/" filename)
> +                                               obarray #'functionp))
> +                             nil '(eshell-find-alias-function))
> +                            completions))
> +              (append (and (or eshell-show-lisp-completions
> +                               (and eshell-show-lisp-alternatives
> +                                    (null completions)))
> +                           (all-completions filename obarray #'functionp))
> +                      completions)))))))))
>  
>  (define-obsolete-function-alias 'eshell-pcomplete #'completion-at-point 
> "27.1")
>  
> -- 
> 2.38.1