From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!not-for-mail From: Samer Masterson Newsgroups: gmane.emacs.bugs Subject: bug#18108: [PATCH] 24.3.92 : eshell-visual-options fails with some output. Date: Thu, 30 Apr 2015 23:53:43 -0700 Message-ID: <87sibgeume.fsf@f-box.i-did-not-set--mail-host-address--so-tickle-me> References: <1428293143.15170.1@mail.samertm.com> NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: ger.gmane.org 1430463704 13640 80.91.229.3 (1 May 2015 07:01:44 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Fri, 1 May 2015 07:01:44 +0000 (UTC) Cc: 18108@debbugs.gnu.org To: Stefan Monnier Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Fri May 01 09:01:33 2015 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by plane.gmane.org with esmtp (Exim 4.69) (envelope-from ) id 1Yo4wz-0000L5-MX for geb-bug-gnu-emacs@m.gmane.org; Fri, 01 May 2015 09:01:29 +0200 Original-Received: from localhost ([::1]:47174 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yo4wz-0006Yx-9S for geb-bug-gnu-emacs@m.gmane.org; Fri, 01 May 2015 03:01:29 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:43277) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yo4ps-0007f2-F9 for bug-gnu-emacs@gnu.org; Fri, 01 May 2015 02:54:09 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yo4pn-0005KX-Gm for bug-gnu-emacs@gnu.org; Fri, 01 May 2015 02:54:08 -0400 Original-Received: from debbugs.gnu.org ([140.186.70.43]:48089) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yo4pn-0005Je-Cw for bug-gnu-emacs@gnu.org; Fri, 01 May 2015 02:54:03 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.80) (envelope-from ) id 1Yo4pm-0007Hr-OZ for bug-gnu-emacs@gnu.org; Fri, 01 May 2015 02:54:03 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Samer Masterson Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 01 May 2015 06:54:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 18108 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: Original-Received: via spool by 18108-submit@debbugs.gnu.org id=B18108.143046323928002 (code B ref 18108); Fri, 01 May 2015 06:54:02 +0000 Original-Received: (at 18108) by debbugs.gnu.org; 1 May 2015 06:53:59 +0000 Original-Received: from localhost ([127.0.0.1]:58064 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1Yo4pi-0007HZ-O5 for submit@debbugs.gnu.org; Fri, 01 May 2015 02:53:59 -0400 Original-Received: from mx2.mailbox.org ([80.241.60.215]:53657) by debbugs.gnu.org with esmtp (Exim 4.80) (envelope-from ) id 1Yo4pf-0007HG-Pc for 18108@debbugs.gnu.org; Fri, 01 May 2015 02:53:57 -0400 Original-Received: from smtp1.mailbox.org (smtp1.mailbox.org [80.241.60.240]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx2.mailbox.org (Postfix) with ESMTPS id 7E75341F19; Fri, 1 May 2015 08:53:49 +0200 (CEST) X-Virus-Scanned: amavisd-new at heinlein-support.de Original-Received: from smtp1.mailbox.org ([80.241.60.240]) by gerste.heinlein-support.de (gerste.heinlein-support.de [91.198.250.173]) (amavisd-new, port 10030) with ESMTP id YtUyHFycdkOr; Fri, 1 May 2015 08:53:48 +0200 (CEST) In-Reply-To: User-Agent: Notmuch/0.19 (http://notmuchmail.org) Emacs/25.0.50.1 (x86_64-unknown-linux-gnu) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.15 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x X-Received-From: 140.186.70.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Xref: news.gmane.org gmane.emacs.bugs:102315 Archived-At: 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 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 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 + + * 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 * 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.