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

server/pty: Crash if shell doesn't exist #2020

Closed
wants to merge 2 commits into from

Conversation

har7an
Copy link
Contributor

@har7an har7an commented Dec 13, 2022

Supplement to #2013, this makes sure that when zellij is being spawned but there is not valid shell found, the application crashes with an error message.

@har7an
Copy link
Contributor Author

har7an commented Dec 13, 2022

@imsnif I know you're busy, but this needs attention before the release. Since #2013 reverts the fix implemented #1769, spawning an instance of zellij with a non-existent shell (however it was configured) now makes the application non-responsive again. I think I found the location where the CommandNotFound wasn't handled correctly, but I'm pretty new to this part of the code. Could you give it a quick look (in pty.rs) to make sure I understood this correctly? Essentially my assumption is that the None branch is equivalent to "Just spawn a new shell here please". Is this correct, or are there other cases where this code is being run?

@har7an har7an temporarily deployed to cachix December 13, 2022 12:54 — with GitHub Actions Inactive
@imsnif
Copy link
Member

imsnif commented Feb 20, 2023

Hey @har7an - what's the status here?

@har7an
Copy link
Contributor Author

har7an commented Mar 5, 2023

@imsnif Well I tried to rebase onto main but the portion of code where I applied this doesn't exist where I expect it any longer, and honestly I couldn't find out where it went either. So I'll close this PR since whoever does this will have to start from scratch anyway. :)

@har7an har7an closed this Mar 5, 2023
@har7an har7an deleted the hotfix/shell-not-found branch July 7, 2024 05:36
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.

None yet

2 participants