Skip to content

fix: quote boolean command names such as if "$boolean_variable"; then #914

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

fix: quote boolean command names such as if "$boolean_variable"; then #914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
eb5630c
fix: quote other boolean variables for custom IFS
akinomyoga Feb 26, 2023
08465a6
refactor(_known_hosts_real): turn `ipv{4,6}` into boolean variables
akinomyoga Feb 26, 2023
9d00d46
refactor(tar): turn `basic_tar` into a boolean variable
akinomyoga Feb 26, 2023
a7ef1bf
refactor(make,mutt,strace): use `true` for boolean table
akinomyoga Feb 26, 2023
a1db873
refactor(upgradepkg): turn `nofiles` into a boolean variable
akinomyoga Feb 26, 2023
700dd97
refactor(mcrypt,postcat,svcadm,tar,wodim): switch to boolean variables
akinomyoga Feb 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions bash_completion
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,7 +868,7 @@ _comp_delimited()
fi
[[ $cur == *$delimiter* ]] && prefix=${cur%"$delimiter"*}$delimiter

if $deduplicate; then
if "$deduplicate"; then
Copy link
Owner

@scop scop Apr 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ouch. I had written this last week, but forgot to click "Review changes", so the comment was left in pending state. Sorry! Anyway, here goes.

I can't say I'm too enthusiastic about this at least in cases where we are in complete control of the variable's contents.

I can see how this would be necessary to make things work in some crazy $IFS cases, but I believe the codebase is riddled with things that would break with them (this PR's just scratching the surface), and I'm not convinced we should try and support everything one might want to throw at it. I suppose there was a PR somewhere (which I've likely failed to follow up with) that would ensure a desired environment at start of a completion -- that would sound much preferable over this approach to me.

To me, reading if "$something"; then raises the question "was this supposed to be if [[ "$something" ]]; then" (even though the quotes within the square brackets are superfluous) and thus makes the code heavier to read. Also I'm quite convinced that it will take me a long time until I learn to take this into account, and there's no tool to fix it up or warn me. And even if I did learn it, I can't say I'd like it.

I'm leaning towards that we either don't do anything about this at all, or switch to comparing variable values instead of invoking boolean commands they contain, similarly as we did e.g. for the _known_hosts_real $ipv4 and $ipv6 cases below before this change. I think I'd prefer the string set might as a "set" marker better than 1 or true to clearly differentiate from the boolean commands and arithmetic context, but that's a bit besides the point, but in case we're modifying these anyway...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I suppose there was a PR somewhere (which I've likely failed to follow up with) that would ensure a desired environment at start of a completion -- that would sound much preferable over this approach to me.

PR #739 and Issue #786 are related. PR #739 contains unrelated long discussions, but the current situation is summarized in the following two comments:

To me, reading if "$something"; then raises the question "was this supposed to be if [[ "$something" ]]; then" (even though the quotes within the square brackets are superfluous) and thus makes the code heavier to read.

I actually prefer [[ $something ]] with something being an empty/non-empty variable. The reason that I decided to switch to this direction was item 2 in the review comment #891 (review). Might I have misread the comment? Anyway, if you would like to switch to [[ $something ]], I will adjust them.

Copy link
Owner

Choose a reason for hiding this comment

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

I actually prefer [[ $something ]] with something being an empty/non-empty variable. The reason that I decided to switch to this direction was item 2 in the review comment #891 (review). Might I have misread the comment?

You've got it right. My opinion is still that if $something; then reads cleaner than if [[ $something ]]; then. But per reasoning here, we can't have the former if we want to protect against funky $IFS. And I think if "$something"; then with the quotes is so weird that I prefer if [[ $something ]]; then over that.

Anyway, if you would like to switch to [[ $something ]], I will adjust them.

Yes, please. I'd also suggest using literal set as the value when the variable is set, instead of true, 1 or the like.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, please. I'd also suggest using literal set as the value when the variable is set, instead of true, 1 or the like.

I reworked them so decided to submit them as another PR #931.

# We could construct a -X pattern to feed to compgen, but that'd
# conflict with possibly already set -X in $@, as well as have
# glob char escaping issues to deal with. Do removals by hand instead.
Expand Down Expand Up @@ -1930,7 +1930,7 @@ _included_ssh_config_files()
_known_hosts_real()
{
local configfile="" flag prefix=""
local cur suffix="" aliases="" i host ipv4="" ipv6=""
local cur suffix="" aliases="" i host ipv4=false ipv6=false
local -a kh tmpkh=() khd=() config=()

# TODO remove trailing %foo from entries
Expand All @@ -1948,8 +1948,8 @@ _known_hosts_real()
configfile=$OPTARG
;;
p) prefix=$OPTARG ;;
4) ipv4=1 ;;
6) ipv6=1 ;;
4) ipv4=true ;;
6) ipv6=true ;;
*)
echo "bash_completion: $FUNCNAME: usage error" >&2
return 1
Expand Down Expand Up @@ -2122,13 +2122,13 @@ _known_hosts_real()
$reset

if ((${#COMPREPLY[@]})); then
if [[ $ipv4 ]]; then
if "$ipv4"; then
COMPREPLY=("${COMPREPLY[@]/*:*$suffix/}")
fi
if [[ $ipv6 ]]; then
if "$ipv6"; then
COMPREPLY=("${COMPREPLY[@]/+([0-9]).+([0-9]).+([0-9]).+([0-9])$suffix/}")
fi
if [[ $ipv4 || $ipv6 ]]; then
if "$ipv4" || "$ipv6"; then
for i in "${!COMPREPLY[@]}"; do
[[ ${COMPREPLY[i]} ]] || unset -v 'COMPREPLY[i]'
done
Expand Down
2 changes: 1 addition & 1 deletion completions/export
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ _comp_cmd_export()
return
fi
local suffix=""
if ! $remove && [[ $action != function ]]; then
if ! "$remove" && [[ $action != function ]]; then
suffix="="
compopt -o nospace
fi
Expand Down
2 changes: 1 addition & 1 deletion completions/find
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ _comp_cmd_find()
done

# handle case where first parameter is not a dash option
if ! $exprfound && [[ $cur != [-\(\),\!]* ]]; then
if ! "$exprfound" && [[ $cur != [-\(\),\!]* ]]; then
_filedir -d
return
fi
Expand Down
2 changes: 1 addition & 1 deletion completions/fio
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ _comp_cmd_fio()
local line="" in_values=false
ret=()
for line in "${cmdhelp[@]}"; do
if $in_values; then
if "$in_values"; then
if [[ $line =~ ^[[:space:]]*:[[:space:]]*([^[:space:]]+) ]]; then
ret+=("${BASH_REMATCH[1]}")
else
Expand Down
2 changes: 1 addition & 1 deletion completions/jarsigner
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ _jarsigner()
fi
done

if ! $jar; then
if ! "$jar"; then
if [[ $cur == -* ]]; then
# Documented as "should not be used": -internalsf, -sectionsonly
COMPREPLY=($(compgen -W '-keystore -storepass -storetype
Expand Down
2 changes: 1 addition & 1 deletion completions/lzip
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ _lzip()
return
fi

if $decompress; then
if "$decompress"; then
_filedir lz
return
fi
Expand Down
2 changes: 1 addition & 1 deletion completions/make
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ _comp_make__truncate_non_unique_paths()
for target in "${COMPREPLY[@]}"; do
local path=${target%/}
while [[ ! ${paths[$path]+set} ]] &&
paths[$path]=1 &&
paths[$path]=true &&
[[ $path == "$prefix"*/* ]]; do
path=${path%/*}
nchild[$path]=$((${nchild[$path]-0} + 1))
Expand Down
6 changes: 3 additions & 3 deletions completions/mcrypt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,15 @@ _mcrypt()
elif [[ ${words[0]} == mdecrypt ]]; then
_filedir nc
else
local i decrypt=0
local i decrypt=false
for ((i = 1; i < ${#words[@]} - 1; i++)); do
if [[ ${words[i]} == -@(d|-decrypt) ]]; then
_filedir nc
decrypt=1
decrypt=true
break
fi
done
if ((decrypt == 0)); then
if ! "$decrypt"; then
_filedir
fi
fi
Expand Down
4 changes: 2 additions & 2 deletions completions/mutt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ _comp_cmd_mutt__get_conffiles()
# @var[ref] visited Dictionary of config files already visited
_comp_cmd_mutt__get_conffiles__visit()
{
[[ -f $1 && ${visited[$1]-} != yes ]] || return 0
visited[$1]=yes
[[ -f $1 && ${visited[$1]-} != true ]] || return 0
visited[$1]=true
conffiles+=("$1")

local -a newconffiles=($(command sed -n 's|^source[[:space:]]\{1,\}\([^[:space:]]\{1,\}\).*$|\1|p' "$1"))
Expand Down
4 changes: 2 additions & 2 deletions completions/pack200
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ _pack200()
esac
done

if ! $pack; then
if ! "$pack"; then
if [[ $cur == -* ]]; then
COMPREPLY=($(compgen -W '--no-gzip --gzip --strip-debug
--no-keep-file-order --segment-limit= --effort= --deflate-hint=
Expand All @@ -65,7 +65,7 @@ _pack200()
else
_filedir 'pack?(.gz)'
fi
elif ! $jar; then
elif ! "$jar"; then
_filedir jar
fi
} &&
Expand Down
6 changes: 3 additions & 3 deletions completions/postcat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ _postcat()
return
fi

local idx qfile=0
local idx qfile=false
for idx in "${words[@]}"; do
[[ $idx == -q ]] && qfile=1 && break
[[ $idx == -q ]] && qfile=true && break
done
if ((qfile == 1)); then
if "$qfile"; then
local len=${#cur} pval
for pval in $(mailq 2>/dev/null |
command sed -e '1d; $d; /^[^0-9A-Z]/d; /^$/d; s/[* !].*$//'); do
Expand Down
2 changes: 1 addition & 1 deletion completions/pytest
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,7 @@ _pytest()
elif [[ $line =~ ^class[[:space:]] ]]; then
in_class=false
fi
if $in_class && [[ $line =~ ^[[:space:]]+(async[[:space:]]+)?def[[:space:]]+(test_[A-Za-z0-9_]+) ]]; then
if "$in_class" && [[ $line =~ ^[[:space:]]+(async[[:space:]]+)?def[[:space:]]+(test_[A-Za-z0-9_]+) ]]; then
COMPREPLY+=(${BASH_REMATCH[2]})
fi
done 2>/dev/null <"$file"
Expand Down
2 changes: 1 addition & 1 deletion completions/ssh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ _comp_xfunc_ssh_scp_local_files()
local files
_comp_expand_glob files '"$cur"*'
((${#files[@]})) || return 0
if $dirsonly; then
if "$dirsonly"; then
COMPREPLY+=($(command ls -aF1dL "${files[@]}" 2>/dev/null |
command sed -e "s/$_comp_cmd_scp__path_esc/\\\\&/g" -e '/[^\/]$/d' \
-e "s/^/${1-}/"))
Expand Down
4 changes: 2 additions & 2 deletions completions/strace
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ _strace()
while read -r define syscall rest; do
[[ $define == "#define" &&
$syscall =~ ^__NR_(.+) ]] &&
syscalls[${BASH_REMATCH[1]}]=1
syscalls[${BASH_REMATCH[1]}]=true
done 2>/dev/null </usr/include/asm/unistd.h
if ((${#syscalls[@]} == 0)); then
local unistd arch=$(command uname -m)
Expand All @@ -52,7 +52,7 @@ _strace()
while read -r define syscall rest; do
[[ $define == "#define" &&
$syscall =~ ^__NR_(.+) ]] &&
syscalls[${BASH_REMATCH[1]}]=1
syscalls[${BASH_REMATCH[1]}]=true
done 2>/dev/null <$unistd
fi

Expand Down
6 changes: 3 additions & 3 deletions completions/svcadm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ _smf_complete_fmri()
{
local cur="$1" prefix="$2"
local cur_prefix fmri fmri_list=""
local exact_mode="" pattern
local exact_mode=false pattern

if [[ $cur == $prefix* ]]; then
[[ $cur == "$prefix" ]] && cur+="/"
pattern="$cur*"
exact_mode=1
exact_mode=true
else
pattern="$prefix*/$cur*"
fi
Expand All @@ -38,7 +38,7 @@ _smf_complete_fmri()

for fmri in $(svcs -H -o FMRI "$pattern" 2>/dev/null); do
local fmri_part_list fmri_part
if [[ ! $exact_mode ]]; then
if ! "$exact_mode"; then
fmri=${fmri#"$prefix/"}

# we generate all possibles abbreviations for the FMRI
Expand Down
38 changes: 18 additions & 20 deletions completions/tar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,16 +171,16 @@ __tar_parse_old_opt()

# current word is the first word
[[ $cword -eq 1 && $cur && ${cur:0:1} != '-' ]] &&
old_opt_progress=1
old_opt_progress=true

# check that first argument does not begin with "-"
first_word=${words[1]}
[[ $first_word && ${first_word:0:1} != "-" ]] &&
old_opt_used=1
old_opt_used=true

# parse the old option (if present) contents to allow later code expect
# corresponding arguments
if ((old_opt_used == 1)); then
if "$old_opt_used"; then
char=${first_word:0:1}
while [[ $char ]]; do
if __tar_is_argreq "$char"; then
Expand All @@ -195,13 +195,12 @@ __tar_parse_old_opt()
# Make the analysis of whole command line.
__tar_preparse_cmdline()
{
local first_arg i modes="ctxurdA"
local first_arg=true i modes="ctxurdA"

shift # progname

__tar_parse_old_opt

first_arg=1
for i in "$@"; do
case "$i" in
--delete | --test-label)
Expand All @@ -220,14 +219,14 @@ __tar_preparse_cmdline()
;;
*[$modes]*)
# Only the first arg may be "MODE" without leading dash
if ((first_arg == 1)); then
if "$first_arg"; then
tar_mode=${i//[^$modes]/}
tar_mode=${tar_mode:0:1}
tar_mode_arg=$i
fi
;;
esac
first_arg=0
first_arg=false
done
}

Expand Down Expand Up @@ -276,7 +275,7 @@ __tar_complete_mode()
filler i

short_modes="ctx"
[[ ! $basic_tar ]] && short_modes="ctxurdA"
! "$basic_tar" && short_modes="ctxurdA"

# Remove prefix when needed
rawopt=${cur#-}
Expand All @@ -289,7 +288,7 @@ __tar_complete_mode()

# when user passed something like 'tar cf' do not put the '-' before
filler=
if [[ ! $cur && ! $basic_tar ]]; then
if [[ ! $cur ]] && ! "$basic_tar"; then
filler=-
fi

Expand All @@ -306,7 +305,7 @@ __tar_complete_mode()

# The last short option requires argument, like '-cf<TAB>'. Cut the
# completion here to enforce argument processing.
if ((old_opt_progress == 0)) &&
if ! "$old_opt_progress" &&
__tar_is_argreq "$(__tar_last_char "$cur")"; then
COMPREPLY=("$cur") && return 0
fi
Expand Down Expand Up @@ -357,14 +356,14 @@ __tar_try_mode()
case "$cur" in
--*)
# posix tar does not support long opts
[[ $basic_tar ]] && return 0
"$basic_tar" && return 0
__gtar_complete_lopts
return $?
;;

-*)
# posix tar does not support short options
[[ $basic_tar ]] && return 0
"$basic_tar" && return 0

__tar_complete_mode && return 0
;;
Expand All @@ -383,7 +382,7 @@ __tar_adjust_PREV_from_old_option()
# deal with old style arguments here
# $ tar cfTC # expects this sequence of arguments:
# $ tar cfTC ARCHIVE_FILE PATTERNS_FILE CHANGE_DIR
if ((old_opt_used == 1 && cword > 1 && cword < ${#old_opt_parsed[@]} + 2)); then
if "$old_opt_used" && ((cword > 1 && cword < ${#old_opt_parsed[@]} + 2)); then
# make e.g. 'C' option from 'cffCT'
prev="-${old_opt_parsed[cword - 2]}"
fi
Expand Down Expand Up @@ -485,11 +484,11 @@ __tar_detect_ext()

_gtar()
{
local long_opts short_opts basic_tar="" \
local long_opts short_opts basic_tar=false \
long_arg_none="" long_arg_opt="" long_arg_req="" \
short_arg_none="" short_arg_opt="" short_arg_req="" \
tar_mode tar_mode_arg old_opt_progress=0 \
old_opt_used=0 old_opt_parsed=()
tar_mode tar_mode_arg old_opt_progress=false \
old_opt_used=false old_opt_parsed=()

# Main mode, e.g. -x or -c (extract/creation)
local tar_mode=none
Expand Down Expand Up @@ -675,11 +674,11 @@ __tar_posix_prev_handle()

_posix_tar()
{
local long_opts short_opts basic_tar="" \
local long_opts short_opts basic_tar=true \
long_arg_none="" long_arg_opt long_arg_req="" \
short_arg_none short_arg_opt short_arg_req \
tar_mode tar_mode_arg old_opt_progress=0 \
old_opt_used=1 old_opt_parsed=()
tar_mode tar_mode_arg old_opt_progress=false \
old_opt_used=true old_opt_parsed=()

# Main mode, e.g. -x or -c (extract/creation)
local tar_mode=none
Expand All @@ -691,7 +690,6 @@ _posix_tar()

_comp_initialize -s -- "$@" || return

basic_tar=yes
tar_mode=none

# relatively compatible modes are {c,t,x}
Expand Down
2 changes: 1 addition & 1 deletion completions/timeout
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ _timeout()
local noargopts='!(-*|*[ks]*)'
for ((i = 1; i <= COMP_CWORD; i++)); do
if [[ ${COMP_WORDS[i]} != -* && ${COMP_WORDS[i - 1]} != = ]]; then
if $found; then
if "$found"; then
_comp_command_offset $i
return
fi
Expand Down
4 changes: 2 additions & 2 deletions completions/unpack200
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,15 @@ _unpack200()
esac
done

if ! $pack; then
if ! "$pack"; then
if [[ $cur == -* ]]; then
COMPREPLY=($(compgen -W '--deflate-hint= --remove-pack-file
--verbose --quiet --log-file= --help --version' -- "$cur"))
[[ ${COMPREPLY-} == *= ]] && compopt -o nospace
else
_filedir 'pack?(.gz)'
fi
elif ! $jar; then
elif ! "$jar"; then
_filedir jar
fi
} &&
Expand Down
Loading