Merged
Conversation
…p causes FNEST to get popped when it shouldnt be
Contributor
Reviewer's GuideThis PR enhances the return-trap handling by tracking BASH_SOURCE depth and per-frame source flags to prevent incorrect frame popping on source-triggered returns, refactors related conditional branches, declares and initializes new state variables, applies minor formatting cleanups, and updates the test script with a new deepseek test path. Class diagram for updated frame and source tracking variablesclassDiagram
class timep {
+timep_FNEST : array
+timep_NEXEC_A : array
+timep_NEXEC_HASH_A : array
+timep_BASH_COMMAND_PREV : array
+timep_NPIPE : array
+timep_STARTTIME : array
+timep_LINENO : array
+timep_LINENO_OFFSET : array
+timep_NUM_SOURCE : array [NEW]
+timep_IS_SOURCE_FLAG : array [NEW]
+timep_BASH_SOURCE_N_PREV : int [NEW]
}
timep_FNEST_CUR : int
timep_NEXEC_CUR : int
timep_NEXEC_HASH_CUR : int
timep_FUNCNAME_STR : string
timep_NEXEC_0 : string
timep_BASH_SOURCE_N_PREV : int
timep_NUM_SOURCE : array
timep_IS_SOURCE_FLAG : array
timep "1" -- "*" timep_NUM_SOURCE : tracks per-frame source depth
timep "1" -- "*" timep_IS_SOURCE_FLAG : tracks per-frame source-triggered return
timep "1" -- "1" timep_BASH_SOURCE_N_PREV : stores previous BASH_SOURCE depth
Flow diagram for enhanced return-trap frame popping logicflowchart TD
A["RETURN trap triggered"] --> B["Check FUNCNAME stack length"]
B --> C["Check BASH_SOURCE depth vs timep_NUM_SOURCE[timep_FNEST_CUR]"]
C -->|BASH_SOURCE depth < timep_NUM_SOURCE| D["Pop frame and unset variables"]
C -->|BASH_SOURCE depth == timep_NUM_SOURCE & !timep_IS_SOURCE_FLAG| D
C -->|BASH_SOURCE depth == timep_NUM_SOURCE & timep_IS_SOURCE_FLAG| E["Set timep_IS_SOURCE_FLAG[timep_FNEST_CUR]=false"]
D --> F["Update FUNCNAME_STR, NEXEC_0, FNEST_CUR, NEXEC_CUR, NEXEC_HASH_CUR"]
E --> F
F --> G["Continue trap handling"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Add inline comments around the new source‐depth tracking logic in _timep_getFuncSrc to explain the popping vs. source‐trap conditions for future maintainers.
- Organize the new global arrays (timep_NUM_SOURCE and timep_IS_SOURCE_FLAG) near related variables in the declaration block to improve readability and consistent grouping.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Add inline comments around the new source‐depth tracking logic in _timep_getFuncSrc to explain the popping vs. source‐trap conditions for future maintainers.
- Organize the new global arrays (timep_NUM_SOURCE and timep_IS_SOURCE_FLAG) near related variables in the declaration block to improve readability and consistent grouping.
## Individual Comments
### Comment 1
<location> `timep.bash:467` </location>
<code_context>
+ timep_NEXEC_CUR="${timep_NEXEC_A[-1]}"
+ timep_NEXEC_HASH_CUR="${timep_NEXEC_HASH_A[-1]}"
+ elif (( ${#BASH_SOURCE[@]} == timep_NUM_SOURCE[${timep_FNEST_CUR}] )) && ${timep_IS_SOURCE_FLAG[${timep_FNEST_CUR}]}; then
+ timep_IS_SOURCE_FLAG[${timep_FNEST_CUR}]=false
+ fi
}
</code_context>
<issue_to_address>
**suggestion:** Directly assigning 'false' to an array element may not be portable across all Bash versions.
Some Bash versions treat array elements as strings, so 'false' may not work as a boolean. Use '0'/'1' or string checks for consistency.
Suggested implementation:
```shell
elif (( ${#BASH_SOURCE[@]} == timep_NUM_SOURCE[${timep_FNEST_CUR}] )) && [[ "${timep_IS_SOURCE_FLAG[${timep_FNEST_CUR}]}" == "1" ]]; then
timep_IS_SOURCE_FLAG[${timep_FNEST_CUR}]="0"
```
You should also update any other code that checks or sets `timep_IS_SOURCE_FLAG[...]` to use `"1"` for true and `"0"` for false, and use string comparison (`== "1"` or `== "0"`) instead of relying on Bash's boolean evaluation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| timep_NEXEC_CUR="${timep_NEXEC_A[-1]}" | ||
| timep_NEXEC_HASH_CUR="${timep_NEXEC_HASH_A[-1]}" | ||
| elif (( ${#BASH_SOURCE[@]} == timep_NUM_SOURCE[${timep_FNEST_CUR}] )) && ${timep_IS_SOURCE_FLAG[${timep_FNEST_CUR}]}; then | ||
| timep_IS_SOURCE_FLAG[${timep_FNEST_CUR}]=false |
Contributor
There was a problem hiding this comment.
suggestion: Directly assigning 'false' to an array element may not be portable across all Bash versions.
Some Bash versions treat array elements as strings, so 'false' may not work as a boolean. Use '0'/'1' or string checks for consistency.
Suggested implementation:
elif (( ${#BASH_SOURCE[@]} == timep_NUM_SOURCE[${timep_FNEST_CUR}] )) && [[ "${timep_IS_SOURCE_FLAG[${timep_FNEST_CUR}]}" == "1" ]]; then
timep_IS_SOURCE_FLAG[${timep_FNEST_CUR}]="0"
You should also update any other code that checks or sets timep_IS_SOURCE_FLAG[...] to use "1" for true and "0" for false, and use string comparison (== "1" or == "0") instead of relying on Bash's boolean evaluation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
initial attempt at fix for issue where aa source-triggered return trap causes FNEST to get popped when it shouldnt be.
Summary by Sourcery
Fix incorrect popping of timep call stack frames when RETURN traps are triggered by sourced scripts by introducing source-depth tracking and refining trap logic.
Bug Fixes:
Enhancements:
Tests:
Chores: