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

Use "exec" to start the shell #67

Merged
merged 1 commit into from
May 25, 2016
Merged

Use "exec" to start the shell #67

merged 1 commit into from
May 25, 2016

Conversation

magicant
Copy link
Contributor

By using "exec", the shell instance that is running the desk script
is replaced by the new child shell. This will enable the original shell
that invoked desk to take care of the child if the user suspends
the child by typing suspend inside the desk.

By using "exec", the shell instance that is running the `desk` script
is replaced by the new child shell. This will enable the original shell
that invoked `desk` to take care of the child if the user suspends
the child by typing `suspend` inside the desk.
@aratno
Copy link

aratno commented May 25, 2016

I think this should be the default, and a desk exit command should be supplied instead of expecting the newly spawned shell process to be closed. It seems more intuitive to the user, and would prevent desk nesting, which doesn't seem desirable.

@jamesob
Copy link
Owner

jamesob commented May 25, 2016

Thanks for the PR!

When testing locally, this doesn't seem to prevent desk nesting (which I agree can be annoying):

(venv) [Tue 24 22:56] job/code/desk pr-67* 
 $ desk go scruffy 


(venv) [Tue 24 22:56] job/counsyl/scruffy master scruffy
 $ desk go charon 


(venv) [Tue 24 22:57] job/counsyl/charon master* charon
 $ exit


(venv) [Tue 24 22:57] job/counsyl/scruffy master scruffy
 $ exit

I haven't looked into why this might be yet... any ideas?

@magicant
Copy link
Contributor Author

Thanks for testing my PR!

There are 3 shell instances involved here:

  1. The original interactive shell that invoked the desk command
  2. The shell that is running the desk script
  3. The child interactive shell that is invoked by the script to load desk-specific configuration

This PR is to remove shell 2 because it is just unnecessary once shell 3 has been started. Since shell 1 is still alive, you can still exit from shell 3 and go back to shell 1.

Removing shell 1 as well would be a difficult job, as suggested in #36. I don't have a good idea on it.

@jamesob
Copy link
Owner

jamesob commented May 25, 2016

Sounds good to me. This looks like a great change -- thanks for contributing. Let's merge!

@jamesob jamesob merged commit 45dbbb0 into jamesob:master May 25, 2016
@aratno aratno mentioned this pull request May 25, 2016
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

Successfully merging this pull request may close these issues.

3 participants