bug#61778: 30.0.50; [PATCH] Be more cautious in completing Eshell variable assignments

bug#61778: 30.0.50; [PATCH] Be more cautious in completing Eshell variable assignments

[Jim Porter]

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

X-Debbugs-Cc: monnier@iro.umontreal.ca

Starting from "emacs -Q":

   M-x shell RET
   tar --directory=<TAB>
   ;; Notice that completions only show directories.

   M-x eshell RET
   tar --directory=<TAB>
   ;; Completions include files *and* directories(!)

After quite a bit of digging, I figured out the issue: Eshell was 
treating "--directory=" as the beginning of a local variable assignment 
(like when you run "CC=gcc make"). The function 
'eshell-complete-variable-assignment' is over-aggressive in identifying 
local variable assignments, and so it picked this up as a false positive.

Attached is a fix for this, plus a regression test.

[-- Attachment #2: 0001-Be-more-cautious-in-completing-Eshell-variable-assig.patch --]
[-- Type: text/plain, Size: 3285 bytes --]

From e826440dc1173fc500c30225516b10c2639ce646 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 24 Feb 2023 21:49:54 -0800
Subject: [PATCH] Be more cautious in completing Eshell variable assignments

Previously, Eshell treated cases like the second argument in "tar
--directory=dir" as a variable assignment, but that prevented
'pcomplete/tar' from implementing its own completion for that
argument.

* lisp/eshell/esh-var.el (eshell-complete-variable-assignment): Only
handle completion when all initial arguments are variable assignments.

* test/lisp/eshell/em-cmpl-tests.el
(em-cmpl-test/variable-assign-completion/non-assignment): New test.
---
 lisp/eshell/esh-var.el            | 18 +++++++++++++-----
 test/lisp/eshell/em-cmpl-tests.el | 14 ++++++++++++++
 2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 0031324b537..5d6299af564 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -862,11 +862,19 @@ eshell-variables-list
 
 (defun eshell-complete-variable-assignment ()
   "If there is a variable assignment, allow completion of entries."
-  (let ((arg (pcomplete-actual-arg)) pos)
-    (when (string-match (concat "\\`" eshell-variable-name-regexp "=") arg)
-      (setq pos (match-end 0))
-      (if (string-match "\\(:\\)[^:]*\\'" arg)
-	  (setq pos (match-end 1)))
+  (catch 'not-assignment
+    ;; The current argument can only be a variable assignment if all
+    ;; arguments leading up to it are also variable assignments.  See
+    ;; `eshell-handle-local-variables'.
+    (dotimes (offset (1+ pcomplete-index))
+      (unless (string-match (concat "\\`" eshell-variable-name-regexp "=")
+                            (pcomplete-actual-arg 'first offset))
+        (throw 'not-assignment nil)))
+    ;; We have a variable assignment.  Handle it.
+    (let ((arg (pcomplete-actual-arg))
+          (pos (match-end 0)))
+      (when (string-match "\\(:\\)[^:]*\\'" arg)
+	(setq pos (match-end 1)))
       (setq pcomplete-stub (substring arg pos))
       (throw 'pcomplete-completions (pcomplete-entries)))))
 
diff --git a/test/lisp/eshell/em-cmpl-tests.el b/test/lisp/eshell/em-cmpl-tests.el
index ecab7332822..be2199c0464 100644
--- a/test/lisp/eshell/em-cmpl-tests.el
+++ b/test/lisp/eshell/em-cmpl-tests.el
@@ -217,6 +217,20 @@ em-cmpl-test/variable-assign-completion
      (should (equal (eshell-insert-and-complete "VAR=f")
                     "VAR=file.txt ")))))
 
+(ert-deftest em-cmpl-test/variable-assign-completion/non-assignment ()
+  "Test completion of things that look like variable assignment, but aren't.
+For example, the second argument in \"tar --directory=dir\" looks
+like it could be a variable assignment, but it's not.  We should
+let `pcomplete-tar' handle it instead.
+
+See <lisp/eshell/esh-var.el>."
+  (with-temp-eshell
+   (ert-with-temp-directory default-directory
+     (write-region nil nil (expand-file-name "file.txt"))
+     (make-directory "dir")
+     (should (equal (eshell-insert-and-complete "tar --directory=")
+                    "tar --directory=dir/")))))
+
 (ert-deftest em-cmpl-test/user-ref-completion ()
   "Test completion of user references like \"~user\".
 See <lisp/eshell/em-dirs.el>."
-- 
2.25.1

bug#61778: 30.0.50; [PATCH] Be more cautious in completing Eshell variable assignments

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

> After quite a bit of digging, I figured out the issue: Eshell was treating
>  "--directory=" as the beginning of a local variable assignment (like when
>  you run "CC=gcc make"). The function 'eshell-complete-variable-assignment'
>  is over-aggressive in identifying local variable assignments, and so it
> picked this up as a false positive.
>
> Attached is a fix for this, plus a regression test.

LGTM, thanks,


        Stefan

bug#61778: 30.0.50; [PATCH] Be more cautious in completing Eshell variable assignments

[Jim Porter]

On 2/25/2023 3:25 PM, Stefan Monnier via Bug reports for GNU Emacs, the 
Swiss army knife of text editors wrote:
>> Attached is a fix for this, plus a regression test.
> 
> LGTM, thanks,

Thanks. Pushed as 9565e34762. Closing this now.