Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
p3r7 opened this issue Jan 26, 2020 · 17 comments
Open

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

p3r7 opened this issue Jan 26, 2020 · 17 comments

Comments

@p3r7
Copy link

p3r7 commented Jan 26, 2020

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
Copy link
Owner

riscy commented Jan 26, 2020

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
Copy link
Owner

riscy commented Jan 26, 2020

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:

  • At some point I had it so that :ssh by itself returned the user to a local shell, but that almost feels like a hidden easter egg.
  • We could bring up a y-or-n-p prompt that would ask: "Reconnect shell to remote host?"

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

@p3r7
Copy link
Author

p3r7 commented Jan 27, 2020

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
Copy link
Owner

riscy commented Jan 29, 2020

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

@p3r7
Copy link
Author

p3r7 commented Jan 29, 2020

Great 🙌

@p3r7
Copy link
Author

p3r7 commented Feb 5, 2020

I've been busy lately.

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

@riscy
Copy link
Owner

riscy commented Feb 7, 2020

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

@p3r7
Copy link
Author

p3r7 commented Feb 10, 2020

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
Copy link
Owner

riscy commented Feb 17, 2020

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
Copy link
Author

p3r7 commented Feb 17, 2020

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
Copy link
Author

p3r7 commented Apr 5, 2020

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
Copy link
Owner

riscy commented Apr 5, 2020

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
Copy link
Author

p3r7 commented Apr 6, 2020

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.

riscy added a commit that referenced this issue Apr 7, 2020
@p3r7
Copy link
Author

p3r7 commented Apr 8, 2020

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.

🍻

riscy added a commit that referenced this issue Apr 9, 2020
@p3r7
Copy link
Author

p3r7 commented Apr 9, 2020

Looks fine.

Thanks for all your work!

@p3r7 p3r7 closed this as completed Apr 9, 2020
riscy added a commit that referenced this issue Apr 10, 2020
@p3r7
Copy link
Author

p3r7 commented Apr 20, 2020

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.

@p3r7 p3r7 reopened this Apr 20, 2020
@riscy
Copy link
Owner

riscy commented Apr 25, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants