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

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 26161 in the body.
You can then email your comments to 26161 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "George D. Plymale" <georgedp <at> orbitalimpact.com>
To: bug-gnu-emacs <at> gnu.org
Subject: 25.1; `eshell-exit-success-p' determines that Lisp commands are
 successful if they return non-nil
Date: Sat, 18 Mar 2017 15:12:41 -0400

[Message part 1 (text/plain, inline)]

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'

[Message part 2 (text/html, inline)]

Message #8 received at 26161 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: "George D. Plymale" <georgedp <at> orbitalimpact.com>
Cc: 26161 <at> debbugs.gnu.org
Subject: Re: bug#26161: 25.1;
 `eshell-exit-success-p' determines that Lisp commands are successful
 if they return non-nil
Date: Thu, 30 Mar 2017 23:52:39 -0400

tags 26161 confirmed
severity 26161 minor
quit

"George D. Plymale" <georgedp <at> 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.

Message #15 received at 26161 <at> debbugs.gnu.org (full text, mbox):

From: "George D. Plymale" <georgedp <at> orbitalimpact.com>
To: npostavs <at> users.sourceforge.net
Cc: 26161 <at> debbugs.gnu.org
Subject: Re: bug#26161: 25.1;  `eshell-exit-success-p' determines that Lisp
 commands are successful  if they return non-nil
Date: Sat, 1 Apr 2017 19:46:00 -0400

[Message part 1 (text/plain, inline)]

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

[Message part 2 (text/html, inline)]

Message #18 received at 26161 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: "George D. Plymale" <georgedp <at> orbitalimpact.com>
Cc: 26161 <at> debbugs.gnu.org
Subject: Re: bug#26161: 25.1;
 `eshell-exit-success-p' determines that Lisp commands are successful
 if they return non-nil
Date: Sat, 01 Apr 2017 20:06:19 -0400

"George D. Plymale" <georgedp <at> 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.

Message #21 received at 26161 <at> debbugs.gnu.org (full text, mbox):

From: "George D. Plymale" <georgedp <at> orbitalimpact.com>
To: npostavs <at> users.sourceforge.net
Cc: 26161 <at> debbugs.gnu.org
Subject: Re: bug#26161: 25.1;  `eshell-exit-success-p' determines that Lisp
 commands are successful  if they return non-nil
Date: Tue, 11 Apr 2017 15:48:05 -0400

[Message part 1 (text/plain, inline)]

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

~ $ 

--=-=-=--

[Message part 2 (text/html, inline)]

Message #24 received at 26161 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: "George D. Plymale" <georgedp <at> orbitalimpact.com>
Cc: 26161 <at> debbugs.gnu.org
Subject: Re: bug#26161: 25.1;
 `eshell-exit-success-p' determines that Lisp commands are successful
 if they return non-nil
Date: Wed, 12 Apr 2017 21:18:25 -0400

tags 26161 patch
quit

"George D. Plymale" <georgedp <at> 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.

Message #29 received at 26161 <at> debbugs.gnu.org (full text, mbox):

From: George Plymale II <georgedp <at> orbitalimpact.com>
To: npostavs <at> users.sourceforge.net
Cc: 26161 <at> debbugs.gnu.org
Subject: Re: bug#26161: 25.1;
 `eshell-exit-success-p' determines that Lisp commands are successful
 if they return non-nil
Date: Thu, 20 Apr 2017 15:47:45 -0400

[0001-Fix-for-bug-26161.patch (text/x-patch, attachment)]

[Message part 2 (text/plain, inline)]

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.

Message #32 received at 26161 <at> debbugs.gnu.org (full text, mbox):

From: npostavs <at> users.sourceforge.net
To: George Plymale II <georgedp <at> orbitalimpact.com>
Cc: 26161 <at> debbugs.gnu.org
Subject: Re: bug#26161: 25.1;
 `eshell-exit-success-p' determines that Lisp commands are successful
 if they return non-nil
Date: Thu, 20 Apr 2017 23:10:52 -0400

tags 26161 fixed
close 26161 26.1
quit

George Plymale II <georgedp <at> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.