-
Notifications
You must be signed in to change notification settings - Fork 379
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
refactor: prefix command
to the awk
command
#1072
Merged
Merged
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
scop
approved these changes
Nov 28, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a couple of comments, pre-approved with them addressed.
To avoid caring about the limited feature set of Solaris awk compared to POSIX awk, we always use `_comp_awk` from now on.
In Bash 5.0, a backslash in an unquoted variable expansion $var induces a pathname expansion, and the backslash is removed. In addition, the execution fails when `shopt -s failglob` is set and there are no filenames matching the pattern. The raw variable expansion $var can also be subject to unexpected word splitting with a custom IFS. One should store command words in an array or should `eval` the command stored in a scalar variable. This PR uses an array variable. Note: This Bash behavior was required by the literal interpretation of the POSIX wording. After Bash 5.0 implemented this literal interpretation, a discussion on the POSIX interpretation arose. Finally the POSIX wording was modified to match the behavior of Bash < 5.0. Then, the Bash behavior was reverted in Bash 5.1. For this reason, this behavior of the pathname expansion is only observed in Bash 5.0.
\? and \+ in BRE are GNU extensions.
The POSIX character classes of the form [[:space:]] or [[:lower:]] are not supported by mawk < 1.3.3-20090727. For example, this old version of mawk is still shipped with Ubuntu 18.04 LTS, which offers extended support until 2028-04.
\s and \S are GNU extensions. We need to replace them with [[:space:]] and [^[:space:]], respectively, in sed. Some awk implementations do not support the POSIX character classes of the form [[:space:]], so \s in awk needs to be replaced with the literal space and tab, $'[ \t]'.
Flag i for the case-insensitive matching is a GNU extension and thus should be avoided. The mock completion contains s/.../.../i while the rpm completion also contains a similar code but without flag i. The related sed code in the rpm completion was introduced in commit 14c9f5b (2009-05-23), and that in the mock completion was introduced in commit 508a88e (2009-05-31). The former did not have flag i while the latter had flag i from the beginning. In this patch, we assume the case-insensitive flag was intended for both cases and update the regular expression so that it matches in a case-insensitive manner also in the rpm completion.
We have been using the connected form of the `-F` option to the `awk` command because Solaris awk does not support the separate form of the option argument `-F <char>`. Nevertheless, there are some cases of the separate form in the codespace, though the majority is in the connected form. Since we now switch to `_comp_awk` for POSIX awk, we unify them to the separate form `-F <char>`.
bae5ee8
to
9ec2e9e
Compare
As mentioned in #1072 (comment), there is a new commit 9ec2e9e. |
Thanks! |
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.
As discussed in #1069 (comment).
In this PR, I change
awk
tocommand awk
where Solaris awk works (65ea699).However, another possibility is that we entirely switch to
_comp_awk
so that we do not have to care about the feature set of Solaris awk. If you like this direction, I can adjust the PR.There are additional changes:
sed
without prefixingcommand
, which were recently added. I also fix thesesed
cases in this PR (2e38286).awk
(b207dfc, 7b975fc, a1c5c30) andsed
(0d798df, 17e6d83, 7bea087).shfmt
andshellcheck
totest/runLint
similarly to test(pre-commit): shfmt+shellcheck test/fallback/update-fallback-links #1071 fortest/fallback/update-fallback-links
(bae5ee8).