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.

Sign up for GitHub

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

Jump to bottom

[FIX] Stop full cmd list if received Ctrl+C #205

Merged
merged 4 commits into from
Mar 18, 2024
Merged

[FIX] Stop full cmd list if received Ctrl+C #205

merged 4 commits into from
Mar 18, 2024

Conversation

itislu
Copy link
Collaborator

@itislu itislu commented Mar 14, 2024 •
edited by LeaYeh
Loading

Test case:

cat | ls && echo 123

Working behavior, but wrong exit code: SOLVED

(cat | ls) && echo 123
  • Wrong exit code: 0 instead of 130 (but this is weird for the user, bc cat | ls exits with 0)
echo 123 && cat | ls
(cat | ls) && echo 123

@itislu itislu added the bug Something isn't working label Mar 14, 2024
@itislu itislu added this to the Signal Handling milestone Mar 14, 2024
@itislu itislu linked an issue Mar 14, 2024 that may be closed by this pull request
[BUG] cat | ls && echo 123 should not print 123 after Ctrl+C #190
Closed
@itislu
Copy link
Collaborator Author

itislu commented Mar 16, 2024 •
edited
Loading

I'm okay with echo 123 && cat | ls having a different exit code when Ctrl+C.
I think it is more consistent that always the exit code of the last executed command counts, as long as no other command list follows.
What this means for the user is that every command has been started successfully.
If an || operator would follow after that and yet it does not execute bc of Ctrl+C being a complete stop of the execution, it then makes sense that the exit code is not 0 but 130.
We handle that.

@itislu
Copy link
Collaborator Author

itislu commented Mar 16, 2024

I'm okay with merging it.

itislu added 4 commits March 18, 2024 10:37
@itislu
style: Rename SIG_STD to SIG_STANDARD
7802e3f 
This is more consistent with the other signal enums.
@itislu
style: Change order of signal enums
62b6b4a 
This aligns them with the values of `SIGDFL` and `SIGIGN`.
@itislu
fix: Stop full cmd list if SIGINT
38f5652 
Test case:
`ls | cat && echo 123`

Working, but wrong exit code:
`(ls | cat) && echo 123`
@itislu
fix: Correct exit code when (cat | ls)
ebd6fa2 
Every exit code seems to be correct, except this:
`echo 123 && cat | ls`
@LeaYeh LeaYeh merged commit 7ea0ca1 into main Mar 18, 2024
29 checks passed
@LeaYeh LeaYeh deleted the fix-issue-190 branch March 19, 2024 16:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LeaYeh LeaYeh LeaYeh approved these changes

Assignees

@LeaYeh LeaYeh

Labels
bug Something isn't working
Projects
minishell (🌊rash)
Status: Done
Milestone
Signal Handling
Development

Successfully merging this pull request may close these issues.

[BUG] cat | ls && echo 123 should not print 123 after Ctrl+C
2 participants
@itislu @LeaYeh