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

more: Disable raw mode before exiting if a panic occurs #5914

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

Ideflop
Copy link
Contributor

@Ideflop Ideflop commented Jan 29, 2024

Currently, if a panic occurs, the raw mode is still active, requiring the terminal to be closed and reopened to use it again.

To test the fix, execute the following command without the new panic hook:

./target/debug/coreutils more -n 2 cargo-test.md cargo-test.md

The terminal should be unusable after running this command.

Then, execute the command with the new panic hook:

./target/debug/coreutils more -n 2 cargo-test.md cargo-test.md

The terminal should remain usable after running this command.

@tertsdiepraam
Copy link
Member

I'm not really opposed to this, but I'd rather see the panics fixed instead. I want it to be possible to compile the coreutils with panic="abort" so I really want to discourage anything that relies on panics.

@Ideflop
Copy link
Contributor Author

Ideflop commented Jan 30, 2024

If I understand correctly, if I remove the panic inside the hook then when the code panics, it will first execute the set_hook and then execute the panic="abort" as specified in the Cargo.toml?

@tertsdiepraam
Copy link
Member

You're right, I was confused 😅

@Ideflop
Copy link
Contributor Author

Ideflop commented Jan 30, 2024

I have made some small changes to the code. First, I have removed the panic. Secondly, I put a print !("\r") which will place the cursor at the beginning of the line and then I print the panic information.

Copy link

GNU testsuite comparison:

Skip an intermittent issue tests/rm/rm1 (fails in this run but passes in the 'main' branch)
GNU test failed: tests/timeout/timeout. tests/timeout/timeout is passing on 'main'. Maybe you have to rebase?

@cakebaker cakebaker merged commit d530771 into uutils:main Feb 1, 2024
58 of 61 checks passed
@cakebaker
Copy link
Contributor

cakebaker commented Feb 1, 2024

Thanks :)

Btw: You can use stty sane instead of closing/reopening the terminal.

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