GNU bug report logs - #57905
[PATCH] Fix eshell directory and executable completion on action t

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 57905 in the body.
You can then email your comments to 57905 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: Daniel Pettersson <daniel <at> dpettersson.net>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] Fix eshell directory and executable completion on action t
Date: Sun, 18 Sep 2022 00:55:48 +0200

When eshell directory/executable completion results in multiple candidates
the candidates are striped of their file path.

emacs -Q

M-x eshell RET
touch test1 test2
chmod +x test1 test2
./test<Tab>

In *Completions* buffer:
2 possible completions:
test1
test2

Expected:
./test1
./test2

Same issue for directories:

mkdir test_dir1 test_dir2
./test_dir<Tab>

In *Completions* buffer:
2 possible completions:
test1_dir1
test2_dir2

This also the case when the candidates are not in the working directory:
mkdir -p 1/2/3/4
touch 1/2/3/4/test1 1/2/3/4/test2
chmod +x 1/2/3/4/test1 1/2/3/4/test2
./1/2/3/4/test<Tab>

In *Completions* buffer:
2 possible completions:
test1
test2

This issue is not present with one completion as further down the call
stack `completion-file-name-table' is called with action t, which concates
completion string directory with completion candidate.

Possible solution:
---
 lisp/eshell/em-cmpl.el | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/lisp/eshell/em-cmpl.el b/lisp/eshell/em-cmpl.el
index 822cc94149..d9261fae27 100644
--- a/lisp/eshell/em-cmpl.el
+++ b/lisp/eshell/em-cmpl.el
@@ -378,6 +378,31 @@ eshell-complete-parse-arguments
        args)
       posns)))

+(defun eshell--pcomplete-executables ()
+  "Complete amongs 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' t
+  ;; 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)
+                     (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
@@ -392,9 +417,7 @@ eshell--complete-commands-list
     (completion-table-dynamic
      (lambda (filename)
        (if (file-name-directory filename)
-           (if eshell-force-execution
-               (pcomplete-dirs-or-entries nil #'file-readable-p)
-             (pcomplete-executables))
+           (eshell--pcomplete-executables)
      (let* ((paths (eshell-get-path))
         (cwd (file-name-as-directory
               (expand-file-name default-directory)))
-- 
2.30.1 (Apple Git-130)


Issue on 28.2.0 as well.

In GNU Emacs 29.0.50 (build 1, aarch64-apple-darwin20.6.0, NS
 appkit-2022.60 Version 11.5.2 (Build 20G95)) of 2022-09-16 built on
 Daniels-Air
Repository revision: fe7c015b20b5bca07aa178d28b9fd5cc66ad16f9
Repository branch: master
Windowing system distributor 'Apple', version 10.3.2022
System Description:  macOS 11.5.2

Configured features:
ACL DBUS GLIB GNUTLS JSON LCMS2 LIBXML2 MODULES NOTIFY KQUEUE NS PDUMPER
PNG RSVG SQLITE3 THREADS TOOLKIT_SCROLL_BARS WEBP XIM ZLIB

Important settings:
  value of $LC_CTYPE: UTF-8
  locale-coding-system: utf-8-unix

Major mode: Eshell

Minor modes in effect:
  shell-dirtrack-mode: t
  eshell-prompt-mode: t
  eshell-hist-mode: t
  eshell-pred-mode: t
  eshell-cmpl-mode: t
  eshell-proc-mode: t
  eshell-arg-mode: t
  tooltip-mode: t
  global-eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils pcmpl-unix cl-seq
cl-macs vc-git diff-mode easy-mmode vc-dispatcher bug-reference byte-opt
gv bytecomp byte-compile cconv time-date em-unix em-term term disp-table
shell subr-x ehelp em-script em-prompt em-ls em-hist em-pred em-glob
em-extpipe em-cmpl em-dirs esh-var pcomplete comint ansi-color ring
em-basic em-banner em-alias esh-mode eshell esh-cmd generator
cl-loaddefs cl-lib esh-ext esh-opt esh-proc esh-io esh-arg esh-module
esh-groups esh-util rmc iso-transl tooltip eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/ns-win ns-win
ucs-normalize mule-util term/common-win tool-bar dnd fontset image
regexp-opt fringe tabulated-list replace newcomment text-mode lisp-mode
prog-mode register page tab-bar menu-bar rfn-eshadow isearch easymenu
timer select scroll-bar mouse jit-lock font-lock syntax font-core
term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
faces cus-face macroexp files window text-properties overlay sha1 md5
base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind kqueue cocoa ns
lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 71482 9836)
 (symbols 48 8490 0)
 (strings 32 24424 2081)
 (string-bytes 1 760661)
 (vectors 16 16661)
 (vector-slots 8 215237 12871)
 (floats 8 32 63)
 (intervals 56 573 57)
 (buffers 1000 13))

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

From: Eli Zaretskii <eliz <at> gnu.org>
To: Daniel Pettersson <daniel <at> dpettersson.net>
Cc: 57905 <at> debbugs.gnu.org
Subject: Re: bug#57905: [PATCH] Fix eshell directory and executable completion
 on action t
Date: Sun, 18 Sep 2022 10:40:36 +0300

> From: Daniel Pettersson <daniel <at> dpettersson.net>
> Date: Sun, 18 Sep 2022 00:55:48 +0200
> 
> +(defun eshell--pcomplete-executables ()
> +  "Complete amongs 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' t
> +  ;; 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)
> +                     (concat specdir cand)
> +                   cand))
> +               cands))
> +          cands)))))

Please don't use 'concat' to create a file name with leading
directories; instead, please use file-name-concat.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Daniel Pettersson <daniel <at> dpettersson.net>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 57905 <at> debbugs.gnu.org
Subject: Re: bug#57905: [PATCH] Fix eshell directory and executable
 completion on action t
Date: Sun, 18 Sep 2022 12:41:45 +0200

Daniel Pettersson <daniel <at> dpettersson.net> writes:

> This issue is not present with one completion as further down the call
> stack `completion-file-name-table' is called with action t, which concates
> completion string directory with completion candidate.
>
> Possible solution:

Hm...  I'm not that familiar with how this works in eshell myself;
perhaps Jim has some comments -- added to the CCs.

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

From: Jim Porter <jporterbugs <at> gmail.com>
To: Lars Ingebrigtsen <larsi <at> gnus.org>,
 Daniel Pettersson <daniel <at> dpettersson.net>
Cc: 57905 <at> debbugs.gnu.org
Subject: Re: bug#57905: [PATCH] Fix eshell directory and executable completion
 on action t
Date: Sun, 18 Sep 2022 17:31:01 -0700

On 9/18/2022 3:41 AM, Lars Ingebrigtsen wrote:
> Daniel Pettersson <daniel <at> dpettersson.net> writes:
> 
>> This issue is not present with one completion as further down the call
>> stack `completion-file-name-table' is called with action t, which concates
>> completion string directory with completion candidate.
>>
>> Possible solution:
> 
> Hm...  I'm not that familiar with how this works in eshell myself;
> perhaps Jim has some comments -- added to the CCs.

With the caveats that I don't know much about pcomplete (maybe someone 
who does would have something interesting to say about this patch?) and 
that I haven't built Emacs with the patch, I think the logic here makes 
sense.

Some regression tests would be nice though. I've been trying to add 
tests as I go through various parts of Eshell, but I haven't looked much 
at the "interactive" bits like em-cmpl.el yet. 'eshell-test/forward-arg' 
in test/lisp/eshell/eshell-tests.el might make for an ok basis to adapt 
into some completion tests (which could probably then go into 
.../em-cmpl-tests.el).

That said, I wouldn't object to merging this without regression tests; 
it doesn't seem fair to me to expect patch authors to write tests when 
the component they're patching doesn't have tests in the first place. :)

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Jim Porter <jporterbugs <at> gmail.com>
Cc: 57905 <at> debbugs.gnu.org, Daniel Pettersson <daniel <at> dpettersson.net>
Subject: Re: bug#57905: [PATCH] Fix eshell directory and executable
 completion on action t
Date: Mon, 19 Sep 2022 10:22:34 +0200

Jim Porter <jporterbugs <at> gmail.com> writes:

> That said, I wouldn't object to merging this without regression tests;
> it doesn't seem fair to me to expect patch authors to write tests when
> the component they're patching doesn't have tests in the first
> place. :)

Thanks.

Daniel, I've now pushed your patch to Emacs 29.

This change was just small enough to apply without assigning copyright
to the FSF, but for future patches you want to submit, it might make
sense to get the paperwork started now, so that subsequent patches can
be applied speedily. Would you be willing to sign such paperwork?

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

From: Daniel Pettersson <daniel <at> dpettersson.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 57905 <at> debbugs.gnu.org
Subject: Re: bug#57905: [PATCH] Fix eshell directory and executable completion
 on action t
Date: Mon, 19 Sep 2022 22:27:39 +0200

> Please don't use 'concat' to create a file name with leading
> directories; instead, please use file-name-concat.

Ah I had know idea of the existence of that function, that definitely
seams like the better option. Thanks for spreading the word.

> Some regression tests would be nice though. I've been trying to add
> tests as I go through various parts of Eshell, but I haven't looked much
> at the "interactive" bits like em-cmpl.el yet. 'eshell-test/forward-arg'
> in test/lisp/eshell/eshell-tests.el might make for an ok basis to adapt
> into some completion tests (which could probably then go into
> .../em-cmpl-tests.el).

I will take a look :)

> Daniel, I've now pushed your patch to Emacs 29.
>
> This change was just small enough to apply without assigning copyright
> to the FSF, but for future patches you want to submit, it might make
> sense to get the paperwork started now, so that subsequent patches can
> be applied speedily. Would you be willing to sign such paperwork?

Great and sure.

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

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Daniel Pettersson <daniel <at> dpettersson.net>
Cc: Jim Porter <jporterbugs <at> gmail.com>, 57905 <at> debbugs.gnu.org
Subject: Re: bug#57905: [PATCH] Fix eshell directory and executable
 completion on action t
Date: Mon, 19 Sep 2022 22:35:16 +0200

Daniel Pettersson <daniel <at> dpettersson.net> writes:

>> Daniel, I've now pushed your patch to Emacs 29.
>>
>> This change was just small enough to apply without assigning copyright
>> to the FSF, but for future patches you want to submit, it might make
>> sense to get the paperwork started now, so that subsequent patches can
>> be applied speedily. Would you be willing to sign such paperwork?
>
> Great and sure.

Great; here's the form to get started:


Please email the following information to assign <at> gnu.org, and we
will send you the assignment form for your past and future changes.

Please use your full legal name (in ASCII characters) as the subject
line of the message.
----------------------------------------------------------------------
REQUEST: SEND FORM FOR PAST AND FUTURE CHANGES

[What is the name of the program or package you're contributing to?]
Emacs

[Did you copy any files or text written by someone else in these changes?
Even if that material is free software, we need to know about it.]

[Do you have an employer who might have a basis to claim to own
your changes?  Do you attend a school which might make such a claim?]

[For the copyright registration, what country are you a citizen of?]

[What year were you born?]

[Please write your email address here.]

[Please write your postal address here.]

[Which files have you changed so far, and which new files have you written
so far?]

Previous Next

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