From 14a8d6b623c76f5adec910db51cf4c645445849a Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 18 Jul 2024 11:43:34 -0700
Subject: [PATCH] Improve implementation of built-in Eshell "kill" command

* lisp/eshell/esh-proc.el (eshell/kill): Fix handling of commands like
"kill 123".  Use REMOTE when signalling PIDs in remote directories.
Signal using process objects when possible.  Report errors when failing
to signal.

* test/lisp/eshell/esh-proc-tests.el (esh-proc-test/kill/process-id)
(esh-proc-test/kill/process-object): New tests (bug#72013).

(cherry picked from commit 259f4613bdea27abf330b58a9683ca4a9e936777)
---
 lisp/eshell/esh-proc.el            | 48 +++++++++++++-----------------
 test/lisp/eshell/esh-proc-tests.el | 24 +++++++++++++++
 2 files changed, 45 insertions(+), 27 deletions(-)

diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index fbeb13362f3..ed417ab0f12 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -237,40 +237,34 @@ Wait until PROCESS(es) have completed execution.")
 Usage: kill [-<signal>] <pid>|<process> ...
 Accepts PIDs and process objects.  Optionally accept signals
 and signal names."
-  ;; The implementation below only supports local PIDs.  For remote
-  ;; connections, fall back to the external "kill" command.
-  (when (file-remote-p default-directory)
-    (declare-function eshell-external-command "esh-ext" (command args))
-    (throw 'eshell-external (eshell-external-command "kill" args)))
-  ;; If the first argument starts with a dash, treat it as the signal
-  ;; specifier.
   (let ((signum 'SIGINT))
     (let ((arg (car args))
           (case-fold-search nil))
       (when (stringp arg)
+        ;; If the first argument starts with a dash, treat it as the
+        ;; signal specifier.
         (cond
          ((string-match "\\`-[[:digit:]]+\\'" arg)
-          (setq signum (abs (string-to-number arg))))
+          (setq signum (abs (string-to-number arg)))
+          (pop args))
          ((string-match "\\`-\\([[:upper:]]+\\|[[:lower:]]+\\)\\'" arg)
-          (setq signum (intern (substring arg 1)))))
-        (setq args (cdr args))))
-    (while args
-      (let ((arg (if (eshell-processp (car args))
-                     (process-id (car args))
-                   (string-to-number (car args)))))
-        (when arg
-          (cond
-           ((null arg)
-            (error "kill: null pid.  Process may actually be a network connection."))
-           ((not (numberp arg))
-            (error "kill: invalid argument type: %s" (type-of arg)))
-           ((and (numberp arg)
-                 (<= arg 0))
-            (error "kill: bad pid: %d" arg))
-           (t
-            (signal-process arg signum)))))
-      (setq args (cdr args))))
-  nil)
+          (setq signum (intern (substring arg 1)))
+          (pop args)))))
+    (dolist (proc args)
+      (when (stringp proc)
+        (setq proc (string-to-number proc)))
+      (let ((result
+             (cond
+              ((numberp proc)
+               (when (<= proc 0)
+                 (error "kill: bad pid: %d" proc))
+               (signal-process proc signum (file-remote-p default-directory)))
+              ((eshell-processp proc)
+               (signal-process proc signum))
+              (t
+               (error "kill: invalid argument type: %s" (type-of proc))))))
+        (when (= result -1)
+          (error "kill: failed to kill process %s" proc))))))
 
 (put 'eshell/kill 'eshell-no-numeric-conversions t)
 
diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el
index 85b02845ab3..d46004688f9 100644
--- a/test/lisp/eshell/esh-proc-tests.el
+++ b/test/lisp/eshell/esh-proc-tests.el
@@ -344,6 +344,30 @@ write the exit status to the pipe.  See bug#54136."
                         output-start (eshell-end-of-output))
                        ""))))))
 
+(ert-deftest esh-proc-test/kill/process-id ()
+  "Test killing processes with the \"kill\" built-in using PIDs."
+  (skip-unless (executable-find "sleep"))
+  (with-temp-eshell
+    (eshell-insert-command "sleep 100 &")
+    (string-match (rx (group (+ digit)) eol) (eshell-last-output))
+    (let ((pid (match-string 1 (eshell-last-output))))
+      (should (= (length eshell-process-list) 1))
+      (eshell-insert-command (format "kill %s" pid))
+      (should (= eshell-last-command-status 0))
+      (eshell-wait-for-subprocess t)
+      (should (= (length eshell-process-list) 0)))))
+
+(ert-deftest esh-proc-test/kill/process-object ()
+  "Test killing processes with the \"kill\" built-in using process objects."
+  (skip-unless (executable-find "sleep"))
+  (with-temp-eshell
+    (eshell-insert-command "sleep 100 &")
+    (should (= (length eshell-process-list) 1))
+    (eshell-insert-command "kill (caar eshell-process-list)")
+    (should (= eshell-last-command-status 0))
+    (eshell-wait-for-subprocess t)
+    (should (= (length eshell-process-list) 0))))
+
 
 ;; Remote processes
 
-- 
2.39.5