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

[Samer Masterson <samer@samertm.com>]

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.