search

magicant / yash-rs

Reimplementation of yash, an extended POSIX shell
64 stars 3 forks source link

Support errexit option in multi-command pipelines #362

Closed magicant closed 4 months ago

magicant commented 4 months ago

This PR implements the missing support for the errexit option in pipelines.

Note that we don't need a special implementation for single-command pipelines, since the option is already handled in the respective inner-command implementations.

Summary by CodeRabbit

coderabbitai[bot] commented 4 months ago

Walkthrough

The recent updates across yash components focus on refining the handling of the ErrExit shell option. Key changes include the introduction of new functions to better determine when ErrExit applies, removal of outdated code, and enhanced error handling in command execution. These modifications aim to improve script reliability and error management in the yash shell environment.

Changes

Files Change Summary
.../scripted_test/errexit-p.sh Removed a TODO comment about broken behavior.
yash-env/CHANGELOG.md,
yash-semantics/CHANGELOG.md		 Updated to version 0.2.0, introducing changes in handling the ErrExit option and other functionality improvements.
yash-env/src/lib.rs,
yash-env/src/semantics.rs		 Refined logic for ErrExit applicability and removed outdated functions.
yash-semantics/src/.../case.rs,
yash-semantics/src/.../for_loop.rs		 Removed apply_errexit calls, integrated error handling directly in execute functions.
yash-semantics/src/command/pipeline.rs,
yash-semantics/src/handle.rs		 Enhanced error handling logic for ErrExit, including new test cases and refactored execution logic.

Poem

🌟 Ode to the Shell of Yash 🌟

In the land of code, where commands are sown,
A rabbit hopped, through scripts it roamed.
Tweaking ErrExit, with care and might,
No error unseen, in the day or night.
Cheers to the devs, with every commit pushed,
In the world of Yash, where bugs are shushed. 🐇💻

Recent Review Details **Configuration used: CodeRabbit UI**
Commits Files that changed from the base of the PR and between 36620793ded027c476aba1816e2e971a502a0bde and 82b206a802e8154ec99bf405e10bc5c28e3319bf.
Files selected for processing (9) * yash-cli/tests/scripted_test/errexit-p.sh (1 hunks) * yash-env/CHANGELOG.md (1 hunks) * yash-env/src/lib.rs (1 hunks) * yash-env/src/semantics.rs (2 hunks) * yash-semantics/CHANGELOG.md (1 hunks) * yash-semantics/src/command/compound_command/case.rs (2 hunks) * yash-semantics/src/command/compound_command/for_loop.rs (3 hunks) * yash-semantics/src/command/pipeline.rs (4 hunks) * yash-semantics/src/handle.rs (1 hunks)
Additional Context Used
LanguageTool (13)
yash-env/CHANGELOG.md (6)
Near line 3: Possible spelling mistake found. Context: # Changelog All notable changes to `yash-env` will be documented in this file. The ... --- Near line 5: Only proper nouns start with an uppercase character (there are exceptions for headlines). Context: ... in this file. The format is based on [Keep a Changelog](https://keepachangelog.com... --- Near line 20: This sentence does not start with an uppercase letter. Context: ...### Fixed - `RealSystem::open_tmpfile` no longer returns a file descriptor with t... --- Near line 20: Possible spelling mistake found. Context: ...nger returns a file descriptor with the `O_CLOEXEC` flag set. ## [0.1.0] - 2024-04-13 ##... --- Near line 23: Unpaired symbol: ‘[’ seems to be missing Context: ...h the `O_CLOEXEC` flag set. ## [0.1.0] - 2024-04-13 ### Added - Initial impl... --- Near line 27: Possible spelling mistake found. Context: ... Added - Initial implementation of the `yash-env` crate [0.1.0]: https://github.com/mag...
yash-semantics/CHANGELOG.md (7)
Near line 3: Possible spelling mistake found. Context: # Changelog All notable changes to `yash-semantics` will be documented in this file. The ... --- Near line 5: Only proper nouns start with an uppercase character (there are exceptions for headlines). Context: ... in this file. The format is based on [Keep a Changelog](https://keepachangelog.com... --- Near line 12: Possible spelling mistake found. Context: ...nreleased ### Added - Support for the `ErrExit` shell option in multi-command pipeline... --- Near line 16: This sentence does not start with an uppercase letter. Context: ...sion::Error as handle::Handle>::handle` now returns `Divert::Exit` instead of `Di... --- Near line 17: Possible spelling mistake found. Context: ...instead of `Divert::Interrupt` when the `ErrExit` shell option is applicable. ## [0.1.0... --- Near line 19: Unpaired symbol: ‘[’ seems to be missing Context: ...` shell option is applicable. ## [0.1.0] - 2024-04-13 ### Added - Initial impl... --- Near line 23: Possible spelling mistake found. Context: ... Added - Initial implementation of the `yash-semantics` crate [0.1.0]: https://github.com/mag...
ShellCheck (8)
yash-cli/tests/scripted_test/errexit-p.sh (8)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive. --- [warning] 3-3: posix appears unused. Verify use (or export if used externally). --- [warning] 32-32: This redirection doesn't have a command. Move to its command (or use 'true' as no-op). --- [warning] 39-39: This redirection doesn't have a command. Move to its command (or use 'true' as no-op). --- [warning] 44-44: Redirecting to 'echo', a command that doesn't read stdin. Did you want 'cat' instead? --- [warning] 51-51: Redirecting to 'echo', a command that doesn't read stdin. Did you want 'cat' instead? --- [warning] 250-250: This word is constant. Did you forget the $ on a variable? --- [warning] 263-263: This word is constant. Did you forget the $ on a variable?
Learnings (1)
yash-cli/tests/scripted_test/errexit-p.sh (1)
``` User: magicant" PR: magicant/yash-rs#320 File: yash/tests/scripted_test/readonly-p.sh:0-0 Timestamp: 2023-11-23T12:47:34.276Z Learning: The test case for verifying the behavior when attempting to reassign a read-only variable in `yash/tests/scripted_test/readonly-p.sh` has been updated to check for an error or exit status. ```
Additional comments not posted (16)
yash-env/CHANGELOG.md (2)
`12-12`: The addition of `Env::errexit_is_applicable` is well-documented. --- `16-16`: The removal of `semantics::apply_errexit` is clearly noted.
yash-semantics/CHANGELOG.md (2)
`12-12`: The addition of support for the `ErrExit` shell option in multi-command pipelines is well-documented. --- `16-17`: The change in error handling behavior, where `::handle` now returns `Divert::Exit` instead of `Divert::Interrupt` when `ErrExit` is applicable, is clearly documented.
yash-semantics/src/handle.rs (1)
`49-69`: The implementation of conditional error handling based on the `ErrExit` option is correctly done. When `ErrExit` is set, `Divert::Exit(Some(ExitStatus::ERROR))` is returned, otherwise, `Divert::Interrupt(Some(ExitStatus::ERROR))` is returned.
yash-cli/tests/scripted_test/errexit-p.sh (1)
`73-78`: > :memo: **NOTE** > This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-263] The tests in `errexit-p.sh` comprehensively cover the behavior of the `errexit` option in various scenarios, including simple commands, pipelines, and compound commands. Each test is well-structured to clearly differentiate between the expected outcomes when `errexit` is set and when it is not.
yash-env/src/semantics.rs (1)
`22-22`: The change in the import of `ControlFlow` to not include `Break` is noted. Ensure that this change aligns with the usage of `ControlFlow` in the rest of the file.
yash-semantics/src/command/compound_command/case.rs (1)
`58-66`: The modifications to directly handle errors in the `execute` function of the `case` command are correctly implemented. Errors are now handled immediately using the `handle` method of the error, which is consistent with the intended simplification of error handling.
yash-semantics/src/command/compound_command/for_loop.rs (3)
`50-50`: Ensure proper error handling when expanding the loop variable name. --- `56-56`: Ensure consistent error handling for word expansion in loop values. --- `94-94`: Confirm that the error handling for variable assignment is aligned with other similar error handling mechanisms in the codebase.
yash-semantics/src/command/pipeline.rs (3)
`66-66`: Clarify the behavior when a pipeline command returns with `Err(Divert::...)` and ensure it aligns with POSIX standards or document the deviation. Consider adding a comment or documentation to clarify this behavior, especially if it deviates from POSIX standards. --- `115-125`: Review the refactored logic in `execute_commands_in_pipeline` for handling job control and multi-command pipelines. --- `507-519`: Ensure the new test for the `ErrExit` option correctly simulates scenarios where the option should affect the pipeline's behavior.
Verification successful --- The search results confirm that the `ErrExit` option is extensively used across various parts of the codebase, particularly in handling command execution scenarios where an exit might be triggered based on the command's success or failure. The test case in `yash-semantics/src/command/pipeline.rs` specifically sets the `ErrExit` option to `On` and checks the behavior of a pipeline command sequence. The assertions in the test verify that the pipeline execution respects the `ErrExit` setting by exiting with the status of the last command when a non-zero exit status occurs. Given the consistent usage and testing patterns observed in other parts of the codebase, it appears that the implementation and testing of the `ErrExit` option are aligned with the expected behavior described in the shell's semantics. The test case provided in the review comment seems to correctly simulate the scenario where the `ErrExit` option should affect the pipeline's behavior, as it checks both the result of the pipeline execution and the exit status. Therefore, the new test for the `ErrExit` option appears to be correctly implemented and effectively tests the intended behavior.
Scripts Executed The following scripts were executed for the analysis: --- Script: ```shell #!/bin/bash # Confirm that the `ErrExit` option behaves as expected in various pipeline scenarios. ast-grep --lang rust --pattern $'ErrExit, $_ { $$$ }' ``` Length of output: 56 --- --- Script: ```shell #!/bin/bash # Search for any usage of `ErrExit` in the codebase to understand its implementation context. rg "ErrExit" --context 5 ``` Length of output: 30262
yash-env/src/lib.rs (2)
`403-410`: The implementation of `errexit_is_applicable` correctly checks the conditions for the applicability of the `ErrExit` option. Well done! --- `414-419`: The `apply_errexit` function correctly handles the shell exit logic based on the `ErrExit` option and the current exit status. This ensures robust error handling as per the PR's objectives.
--- Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)
Tips ### Chat There are 3 ways to chat with [CodeRabbit](https://coderabbit.ai): - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit testing code for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit testing code for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit testing code.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger a review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - Please see the [configuration documentation](https://docs.coderabbit.ai/guides/configure-coderabbit) for more information. - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json` ### Documentation and Community - Visit our [Documentation](https://coderabbit.ai/docs) for detailed information on how to use CodeRabbit. - Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback. - Follow us on [X/Twitter](https://twitter.com/coderabbitai) for updates and announcements.