bug#29821: Ensure quick substitution only occurs at start of line

[Jay Kamat <jaygkamat@gmail.com>]

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

Noam Postavsky <npostavs@users.sourceforge.net> writes:

> Hmm, using history expansion would mean typing
>
>     M-p DEL C-a M-f M-d M-d ! ! : $
>
> to get
>
>     mv !!:$ two.txt
>
> vs 
>
>     M-p C-a M-f M-d M-d C-k C-y C-y DEL
>
> to get
>
>     mv two.txtt two.txt
>
> Hardly seems worth the trouble of learning this syntax (and occasionally
> triggering accidentally, which is why I disable it in bash too).  Is
> having history expansion enabled by default very important?  You can
> still enable it in your config.

I do suppose that is true, but I still find myself preferring history
substitution in some cases, such as typing a new command but preserving
the last argument.

I don't think it's too important that history expansion is enabled by
default, but I do think it's important to have it available as an
option. I think that turning it off would startle some users, especially
because it's featured in some popular 'getting started' articles for
eshell (such as this one:
https://masteringemacs.org/article/complete-guide-mastering-eshell), but
the decision is up to you.

I would much prefer a variable (perhaps defaulting to off) to tweak this
setting on or of rather than adding/removing a function to the hook.
Removing it in the current way makes it feel more 'deprecated' to me,
rather than 'disabled by default'.

Would you mind if I submitted a patch to add a new
`eshell-history-expansion-enabled' variable (or similar)?

> I guess it's an improvement on what we have currently (the feature is
> rather underspecified).  Should we consider also handling spaces like
> bash does?  In bash I can do this:
>
>     ~/tmp$ echo foo bar
>     foo bar
>     ~/tmp$ ^foo bar^blah^
>     echo blah
>     blah
>
> In eshell (with and without your patch) I get:
>
>     ~/src/emacs $ echo foo bar
>     ("foo" "bar")
>     ~/src/emacs $ ^foo bar^blah^
>     ^foo: command not found

I've attached a new patch which attempts to solve this as well. I'm
unfamiliar with eshell internals though, so I'm not sure if it's done
properly. Let me know if anyone sees any issues with it!

Thanks,
-Jay


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Prevent-expansion-of-quick-substitutions-when-not-at.patch --]
[-- Type: text/x-diff, Size: 3995 bytes --]

From 573b03a76798496b3bcfcada1557f1a9d83cc987 Mon Sep 17 00:00:00 2001
From: Jay Kamat <jaygkamat@gmail.com>
Date: Fri, 22 Dec 2017 15:34:44 -0800
Subject: [PATCH] Prevent expansion of quick substitutions when not at start of
 line

See bug #29157 for an initial report

Also, allow spaces inside substitutions, so ^foo bar^baz works.

* lisp/eshell/em-hist.el (eshell-expand-history-references): Expand
  history substitution before other types of expansions, and expand
  them with the whole line.
(eshell-history-substitution): New function to expand only
substitutions, taking in the entire typed line rather than individual
arguments.
---
 lisp/eshell/em-hist.el | 57 ++++++++++++++++++++++++++++++++------------------
 1 file changed, 37 insertions(+), 20 deletions(-)

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index df462a7058..d4d4b93b81 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -581,21 +581,30 @@ eshell-hist-parse-arguments
 
 (defun eshell-expand-history-references (beg end)
   "Parse and expand any history references in current input."
-  (let ((result (eshell-hist-parse-arguments beg end)))
+  (let ((result (eshell-hist-parse-arguments beg end))
+	(full-line (buffer-substring-no-properties beg end)))
     (when result
       (let ((textargs (nreverse (nth 0 result)))
 	    (posb (nreverse (nth 1 result)))
-	    (pose (nreverse (nth 2 result))))
+	    (pose (nreverse (nth 2 result)))
+	    (full-line-subst (eshell-history-substitution full-line)))
 	(save-excursion
-	  (while textargs
-	    (let ((str (eshell-history-reference (car textargs))))
-	      (unless (eq str (car textargs))
-		(goto-char (car posb))
-		(insert-and-inherit str)
-		(delete-char (- (car pose) (car posb)))))
-	    (setq textargs (cdr textargs)
-		  posb (cdr posb)
-		  pose (cdr pose))))))))
+	  (if full-line-subst
+	      ;; Found a ^foo^bar substitution
+	      (progn
+		(goto-char beg)
+		(insert-and-inherit full-line-subst)
+		(delete-char (- end beg)))
+	    ;; Try to expand other substitutions
+	    (while textargs
+	      (let ((str (eshell-history-reference (car textargs))))
+		(unless (eq str (car textargs))
+		  (goto-char (car posb))
+		  (insert-and-inherit str)
+		  (delete-char (- (car pose) (car posb)))))
+	      (setq textargs (cdr textargs)
+		    posb (cdr posb)
+		    pose (cdr pose)))))))))
 
 (defvar pcomplete-stub)
 (defvar pcomplete-last-completion-raw)
@@ -630,20 +639,28 @@ eshell-complete-history-reference
 		   (setq history (cdr history)))
 		 (cdr fhist)))))))
 
+(defun eshell-history-substitution (line)
+  "Expand whole-line history substitutions by converting them to
+!!:s/a/b/ syntax.
+Returns nil if no match found."
+  ;; `^string1^string2^'
+  ;;      Quick Substitution.  Repeat the last command, replacing
+  ;;      STRING1 with STRING2.  Equivalent to `!!:s/string1/string2/'
+  (when (and (eshell-using-module 'eshell-pred)
+	     (string-match "^\\^\\([^^]+\\)\\^\\([^^]+\\)\\^?\\s-*$"
+			   line))
+    (let* ((reference (format "!!:s/%s/%s/"
+			      (match-string 1 line)
+			      (match-string 2 line)))
+	   (result (eshell-history-reference reference)))
+      (unless (eq result reference)
+	result))))
+
 (defun eshell-history-reference (reference)
   "Expand directory stack REFERENCE.
 The syntax used here was taken from the Bash info manual.
 Returns the resultant reference, or the same string REFERENCE if none
 matched."
-  ;; `^string1^string2^'
-  ;;      Quick Substitution.  Repeat the last command, replacing
-  ;;      STRING1 with STRING2.  Equivalent to `!!:s/string1/string2/'
-  (if (and (eshell-using-module 'eshell-pred)
-	   (string-match "\\^\\([^^]+\\)\\^\\([^^]+\\)\\^?\\s-*$"
-			 reference))
-      (setq reference (format "!!:s/%s/%s/"
-			      (match-string 1 reference)
-			      (match-string 2 reference))))
   ;; `!'
   ;;      Start a history substitution, except when followed by a
   ;;      space, tab, the end of the line, = or (.
-- 
2.11.0