Fix shellcheck warnings

[a1346054]

Split from #4497 for further discussion.

I did not modify the configure script, which is a source of a lot of the remaining shellcheck warnings, because it comes from autoconf and so it makes little sense to try to fix it here.

I also did not modify the scripts in contrib, because they possibly are maintained at some other place. Similarly with the other scripts that don't appear to be called from any of the makefiles.

[kmk3]

NAK, as written in here:

#4497 (review)

These changes produce a lot of noise, but the fixes are incomplete.

[reinerh]

@kmk3, what do you mean with noise? I think their proposed changes are already an improvement over the current state, so the change makes sense in my opinion.

And not fixing everything is in my opinion not a reason to not merge an improvement (especially as they gave a reason for leaving out some files like auto-generated files).

[kmk3]

I'm writing a better review; please ignore the previous one for now.

[kmk3]

Sorry for the delay.

I made a few fixup commits and I'd like to propose them by pushing them to this
branch, but I'm getting "permission denied" on push.

@a1346054 Could you check the following option on the right?

[ ] Allow edits and access to secrets by maintainers

[a1346054]

That option has been checked since the start. I don't know why it is not working as it should.

[kmk3]

@a1346054 commented on Oct 13:

That option has been checked since the start. I don't know why it is not
working as it should.

I was trying to push the wrong branch actually, my bad. Now it worked.

---

So, the new commits contain my (and @rusty-snake's) suggestions.

If you agree, you can squash them with something like:

git checkout shellcheck-fix
git rebase -i --autosquash "$(git merge-base master HEAD)"

Then, for the commit message of the main commit:

Suggestion: Change it to the following:

Fix some shellcheck warnings

Note: This does not modify the configure script, which is a source of a lot of
the remaining shellcheck warnings, because it comes from autoconf and so it
makes little sense to try to fix it here.

Also, it does not modify the scripts in contrib, because they possibly are
maintained at some other place. Similarly with the other scripts that don't
appear to be called from any of the makefiles.

It's taken from the first post, but adapted to be more in line with the
following:

• https://chris.beams.io/posts/git-commit/

This information helps understand the intent, as only some shell scripts are
changed and it is not clear from the current message which ones were considered
and why.

By the way, there are more shellcheck warnings on these files, but let's ignore
them for this PR, as: one doesn't make much sense ($CONFIG_ARGS), others I'm
not sure about (unused vars) and for one of them I already had a better
solution than the suggested fix anyway (redirects).

[kmk3]

@a1346054

Hello, any thoughts on the above suggestions?

I can do the changes and rebasing if you want.

[a1346054]

I should be able to look at this some more in 2 days.

[a1346054]

I looked it over and all the suggested changes are fine. It's optional and possibly just a matter of taste whether the extra quotes are added or not, I don't have any hard feelings about it.

Feel free to reword the commit message and merge, I am away from the computer that has the required gpg key to make the change myself.

[kmk3]

@a1346054 commented on Nov 3:

I looked it over and all the suggested changes are fine. It's optional and
possibly just a matter of taste whether the extra quotes are added or not, I
don't have any hard feelings about it.

I'll say that I was surprised when I noticed that shellcheck doesn't warn about
things like USER=$(whoami), as I thought that assignments were also
susceptible to expansion pitfalls (and I think I've seen both versions being
used in different projects), but now I'm not sure if that's the case. But
either way, I like the consistency of always quoting variables and command
expansions and I suppose that other people could be confused by the differing
behavior (and quoting usage) as well, so quoting assignments with expansions
may also help with clarity in that regard.

Misc: If anyone knows of a good explanation of why assignments behave
differently in this way, please let me know.

Feel free to reword the commit message and merge, I am away from the computer
that has the required gpg key to make the change myself.

Very well, I'll do the changes then.

[kmk3]

LGTM.

[a1346054]

Misc: If anyone knows of a good explanation of why assignments behave
differently in this way, please let me know.

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01 says "Each variable assignment shall be expanded for tilde expansion, parameter expansion, command substitution, arithmetic expansion, and quote removal prior to assigning the value." So field splitting is not done.

[kmk3]

@a1346054 commented on Nov 5:

Misc: If anyone knows of a good explanation of why assignments behave
differently in this way, please let me know.

https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01
says "Each variable assignment shall be expanded for tilde expansion,
parameter expansion, command substitution, arithmetic expansion, and quote
removal prior to assigning the value." So field splitting is not done.

Thanks a lot!