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

feat: implement $SHLVL and $BASH_SUBSHELL #251

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

39555
Copy link
Contributor

@39555 39555 commented Oct 27, 2024

Implementation of $SHLVL and $BASH_SUBSHELL variables. https://www.gnu.org/software/bash/manual/html_node/Bash-Variables.html.

SHLVL

Incremented by one each time a new instance of Bash is started. This is intended to be a count of how deeply your Bash shells are nested.

BASH_SUBSHELL

Incremented by one within each subshell or subshell environment when the shell begins executing in that environment. The initial value is 0. If BASH_SUBSHELL is unset, it loses its special properties, even if it is subsequently reset.

Im not sure how to test SHLVL, because it requires starting a new shell inside the test.

Copy link

github-actions bot commented Oct 27, 2024

Performance Benchmark Report

Benchmark name Baseline (μs) Test/PR (μs) Delta (μs) Delta %
expand_one_string 3.41 μs 3.41 μs 0.00 μs ⚪ Unchanged
instantiate_shell 60.87 μs 60.63 μs -0.24 μs ⚪ Unchanged
instantiate_shell_with_init_scripts 30489.21 μs 30686.59 μs 197.38 μs ⚪ Unchanged
parse_bash_completion 2776.43 μs 2754.00 μs -22.43 μs ⚪ Unchanged
parse_sample_script 4.28 μs 4.11 μs -0.17 μs 🟢 -3.93%
run_echo_builtin_command 90.38 μs 90.88 μs 0.50 μs ⚪ Unchanged
run_one_builtin_command 109.24 μs 110.45 μs 1.21 μs ⚪ Unchanged
run_one_external_command 1939.54 μs 2040.15 μs 100.61 μs 🟠 +5.19%
run_one_external_command_directly 1002.93 μs 1007.75 μs 4.82 μs ⚪ Unchanged

Code Coverage Report: Only Changed Files listed

Package Base Coverage New Coverage Difference
brush-core/src/builtins/test.rs 🟢 86.96% 🟢 91.3% 🟢 4.34%
brush-core/src/extendedtests.rs 🟠 69.83% 🟠 74.24% 🟢 4.41%
brush-core/src/interp.rs 🟢 90.28% 🟢 90.38% 🟢 0.1%
brush-core/src/shell.rs 🟢 82.63% 🟢 83.22% 🟢 0.59%
Overall Coverage 🟢 77.26% 🟢 77.41% 🟢 0.15%

Minimum allowed coverage is 70%, this run produced `77.41%``

@reubeno reubeno added the needs_review PR or issue author is waiting for a review label Oct 28, 2024
@reubeno
Copy link
Owner

reubeno commented Nov 2, 2024

Thanks for working on closing this gap!

One main question for you: what do you think about the idea of lazily computing the value of these variables? I've been toying with the idea of generalizing the way we implement $RANDOM and having the notion of a "dynamically computed" ShellValue. It would let us insert the variable in the environment at initialization time but not spend energy on computing it, stringifying the result, and storing it. When actually needed for an expansion, we can evaluate it via call out to a Fn of some sort, which may look at the Shell first to generate the correct value. For some other variables like $BASHOPTS (tracked by #233) it also avoids us needing to keep updating the variable when its value would change (i.e., whenever options are updated) and reduce it down to just needing a way to poll the correct result at expansion-time.

@reubeno reubeno removed the needs_review PR or issue author is waiting for a review label Nov 3, 2024
@reubeno
Copy link
Owner

reubeno commented Nov 5, 2024

Ok, I went back and tried to pull on the idea I mention above ^^^. I still think it's worth pursuing, but I think it's sufficiently complicated not to rush it.

I'll do a proper review of the changes in this PR so we can get them in.

@@ -1182,3 +1194,21 @@ fn parse_string_impl(
tracing::debug!(target: trace_categories::PARSE, "Parsing string as program...");
parser.parse(true)
}

// parse a numeric variable from a string, increment it by 1, then re-export
fn increment_level_var(
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shell.rs is already, well, longer than I'd like. Could you turn this into a pub(crate) helper in variables.rs or similar?

For what it's worth, it seems more understandable to me to have the default value expressing the pre-increment default (i.e., 0 here).

@@ -23,3 +23,37 @@ cases:
- name: "Piped subshell usage"
stdin: |
(echo hi) | wc -l

- name: "SHLVL subshell"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we need some tests to ensure that SHLVL will work correctly across process invocations (i.e., nested shells and not just subshells). Thoughts on how to do that? Nested invocation of $0? Call through bash in the middle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reubeno I thought about it and created a test using $SHELL but is points to nowhere inside the Oracle. Now, looking at it, I learned about $0, so I will try using that instead. I'm not sure yet how interactivity works there.

$0
$0
$0 -c 'echo $SHLVL; $0'

@reubeno reubeno added the updates_requested Pull requests with updates requested label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
updates_requested Pull requests with updates requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants