bug#18108: 24.3.92 : eshell-visual-options fails with some output.

bug#18108: 24.3.92 : eshell-visual-options fails with some output.

[Charles Rendleman]

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

1) emacs -Q
2) invoke eshell

The following inputs are at the eshell prompt.
3) git --help                                     # shows git help summary
3') *git --help                                   # shows git help summary
4) (add-to-list 'eshell-visual-options '("git" "--help"))
5) git --help                                     # shows nothing
6) bash -c "git --help"                      # shows git help summary
7) *git --help                                    # displays '/bin/git:
external command failed'
8) git --version                                 # displays "git version
2.0.2"

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

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[samer]

Hi friends, Charles, countrymen, and debbugs,

When eshell opens a visual command, it creates a buffer for the process,
switches to that buffer, and, if the process is still running, attaches
a sentinel to the process that kills the process when the sentinel dies.
If the process is not running, the visual command signals an error,
I assume because there is no good reason to show the term buffer for a
process that isn't running.

There is a race condition in the part that handles whether a command is
running: even short-lived commands, like "git --version", are running
when polled, and the error is never thrown. That means we've attached
the buffer-killing sentinel to these short-lived commands, so the buffer
they're attached to gets killed immediately afterwards.

To sum up everything wrong with the current behavior:

1. Killing the process's buffer when the process dies is not the desired
behavior.

2. Even if we wanted to kill the process's buffer for long running
processes but not short ones, there is *no way* to tell how long a
process will run. That means, there is no way to do this correctly.

The solution is simple: for every visual application, simply create the
term-mode buffer for it and switch to that buffer. This satisfies every
use case: if a process dies quickly with an error message, the user will
be able to read the error; if the process is running, it doesn't quit
(this is also the current behavior); if the process is long running and
dies, the user will be able to read the output of the process. I've
attached a patch that implements this behavior below.

There is a small issue with this patch: eshell does not start on a new
line when you execute a visual command, and so you need to press 'enter'
before entering a new command. If anyone has any insight into this, I'm
all ears, otherwise I can probably figure it out eventually.

This is a longer patch, and so it may need to wait for my papers to be
processed. I am not sure how long that will take, as it's been more than
a month since I first submitted my application.

-samer

Patch below:

Changes in 3e0d44a..b9f2247
2 files changed, 14 insertions(+), 37 deletions(-)
  ChangeLog              |    6 ++++++
  lisp/eshell/em-term.el |   45 
++++++++-------------------------------------

	Modified   ChangeLog
diff --git a/ChangeLog b/ChangeLog
index 36edfe6..d09380a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2015-01-19  Samer Masterson  <samer@samertm.com>
+
+	* eshell/em-term.el (eshell-exec-visual, eshell-term-sentinel):
+	Remove eshell-term-sentinel, show term-mode buffer regardless of
+	whether the process has died (bug#18108).
+
  2015-01-04  Paul Eggert  <eggert@cs.ucla.edu>

  	* INSTALL: Mention 'make WERROR_CFLAGS='.
	Modified   lisp/eshell/em-term.el
diff --git a/lisp/eshell/em-term.el b/lisp/eshell/em-term.el
index 4a6ac23..6870a6d 100644
--- a/lisp/eshell/em-term.el
+++ b/lisp/eshell/em-term.el
@@ -132,10 +132,6 @@ character to the invoked process."
    :type 'boolean
    :group 'eshell-term)

-;;; Internal Variables:
-
-(defvar eshell-parent-buffer)
-
  ;;; Functions:

  (defun eshell-term-initialize ()
@@ -171,39 +167,14 @@ allowed."
  					       (cdr args)))))
  	 (term-buf
  	  (generate-new-buffer
-	   (concat "*" (file-name-nondirectory program) "*")))
-	 (eshell-buf (current-buffer)))
-    (save-current-buffer
-      (switch-to-buffer term-buf)
-      (term-mode)
-      (set (make-local-variable 'term-term-name) eshell-term-name)
-      (make-local-variable 'eshell-parent-buffer)
-      (setq eshell-parent-buffer eshell-buf)
-      (term-exec term-buf program program nil args)
-      (let ((proc (get-buffer-process term-buf)))
-	(if (and proc (eq 'run (process-status proc)))
-	    (set-process-sentinel proc 'eshell-term-sentinel)
-	  (error "Failed to invoke visual command")))
-      (term-char-mode)
-      (if eshell-escape-control-x
-	  (term-set-escape-char ?\C-x))))
-  nil)
-
-;; Process sentinels receive two arguments.
-(defun eshell-term-sentinel (proc _string)
-  "Destroy the buffer visiting PROC."
-  (let ((proc-buf (process-buffer proc)))
-    (when (and proc-buf (buffer-live-p proc-buf)
-	       (not (eq 'run (process-status proc)))
-	       (= (process-exit-status proc) 0))
-      (if (eq (current-buffer) proc-buf)
-	  (let ((buf (and (boundp 'eshell-parent-buffer)
-			  eshell-parent-buffer
-			  (buffer-live-p eshell-parent-buffer)
-			  eshell-parent-buffer)))
-	    (if buf
-		(switch-to-buffer buf))))
-      (kill-buffer proc-buf))))
+	   (concat "*" (file-name-nondirectory program) "*"))))
+    (switch-to-buffer term-buf)
+    (term-mode)
+    (set (make-local-variable 'term-term-name) eshell-term-name)
+    (term-exec term-buf program program nil args)
+    (term-char-mode)
+    (if eshell-escape-control-x
+        (term-set-escape-char ?\C-x))))

  ;; jww (1999-09-17): The code below will allow Eshell to send input
  ;; characters directly to the currently running interactive process.

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Samer Masterson]

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

Hi,

My copyright papers have been processed. Can we apply this to master?

Best,
Samer

Patch below:

Changes in 3e0d44a..b9f2247
2 files changed, 14 insertions(+), 37 deletions(-)
 ChangeLog | 6 ++++++
 lisp/eshell/em-term.el | 45
++++++++-------------------------------------

 Modified ChangeLog
diff --git a/ChangeLog b/ChangeLog
index 36edfe6..d09380a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2015-01-19 Samer Masterson <samer <at> samertm.com>
+
+	* eshell/em-term.el (eshell-exec-visual, eshell-term-sentinel):
+	Remove eshell-term-sentinel, show term-mode buffer regardless of
+	whether the process has died (bug#18108).
+
 2015-01-04 Paul Eggert <eggert <at> cs.ucla.edu>

  * INSTALL: Mention 'make WERROR_CFLAGS='.
 Modified lisp/eshell/em-term.el
diff --git a/lisp/eshell/em-term.el b/lisp/eshell/em-term.el
index 4a6ac23..6870a6d 100644
--- a/lisp/eshell/em-term.el
+++ b/lisp/eshell/em-term.el
@@ -132,10 +132,6 @@ character to the invoked process."
   :type 'boolean
   :group 'eshell-term)

-;;; Internal Variables:
-
-(defvar eshell-parent-buffer)
-
 ;;; Functions:

 (defun eshell-term-initialize ()
@@ -171,39 +167,14 @@ allowed."
             (cdr args)))))
   (term-buf
    (generate-new-buffer
-	 (concat "*" (file-name-nondirectory program) "*")))
-	 (eshell-buf (current-buffer)))
- (save-current-buffer
- (switch-to-buffer term-buf)
- (term-mode)
- (set (make-local-variable 'term-term-name) eshell-term-name)
- (make-local-variable 'eshell-parent-buffer)
- (setq eshell-parent-buffer eshell-buf)
- (term-exec term-buf program program nil args)
- (let ((proc (get-buffer-process term-buf)))
-	(if (and proc (eq 'run (process-status proc)))
-	 (set-process-sentinel proc 'eshell-term-sentinel)
-	 (error "Failed to invoke visual command")))
- (term-char-mode)
- (if eshell-escape-control-x
-	 (term-set-escape-char ?\C-x))))
- nil)
-
-;; Process sentinels receive two arguments.
-(defun eshell-term-sentinel (proc _string)
- "Destroy the buffer visiting PROC."
- (let ((proc-buf (process-buffer proc)))
- (when (and proc-buf (buffer-live-p proc-buf)
-	 (not (eq 'run (process-status proc)))
-	 (= (process-exit-status proc) 0))
- (if (eq (current-buffer) proc-buf)
-	 (let ((buf (and (boundp 'eshell-parent-buffer)
-	 eshell-parent-buffer
-	 (buffer-live-p eshell-parent-buffer)
-	 eshell-parent-buffer)))
-	 (if buf
-	 (switch-to-buffer buf))))
- (kill-buffer proc-buf))))
+	 (concat "*" (file-name-nondirectory program) "*"))))
+ (switch-to-buffer term-buf)
+ (term-mode)
+ (set (make-local-variable 'term-term-name) eshell-term-name)
+ (term-exec term-buf program program nil args)
+ (term-char-mode)
+ (if eshell-escape-control-x
+ (term-set-escape-char ?\C-x))))

 ;; jww (1999-09-17): The code below will allow Eshell to send input
 ;; characters directly to the currently running interactive process.

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

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Samer Masterson]

Hi,

Pinging the list with a well-formed patch. Info below (quoted from my reply to 18108):

 > When eshell opens a visual command, it creates a buffer for the process,
 > switches to that buffer, and, if the process is still running, attaches
 > a sentinel to the process that kills the process when the sentinel dies.
 > If the process is not running, the visual command signals an error,
 > I assume because there is no good reason to show the term buffer for a
 > process that isn't running.

 > There is a race condition in the part that handles whether a command is
 > running: even short-lived commands, like "git --version", are running
 > when polled, and the error is never thrown. That means we've attached
 > the buffer-killing sentinel to these short-lived commands, so the buffer
 > they're attached to gets killed immediately afterwards.

 > To sum up everything wrong with the current behavior:

 > 1. Killing the process's buffer when the process dies is not the desired
 > behavior.

 > 2. Even if we wanted to kill the process's buffer for long running
 > processes but not short ones, there is *no way* to tell how long a
 > process will run. That means, there is no way to do this correctly.

 > The solution is simple: for every visual application, simply create the
 > term-mode buffer for it and switch to that buffer. This satisfies every
 > use case: if a process dies quickly with an error message, the user will
 > be able to read the error; if the process is running, it doesn't quit
 > (this is also the current behavior); if the process is long running and
 > dies, the user will be able to read the output of the process. I've
 > attached a patch that implements this behavior below.

 > There is a small issue with this patch: eshell does not start on a new
 > line when you execute a visual command, and so you need to press 'enter'
 > before entering a new command. If anyone has any insight into this, I'm
 > all ears, otherwise I can probably figure it out eventually.

And the patch is below.

-samer

ec2e573b4644ffdfa7e035aeb66f55e847b7c991 HEAD 18108
Author: Samer Masterson <samer@samertm.com>
Date:   Sun Apr 5 20:59:37 2015 -0700

    Fixes 18108.
    
    * eshell/em-term.el (eshell-exec-visual): Show term-mode buffer
    regardless of whether the process has died (bug#18108).
    (eshell-term-sentinel, eshell-parent-buffer): Now unused, remove.

2 files changed, 14 insertions(+), 37 deletions(-)
 lisp/ChangeLog         |  6 ++++++
 lisp/eshell/em-term.el | 45 ++++++++-------------------------------------

	Modified   lisp/ChangeLog
diff --git a/lisp/ChangeLog b/lisp/ChangeLog
index b2d431c..a06ad01 100644
--- a/lisp/ChangeLog
+++ b/lisp/ChangeLog
@@ -1,3 +1,9 @@
+2015-04-06  Samer Masterson  <samer@samertm.com>
+
+	* eshell/em-term.el (eshell-exec-visual): Show term-mode buffer
+	regardless of whether the process has died (bug#18108).
+	(eshell-term-sentinel, eshell-parent-buffer): Now unused, remove.
+
 2015-03-27  Wolfgang Jenkner  <wjenkner@inode.at>
 
 	* font-lock.el (font-lock--remove-face-from-text-property): New
	Modified   lisp/eshell/em-term.el
diff --git a/lisp/eshell/em-term.el b/lisp/eshell/em-term.el
index 4a6ac23..7f98ee6 100644
--- a/lisp/eshell/em-term.el
+++ b/lisp/eshell/em-term.el
@@ -132,10 +132,6 @@ character to the invoked process."
   :type 'boolean
   :group 'eshell-term)
 
-;;; Internal Variables:
-
-(defvar eshell-parent-buffer)
-
 ;;; Functions:
 
 (defun eshell-term-initialize ()
@@ -171,39 +167,14 @@ allowed."
 					       (cdr args)))))
 	 (term-buf
 	  (generate-new-buffer
-	   (concat "*" (file-name-nondirectory program) "*")))
-	 (eshell-buf (current-buffer)))
-    (save-current-buffer
-      (switch-to-buffer term-buf)
-      (term-mode)
-      (set (make-local-variable 'term-term-name) eshell-term-name)
-      (make-local-variable 'eshell-parent-buffer)
-      (setq eshell-parent-buffer eshell-buf)
-      (term-exec term-buf program program nil args)
-      (let ((proc (get-buffer-process term-buf)))
-	(if (and proc (eq 'run (process-status proc)))
-	    (set-process-sentinel proc 'eshell-term-sentinel)
-	  (error "Failed to invoke visual command")))
-      (term-char-mode)
-      (if eshell-escape-control-x
-	  (term-set-escape-char ?\C-x))))
-  nil)
-
-;; Process sentinels receive two arguments.
-(defun eshell-term-sentinel (proc _string)
-  "Destroy the buffer visiting PROC."
-  (let ((proc-buf (process-buffer proc)))
-    (when (and proc-buf (buffer-live-p proc-buf)
-	       (not (eq 'run (process-status proc)))
-	       (= (process-exit-status proc) 0))
-      (if (eq (current-buffer) proc-buf)
-	  (let ((buf (and (boundp 'eshell-parent-buffer)
-			  eshell-parent-buffer
-			  (buffer-live-p eshell-parent-buffer)
-			  eshell-parent-buffer)))
-	    (if buf
-		(switch-to-buffer buf))))
-      (kill-buffer proc-buf))))
+           (concat "*" (file-name-nondirectory program) "*"))))
+    (switch-to-buffer term-buf)
+    (term-mode)
+    (set (make-local-variable 'term-term-name) eshell-term-name)
+    (term-exec term-buf program program nil args)
+    (term-char-mode)
+    (if eshell-escape-control-x
+        (term-set-escape-char ?\C-x))))
 
 ;; jww (1999-09-17): The code below will allow Eshell to send input
 ;; characters directly to the currently running interactive process.

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Stefan Monnier]

>> To sum up everything wrong with the current behavior:

I wonder: for which commands is this used?
I mean, if those commands are things like `vi', or `less', or `emacs',
then it might be inconvenient for the user if the term-buffer just sits
there when the command exits.


        Stefan

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Samer Masterson]

Thanks again for looking at my patch, and sorry (again) for taking so
long to reply! Haha.

Just to refresh our memory, this is the bug:

Charles Rendleman <carendle@gmail.com> writes:
> 3) git --help # shows git help summary
...
> 4) (add-to-list 'eshell-visual-options '("git" "--help"))
> 5) git --help # shows nothing

The reason for this is that eshell kills visual command buffers after
their process dies.

My reply:

Stefan Monnier <monnier@iro.umontreal.ca> writes:
> I wonder: for which commands is this used?
> I mean, if those commands are things like `vi', or `less', or `emacs',
> then it might be inconvenient for the user if the term-buffer just sits
> there when the command exits.

Ah, I hadn't thought of that :) That's a good point, changing this
behavior may be less convenient for some users.

There's a very slim "golden path", though -- the behavior of eshell
"visual" commands is only consistent for programs that 1. are long
running, and 2. only quit when the user asks them, and *never* on their
own, *even* when encountering errors or printing help flags (because the
user will never see output on stderr/stdout). (By consistent, I mean
that the user will always know what to expect when running a visual
command.)

The second point excludes basically all programs from our "golden path"
of consistent behavior. It excludes "less", because less quits during
error conditions (like if you call less without any arguments). It
excludes programs like "mutt" unless you never run "mutt --help".

It also excludes many of the example commands listed in the
documentation. This bug is actually from an example in the
documentation: Adding '("git" "--help") to `eshell-visual-options' is
useful because git shells out to PAGER most of the time... but sometimes
it doesn't! If you add the above and run "git --help", the screen
flashes and you have no indication that something went wrong.

Adding '("git" "log") to `eshell-visual-subcommands' is also in the
documentation, and it works most of the time... except git doesn't shell
out to PAGER when the number of commits in the repo is small. In that
case, the screen flashes and there is no indication to the user why "git
log" works 90% of the time but fails here.

As you can see, eshell visual commands in their visual state are very
hard to use correctly.

But what's worse is that it's very hard for the user to tell what's even
going on. As far as they can tell, their commands are doing nothing
except flickering the eshell window. They have no idea that their
command actually ran and failed, because the command's output isn't
printed. And so they have no way to actually find the "golden path" of
programs that don't break eshell-visual-command, because they cannot
reason about what is going on.

The author of this feature tries to avoid this bug by checking that the
process is not dead before attaching the buffer-killing sentinel to it:

    ;; On lisp/eshell/em-term.el:183
    (let ((proc (get-buffer-process term-buf)))
      (if (and proc (eq 'run (process-status proc)))
          (set-process-sentinel proc 'eshell-term-sentinel)
        (error "Failed to invoke visual command")))

But there's a race here: most programs that will eventually quit (like
"git --help") are still running when the `if' sexp runs.

So that's obviously not ideal :) Arguably, the convenience of being able
to run "git .* --help" as a visual command and have that not break
outweighs the convenience of having a buffer die after its process dies.

I've given a couple reasons why the current behavior is confusing, and
should be removed. The alternative that I've propsed in my patch is to
just to not kill the buffer after its process dies. I think that this
behavior is acceptable because it's consistent with how inferior modes
currently work, e.g. `run-python', `shell', and `ansi-term' all stick
around after their process dies. It's also the easiest way to make
visual commands usable.

I think the absolute ideal would be to detect when a command is
attempting to use "visual" shell features and automatically jump into
"term" mode. I have no idea if that's possible, though.

Thanks again for reviewing my patch! I really appreciate it.

Best,
Samer

Patch below:

Changes in 3e0d44a..b9f2247
2 files changed, 14 insertions(+), 37 deletions(-)
 ChangeLog              |    6 ++++++
 lisp/eshell/em-term.el |   45
++++++++-------------------------------------

	Modified   ChangeLog
diff --git a/ChangeLog b/ChangeLog
index 36edfe6..d09380a 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2015-01-19  Samer Masterson  <samer@samertm.com>
+
+	* eshell/em-term.el (eshell-exec-visual, eshell-term-sentinel):
+	Remove eshell-term-sentinel, show term-mode buffer regardless of
+	whether the process has died (bug#18108).
+
 2015-01-04  Paul Eggert  <eggert@cs.ucla.edu>

 	* INSTALL: Mention 'make WERROR_CFLAGS='.
	Modified   lisp/eshell/em-term.el
diff --git a/lisp/eshell/em-term.el b/lisp/eshell/em-term.el
index 4a6ac23..6870a6d 100644
--- a/lisp/eshell/em-term.el
+++ b/lisp/eshell/em-term.el
@@ -132,10 +132,6 @@ character to the invoked process."
   :type 'boolean
   :group 'eshell-term)

-;;; Internal Variables:
-
-(defvar eshell-parent-buffer)
-
 ;;; Functions:

 (defun eshell-term-initialize ()
@@ -171,39 +167,14 @@ allowed."
 					       (cdr args)))))
 	 (term-buf
 	  (generate-new-buffer
-	   (concat "*" (file-name-nondirectory program) "*")))
-	 (eshell-buf (current-buffer)))
-    (save-current-buffer
-      (switch-to-buffer term-buf)
-      (term-mode)
-      (set (make-local-variable 'term-term-name) eshell-term-name)
-      (make-local-variable 'eshell-parent-buffer)
-      (setq eshell-parent-buffer eshell-buf)
-      (term-exec term-buf program program nil args)
-      (let ((proc (get-buffer-process term-buf)))
-	(if (and proc (eq 'run (process-status proc)))
-	    (set-process-sentinel proc 'eshell-term-sentinel)
-	  (error "Failed to invoke visual command")))
-      (term-char-mode)
-      (if eshell-escape-control-x
-	  (term-set-escape-char ?\C-x))))
-  nil)
-
-;; Process sentinels receive two arguments.
-(defun eshell-term-sentinel (proc _string)
-  "Destroy the buffer visiting PROC."
-  (let ((proc-buf (process-buffer proc)))
-    (when (and proc-buf (buffer-live-p proc-buf)
-	       (not (eq 'run (process-status proc)))
-	       (= (process-exit-status proc) 0))
-      (if (eq (current-buffer) proc-buf)
-	  (let ((buf (and (boundp 'eshell-parent-buffer)
-			  eshell-parent-buffer
-			  (buffer-live-p eshell-parent-buffer)
-			  eshell-parent-buffer)))
-	    (if buf
-		(switch-to-buffer buf))))
-      (kill-buffer proc-buf))))
+	   (concat "*" (file-name-nondirectory program) "*"))))
+    (switch-to-buffer term-buf)
+    (term-mode)
+    (set (make-local-variable 'term-term-name) eshell-term-name)
+    (term-exec term-buf program program nil args)
+    (term-char-mode)
+    (if eshell-escape-control-x
+        (term-set-escape-char ?\C-x))))

 ;; jww (1999-09-17): The code below will allow Eshell to send input
 ;; characters directly to the currently running interactive process.

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Stefan Monnier]

> As you can see, eshell visual commands in their visual state are very
> hard to use correctly.

No doubt.  I think the best way to try and make it work well in "both"
cases is to try and make the behavior closer to what would happen inside
an xterm (say).  E.g. one possibility is that when the process dies,
rather than simply deleting the corresponding term buffer, we first copy
the term buffer's contents to the Eshell's buffer, so as not to lose
this process output.
WDYT?


        Stefan

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Samer Masterson]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> No doubt.  I think the best way to try and make it work well in "both"
> cases is to try and make the behavior closer to what would happen inside
> an xterm (say).  E.g. one possibility is that when the process dies,
> rather than simply deleting the corresponding term buffer, we first copy
> the term buffer's contents to the Eshell's buffer, so as not to lose
> this process output.
> WDYT?

I agree, it's best to emulate xterm when we can. In this case, though,
visual programs are pretty different from "visual" commands in a normal
xterm. The solution you suggested comes close, but visual commands are
"asynchronous" because they don't block the eshell buffer that spawned
them. So it might be confusing if you spawn a visual program, switch
back to eshell and forget about the visual program, and then later kill
the visual program and have its buffer spit into your eshell. It seems a
bit too action-at-a-distance for me, and at the end of the day, it's
more complexity for little gain.

An ideal solution would be to detect when a command is trying to control
the screen and then automatically jump into term mode. I'm not sure how
hard that would be, though.

FWIW, I've been using this patch for the last couple days and it feels
very natural. I've attached a patch that adds a custom var to get the
old behavior, tho the new behavior is default. Let me know if that's the
right direction.

Thanks! I hope you're doing well.
-s

Patch below:


From c58d82ddb2a2b893f39a926a0de98ff8195a8cad Mon Sep 17 00:00:00 2001
From: Samer Masterson <samer@samertm.com>
Date: Mon, 4 May 2015 05:50:57 -0700
Subject: [PATCH] Fix bug#18108

* lisp/eshell/em-term.el (eshell-destroy-buffer-when-process-dies):
New custom to preserve previous behavior.

* lisp/eshell/em-term.el (eshell-term-sentinel): No-op by default,
only kills term if `eshell-destroy-buffer-when-process-dies' is
non-nil.
---
 etc/NEWS               |  5 +++++
 lisp/eshell/em-term.el | 39 +++++++++++++++++++++++++--------------
 2 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 1193055..e779b1b 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -632,6 +632,11 @@ command line's password prompt.
 *** The new built-in command `clear' can scroll window contents out of sight.
 If provided with an optional non-nil argument, the scrollback contents will be cleared.
 
+*** By default, eshell "visual" program buffers (as defined by
+`eshell-custom-program' and the `eshell-term' group) are no longer
+killed when their processes die.  For the old behavior, set
+`eshell-destroy-buffer-when-process-dies' to non-nil.
+
 ** Browse-url
 
 *** Support for the Conkeror web browser.
diff --git a/lisp/eshell/em-term.el b/lisp/eshell/em-term.el
index 4a6ac23..9ac2813 100644
--- a/lisp/eshell/em-term.el
+++ b/lisp/eshell/em-term.el
@@ -132,6 +132,13 @@ character to the invoked process."
   :type 'boolean
   :group 'eshell-term)
 
+(defcustom eshell-destroy-buffer-when-process-dies nil
+  "If non-nil, term buffers are destroyed after their processes die.
+WARNING: Setting this to non-nil may result in unexpected
+behavior for short-lived processes, see bug#18108."
+  :type 'boolean
+  :group 'eshell-term)
+
 ;;; Internal Variables:
 
 (defvar eshell-parent-buffer)
@@ -190,20 +197,24 @@ allowed."
   nil)
 
 ;; Process sentinels receive two arguments.
-(defun eshell-term-sentinel (proc _string)
-  "Destroy the buffer visiting PROC."
-  (let ((proc-buf (process-buffer proc)))
-    (when (and proc-buf (buffer-live-p proc-buf)
-	       (not (eq 'run (process-status proc)))
-	       (= (process-exit-status proc) 0))
-      (if (eq (current-buffer) proc-buf)
-	  (let ((buf (and (boundp 'eshell-parent-buffer)
-			  eshell-parent-buffer
-			  (buffer-live-p eshell-parent-buffer)
-			  eshell-parent-buffer)))
-	    (if buf
-		(switch-to-buffer buf))))
-      (kill-buffer proc-buf))))
+(defun eshell-term-sentinel (proc msg)
+  "Clean up the buffer visiting PROC.
+If `eshell-destroy-buffer-when-process-dies' is non-nil, destroy
+the buffer."
+  (term-sentinel proc msg) ;; First call the normal term sentinel.
+  (when eshell-destroy-buffer-when-process-dies
+    (let ((proc-buf (process-buffer proc)))
+      (when (and proc-buf (buffer-live-p proc-buf)
+                 (not (eq 'run (process-status proc)))
+                 (= (process-exit-status proc) 0))
+        (if (eq (current-buffer) proc-buf)
+            (let ((buf (and (boundp 'eshell-parent-buffer)
+                            eshell-parent-buffer
+                            (buffer-live-p eshell-parent-buffer)
+                            eshell-parent-buffer)))
+              (if buf
+                  (switch-to-buffer buf))))
+        (kill-buffer proc-buf)))))
 
 ;; jww (1999-09-17): The code below will allow Eshell to send input
 ;; characters directly to the currently running interactive process.

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Stefan Monnier]

> FWIW, I've been using this patch for the last couple days and it feels
> very natural.  I've attached a patch that adds a custom var to get the
> old behavior, tho the new behavior is default.  Let me know if that's the
> right direction.

Well, I don't use Eshell so my judgment is not that important, and it
looks like noone is actively maintaining it, so how'bout this deal:
- you make this change.
- and you sign up as maintainer of Eshell.
Do we have a deal?


        Stefan

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Samer Masterson]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> Well, I don't use Eshell so my judgment is not that important, and it
> looks like noone is actively maintaining it, so how'bout this deal:
> - you make this change.
> - and you sign up as maintainer of Eshell.
> Do we have a deal?

Sounds good to me :) Does this mean I have to answer emails promptly
now? (Only kind of joking.)
-s

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Stefan Monnier]

> Sounds good to me :) Does this mean I have to answer emails promptly
> now? (Only kind of joking.)

Well, as promptly as I yes ;-)


        Stefan

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Samer Masterson]

Ping, don't worry if you haven't gotten around to installing this. Also,
what is the process for requesting write access to the Emacs repo?

-s

bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output.

[Samer Masterson]

I've installed this as 76f2d19edd7180874f9539897f674ae05d892419. Thanks
for the review!

-s