Shell script cleanups:
[ -n ].cd fails.\n which is not handled by echo.$ in arithmetic variable expression.$(command) instead of legacy form `command`.sh. Use bash when arrays are used.[ foo -a bar ] is not well defined, use [ foo ] && [ bar ] instead.[ foo -o bar ] is not well defined, use [ foo ] || [ bar ] instead.
do we have someone on OpenBSD or likewise to test this?
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
@MarcoFalke Fixed! :-)
do we have someone on OpenBSD or likewise to test this?
We don't have any tests for most of these scripts, period :(
With the exception of the sh -> bash change when needed and clear fixes for edge cases (eg the || exit 1s), both of which I think we should do, I'm not sure what this accomplishes aside from code churn...are the other things going to break on some shell, or are they deprecated in a way that they will be removed within the next ~decade? We've had plenty of issues with otherwise innocuous shell script changes breaking dash-based systems, not to mention BSD incompatibilities, plus now WSL, so there is nontrivial review for some of these things, if its just because they are prettier or feel nicer, I really think we should cut back some of these changes.
@TheBlueMatt Thanks for reviewing.
I've now removed the changes that could be claimed to be in the "deprecated but not likely to be removed any time soon" and the "feels prettier/nicer" categories.
I've also unsquashed the changes so that this PR can be reviewed on a more detailed per-change/commit level. The requested squashing was perhaps a bit premature. I'll squash again when the only ACK:ed changes remain :-)
The remaining changes are:
Remove unused variables Rationale: Obvious mistake corrected.
Remove "\n" from echo argument. echo does not support escape sequences.
Rationale: Obvious mistake corrected - echo "\n" prints a backslash, the letter n and a newline.
Add error handling: exit if cd fails Rationale: Obvious mistake corrected - missing error handling.
Add required space to [[ -n "$1" ]] (previously [[ -n"$1" ]])
Rationale: Obvious mistake corrected - the syntax is [[ -n STRING ]], not [[ -nSTRING ]].
Fix incorrect quoting of quotes (the previous quotes had no effect beyond unquoting)
Rationale: echo "1 "2" 3" prints 1 2 3 and not 1 "2" 3. Change to echo "1 \"2\" 3" where quotes are intended in the output.
Use bash instead of POSIX sh. POSIX sh does not support arrays. Rationale: Assumptions are better stated explicitly than implicitly :-) @TheBlueMatt How do you feel about the remaining changes? All of those clear fixes? :-)
utACK 6c6494092e57e78bed4b6f2b315ed3e9d328ad9f
35 | @@ -38,8 +36,8 @@ PREV_COMMIT="" 36 | 37 | while true; do 38 | if [ "$CURRENT_COMMIT" = $VERIFIED_ROOT ]; then 39 | - echo "There is a valid path from "$CURRENT_COMMIT" to $VERIFIED_ROOT where all commits are signed!" 40 | - exit 0; 41 | + echo "There is a valid path from \"$CURRENT_COMMIT\" to $VERIFIED_ROOT where all commits are signed!"
According to the line above this is only printed when both vars are the same. No need to print them both.
@MarcoFalke Do you have any suggestion regarding the correct output to print? :-)
maybe
PREV_COMMIT=""
+INITIAL_COMMIT=${CURRENT_COMMIT}
while true; do
if [ "$CURRENT_COMMIT" = $VERIFIED_ROOT ]; then
- echo "There is a valid path from "$CURRENT_COMMIT" to $VERIFIED_ROOT where all commits are signed!"
+ echo "There is a valid path from \"$INITIAL_COMMIT\" to $VERIFIED_ROOT where all commits are signed!"
utACK 6c6494092
@MarcoFalke Fixed. Would you mind re-reviewing? :-)
32 | @@ -33,10 +33,11 @@ fi 33 | 34 | NO_SHA1=1 35 | PREV_COMMIT="" 36 | +INITIAL_COMMIT=${CURRENT_COMMIT}
In quotes, please.
@TheBlueMatt Fixed! :-)
Rebased!
Note, you can go ahead and squash fixup commits when you are doing a rebase anyway.
@MarcoFalke Good idea! Now squashed. Would you mind re-reviewing? :-)
utACK 13a81b19df7acff51187655bb755f1371f9c83ca
utACK 13a81b1 (Only looked at the diff, didn't check that the 8 commits do what they advertise)
Ready for merge? :-)
utACK 13a81b19df7acff51187655bb755f1371f9c83ca
utACK 13a81b19df7acff51187655bb755f1371f9c83ca