QA - shellcheck
Closed this issue · 6 comments
This looks like a great tool for both power users and newbies alike.
I see some potential issues with quoting. eg if $cache has a quote or semi-colon character in it.
Have you seen shellcheck? In my experience, there are very few times when it's wrong, and those can be easily silenced with comments.
thanks.
you are probably talking about these lines of code:
cache=$(grep CacheDir /etc/pacman.conf | awk '{print $3}')
find $cache -name "${line}-[0-9]*[0-9a-z.-_]*.pkg.tar.[gx]z" | sort -r | sed -n '1p' >> /tmp/pacui-install
the "cache" variable contains the path to your pacman chache directory. i doubt that it contains semicolons or quotes.
but to be honest, i am not an expert about bash and i constantly learn new syntax about it every time i try to do something bigger in pacui. i am never sure about using quotes (when i need them and when my code stops working because i use them). if you have a good read about, i will look into it.
i have heard of shellcheck before, and it found some issues with pacui before, as you can see in the commits 41176af, 9a96709, eacbad0. but when it comes to using quotes i am very disappointed in shellcheck. parts of pacui actually stopped working, because i used "$variable" instead of $variable (this was a suggestion by shellcheck). i actually released a broken version of pacui/pacli-simple and i have only realized my mistake later. this happened twice.
so, as you can see, i rather like a good read about how to use quotes in bash than tips from shellcheck. either shellcheck suggests bad quotes or my code is written in a fragile way, so that simple quotes can brake it.
feel free to send pull requests or simply tell me your suggested improvements.
in this case, i assume you want me to use
find "$cache" -name ....
is this correct?
Yes, as a rule, quotes are always used
but in this exemple, we can have a bug, if :
#Cachedir is a directory for ....
#CacheDir = /var/cache/pacman/pkg/
previous code returns comments :( not a quotes problem ;)
cache=($(awk -F'=' '/^CacheDir/ {print $2}' '/etc/pacman.conf'))
cache="${cache:-/var/cache/pacman/pkg/}" # if empty cache assign default value
thanks @papajoker.
i have just used your tips in the latest 2 commits about awk optimisation.
i did not use
cache="${cache:-/var/cache/pacman/pkg/}"
but rather an if-statement. i think this is easier to read code.
@excalibur1234 feel free to contact me privately about the cases where quoting is suggested and when implemented breaks the code. In my experience, shellcheck is seldom wrong. I'm more than happy to share what I know about bash syntax. I'll contact you on the Manjaro forum with my details.
when i previously quoted variable names, i simply did a search + replace.
this time, i manually went through the code and quoted variables. i am currently testing this on my machine and will do a commit when i think it is stable. preliminary tests look good, though.