Subject: 30.0.50; [PATCH] Simplify some internal Eshell command parsing functions
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Tue, 8 Aug 2023 17:16:30 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

These changes are just there for cleanup, and to make it easier to work 
with this code going forward. Since Eshell command parsing now has 
fairly-thorough regression tests, I think these changes should be pretty 
safe.

In the second patch, I removed the need to use a dynamically-bound 
variable, but kept the old (now obsolete) function around for 
compatibility. I doubt any external packages are using this, but better 
safe than sorry...

From 318d09fb6ba7090c35147741e046924f61983342 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Mon, 7 Aug 2023 22:15:18 -0700
Subject: [PATCH 1/2] Simplify command parsing in Eshell

* lisp/eshell/esh-cmd.el (eshell-parse-command): Do all modifications
to each command in a single pass.
(eshell-parse-pipeline): Remove unncessary reversing of parsed
results.
---
 lisp/eshell/esh-cmd.el | 92 +++++++++++++++++-------------------------
 1 file changed, 38 insertions(+), 54 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 94aa2ed8906..bd00e9f5406 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -416,26 +416,19 @@ eshell-parse-command
 	 (commands
 	  (mapcar
            (lambda (cmd)
-             (setq cmd
-                   (if (or (not (car eshell--sep-terms))
-                           (string= (car eshell--sep-terms) ";"))
-                       (eshell-parse-pipeline cmd)
-                     `(eshell-do-subjob
-                       (cons :eshell-background
-                             ,(eshell-parse-pipeline cmd)))))
-             (setq eshell--sep-terms (cdr eshell--sep-terms))
-             (if eshell-in-pipeline-p
-                 cmd
-               `(eshell-trap-errors ,cmd)))
+             (let ((sep (pop eshell--sep-terms)))
+               (setq cmd (eshell-parse-pipeline cmd))
+               (when (equal sep "&")
+                 (setq cmd `(eshell-do-subjob (cons :eshell-background ,cmd))))
+               (unless eshell-in-pipeline-p
+                 (setq cmd `(eshell-trap-errors ,cmd)))
+               ;; Copy I/O handles so each full statement can manipulate
+               ;; them if they like.  Steal the handles for the last
+               ;; command in the list; we won't use the originals again
+               ;; anyway.
+               (setq cmd `(eshell-with-copied-handles ,cmd ,(not sep)))
+               cmd))
 	   (eshell-separate-commands terms "[&;]" nil 'eshell--sep-terms))))
-    (let ((cmd commands))
-      (while cmd
-        ;; Copy I/O handles so each full statement can manipulate them
-        ;; if they like.  Steal the handles for the last command in
-        ;; the list; we won't use the originals again anyway.
-        (setcar cmd `(eshell-with-copied-handles
-                      ,(car cmd) ,(not (cdr cmd))))
-	(setq cmd (cdr cmd))))
     (if toplevel
 	`(eshell-commands (progn
                             (run-hooks 'eshell-pre-command-hook)
@@ -638,46 +631,37 @@ eshell-parse-pipeline
   (let* (eshell--sep-terms
 	 (bigpieces (eshell-separate-commands terms "\\(&&\\|||\\)"
 					      nil 'eshell--sep-terms))
-	 (bp bigpieces)
-	 (results (list t))
-	 final)
-    (while bp
-      (let ((subterms (car bp)))
-	(let* ((pieces (eshell-separate-commands subterms "|"))
-	       (p pieces))
-	  (while p
-	    (let ((cmd (car p)))
-	      (run-hook-with-args 'eshell-pre-rewrite-command-hook cmd)
-	      (setq cmd (run-hook-with-args-until-success
-			 'eshell-rewrite-command-hook cmd))
-	      (let ((eshell--cmd cmd))
-		(run-hook-with-args 'eshell-post-rewrite-command-hook
-				    'eshell--cmd)
-		(setq cmd eshell--cmd))
-	      (setcar p (funcall eshell-post-rewrite-command-function cmd)))
-	    (setq p (cdr p)))
-	  (nconc results
-		 (list
-		  (if (<= (length pieces) 1)
-		      (car pieces)
-		    (cl-assert (not eshell-in-pipeline-p))
-		    `(eshell-execute-pipeline (quote ,pieces))))))
-	(setq bp (cdr bp))))
+         results final)
+    (dolist (subterms bigpieces)
+      (let* ((pieces (eshell-separate-commands subterms "|"))
+             (p pieces))
+        (while p
+          (let ((cmd (car p)))
+            (run-hook-with-args 'eshell-pre-rewrite-command-hook cmd)
+            (setq cmd (run-hook-with-args-until-success
+                       'eshell-rewrite-command-hook cmd))
+            (let ((eshell--cmd cmd))
+              (run-hook-with-args 'eshell-post-rewrite-command-hook
+                                  'eshell--cmd)
+              (setq cmd eshell--cmd))
+            (setcar p (funcall eshell-post-rewrite-command-function cmd)))
+          (setq p (cdr p)))
+        (push (if (<= (length pieces) 1)
+                  (car pieces)
+                (cl-assert (not eshell-in-pipeline-p))
+                `(eshell-execute-pipeline (quote ,pieces)))
+              results)))
     ;; `results' might be empty; this happens in the case of
     ;; multi-line input
-    (setq results (cdr results)
-	  results (nreverse results)
-	  final (car results)
-	  results (cdr results)
-	  eshell--sep-terms (nreverse eshell--sep-terms))
+    (setq final (car results)
+          results (cdr results)
+          eshell--sep-terms (nreverse eshell--sep-terms))
     (while results
       (cl-assert (car eshell--sep-terms))
       (setq final (eshell-structure-basic-command
-		   'if (string= (car eshell--sep-terms) "&&") "if"
-		   `(eshell-protect ,(car results))
-		   `(eshell-protect ,final))
-	    results (cdr results)
-	    eshell--sep-terms (cdr eshell--sep-terms)))
+                   'if (string= (pop eshell--sep-terms) "&&") "if"
+                   `(eshell-protect ,(pop results))
+                   `(eshell-protect ,final))))
     final))
 
 (defun eshell-parse-subcommand-argument ()
-- 
2.25.1

From 99fac65da8e0e96fca8290e9e481a7a97a70cbf4 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Mon, 7 Aug 2023 22:28:24 -0700
Subject: [PATCH 2/2] Return separators from 'eshell-split-commands' directly
 when requested

This eliminates the need for using a dynamically-bound variable to
hold the list of separators.

* lisp/eshell/esh-cmd.el (eshell--sep-terms): Remove.
(eshell-split-commands): New function, adapted from
'eshell-separate-commands'.
(eshell-separate-commands): Make obsolete, and call
'eshell-split-commands'.
(eshell-parse-command, eshell-parse-pipeline): Use
'eshell-split-commands'.

* lisp/eshell/esh-arg.el (eshell-parse-delimiter): Update comment.
---
 lisp/eshell/esh-arg.el |   2 +-
 lisp/eshell/esh-cmd.el | 130 ++++++++++++++++++++++-------------------
 2 files changed, 72 insertions(+), 60 deletions(-)

diff --git a/lisp/eshell/esh-arg.el b/lisp/eshell/esh-arg.el
index aa1e8f77ea5..26be1127880 100644
--- a/lisp/eshell/esh-arg.el
+++ b/lisp/eshell/esh-arg.el
@@ -541,7 +541,7 @@ eshell-parse-special-reference
 (defun eshell-parse-delimiter ()
   "Parse an argument delimiter, which is essentially a command operator."
   ;; this `eshell-operator' keyword gets parsed out by
-  ;; `eshell-separate-commands'.  Right now the only possibility for
+  ;; `eshell-split-commands'.  Right now the only possibility for
   ;; error is an incorrect output redirection specifier.
   (when (looking-at "[&|;\n]\\s-*")
     (let ((end (match-end 0)))
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index bd00e9f5406..80066263396 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -364,8 +364,6 @@ eshell-complete-lisp-symbols
 
 ;; Command parsing
 
-(defvar eshell--sep-terms)
-
 (defmacro eshell-with-temp-command (region &rest body)
   "Narrow the buffer to REGION and execute the forms in BODY.
 
@@ -404,31 +402,33 @@ eshell-parse-command
 region.  TOPLEVEL, if non-nil, means that the outermost command (the
 user's input command) is being parsed, and that pre and post command
 hooks should be run before and after the command."
-  (let* (eshell--sep-terms
-	 (terms
-	  (append
-	   (if (consp command)
-	       (eshell-parse-arguments (car command) (cdr command))
-             (eshell-with-temp-command command
-               (goto-char (point-max))
-               (eshell-parse-arguments (point-min) (point-max))))
-	   args))
-	 (commands
-	  (mapcar
-           (lambda (cmd)
-             (let ((sep (pop eshell--sep-terms)))
-               (setq cmd (eshell-parse-pipeline cmd))
-               (when (equal sep "&")
-                 (setq cmd `(eshell-do-subjob (cons :eshell-background ,cmd))))
-               (unless eshell-in-pipeline-p
-                 (setq cmd `(eshell-trap-errors ,cmd)))
-               ;; Copy I/O handles so each full statement can manipulate
-               ;; them if they like.  Steal the handles for the last
-               ;; command in the list; we won't use the originals again
-               ;; anyway.
-               (setq cmd `(eshell-with-copied-handles ,cmd ,(not sep)))
-               cmd))
-	   (eshell-separate-commands terms "[&;]" nil 'eshell--sep-terms))))
+  (pcase-let*
+    ((terms
+      (append
+       (if (consp command)
+           (eshell-parse-arguments (car command) (cdr command))
+         (eshell-with-temp-command command
+           (goto-char (point-max))
+           (eshell-parse-arguments (point-min) (point-max))))
+       args))
+     (`(,sub-chains . ,sep-terms)
+      (eshell-split-commands terms "[&;]" nil t))
+     (commands
+      (mapcar
+       (lambda (cmd)
+         (let ((sep (pop sep-terms)))
+           (setq cmd (eshell-parse-pipeline cmd))
+           (when (equal sep "&")
+             (setq cmd `(eshell-do-subjob (cons :eshell-background ,cmd))))
+           (unless eshell-in-pipeline-p
+             (setq cmd `(eshell-trap-errors ,cmd)))
+           ;; Copy I/O handles so each full statement can manipulate
+           ;; them if they like.  Steal the handles for the last
+           ;; command in the list; we won't use the originals again
+           ;; anyway.
+           (setq cmd `(eshell-with-copied-handles ,cmd ,(not sep)))
+           cmd))
+       sub-chains)))
     (if toplevel
 	`(eshell-commands (progn
                             (run-hooks 'eshell-pre-command-hook)
@@ -628,12 +628,12 @@ eshell--cmd
 
 (defun eshell-parse-pipeline (terms)
   "Parse a pipeline from TERMS, return the appropriate Lisp forms."
-  (let* (eshell--sep-terms
-	 (bigpieces (eshell-separate-commands terms "\\(&&\\|||\\)"
-					      nil 'eshell--sep-terms))
-         results final)
+  (pcase-let*
+      ((`(,bigpieces . ,sep-terms)
+        (eshell-split-commands terms "\\(&&\\|||\\)" nil t))
+       (results) (final))
     (dolist (subterms bigpieces)
-      (let* ((pieces (eshell-separate-commands subterms "|"))
+      (let* ((pieces (eshell-split-commands subterms "|"))
              (p pieces))
         (while p
           (let ((cmd (car p)))
@@ -655,11 +655,11 @@ eshell-parse-pipeline
     ;; multi-line input
     (setq final (car results)
           results (cdr results)
-          eshell--sep-terms (nreverse eshell--sep-terms))
+          sep-terms (nreverse sep-terms))
     (while results
-      (cl-assert (car eshell--sep-terms))
+      (cl-assert (car sep-terms))
       (setq final (eshell-structure-basic-command
-                   'if (string= (pop eshell--sep-terms) "&&") "if"
+                   'if (string= (pop sep-terms) "&&") "if"
                    `(eshell-protect ,(pop results))
                    `(eshell-protect ,final))))
     final))
@@ -696,6 +696,34 @@ eshell-parse-lisp-argument
               (eshell-lisp-command (quote ,obj)))
 	  (ignore (goto-char here))))))
 
+(defun eshell-split-commands (terms separator &optional
+                                    reversed return-seps)
+  "Split TERMS using SEPARATOR.
+If REVERSED is non-nil, the list of separated term groups will be
+returned in reverse order.
+
+If RETURN-SEPS is nil, return just the separated terms as a list;
+otherwise, return both the separated terms and their separators
+as a pair of lists."
+  (let (sub-chains sub-terms sep-terms)
+    (dolist (term terms)
+      (if (and (eq (car-safe term) 'eshell-operator)
+               (string-match (concat "^" separator "$")
+                             (nth 1 term)))
+          (progn
+            (push (nth 1 term) sep-terms)
+            (push (nreverse sub-terms) sub-chains)
+            (setq sub-terms nil))
+        (push term sub-terms)))
+    (when sub-terms
+      (push (nreverse sub-terms) sub-chains))
+    (unless reversed
+      (setq sub-chains (nreverse sub-chains)
+            sep-terms (nreverse sep-terms)))
+    (if return-seps
+        (cons sub-chains sep-terms)
+      sub-chains)))
+
 (defun eshell-separate-commands (terms separator &optional
 				       reversed last-terms-sym)
   "Separate TERMS using SEPARATOR.
@@ -703,30 +731,14 @@ eshell-separate-commands
 returned in reverse order.  If LAST-TERMS-SYM is a symbol, its value
 will be set to a list of all the separator operators found (or (nil)
 if none)."
-  (let ((sub-terms (list t))
-	(eshell-sep-terms (list t))
-	subchains)
-    (while terms
-      (if (and (consp (car terms))
-	       (eq (caar terms) 'eshell-operator)
-	       (string-match (concat "^" separator "$")
-			     (nth 1 (car terms))))
-	  (progn
-	    (nconc eshell-sep-terms (list (nth 1 (car terms))))
-	    (setq subchains (cons (cdr sub-terms) subchains)
-		  sub-terms (list t)))
-	(nconc sub-terms (list (car terms))))
-      (setq terms (cdr terms)))
-    (if (> (length sub-terms) 1)
-	(setq subchains (cons (cdr sub-terms) subchains)))
-    (if reversed
-	(progn
-	  (if last-terms-sym
-	      (set last-terms-sym (reverse (cdr eshell-sep-terms))))
-	  subchains)                    ; already reversed
-      (if last-terms-sym
-	  (set last-terms-sym (cdr eshell-sep-terms)))
-      (nreverse subchains))))
+  (declare (obsolete eshell-split-commands "30.1"))
+  (let ((split-terms (eshell-split-commands terms separator reversed
+                                            last-terms-sym)))
+    (if last-terms-sym
+        (progn
+          (set last-terms-sym (cdr split-terms))
+          (car split-terms))
+      split-terms)))
 
 ;;_* Command evaluation macros
 ;;
-- 
2.25.1