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

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

[Jay Kamat]

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

Hi!

I'm filing this separately from #29157, because I think that issue got a
bit overloaded with multiple eshell problems and is very hard to follow.

I recently noticed the changes in #29157, and I'm disappointed that we
came to the conclusion to disable history expansion completely. I find
it's rather useful, especially for things like:

$ mv one.txt two.txtt
# whoops!
$ mv !!:$ two.txt

This is preferred (in my opinion) over lisp functions to keep muscle
memory working between shells. If anything, I would suggest disabling
quick substitution (as I don't find it more useful than using history
directly most of the time)

I've created a patch to try to fix the bug found in #29157, which was:

> echo $PATH | sed "s/[^o]foo[^o]/bar/g"
> Unknown predicate character ‘b’

The fix is rather simple, it simply limits the quick substitution to the
start of the line only (as observed in bash, as Andreas noted in the
previous thread).

I hope that we reconsider the decision to disable history expansion by
default, it's a nice feature of eshell (which I have another patch I
would like to submit later to try to expand it's functionality a bit
more).

Please let me know if you think this is a poor way of solving this issue
(or if anything else seems wrong or missing), and I'll try to follow up.

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: 2146 bytes --]

From 2c14085989a1edb5d3420150dcf91bc0914f012b 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

* lisp/eshell/em-hist.el (eshell-expand-history-references): Calculate
  and send a start-of-line variable to eshell-history-reference
(eshell-history-reference): Use start-of-line variable to force
expansion of quick substitutions only on start of line
---
 lisp/eshell/em-hist.el | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index df462a7058..9561d8b988 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -588,8 +588,9 @@ eshell-expand-history-references
 	    (pose (nreverse (nth 2 result))))
 	(save-excursion
 	  (while textargs
-	    (let ((str (eshell-history-reference (car textargs))))
-	      (unless (eq str (car textargs))
+            (let ((str (eshell-history-reference (car textargs)
+                                                 (not (cdr-safe textargs)))))
+              (unless (eq str (car textargs))
 		(goto-char (car posb))
 		(insert-and-inherit str)
 		(delete-char (- (car pose) (car posb)))))
@@ -630,7 +631,7 @@ eshell-complete-history-reference
 		   (setq history (cdr history)))
 		 (cdr fhist)))))))
 
-(defun eshell-history-reference (reference)
+(defun eshell-history-reference (reference start-of-line)
   "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
@@ -638,7 +639,8 @@ eshell-history-reference
   ;; `^string1^string2^'
   ;;      Quick Substitution.  Repeat the last command, replacing
   ;;      STRING1 with STRING2.  Equivalent to `!!:s/string1/string2/'
-  (if (and (eshell-using-module 'eshell-pred)
+  (if (and start-of-line
+           (eshell-using-module 'eshell-pred)
 	   (string-match "\\^\\([^^]+\\)\\^\\([^^]+\\)\\^?\\s-*$"
 			 reference))
       (setq reference (format "!!:s/%s/%s/"
-- 
2.11.0

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

[Noam Postavsky]

Jay Kamat <jaygkamat@gmail.com> writes:

> I'm filing this separately from #29157, because I think that issue got a
> bit overloaded with multiple eshell problems and is very hard to follow.

Yes, thanks.

> I recently noticed the changes in #29157, and I'm disappointed that we
> came to the conclusion to disable history expansion completely. I find
> it's rather useful, especially for things like:
>
> $ mv one.txt two.txtt
> # whoops!
> $ mv !!:$ two.txt

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.

> This is preferred (in my opinion) over lisp functions to keep muscle
> memory working between shells. If anything, I would suggest disabling
> quick substitution (as I don't find it more useful than using history
> directly most of the time)

(PS my suggestion is almost compatible with bash readline too, just M-p
needs to be C-p instead, and that incompatibility is present in the
history expansion case too).

> I've created a patch to try to fix the bug found in #29157, which was:
>
>> echo $PATH | sed "s/[^o]foo[^o]/bar/g"
>> Unknown predicate character ‘b’
>
> The fix is rather simple, it simply limits the quick substitution to the
> start of the line only (as observed in bash, as Andreas noted in the
> previous thread).
>
> I hope that we reconsider the decision to disable history expansion by
> default, it's a nice feature of eshell (which I have another patch I
> would like to submit later to try to expand it's functionality a bit
> more).
>
> Please let me know if you think this is a poor way of solving this issue
> (or if anything else seems wrong or missing), and I'll try to follow up.

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

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

[Andreas Schwab]

On Dez 31 2017, Noam Postavsky <npostavs@users.sourceforge.net> wrote:

> Hmm, using history expansion would mean typing
>
>     M-p DEL C-a M-f M-d M-d ! ! : $
>
> to get
>
>     mv !!:$ two.txt

History expansion was invented when command line editing wasn't
available, it typically makes only sense for entering a new command line
(while referencing parts of the previous ones).

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Jay Kamat]

[-- 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

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

[Noam Postavsky]

Andreas Schwab <schwab@linux-m68k.org> writes:

> On Dez 31 2017, Noam Postavsky <npostavs@users.sourceforge.net> wrote:
>
>> Hmm, using history expansion would mean typing
>>
>>     M-p DEL C-a M-f M-d M-d ! ! : $
>>
>> to get
>>
>>     mv !!:$ two.txt
>
> History expansion was invented when command line editing wasn't
> available, it typically makes only sense for entering a new command line
> (while referencing parts of the previous ones).

Yes, but I didn't want to handicap the history expansion case unfairly
by rejecting use of other history/editing commands.

Jay Kamat <jaygkamat@gmail.com> writes:

> 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)?

Seems like pointless duplication to me.  I think I would prefer having
history expansion enabled by default to that (it wouldn't be that hard
to disable it, after all).

>> 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!

With your patch, if I do

    ~/src/emacs $ ^this string not present in history^blah^

I get the latest entry in the history substituted and re-executed.  In
bash I get

    ~/tmp$ ^this string not present in history^blah^
    bash: :s^this string not present in history^blah^: substitution failed

(if it's easier, I think it would be okay if eshell prints an error
using the !!:s/foo/bar/ syntax, but this case must be an error)

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

[Jay Kamat]

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

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

> With your patch, if I do
>
>     ~/src/emacs $ ^this string not present in history^blah^
>
> I get the latest entry in the history substituted and re-executed.  In
> bash I get
>
>     ~/tmp$ ^this string not present in history^blah^
>     bash: :s^this string not present in history^blah^: substitution failed
>
> (if it's easier, I think it would be okay if eshell prints an error
> using the !!:s/foo/bar/ syntax, but this case must be an error)

Ah, I did notice that, but I was not sure whether it was a bug or
desired behavior (as it seemed to occur for me even before this patch).
I've added a tiny change to the patch to fix that, but it has the side
effect of doing:

> *[j@laythe emacs-bisect]$ echo "foo"(:s/bar/baz/)
> foo: substitution failed

But I think that's an OK change, especially if we want to error out on
^bar^baz when no search is found.

I also discovered another issue (which existed before as well):

> *[j@laythe emacs-bisect]$ echo one one one
> ("one" "one" "one")
> *[j@laythe emacs-bisect]$ !!:sg/one/two
> :sg/one/two
> Wrong type argument: integer-or-marker-p, nil

but I'd rather take a look at that later on to avoid cluttering this
changeset. (and I'm not sure if I'm just using the feature incorrectly).

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: 4529 bytes --]

From d4ba97c7b15c48d3394b8567b05fae31874220a7 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 ++++++++++++++++++++++++++++++++------------------
 lisp/eshell/em-pred.el |  3 ++-
 2 files changed, 39 insertions(+), 21 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 (.
diff --git a/lisp/eshell/em-pred.el b/lisp/eshell/em-pred.el
index 72a7bc4afc..86a9d4262a 100644
--- a/lisp/eshell/em-pred.el
+++ b/lisp/eshell/em-pred.el
@@ -545,7 +545,8 @@ eshell-pred-substitute
 	  (function
 	   (lambda (str)
 	     (if (string-match ,match str)
-		 (setq str (replace-match ,replace t nil str)))
+		 (setq str (replace-match ,replace t nil str))
+	       (error (concat str ": substitution failed")))
 	     str)) lst)))))
 
 (defun eshell-include-members (&optional invert-p)
-- 
2.11.0

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

[Noam Postavsky]

Jay Kamat <jaygkamat@gmail.com> writes:

> Ah, I did notice that, but I was not sure whether it was a bug or
> desired behavior (as it seemed to occur for me even before this
> patch).

Oh right, I didn't notice because I tested with spaces.  Still, I think
since we're trying to make this behave like bash, we should try to get
as close as possible.

> I've added a tiny change to the patch to fix that, but it has the side
> effect of doing:
>
>> *[j@laythe emacs-bisect]$ echo "foo"(:s/bar/baz/)
>> foo: substitution failed
>
> But I think that's an OK change, especially if we want to error out on
> ^bar^baz when no search is found.

> I also discovered another issue (which existed before as well):
>
>> *[j@laythe emacs-bisect]$ echo one one one
>> ("one" "one" "one")
>> *[j@laythe emacs-bisect]$ !!:sg/one/two
>> :sg/one/two
>> Wrong type argument: integer-or-marker-p, nil
>
> but I'd rather take a look at that later on to avoid cluttering this
> changeset. (and I'm not sure if I'm just using the feature incorrectly).

!!:gs/one/two/ seems to work.  The error message could be improved
though (but yes, we should do that separately).

> +(defun eshell-history-substitution (line)
> +  "Expand whole-line history substitutions by converting them to
> +!!:s/a/b/ syntax.
> +Returns nil if no match found."

Couldn't you error here (if the line matches ^...^...^) instead of
returning nil, and then avoid affecting the other substitution?
(although I agree signaling an error in the other place is probably
acceptable)

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

[Jay Kamat]

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

> Couldn't you error here (if the line matches ^...^...^) instead of
> returning nil, and then avoid affecting the other substitution?
> (although I agree signaling an error in the other place is probably
> acceptable)

I could be missing something, but I don't think this is that easy. In
the case of a failed search for something like '!!:s/a/b/',
`eshell-history-reference' previously returned the previous line,
unmodified. I could pull the previous line and compare it with the one
returned to see if `eshell-history-reference' has modified it, but I
don't like that solution, it seems like a bit of a hack. Let me know if
you think that's better though, or if I have it wrong...

If we really want to preserve the previous behavior of
'echo "foo"(:s/bar/baz/)', I would prefer setting a lexical variable
around functions like `eshell-pred-substitute' so it can figure out
which type of substitution it's in and error accordingly.

Thanks,
-Jay

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

[Noam Postavsky]

Jay Kamat <jaygkamat@gmail.com> writes:

> Noam Postavsky <npostavs@users.sourceforge.net> writes:
>
>> Couldn't you error here (if the line matches ^...^...^) instead of
>> returning nil, and then avoid affecting the other substitution?
>> (although I agree signaling an error in the other place is probably
>> acceptable)
>
> I could be missing something, but I don't think this is that easy. In
> the case of a failed search for something like '!!:s/a/b/',
> `eshell-history-reference' previously returned the previous line,
> unmodified.

Oh, yes, I was confused by your docstring.  By "if no match found" you
meant when the line doesn't match ^foo^bar^ at all; I had somehow got
the impression you meant that there was no match for "foo".

> +(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)

This eq test will always be nil, right?  Because the only time it's t
is when you pass something that's not a history reference, but the thing
we passed is a history reference by construction.  So this could be
simplified to

  (when (and (eshell-using-module 'eshell-pred)
             (string-match "^\\^\\([^^]+\\)\\^\\([^^]+\\)\\^?\\s-*$"
                           line))
    (eshell-history-reference
     (format "!!:s/%s/%s/"
             (match-string 1 line)
             (match-string 2 line))))

That, plus rephrasing the docstring so the first sentence fits on one
line (it should probably also mention ^foo^bar^ syntax), and I think the
patch is good to go (for master).

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

[Jay Kamat]

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

Noam Postavsky <npostavs@users.sourceforge.net> writes:
> Oh, yes, I was confused by your docstring.  By "if no match found" you
> meant when the line doesn't match ^foo^bar^ at all; I had somehow got
> the impression you meant that there was no match for "foo".

Ah, yes, I'll try to make the docstring a bit more clear!

> This eq test will always be nil, right?  Because the only time it's t
> is when you pass something that's not a history reference, but the thing
> we passed is a history reference by construction.  So this could be
> simplified to

Yup, that's true, I'll simplify that down.

Here's a patch which tries to fix those issues.

Thanks again,
-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: 4485 bytes --]

From e7632214858e9f12eed5d9c3cba1fb3c37529db5 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 | 55 ++++++++++++++++++++++++++++++++------------------
 lisp/eshell/em-pred.el |  3 ++-
 2 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index df462a7058..b75d218110 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,26 @@ eshell-complete-history-reference
 		   (setq history (cdr history)))
 		 (cdr fhist)))))))
 
+(defun eshell-history-substitution (line)
+  "Expand quick hist substitutions formatted as ^foo^bar^.
+Returns nil if string does not match quick substitution format,
+and acts like !!:s/foo/bar/ otherwise."
+  ;; `^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))
+    (eshell-history-reference
+     (format "!!:s/%s/%s/"
+	     (match-string 1 line)
+	     (match-string 2 line)))))
+
 (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 (.
diff --git a/lisp/eshell/em-pred.el b/lisp/eshell/em-pred.el
index 72a7bc4afc..86a9d4262a 100644
--- a/lisp/eshell/em-pred.el
+++ b/lisp/eshell/em-pred.el
@@ -545,7 +545,8 @@ eshell-pred-substitute
 	  (function
 	   (lambda (str)
 	     (if (string-match ,match str)
-		 (setq str (replace-match ,replace t nil str)))
+		 (setq str (replace-match ,replace t nil str))
+	       (error (concat str ": substitution failed")))
 	     str)) lst)))))
 
 (defun eshell-include-members (&optional invert-p)
-- 
2.11.0

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

[Noam Postavsky]

Jay Kamat <jaygkamat@gmail.com> writes:

> Noam Postavsky <npostavs@users.sourceforge.net> writes:
>> Oh, yes, I was confused by your docstring.  By "if no match found" you
>> meant when the line doesn't match ^foo^bar^ at all; I had somehow got
>> the impression you meant that there was no match for "foo".
>
> Ah, yes, I'll try to make the docstring a bit more clear!

Thanks, looks good now.

> Here's a patch which tries to fix those issues.

I almost regret to prolong this, but I found another mismatch with bash.
It seems the quick substitution does not need to take up the entire
line:

    ~/tmp$ echo foo bar
    foo bar
    ~/tmp$ ^foo^blah^ etc
    echo blah bar etc
    blah bar etc

Whereas, with your patch:

    ~/src/emacs $ echo foo bar
    ("foo" "bar")
    ~/src/emacs $ ^foo^blah^ etc
    ^foo^blah^: command not found

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

[Jay Kamat]

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

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

> I almost regret to prolong this, but I found another mismatch with bash.
> It seems the quick substitution does not need to take up the entire
> line:
>
>     ~/tmp$ echo foo bar
>     foo bar
>     ~/tmp$ ^foo^blah^ etc
>     echo blah bar etc
>     blah bar etc
>
> Whereas, with your patch:
>
>     ~/src/emacs $ echo foo bar
>     ("foo" "bar")
>     ~/src/emacs $ ^foo^blah^ etc
>     ^foo^blah^: command not found

Ah, nice catch! I've updated the patch to handle that case as well. I've
tested it as well as I could and it seems good to me, but in order to
fix that case, I had to mess with the regexes a bit, so It's possible I
might have missed a few cases.

    *[j@laythe emacs]$ echo one two three
    ("one" "two" "three")
    *[j@laythe emacs]$ ^one two^five six^ seven eight
    *[j@laythe emacs]$ echo five six three seven eight
    ("five" "six" "three" "seven" "eight")
    *[j@laythe emacs]$ 

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: 4804 bytes --]

From 729a73af7352b2870e65f8366c97cde49e5b1e78 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

In Addition:
- Allow spaces inside substitutions, so ^foo bar^baz works.
- Allow trailing characters after substitution, so ^foo^bar^ trailing
works.
- Throw an error when substitution does not match.

* 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 | 60 +++++++++++++++++++++++++++++++++-----------------
 lisp/eshell/em-pred.el |  3 ++-
 2 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index df462a7058..f77b831b56 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,31 @@ eshell-complete-history-reference
 		   (setq history (cdr history)))
 		 (cdr fhist)))))))
 
+(defun eshell-history-substitution (line)
+  "Expand quick hist substitutions formatted as ^foo^bar^.
+Returns nil if string does not match quick substitution format,
+and acts like !!:s/foo/bar/ otherwise."
+  ;; `^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-+.*\\)?\\)?\\s-*$"
+	      line))
+    ;; Save trailing match as `eshell-history-reference' runs string-match.
+    (let ((matched-end (match-string 4 line)))
+      (concat
+       (eshell-history-reference
+	(format "!!:s/%s/%s/"
+		(match-string 1 line)
+		(match-string 2 line)))
+       matched-end))))
+
 (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 (.
diff --git a/lisp/eshell/em-pred.el b/lisp/eshell/em-pred.el
index 72a7bc4afc..86a9d4262a 100644
--- a/lisp/eshell/em-pred.el
+++ b/lisp/eshell/em-pred.el
@@ -545,7 +545,8 @@ eshell-pred-substitute
 	  (function
 	   (lambda (str)
 	     (if (string-match ,match str)
-		 (setq str (replace-match ,replace t nil str)))
+		 (setq str (replace-match ,replace t nil str))
+	       (error (concat str ": substitution failed")))
 	     str)) lst)))))
 
 (defun eshell-include-members (&optional invert-p)
-- 
2.11.0

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

[Noam Postavsky]

Jay Kamat <jaygkamat@gmail.com> writes:

> Ah, nice catch! I've updated the patch to handle that case as well. I've
> tested it as well as I could and it seems good to me, but in order to
> fix that case, I had to mess with the regexes a bit, so It's possible I
> might have missed a few cases.

There doesn't need to be trailing whitespace between the last "^" and
subsequent text, bash:

    ~/tmp$ echo foo bar
    foo bar
    ~/tmp$ ^foo^blah^x
    echo blah barx
    blah barx

eshell (with your patch):

    ~/src/emacs $ echo foo bar
    ("foo" "bar")
    ~/src/emacs $ ^foo^blah^x
    ^foo^blah^x: command not found

And as far as I can tell, trailing whitespace should not be dropped
(though it's hard to come up with cases where it matters):

    ~/tmp$ echo 'foo bar '
    foo bar 
    ~/tmp$ ^bar^zz  
    echo 'foo zz   '
    foo zz   

So I think this should do it?

    ...	     
         (string-match
	      "^\\^\\([^^]+\\)\\^\\([^^]+\\)\\(?:\\^\\(.*\\)\\)?$"
	      line))
    ;; Save trailing match as `eshell-history-reference' runs string-match.
    (let ((matched-end (match-string 3 line)))
    ...

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

[Jay Kamat]

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

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

> There doesn't need to be trailing whitespace between the last "^" and
> subsequent text, bash:


> So I think this should do it?
>
>     ...	     
>          (string-match
> 	      "^\\^\\([^^]+\\)\\^\\([^^]+\\)\\(?:\\^\\(.*\\)\\)?$"
> 	      line))
>     ;; Save trailing match as `eshell-history-reference' runs string-match.
>     (let ((matched-end (match-string 3 line)))
>     ...

Yup, that regex works perfectly for everything I tried, and preserves
the white-space at the end of the target (as bash does). I think it's a
good idea to follow bash as closely as possible.

For convenience, I've attached a new patch with those changes.

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: 4795 bytes --]

From b901ce56d646d7145989825f7c4b8d408bc500ac 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

In Addition:
- Allow spaces inside substitutions, so ^foo bar^baz works.
- Allow trailing characters after substitution, so ^foo^bar^ trailing
works.
- Throw an error when substitution does not match.

* 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 | 60 +++++++++++++++++++++++++++++++++-----------------
 lisp/eshell/em-pred.el |  3 ++-
 2 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index df462a7058..f923ca679b 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,31 @@ eshell-complete-history-reference
 		   (setq history (cdr history)))
 		 (cdr fhist)))))))
 
+(defun eshell-history-substitution (line)
+  "Expand quick hist substitutions formatted as ^foo^bar^.
+Returns nil if string does not match quick substitution format,
+and acts like !!:s/foo/bar/ otherwise."
+  ;; `^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
+	      "^\\^\\([^^]+\\)\\^\\([^^]+\\)\\(?:\\^\\(.*\\)\\)?$"
+	      line))
+    ;; Save trailing match as `eshell-history-reference' runs string-match.
+    (let ((matched-end (match-string 3 line)))
+      (concat
+       (eshell-history-reference
+	(format "!!:s/%s/%s/"
+		(match-string 1 line)
+		(match-string 2 line)))
+       matched-end))))
+
 (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 (.
diff --git a/lisp/eshell/em-pred.el b/lisp/eshell/em-pred.el
index 72a7bc4afc..86a9d4262a 100644
--- a/lisp/eshell/em-pred.el
+++ b/lisp/eshell/em-pred.el
@@ -545,7 +545,8 @@ eshell-pred-substitute
 	  (function
 	   (lambda (str)
 	     (if (string-match ,match str)
-		 (setq str (replace-match ,replace t nil str)))
+		 (setq str (replace-match ,replace t nil str))
+	       (error (concat str ": substitution failed")))
 	     str)) lst)))))
 
 (defun eshell-include-members (&optional invert-p)
-- 
2.11.0

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

[Noam Postavsky]

tags 29821 fixed
close 29821 27.1
quit

Jay Kamat <jaygkamat@gmail.com> writes:

> Yup, that regex works perfectly for everything I tried, and preserves
> the white-space at the end of the target (as bash does). I think it's a
> good idea to follow bash as closely as possible.
>
> For convenience, I've attached a new patch with those changes.

Thanks, pushed to master (with slight changes to the commit message to
better reflect the expanded scope of this bug).

[1: 933d8fc0b7]: 2018-01-05 09:29:00 -0500
  Make eshell history expansion more like bash (Bug#29821)
  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=933d8fc0b70452f8a266e761231e58a759a7c80a