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

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

[Nicolas Martyanoff]

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
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.

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

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

[Nicolas Martyanoff]

First time I am sending an Emacs patch, so I'm probably not doing it
right. Feel free to tell me how to do it better.

I made the patch based on the emacs-29 branch, hoping it could get
merged before the release. It would be infortunate to ship Emacs 29 with
broken command path completion in eshell.

-- 
Nicolas Martyanoff
https://n16f.net
nicolas@n16f.net

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

[Nicolas Martyanoff]

Quick bump just in case my emails falled through the cracks :)

Could someone have a quick look to see if we can get this fix merged for
Emacs 29 ?

-- 
Nicolas Martyanoff
https://n16f.net
nicolas@n16f.net

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)))

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: emacs-devel@gnu.org
> Date: Wed, 25 Jan 2023 12:03:38 -0500
> 
> 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?

Yes, this is okay for emacs-29.  Just one nit:

> > +    ;; 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/ ").

Please use "file name", not "path", when referring to file or
directory names.  If you need to make it clear you are talking about
absolute file name, use "absolute file name" or "file name with
leading directories".  GNU coding standards frown on using "path" for
anything but PATH-style lists of directories.  (Yes, I know that most
of this text came from a comment in the original code.)

[PATCH] Update compat

[Nicolas Martyanoff]

From: GNU ELPA Mirror Bot <emacs-devel@gnu.org>

Timestamp: 2022-11-23 00:00:55
GNU ELPA commit: c1e7d260a3e9305188aea0b5a372f4ccd4dfb22c
Emacs commit: 057901f55ad12ebbc9cf092dd6ad0f02539849f9
---
 compat-27.el | 11 +++++++++++
 compat.texi  |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/compat-27.el b/compat-27.el
index 04abe6e..714df89 100644
--- a/compat-27.el
+++ b/compat-27.el
@@ -662,6 +662,17 @@ defvar json-null)
         (when (stringp res) (compat--file-local-name res)))
     (executable-find command)))
 
+;;*UNTESTED
+(compat-defun make-empty-file (filename &optional parents)
+  "Create an empty file FILENAME.
+Optional arg PARENTS, if non-nil then creates parent dirs as needed."
+  (when (and (file-exists-p filename) (null parents))
+    (signal 'file-already-exists (list "File exists" filename)))
+  (let ((paren-dir (file-name-directory filename)))
+    (when (and paren-dir (not (file-exists-p paren-dir)))
+      (make-directory paren-dir parents)))
+  (write-region "" nil filename nil 0))
+
 ;; TODO provide advice for directory-files-recursively
 
 ;;;; Defined in format-spec.el
diff --git a/compat.texi b/compat.texi
index f19d27f..9953269 100644
--- a/compat.texi
+++ b/compat.texi
@@ -1482,6 +1482,14 @@ derived from any of the major modes given by the symbols @var{modes}.
 @xref{Derived Modes,,,elisp}.
 @end defun
 
+@c based on lispref/files.texi
+@defun make-empty-file filename &optional parents
+This function creates an empty file named @var{filename}.  As
+@code{make-directory}, this function creates parent directories if
+@var{parents} is non-@code{nil}.  If @var{filename} already exists, this
+function signals an error.
+@end defun
+
 @subsection Prefixed Definitions
 These functions are prefixed with @code{compat} prefix, and are only
 loaded when @code{compat-27} is required:
-- 
2.38.1

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

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

[Eli Zaretskii]

> Date: Thu, 26 Jan 2023 08:02:51 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> CC: emacs-devel@gnu.org, Stefan Monnier <monnier@iro.umontreal.ca>
> 
> > >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

Oops!  I now see that we don't have a copyright assignment from you,
which we need for such a substantial contribution.  Would you like to
start your legal paperwork, so that we could accept this contribution?
If so, I will send you the form to fill.

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

[Nicolas Martyanoff]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Thu, 26 Jan 2023 08:02:51 +0200
>> From: Eli Zaretskii <eliz@gnu.org>
> Oops!  I now see that we don't have a copyright assignment from you,
> which we need for such a substantial contribution.  Would you like to
> start your legal paperwork, so that we could accept this contribution?
> If so, I will send you the form to fill.

I started the process while sending the patch. I sent the signed
documents to Craig Topham two minutes ago.

Regards,

-- 
Nicolas Martyanoff
https://n16f.net
nicolas@n16f.net

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

[Eli Zaretskii]

> From: Nicolas Martyanoff <nicolas@n16f.net>
> Cc: nicolas@n16f.net,  emacs-devel@gnu.org,  monnier@iro.umontreal.ca
> Date: Thu, 26 Jan 2023 13:21:28 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Date: Thu, 26 Jan 2023 08:02:51 +0200
> >> From: Eli Zaretskii <eliz@gnu.org>
> > Oops!  I now see that we don't have a copyright assignment from you,
> > which we need for such a substantial contribution.  Would you like to
> > start your legal paperwork, so that we could accept this contribution?
> > If so, I will send you the form to fill.
> 
> I started the process while sending the patch. I sent the signed
> documents to Craig Topham two minutes ago.

Great, then I will install the changes when the paperwork is complete.

Thanks.

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

[Stefan Monnier]

> Oops!  I now see that we don't have a copyright assignment from you,
> which we need for such a substantial contribution.  Would you like to
> start your legal paperwork, so that we could accept this contribution?
> If so, I will send you the form to fill.

Actually, the patch falls within the "trivial"category if you ignore the
reindentation.


        Stefan

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

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: nicolas@n16f.net,  emacs-devel@gnu.org
> Date: Fri, 27 Jan 2023 19:11:12 -0500
> 
> > Oops!  I now see that we don't have a copyright assignment from you,
> > which we need for such a substantial contribution.  Would you like to
> > start your legal paperwork, so that we could accept this contribution?
> > If so, I will send you the form to fill.
> 
> Actually, the patch falls within the "trivial"category if you ignore the
> reindentation.

Not AFAICT.

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

[Stefan Monnier]

>> Eli, what do you think?
> Yes, this is okay for emacs-29.

Thanks, I'll push it soon.

>> > +    ;; 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/ ").
>
> Please use "file name", not "path", when referring to file or
> directory names.  If you need to make it clear you are talking about
> absolute file name, use "absolute file name" or "file name with
> leading directories".  GNU coding standards frown on using "path" for
> anything but PATH-style lists of directories.  (Yes, I know that most
> of this text came from a comment in the original code.)

Thanks for catching this.


        Stefan

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

[Jim Porter]

On 1/25/2023 9:03 AM, Stefan Monnier wrote:
>> 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`.

I'm not sure about all the details of this bug, but over in bug#60845, 
I'm adding some regression tests for completion in Eshell. It probably 
wouldn't be too hard for someone (possibly me if I have time) to write 
some regression tests for this command path issue too. Then hopefully 
this won't cause problems in the future.

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

[Jim Porter]

Hi Nicolas,

On 1/7/2023 4:19 AM, Nicolas Martyanoff wrote:
> 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
> path, absolute ("/usr/bin/") or relative ("./subdir/"), a space is
> automatically added at the end.

Thanks for this patch. I'm looking to add a regression test for this so 
that it doesn't crop up again sometime in the future. However, I can't 
seem to cause completion to fail as you describe, so it's hard to write 
the test. I tried reverting your patch on the emacs-29 branch and then 
running:

emacs -Q -f eshell

;; Within Eshell, enter:
/usr/bi<TAB>

This completes to "/usr/bin/|", where "|" is the point. However, that's 
the correct behavior as far as I understand it, so I must be missing 
something. Do you have more complete steps to reproduce this?