bug#26161: 25.1; `eshell-exit-success-p' determines that Lisp commands are successful if they return non-nil

bug#26161: 25.1; `eshell-exit-success-p' determines that Lisp commands are successful if they return non-nil

[George D. Plymale]

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

Hi,

I believe that it is sub-optimal behavior for `eshell-exit-success-p' to
determine that Lisp commands are successful by checking whether or not
they return a non-nil value. A demonstration of why this behavior can be
considered problematic is found in a command like this: `$ cd .. && pwd'

Such a command will not execute its second part (which is `pwd') because
`eshell/cd' returns a nil value whether it's successful or not. This
behavior is a bit confusing for someone who expects common shell
operators such as `&&' to "just work."

A better solution would be to check whether the last command threw an
actual error.

Thanks,

- George Plymale II

In GNU Emacs 25.1.1 (x86_64-apple-darwin16.1.0, NS appkit-1504.60 Version 10.12.1 (Build 16B2657))
of 2016-11-14 built on [REDACTED]
Repository revision: f0eb70d8935be90f7c03e187c12d9b60e7214cc6
Windowing system distributor 'Apple', version 10.3.1504
Configured using:
'configure --with-ns'




[-- Attachment #2: Type: text/html, Size: 1734 bytes --]

bug#26161: 25.1; `eshell-exit-success-p' determines that Lisp commands are successful if they return non-nil

[npostavs]

tags 26161 confirmed
severity 26161 minor
quit

"George D. Plymale" <georgedp@orbitalimpact.com> writes:

> I believe that it is sub-optimal behavior for `eshell-exit-success-p' to
> determine that Lisp commands are successful by checking whether or not
> they return a non-nil value. A demonstration of why this behavior can be
> considered problematic is found in a command like this: `$ cd .. && pwd'

So maybe `eshell/cd' should be changed to return t when it succeeds?

> Such a command will not execute its second part (which is `pwd') because
> `eshell/cd' returns a nil value whether it's successful or not. This
> behavior is a bit confusing for someone who expects common shell
> operators such as `&&' to "just work."
>
> A better solution would be to check whether the last command threw an
> actual error.

AFAICT, when you cd to a non-existent directory it doesn't throw an
error, but I don't think that should be considered success.

bug#26161: 25.1; `eshell-exit-success-p' determines that Lisp commands are successful if they return non-nil

[George D. Plymale]

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



> So maybe `eshell/cd' should be changed to return t when it succeeds?

That would only solve the problem for this particular command. It's worth
noting that even doing things like, e.g., `$ .. && pwd' or `$ cat ~/.emacs &&
pwd' don't execute their latter halves because the functions that the former
parts expand to return nil, even if they are in fact successful. Honestly,
it'd probably be better off if `eshell-exit-success-p' just
checked`eshell-last-command-status' and Eshell makes sure that erring commands
always set that to non-zero (which I think is already covered by
`eshell-trap-errors').

> AFAICT, when you cd to a non-existent directory it doesn't throw an
> error, but I don't think that should be considered success.

Yeah, `eshell/cd' actually is able to bypass something like
`eshell-trap-errors' because it uses `cd' under the hood through this
function invocation: `(eshell-printn result)' where `result' is `(cd
newdir)'. See, `eshell-printn' just captures the result of its given
object and prints that out. In some cases, that object is an error (like
when you cd into a non-existent directory), but you can't really tell
that because `eshell-printn' doesn't care about errors; it just prints
out the object it's given. Functions that do this sort of thing may also
exist aside from `eshell/cd', so I'm unsure what can be done about that.

[-- Attachment #2: Type: text/html, Size: 2120 bytes --]

bug#26161: 25.1; `eshell-exit-success-p' determines that Lisp commands are successful if they return non-nil

[npostavs]

"George D. Plymale" <georgedp@orbitalimpact.com> writes:

> it'd probably be better off if `eshell-exit-success-p' just
> checked`eshell-last-command-status' and Eshell makes sure that erring commands
> always set that to non-zero (which I think is already covered by
> `eshell-trap-errors').

That makes sense to me.

>> AFAICT, when you cd to a non-existent directory it doesn't throw an
>> error, but I don't think that should be considered success.
>
> Yeah, `eshell/cd' actually is able to bypass something like
> `eshell-trap-errors' because it uses `cd' under the hood through this
> function invocation: `(eshell-printn result)' where `result' is `(cd
> newdir)'. See, `eshell-printn' just captures the result of its given
> object and prints that out. In some cases, that object is an error (like
> when you cd into a non-existent directory), but you can't really tell
> that because `eshell-printn' doesn't care about errors; it just prints
> out the object it's given. Functions that do this sort of thing may also
> exist aside from `eshell/cd', so I'm unsure what can be done about that.

It looks like `eshell-exec-lisp' catches the error, so probably
`eshell-last-command-status' could be set there.  Patches welcome.

bug#26161: 25.1; `eshell-exit-success-p' determines that Lisp commands are successful if they return non-nil

[George D. Plymale]

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


--=-=-=
Content-Type: text/x-patch
Content-Disposition: attachment; filename=bug_26161_fix.diff
Content-Description: Fix for bug#26161

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 583ba6ac42..86e7b83c28 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -575,14 +575,9 @@ eshell-rewrite-if-command
(defvar eshell-last-command-result)     ;Defined in esh-io.el.

(defun eshell-exit-success-p ()
-  "Return non-nil if the last command was \"successful\".
-For a bit of Lisp code, this means a return value of non-nil.
-For an external command, it means an exit code of 0."
-  (if (save-match-data
-	(string-match "#<\\(Lisp object\\|function .*\\)>"
-		      eshell-last-command-name))
-      eshell-last-command-result
-    (= eshell-last-command-status 0)))
+  "Return non-nil if the last command was successful.
+This means an exit code of 0."
+  (= eshell-last-command-status 0))

(defvar eshell--cmd)

@@ -1257,6 +1252,7 @@ eshell-exec-lisp
         (and result (funcall printer result))
         result)
     (error
+     (setq eshell-last-command-status 1)
      (let ((msg (error-message-string err)))
        (if (and (not form-p)
                 (string-match "^Wrong number of arguments" msg)

--=-=-=
Content-Type: text/plain

Okay, so I created a patch to fix this issue. Go easy on me since this
is my first patch to Emacs ;)

The changes are pretty simple:

- `eshell-exit-success-p' has been changed to only check if the last
  exit code was zero, rather than first checking whether the last command
  returned nil.
- `eshell-exec-lisp' has been changed so that it will set
  `eshell-last-command-status' to 1 if it catches an error.
- These changes together make it so that the `&&' operator in Eshell
  behaves more expectedly to someone who has used a bash-like shell and
  so that other things involving the success of Lisp commands in Eshell
  are more reliable.
  
Feel free to point out anything else that should be done here or any
errors on my part. I have tested these changes out and everything seems
okay. I can run the aforementioned commands that were problematic. Examples:

~ $ cd .. && pwd
/Users

/Users $ cd - && pwd
/Users/my_username

~ $ .. && pwd
/Users

/Users $ cd - && pwd
/Users/my_username

~ $ cat ~/.emacs && pwd
;; Emacs init file stuff
(foo bar)/Users/my_username

~ $ cat nowhere && pwd
cat: nowhere: No such file or directory

~ $ cat nowhere ; pwd
cat: nowhere: No such file or directory
/Users/my_username

~ $ 

--=-=-=--




[-- Attachment #2: Type: text/html, Size: 5598 bytes --]

bug#26161: 25.1; `eshell-exit-success-p' determines that Lisp commands are successful if they return non-nil

[npostavs]

tags 26161 patch
quit

"George D. Plymale" <georgedp@orbitalimpact.com> writes:
   
> Feel free to point out anything else that should be done here or any
> errors on my part. I have tested these changes out and everything seems
> okay.

Looks good, could you try adding a commit message as decribed in
CONTRIBUTE, and then 'git format-patch' will produce the patch along
with the message.

bug#26161: 25.1; `eshell-exit-success-p' determines that Lisp commands are successful if they return non-nil

[George Plymale II]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Patch --]
[-- Type: text/x-patch, Size: 1950 bytes --]

From ed579eaaaaf4fd6a0b78a8c52a897eaa5cb322de Mon Sep 17 00:00:00 2001
From: "George D. Plymale II" <georgedp@orbitalimpact.com>
Date: Thu, 20 Apr 2017 14:05:11 -0400
Subject: [PATCH] Fix for bug#26161

* `eshell-exit-success-p' has been changed to only check if the last
  exit code was zero, rather than first checking whether the last command
  returned nil.
* `eshell-exec-lisp' has been changed so that it will set
  `eshell-last-command-status' to 1 if it catches an error.
* These changes together make it so that the `&&' operator in Eshell
  behaves more expectedly to someone who has used a bash-like shell and
  so that other things involving the success of Lisp commands in Eshell
  are more reliable.
---
 lisp/eshell/esh-cmd.el | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index d3613d3140..53a266d149 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -575,14 +575,9 @@ eshell-rewrite-if-command
 (defvar eshell-last-command-result)     ;Defined in esh-io.el.
 
 (defun eshell-exit-success-p ()
-  "Return non-nil if the last command was \"successful\".
-For a bit of Lisp code, this means a return value of non-nil.
-For an external command, it means an exit code of 0."
-  (if (save-match-data
-	(string-match "#<\\(Lisp object\\|function .*\\)>"
-		      eshell-last-command-name))
-      eshell-last-command-result
-    (= eshell-last-command-status 0)))
+  "Return non-nil if the last command was successful.
+This means an exit code of 0."
+  (= eshell-last-command-status 0))
 
 (defvar eshell--cmd)
 
@@ -1257,6 +1252,7 @@ eshell-exec-lisp
         (and result (funcall printer result))
         result)
     (error
+     (setq eshell-last-command-status 1)
      (let ((msg (error-message-string err)))
        (if (and (not form-p)
                 (string-match "^Wrong number of arguments" msg)
-- 
2.11.0 (Apple Git-81)


[-- Attachment #2: Type: text/plain, Size: 395 bytes --]

Thanks for the advice. Apologies for the crappy formatting on my last
message. I think my system mail client and my Emacs mail client had a
disagreement and it ended up looking like that. So now I'm sending this
directly via the Emacs mail client (hopefully it doesn't screw stuff up
either). The requested patch is attached now to this message, so please
let me know if you need anything else.

bug#26161: 25.1; `eshell-exit-success-p' determines that Lisp commands are successful if they return non-nil

[npostavs]

tags 26161 fixed
close 26161 26.1
quit

George Plymale II <georgedp@orbitalimpact.com> writes:

> Thanks for the advice. Apologies for the crappy formatting on my last
> message. I think my system mail client and my Emacs mail client had a
> disagreement and it ended up looking like that.

It looked a bit funny, but there was no line wrapping so it didn't do
any harm.

> So now I'm sending this
> directly via the Emacs mail client (hopefully it doesn't screw stuff up
> either). The requested patch is attached now to this message, so please
> let me know if you need anything else.

Thanks, pushed to master [1: e8875bcbe0].  I reformatted your commit
message to conform to the ChangeLog format and marked this as a
copyright exempt change.

1: 2017-04-20 23:03:10 -0400
  Treat non-erroring lisp call as successful eshell command (Bug#26161)
http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=e8875bcbe067ea020dba95530ec4e9485942babd