Make -c useful again #88
Conversation
* Replace all checks for an empty string with `-z` * Replace all instances of `==` inside single brackets with `=` These are all intentionally in a single commit so they can easily be undone if anyone has a problem with the changes.
|
Any feedback on this or are you still reviewing? |
| @@ -327,6 +331,11 @@ keypair() { | |||
|
|
|||
| # Setup an cronexit handler so we cleanup | |||
| function cleanup { | |||
| if [ "${CRON}" = yes -a "${rawexit}" -ne 5 -a -f "${stdoutlog}" ]; then | |||
|
Sorry for the lag, was on a mini-vacation and didn't have much (if any) internet. |
| cronexit 1 | ||
| fi | ||
| fi | ||
|
|
||
|
|
||
| # Remove any ~ or other oddness in the path we're given | ||
| DOWNLOADDIR="$(eval cd ${DOWNLOADDIR// /\\ } ; if [ $? -eq 0 ]; then pwd; fi)" |
There was a problem hiding this comment.
There was a reason for doing this, and I think it relates to the use of ~ in the path. Without eval, ~ doesn't work.
There was a problem hiding this comment.
Going to investigate this one further. My command-line tests with eval removed were fine, but I want to check for any edge cases where it might break.
There was a problem hiding this comment.
It gets tricky when you source a file where a variable is assigned a path with ~ in it, that's what I can remember anyway. Doing it on cmdline is different since bash might be expanding it for you.
There was a problem hiding this comment.
The reason my tests ended up working was, in fact, due to expansion. When I tried single-quotes or variables it ended up breaking.
So I spent way too much time after that looking for an alternative and what it's come down to is either using an overly-complicated block of code, or just accepting the fact that the value of DOWNLOADDIR could, theoretically, be an attack vector for an injection attack, but it's just not worth obsessing over.
So this is how I learned to stop worrying and love the eval. ¯\_(ツ)_/¯
| @@ -275,7 +275,7 @@ fi | |||
|
|
|||
| # Remove any ~ or other oddness in the path we're given | |||
| DOWNLOADDIR="$(cd ${DOWNLOADDIR// /\\ } && pwd)" | |||
| if [ -z "${DOWNLOADDIR}" ]; then | |||
| if [ ! -d "${DOWNLOADDIR}" ]; then | |||
There was a problem hiding this comment.
wouldn't "" point to current directory and be somewhat unexpected since that should never happen? Because if cd fails, then I think downloaddir will be blank, right?
There was a problem hiding this comment.
A null string only points to current directory for things like cd. For tests, it's treated as a "literal" null string which has no path equivalent.
$ [ -d "" ] || echo nope
nope
$And while you're right that downloaddir would be blank if cd failed anyway, checking to see if a directory path is a valid directory seemed more readable to me than checking if a directory path is an empty string.
There was a problem hiding this comment.
Fair enough, tested it too after I wrote the comment and ! -d "" will fail, so all good.
|
I think this covers most of it. Does everything else look good? |
This changes the behavior of the -c option to make it useful for cron users with or without the -q option. Without -q, output will only be generated if something is actually done. With -q, output will only be generated in the event of an error. In both cases, exit codes will be "normalized" so that systemd doesn't get false alarms.
There are also a handful of other commits here for random code cleanup that I did as part of this work. I intentionally didn't squash these so that if anyone has a problem with some of the changes we can easily do a git revert.
Closes #84