Conversation
lu-zero
force-pushed
the
improve-IFS-support
branch
from
e6f6fe9 to
899cb67
Compare
Performance Benchmark Report
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is Test Summary: bash-completion test suite
|
There was a problem hiding this comment.
Pull request overview
This PR improves IFS (Internal Field Separator) support in brush, specifically fixing the handling of empty vs. unset IFS values. This is critical for bash compatibility, as empty IFS should disable field splitting while unset IFS should use the default " \t\n" splitting behavior. The fix was discovered while using brush to parse the portage tree.
Changes:
- Fixed environment variable retrieval to distinguish between empty and unset IFS values
- Refactored brace expansion to return a vector of words instead of joining them with spaces, preserving field boundaries
- Added 20 comprehensive test cases covering empty/unset IFS behavior across various expansion types
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| brush-shell/tests/cases/compat/ifs.yaml | Added 20 test cases covering empty/unset IFS with variable expansion, brace expansion, command substitution, arithmetic expansion, and mixed cases |
| brush-core/src/expansion.rs | Refactored brace_expand_if_needed to brace_expand_to_words returning Vec instead of joined string; updated basic_expand to process each brace-expanded word separately; updated unit tests |
| brush-core/src/env.rs | Changed get_str from map(to_cow_str) to and_then(try_get_cow_str) to distinguish empty string from unset values |
reubeno
left a comment
reubeno
left a comment
There was a problem hiding this comment.
A few comments... but apart from them I think I understand what this is about and why it would fix some cases. Could you update the PR description or commits with a brief summary of what the actual issue(s) were? I want to make sure I really understand, and it will also be useful for composing the merge commit. (Your original commit for the env fix had a great description. The other commit was a bit more sparse in its details.)
brush-core/src/expansion.rs
Outdated
| for piece in brush_parser::word::parse(brace_expanded.as_ref(), &self.parser_options)? { | ||
| let piece_expansion = self.expand_word_piece(piece.piece).await?; | ||
| expansions.push(piece_expansion); | ||
| // When there's no actual brace expansion (single word), preserve all expansion |
There was a problem hiding this comment.
The comment says "when there's no actual brace expansion" but it and the code below only checks for how many words came back. {1..1} is a concrete example where brace expansion does happen but only a single word is returned (i.e., 1).
Fixes IFS handling when unset in local scope and potentially makes the HOME fallback more correct. The get_str method was incorrectly converting unset variables to empty strings instead of returning None. This caused issues with IFS handling where unset IFS variables should fall back to default behavior but were instead treated as empty IFS.
… tracking Add test exercising the corner cases.
lu-zero
force-pushed
the
improve-IFS-support
branch
from
899cb67 to
d572e33
Compare
I tried to use brush(winnow) to parse the portage tree and this is one of the issue found and solved.