fix: work around shopt -s patsub_replacement newly supported in Bash 5.2
#1074
Conversation
This file contains 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
akinomyoga
force-pushed
the
patsub_replacement
branch
from
c188381 to
a449cd8
Compare
akinomyoga
changed the title
shopt -s patsub_replacement newly supported in Bash 5.2shopt -s patsub_replacement newly supported in Bash 5.2
akinomyoga
force-pushed
the
patsub_replacement
branch
2 times, most recently
from
e7401f2 to
bc2bef0
Compare
scop
approved these changes
Dec 24, 2023
test/runLint
Outdated
| @@ -61,3 +64,9 @@ gitgrep "$cmdstart"'unset [^-]' 'Explicitly specify "unset -v/-f"' | |||
|
|
|||
| gitgrep "$cmdstart"'((set|shopt)\s+[+-][a-z]+\s+posix\b|(local\s+)?POSIXLY_CORRECT\b)' \ | |||
| 'fiddling with posix mode breaks keybindings with some bash versions' | |||
|
|
|||
| gitgrep '\$\{([^{}\n]|\{.*\})+/([^{}\n]|\{.*\})+/([^{}"\n]|\{.*\})*\$.*\}' \ | |||
| '$rep of ${var/pat/$rep} needs to be double-quoted for shopt -s patsub_replacement (bash >= 5.2)' | |||
There was a problem hiding this comment.
The commit message of 5b959a7 details a case why we should be rather using _comp_compgen than double quoting. Should we update this to suggest that, too?
Looks good to me otherwise, pre-approved and good to merge with this addressed (or not addressed if that's the right thing to do).
There was a problem hiding this comment.
This becomes long, so in commit db5015a, I added a new subsection in doc/styleguide.md under the quoting section and added pointers to the related lint items.
akinomyoga
force-pushed
the
patsub_replacement
branch
3 times, most recently
from
c35af29 to
db5015a
Compare
The construct "${cur/^!/$spoolfile}" appears to try to replace the
beginning `!` with "$spoolfile" because cur starts with `!` in this
context. However, this does not work because ^ does not mean the
beginning of the string. We may instead use "${cur/#!/$spoolfile}".
In addition, `eval` can be applied to only the content of `spoolfile`, which was originally extracted from the mutt output of the form `spoolfile="..."`. After expanding the escape sequences in $spoolfile, we combine it with `cur`.
The raw assignment can be affected by word splitting and pathname expansions.
The construct cur="${cur/#!/$spoolfile}" tries to replace the first
character of `cur` with the content of $spoolfile. However, this has
a problem that the characters `&` in $spoolfile can be unexpectedly
replaced with the matched string when `shopt -s patsub_replacement`
(bash >= 5.2) is enabled.
In principle, we can rewrite it to `cur=${cur/#!/"$spoolfile"}`, but
we can use a simpler solution this time. In this context, `cur` is
ensured to start with `!`, so we can directly replace
${cur/#!/$spoolfile} with $spoolfile${cur:1}.
To add a prefix to every element in an array, we have used the
following construct:
arr=("${arr[@]/#/$prefix}")
However, this can be broken when `shopt -s patsub_replacement` (bash
>= 5.2) is turned on. In bash >= 5.2, patsub_replacement is enabled
by default. In particular, the characters `&` contained in $prefix,
if any, will be replaced with the matched string. To avoid the
unexpected patsub_replacement, one may think about quoting the
replacement as
arr=("${arr[@]/#/"$prefix"}")
However, this has another problem in bash < 4.3 or when `shopt -s
compat42` is turned on. In such a case, the inner double quotations
are treated literally so that the `PREFIX` instead of ``"PREFIX"` is
prefixed to elements. To avoid this situation the outer double
quotations might be removed, but this has even another problem of the
pathname expansions and IFS. We here instead use _comp_compgen:
_comp_compgen -Rv arr -- -P "$prefix" -W '"${arr[@]}"'
This patch also contains similar cases for a suffix.
In bash >= 4.3 with `shopt -s compat42` enabled, ${var/pat/$'...'}
would produce extra single quotations around the replacements. The mr
completion uses $'\n' to insert delimiter, but the delimiter does not
need to be the newline in this context. In this patch, we can instead
insert a space.
akinomyoga
force-pushed
the
patsub_replacement
branch
2 times, most recently
from
32f0088 to
2a11209
Compare
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The main fixes are the last three commits: caeedee, cfdcc93, and c188381. See the commit messages.
The other commits are relatively trivial fixes overlapping with the above ones: 87c535a, 59ad04d, 85203cb, a15f630, 21d28ad.