riscy / shx-for-emacs

An Emacs shell-mode (and comint-mode) extension that enables displaying small plots and graphics and lets users write shell commands in Emacs Lisp.
GNU General Public License v3.0
218 stars 10 forks source link

Give the choice to respawn dead shell at remote location #16

Open p3r7 opened 4 years ago

p3r7 commented 4 years ago

Hello,

First of all, thanks for this really neat package. I'm using it daily and enjoying it.

I would like to propose a customization toggle for the behavior of respawning dead shell buffers.

I can submit a PR if you want, but would first like to propose the idea.

Current behavior

When the comint process in interactive shell buffer dies, shx makes it come back to life whihc is really handy.

This is handled by the first cond of shx-send-input.

The only problem is that when the shell was remote, it will respawn in a local shell instead.

Expected behavior

I understand that some users might want this behavior by default, but what I would personally expect is to have it respawned at the previous location (i.e. on the remote server).

I work daily with lots of remote server connections and have helper functions for connecting to them and naming the shell buffers according to the remote location.

I guess a var could be defined to let the user choose the behavior (respawn local or keep old location).

In the meantime, I have overridden shx-send-input in my own config to keep my sanity.

riscy commented 4 years ago

Yeah, this change definitely makes sense.

The reason I initially chose to restart in what I called a "safe" directory is because the reconnect blocks Emacs so, if the remote is unreachable, Emacs appears to lock up.

I think a simple solution to the blocking would be to add a message to shx--reconnect to tell the user what's happening and how to stop it (C-g).

riscy commented 4 years ago

I'm playtesting this a little (see the attached commit) and realized #ef6b7ea makes it impossible to return to a local shell once we've :ssh'd out. I feel like that should be addressed. A couple options:

I'm leaning toward the second option, but I thought I'd see what you thought. :)

p3r7 commented 4 years ago

I agree with the y-or-n-p option.

Additionally, I think it would be nice to have a defcustom to allow the user to configure for always yes or always no.

riscy commented 4 years ago

This sounds good to me! Watch for it to drop this weekend. :)

p3r7 commented 4 years ago

Great 🙌

p3r7 commented 4 years ago

I've been busy lately.

I'll attempt to take a look at those 2 commits this evening.

riscy commented 4 years ago

No worries! No rush either. I went with a solution that doesn't have a prompt, but I think it works.

p3r7 commented 4 years ago

I've tested your solution and it works indeed quite smoothly.

I just have a small remark, but it might be out of the scope of shx.

Function shx--validate-shell-file-name attempts to guess which interpreter to use. It first tries value of explicit-shell-file-name.

One idiosyncrasy of shell.el is that when spawning an interactive shell (via command shell) on a remote connection, the user gets prompted for an explicit-shell-file-name but this value does not get stored as a (buffer-local) var.

As such, the value of command for shx--validate-shell-file-name will always default to /bin/sh, which is kinda sad (I would rather be prompted for a path than be retrograded to this old thing).

I personally deal with this annoyance by defvaring a default remote interpreter (var with-shell-interpreter-default-remote):

(defadvice shx--validate-shell-file-name (around shx--validate-shell-file-name-default-remote-interpreter activate)
    "Set `explicit-shell-file-name' to `with-shell-interpreter-default-remote' if exists"
    (let ((remote-id (file-remote-p default-directory))
          (explicit-shell-file-name explicit-shell-file-name))
      (when (and remote-id
                 (file-exists-p (concat remote-id with-shell-interpreter-default-remote)))
        (setq explicit-shell-file-name with-shell-interpreter-default-remote))
      ad-do-it))

But this works for me as I always use the same interpreter for remote connections.

Ideally interactive shell buffers should keep a memory of their interpreter (via a buffer-local var, at least their initial value) so that the shell could get respawned with the exact same one. But this would be quite intrusive (relying on a wrapper around command shell or via an advice).

riscy commented 4 years ago

Ideally interactive shell buffers should keep a memory of their interpreter (via a buffer-local var, at least their initial value) so that the shell could get respawned with the exact same one. But this would be quite intrusive (relying on a wrapper around command shell or via an advice).

It feels like it would be useful to have a customization variable that lets you map hostnames (or regexps covering different hostnames) to preferred interpreters, maybe like:

(defcustom shx-remote-shell-commands
  '((".*.host.com" . "/bin/zsh"))
  "Mapping from remote hosts to (preferred) shell commands."
  :type '(alist :key-type regexp :value-type string))

and then having shx--validate-shell-file-name swap in the right interpreter when connecting/reconnecting.

p3r7 commented 4 years ago

Yes, that would be even better than my solution of having a single default value.

This way one could still have a catch-all entry with ".*".

p3r7 commented 4 years ago

Regarding your idea of defining an alist of host regexp / interpreter couples, I came up with this:

(defcustom shx-remote-shell-commands
  '((".*\.host\.com" . "/bin/zsh")
    (".*" . "/bin/bash"))
  "Mapping from remote hosts to (preferred) shell commands."
  :type '(alist :key-type regexp :value-type string))

(defun shx--keep-first (fn list)
  "Keep the first element from LIST for which FN is non-nil."
  (let (e res)
    (while (not res)
      (setq res (funcall fn (car list))
            list (cdr list)))
    res))

(defun shx--interpreter-for-host (host)
  (shx--keep-first
   (lambda (e)
     (let ((regexp (car e))
           (interp (cdr e)))
       (when
           (string-match regexp host)
         interp)))
   shx-remote-shell-commands))

;; example usage:
(shx--interpreter-for-host "hello.host.com") ; => /bin/zsh
(shx--interpreter-for-host "foobar")              ; => /bin/bash

Please note that the name of function shx--keep-first is inspired by function -keep from dash.el.

I've also modified my wrapper around shell functions to save explicit-shell-file-name as a buffer-local value. It works but I'm not super satisfied with the implementation (super verbose, comint was a PITA, resetting local values).

riscy commented 4 years ago

I was just thinking about this! I usually hesitate to use recursion, but another solution might be something like:

(defun shx--interpreter-for-host (host &optional options)
  (setq options (or options shx-remote-shell-commands))
  (when options
    (let ((regexp (caar options))
          (interp (cdar options)))
      (cond ((string-match regexp host) interp)
            ((cdr options) (shx--interpreter-for-host host (cdr options)))))))
p3r7 commented 4 years ago

Yes, that'd be a tad more compact.

The only drawback is a bigger stack usage (no native tail-call optimization in elisp) but given that shx-remote-shell-commands / options would never exceed a few dozen items that's a non-issue.

p3r7 commented 4 years ago

Other than the minor comment I made regarding the regexp format, everything looks mighty fine.

I guess a mention of the feature in the README would be the last remaining thing for this ticket to be closed.

:beers:

p3r7 commented 4 years ago

Looks fine.

Thanks for all your work!

p3r7 commented 4 years ago

Oh, I just stumbled upon this.

It seems that we have re-implemented something that is standard since Emacs 26.

In section 6.5.2 in the TRAMP manual, it appears that we can configure which shell to default to for remote hosts.

When reading the docstring for connection-local-criteria-alist, it appears that we could even have a wildcard entry (if CRITERIA is nil, it always applies.).

It lacks regexp support, though.

riscy commented 4 years ago

Ooof, good point! I think it is worth documenting this, and then I'll probably just pull it out of shx and bump the version as a breaking change.