Subject: bug#65162: closed (Re: bug#65162: 30.0.50; [PATCH] Simplify some internal Eshell command parsing functions)

From: [email-masked] (GNU bug Tracking System)
Date: Thu, 10 Aug 2023 17:59:03 +0000

Your bug report

#65162: 30.0.50; [PATCH] Simplify some internal Eshell command parsing functions

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
65162: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=65162
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#65162: 30.0.50; [PATCH] Simplify some internal Eshell command parsing functions
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Thu, 10 Aug 2023 10:57:55 -0700
List-Id: <debbugs-submit.debbugs.gnu.org>

On 8/8/2023 5:16 PM, Jim Porter wrote:
> 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.

Merged to master as 60090abcbc5, and closing this now.

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