Support errexit option in multi-command pipelines

[magicant]

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

• New Features

  • Enhanced support for the ErrExit shell option across multiple command pipelines.
  • Introduced refined error handling logic for ErrExit in shell scripting.

• Refactor

  • Updated and streamlined error handling processes in various scripting components.
  • Removed outdated functions and replaced them with direct error handling.

• Documentation

  • Updated CHANGELOG for yash-env and yash-semantics to reflect new features and changes.

• Bug Fixes

  • Fixed incorrect inversion of exit status in command pipelines when ErrExit is set.

[coderabbitai]

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 3662079 and 82b206a.

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>::handlenow returnsDivert::Exit instead ofDi...

---

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 <expansion::Error as handle::Handle>::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: > 📝 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:

#!/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:

#!/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
• Mastodon
• Reddit
• LinkedIn

Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>.
  • 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 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 for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.