bug#70198: M-x shell: deal with environment variables present when tab expanding

bug#70198: M-x shell: deal with environment variables present when tab expanding

[Dan Jacobson]

In M-x shell
$ dat<TAB>
expands to date.
Alas, unlike bash readline,
$ LC_ALL=C dat<TAB>
doesn't yet.

emacs-version "29.3"

bug#70198: M-x shell: deal with environment variables present when tab expanding

[Federico Tedin via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

Dan Jacobson <jidanni@jidanni.org> writes:

> In M-x shell
> $ dat<TAB>
> expands to date.
> Alas, unlike bash readline,
> $ LC_ALL=C dat<TAB>
> doesn't yet.
>
> emacs-version "29.3"

I took a crack at fixing this, I'm attaching a patch.

It's been some time since my last contribution, but I've kept the
copyright assignment updated (should be under federicotedin@gmail.com).
There's a chance the formatting for the patch may be a bit off too but I
tried to re-read the guide at CONTRIBUTE.

I've also found something interesting with the
`shell-dynamic-complete-command' function. I do not see it being called,
referred to, or assigned to a key anywhere in the Emacs code, but the
manual mentions it as if it being were actively used:

> Some implementation details of the shell command completion may also be found
> in the lisp documentation of the @code{shell-dynamic-complete-command}
> function.

Maybe the manual is outdated?

- Fede


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 3067 bytes --]

From 9250d0c78fbc1c23469c125279604bbb8c965626 Mon Sep 17 00:00:00 2001
From: Federico Tedin <federicotedin@gmail.com>
Date: Mon, 8 Jul 2024 14:47:33 +0200
Subject: [PATCH] Fix tab expanding not working in shell-mode (bug#70198)

In shell-mode, fix tab expanding when environment variables are
present before the command.
* lisp/shell.el (shell-command-completion): Fix indentation and
call use `shell--skip-environment-variables'.
(shell--skip-environment-variables): New function.
* test/lisp/shell-tests.el (shell-skip-environment-variables):
Test new function.
---
 lisp/shell.el            | 22 +++++++++++++++++-----
 test/lisp/shell-tests.el | 17 +++++++++++++++++
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/lisp/shell.el b/lisp/shell.el
index e1936ff1119..fff7bdd4d71 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -1385,12 +1385,24 @@ shell-dynamic-complete-command

 (defun shell-command-completion ()
   "Return the completion data for the command at point, if any."
-  (let ((filename (comint-match-partial-filename)))
+  (let ((filename (comint-match-partial-filename))
+        (pt (point)))
     (if (and filename
-	     (save-match-data (not (string-match "[~/]" filename)))
-	     (eq (match-beginning 0)
-		 (save-excursion (shell-backward-command 1) (point))))
-	(shell--command-completion-data))))
+	         (save-match-data (not (string-match "[~/]" filename)))
+	         (eq (match-beginning 0)
+		         (save-excursion
+                   ;; Go back to beginning of command
+                   (shell-backward-command 1)
+                   ;; Skip any potential environment variables
+                   (shell--skip-environment-variables pt)
+                   (point))))
+	    (shell--command-completion-data))))
+
+(defun shell--skip-environment-variables (pt)
+  "Move forward up to PT through any present environment variables."
+  (while (re-search-forward "=" pt t)
+    (skip-syntax-forward "^ " pt)
+    (skip-syntax-forward " " pt)))

 (defun shell--command-completion-data ()
   "Return the completion data for the command at point."
diff --git a/test/lisp/shell-tests.el b/test/lisp/shell-tests.el
index 9bdf6b1c0eb..f07166d5995 100644
--- a/test/lisp/shell-tests.el
+++ b/test/lisp/shell-tests.el
@@ -95,4 +95,21 @@ shell-directory-tracker-cd
       (should (not (equal start-dir list-buffers-directory)))
       (should (string-prefix-p list-buffers-directory start-dir)))))

+(ert-deftest shell-skip-environment-variables ()
+  (with-temp-buffer
+    (shell-mode)
+    (insert "FOO=BAR BAZ= QUUX=abc=def whoami")
+    (let ((pt (point)))
+      (shell-backward-command 1)
+      (shell--skip-environment-variables pt))
+    (should (looking-at-p "whoami"))
+
+    (shell-backward-command 1)
+    (delete-line)
+    (insert "echo")
+    (let ((pt (point)))
+      (shell-backward-command 1)
+      (shell--skip-environment-variables pt))
+    (should (looking-at-p "echo"))))
+
 ;;; shell-tests.el ends here
--
2.43.0

bug#70198: M-x shell: deal with environment variables present when tab expanding

[Eli Zaretskii]

> Cc: 70198@debbugs.gnu.org
> Date: Wed, 10 Jul 2024 18:22:35 +0200
> From:  Federico Tedin via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> +	         (save-match-data (not (string-match "[~/]" filename)))
> +	         (eq (match-beginning 0)
> +		         (save-excursion
> +                   ;; Go back to beginning of command
> +                   (shell-backward-command 1)
> +                   ;; Skip any potential environment variables
> +                   (shell--skip-environment-variables pt)
> +                   (point))))
> +	    (shell--command-completion-data))))
> +
> +(defun shell--skip-environment-variables (pt)
> +  "Move forward up to PT through any present environment variables."
> +  (while (re-search-forward "=" pt t)
> +    (skip-syntax-forward "^ " pt)
> +    (skip-syntax-forward " " pt)))

What happens if the command itself has embedded '='?
What happens if the '=' character is quoted?

I think a better idea might be first to try to find what is "the word
at point", and then complete only that word.  WDYT?

bug#70198: M-x shell: deal with environment variables present when tab expanding

[Federico Tedin via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> What happens if the command itself has embedded '='?

Ah good point, I incorrectly assumed that after
`comint-match-partial-filename' was called, the point would be at the
beginning of the command rather than at the end. That being said if we
used (match-beginning 0) as PT then it would address this particular
scenario (when calling `shell--skip-environment-variables' I mean).

> What happens if the '=' character is quoted?

What would be an example of this? (in the context of writing shell
commands).

> I think a better idea might be first to try to find what is "the word
> at point", and then complete only that word.  WDYT?

Yep, however I do think we still need to walk back some words in order
to ensure that we are not autocompleting an argument; for example

$ echo whoam[TAB]

should not autocomplete to "whoami", I'm assuming.

So for the actual walking backwards we need to maybe:
- modify shell-backward-command so that it leaves the point at the
beginning of the command, but after any env vars
- or, move backwards using `shell-command-regexp' until the last thing
that does not look like FOO=BAR or FOO= is found.

All of this just to ensure that the word at point is an actual command
and not an argument to a command. Maybe this approach itself could be
re-evaluated though !

bug#70198: M-x shell: deal with environment variables present when tab expanding

[Eli Zaretskii]

> From: Federico Tedin <federicotedin@gmx.de>
> Cc: jidanni@jidanni.org,  70198@debbugs.gnu.org
> Date: Wed, 10 Jul 2024 20:11:07 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What happens if the '=' character is quoted?
> 
> What would be an example of this? (in the context of writing shell
> commands).

Something like

  $ FOO='foo=bar' date

> > I think a better idea might be first to try to find what is "the word
> > at point", and then complete only that word.  WDYT?
> 
> Yep, however I do think we still need to walk back some words in order
> to ensure that we are not autocompleting an argument; for example
> 
> $ echo whoam[TAB]
> 
> should not autocomplete to "whoami", I'm assuming.

It should, just not command completion.

> So for the actual walking backwards we need to maybe:
> - modify shell-backward-command so that it leaves the point at the
> beginning of the command, but after any env vars
> - or, move backwards using `shell-command-regexp' until the last thing
> that does not look like FOO=BAR or FOO= is found.
> 
> All of this just to ensure that the word at point is an actual command
> and not an argument to a command. Maybe this approach itself could be
> re-evaluated though !

If we want to detect FOO=BAR, we need to use syntax classes, and I'm
not sure regexps are the best instrument for that.

bug#70198: M-x shell: deal with environment variables present when tab expanding

[Federico Tedin via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

> Something like
>
>   $ FOO='foo=bar' date
>

Ah right, that makes sense.

> It should, just not command completion.

> If we want to detect FOO=BAR, we need to use syntax classes, and I'm
> not sure regexps are the best instrument for that.

But then, just to make sure I'm on the right track:

- When the word at point is a command (not an arg)
`shell-command-completion' should return the proper command completions.

- When the word at point is *not* a command, `shell-command-completion' should
return nil so that other completion functions are attempted.

And these two scenarios need to be decided on using syntax classes
instead of regexps.

bug#70198: M-x shell: deal with environment variables present when tab expanding

[Eli Zaretskii]

> From: Federico Tedin <federicotedin@gmx.de>
> Cc: 70198@debbugs.gnu.org,  jidanni@jidanni.org
> Date: Wed, 10 Jul 2024 22:17:41 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If we want to detect FOO=BAR, we need to use syntax classes, and I'm
> > not sure regexps are the best instrument for that.
> 
> But then, just to make sure I'm on the right track:
> 
> - When the word at point is a command (not an arg)
> `shell-command-completion' should return the proper command completions.
> 
> - When the word at point is *not* a command, `shell-command-completion' should
> return nil so that other completion functions are attempted.
> 
> And these two scenarios need to be decided on using syntax classes
> instead of regexps.

Yes, I think so.  I added Stefan to the discussion in case he has
comments or suggestions.  Stefan, what do you think is the best method
of parsing the beginning of the command line to detect where a
program's name starts?