koalaman
Sometimes too overzealous in regards to unquoted variables
I have the code:
# Unset all FSTAB_* variables
unset $(env | grep ^FSTAB_ | sed 's,=.*,,')
Here, I want word splitting, but shellcheck is complaining (giving me a warning) about this.
Also the following code is on purpose:
#!/bin/sh
my_function() {
local CMD="$1"; shift
local ARGS="$*"
"$CMD" $ARGS
}
ARGS="-C -F -l -a"
my_function /bin/ls "$ARGS"
Gives /tmp/tst.sh:7:12: note: Double quote to prevent globbing and word splitting. [SC2086].
However, if I put double quotes around $ARGS in the function (to silence shellcheck:
# /tmp/tst.sh
/bin/ls: invalid option -- ' '
Try '/bin/ls --help' for more information.
This time only note, but it's still wrong. This because it interprets the -C -F -l -a part as ONE command, not FOUR.
Putting command arguments in a string is an anti-pattern. It fails to work in many cases. You don't want to do it. Use an array. (See http://mywiki.wooledge.org/BashFAQ/050 for discussion of this topic.)
A simpler (and safer) way to do what you want in the first snippet (assuming bash and that you want local as well as exported variables unset) would be to use unset -v "${!FSTAB_*}". Which uses a bash parameter expansion for "Expands to the names of variables whose names begin with prefix".
Hi Turbo! =) When you have reviewed the warnings and debated if you want to change some of them to other solutions that might be better for one reason or other you can slap a directive to hide the error in the future. See https://github.com/koalaman/shellcheck/wiki/Directive.
Yes, I saw that. But that, on the other hand, is suppressing ALL message about this in the whole file. And many of them are valid. Just not those two examples I've mentioned.
… or in the whole function (didn't notice the first comment part - nice touch!).
Unfortunately, I honestly don't know what I expect that you do about this… In the function example, the call to $CMD SHOULD be quoted, but not the args.
But if we consider something like this:
#!/bin/sh
my_function() {
local PIDFILE="$1" ; shift
local CMD="$1" ; shift
local ARGS="$*"
"$CMD" -p "$PIDFILE" $ARGS
}
ARGS="-C -F -l -a"
my_function /bin/ls "$ARGS"
(ok, so this doesn't make much sense, since I'm calling it with ls but still, I'm sure you get the intention). Here, both $CMD and $PIDFILE must/should be escaped, but NOT ARGS. I have no idea how to programmatically distinguish between the two...
POSSIBLY (just spit-balling here), the comment directive in your example could be extended to ignore a variable?
Like:
my_function() {
local PIDFILE="$1" ; shift
local CMD="$1" ; shift
local ARGS="$*"
# shellcheck disable=SC2059:PIDFILE
[something or other]
}
… and extending that to multiple variables:
# shellcheck disable=SC2059:PIDFILE;VAR1;VAR2;[etc],SC2001:VAR19
In your example you want "$cmd" -p "$pidfile" "$@" (quoted!)
If you want to save it in a local variable, use an array: local args=("$@")
(same for the args to pass to ls, just use an array)
That is both a bashism and a "Assigning an array to a string"…
my_function() {
local PIDFILE="$1" ; shift
local CMD="$1" ; shift
local ARGS=("$@")
"$CMD" -p "$PIDFILE" "$ARGS"
}
In /tmp/tst.sh line 6:
local ARGS=("$@")
^-- SC2039: #!/bin/sh was specified, but arrays are not standard.
In /tmp/tst.sh line 8:
"$CMD" -p "$PIDFILE" "$ARGS"
^-- SC2128: Expanding an array without an index only gives the first element.
And if I use local ARGS="$@" I get:
In /tmp/tst.sh line 6:
local ARGS="$@"
^-- SC2124: Assigning an array to a string! Assign as array, or use * instead of @ to concatenate.
You're using local, I guess you can call it a bashism since it's not supported in posix sh.
And to expand an array use "${array[@]}"
Because I'm using bourne shell, It shouldn't tell me about the "Assign as array" (because the're not available).
my_function() {
PIDFILE="$1" ; shift
CMD="$1" ; shift
"$CMD" -p "$PIDFILE" "$@"
}
is posix
my_function /bin/ls "${ARGS[@]}":
In /tmp/tst.sh line 11:
my_function /bin/ls "${ARGS[@]}"
^-- SC2039: #!/bin/sh was specified, but array references are not standard.
Sorry, that was in the wrong place, wrong example. But still. No arrays in bourne shell.
@lucy That still is quoted and will be treated as ONE option, not FOUR… It passes the shellcheck tests though. But it's wrong programmatically...
@izabera That might work in this simple example, but I have a couple of real code where it's required to do it this way. I really don't want one separate variable for each of the options. It's a pain to maintain and extend...
my_function() {
local PIDFILE="$1" ; shift
local CMD="$1" ; shift
"$CMD" -p "$PIDFILE" "$@"
}
p() {
printf '%s\n' "$@"
}
my_function pid p arg1 arg2 arg3
prints -p\npid\narg1\narg2\narg3\n in dash
"$@" has special expansion that can be used to emulate arrays and get proper splitting
@lucy and that's your problem. It should print:
# /tmp/tst.sh
-p
pid
arg1
arg2
arg3
As I've said, if you quote $@, it will be treated as ONE command, not multiple...
No, hang on… Something is amiss here. @izabera Yes, you're right… I need to look at this some more.
However, there's a difference here…@lucy is here calling my_function() with each option as separate. I'm calling them as one string.
Even if I use /bin/dash, I still get one option per line (if not quoting the args).
Also, come to think of it, I think using arguments to a function (without saving it) is bad coding. It will most likely work most of the time, but sooner or later one modifies that ($@ and/or $*) and then one uses the wrong values.
I believe in saving the values in the very beginning and then using that. It's … "cleaner" some how.
"$@" expansion as multiple words is posix. That's the correct way to handle this sort of thing.
If you can't do that then you get to deal with the fact that shellcheck is going to tell you that you should.
I agree with you that a variable specifier on disabling comments is a good idea though.
Stuffing multiple options into a single string is just a bad idea and you really shouldn't do it. It isn't avoidable in some cases if you don't have arrays (well technically you always have one, the positional parameters, but using that can be hard/impossible) but shellcheck is still, to my mind, correct to complain about the usage.
I very often create a config file, like something like this:
# Extra arguments to "some command"
EXTRA_ARGS="-a -b -c -d"
and then source that in the script
#!/bin/sh
. /some/dir/config
[automatically/programmatically figure out CALCULATED_ARGS]
"some command" $CALCULATED_ARGS $EXTRA_ARGS
I think this is very common, and correct in every way. Quoting these variables is programmatically wrong, which is the point I'm trying to make.
It is not "correct in every way". It doesn't work if any arguments need to have embedded spaces or need shell glob metacharacters and it cannot be made to support that.
Yes, despite that, it is quite common and works for many cases. That doesn't make it less wrong though.
Ok, fair enough.
But the config example is very common and correct here. UNLESS, as you say, you have an argument that contain spaces etc. But that should be used as:
# Extra arguments to "some command"
# NOTE: Remember to quote args containing spaces
EXTRA_ARGS="-a -b -c -d 'extra arg'"
That won't work. Those single quotes make it through to the application and the string is two words/arguments and the single quotes don't prevent shell expansion of the contents of the single quoted string.
Like I said this cannot be made to work for those cases. The attempt is fundamentally broken. It just so happens to work for the simple cases that most people need most of the time.