#2771 PR merged: Avoid SC2068 and SC2145 in finalize/Fedora/i386/550_rebuild_initramfs.sh

Labels: cleanup, fixed / solved / done

jsmeix opened issue at 2022-03-11 14:05:

I don't use Fedora so cannot test it.
@pcahyna @rmetrich
please have a look if my changes make sense.

  • Brief description of the changes in this pull request:

In finalize/Fedora/i386/550_rebuild_initramfs.sh
avoid ShellCheck
SC2068: Double quote array expansions to avoid re-splitting elements
https://github.com/koalaman/shellcheck/wiki/SC2068
and avoid
SC2145: Argument mixes string and array. Use * or separate argument
https://github.com/koalaman/shellcheck/wiki/SC2145
cf. https://github.com/rear/rear/issues/1040#issuecomment-1062945160

jsmeix commented at 2022-03-14 14:06:

My basic reasoning for my changes to use ${array[*]} instead of ${array[@]}
is that here the array elements are single words (kernel module names)
so we can "simply" use the simple ${array[*]} form everywhere here.

FYI:
I think we use often arrays where simple strings would be sufficient,
in particular when the array elements can be only single words.

pcahyna commented at 2022-03-14 15:05:

I agree with using ${OLD_INITRD_MODULES[*]} here, but my reasoning is different. The reason to use this form is because it occurs in a longer string. For example. as "Original OLD_INITRD_MODULES='${OLD_INITRD_MODULES[@]}'". This way, if OLD_INITRD_MODULES is for example ( a b c ), the resulting string will be expanded into three strings:

"Original OLD_INITRD_MODULES=a'"
"b"
"c'"

which is not what is intended, although in the special case of Log it is harmless, because Log concatenates its arguments back together.
Try it:

$ for i in "Original OLD_INITRD_MODULES='${OLD_INITRD_MODULES[@]}'"; do echo $i; done
Original OLD_INITRD_MODULES='a
b
c'

jsmeix commented at 2022-03-15 08:42:

https://github.com/rear/rear/pull/2771/commits/2fbb8ed8230e4af972d3a5933c6505c852950866
also should do better Log messages that match what the variables actually are

jsmeix commented at 2022-03-15 08:44:

@pcahyna in
https://github.com/rear/rear/pull/2771/commits/2fbb8ed8230e4af972d3a5933c6505c852950866
I added a TODO question about the unalias ls command.
Do you perhaps have an idea why it is there?

jsmeix commented at 2022-03-15 08:54:

@pcahyna
thank you for your review!
As always it helped a lot,
cf. https://en.wiktionary.org/wiki/given_enough_eyeballs,_all_bugs_are_shallow
I did not recognize that INITRD_MODULES and WITH_INITRD_MODULES are strings.
I was too much focussed on "just getting SC2068 and SC2145 out of the way" on Friday afternoon.
This basically proves what I wrote in
https://github.com/rear/rear/issues/1040#issuecomment-1062932894

pcahyna commented at 2022-03-15 13:00:

@pcahyna in 2fbb8ed I added a TODO question about the unalias ls command. Do you perhaps have an idea why it is there?

It should have no effect, according to bash(1):

Aliases are not expanded when the shell is not interactive, unless the
expand_aliases shell option is set using shopt

... and unless bash is in POSIX mode (item 6 in https://tiswww.case.edu/php/chet/bash/POSIX ).
I suspect the author of this code was debugging the script in an interactive shell and needed this unalias command because of having a conflicting alias defined, and did not realize that in the final code it is useless.

pcahyna commented at 2022-03-15 13:06:

@pcahyna thank you for your review! As always it helped a lot, cf. https://en.wiktionary.org/wiki/given_enough_eyeballs,_all_bugs_are_shallow I did not recognize that INITRD_MODULES and WITH_INITRD_MODULES are strings. I was too much focussed on "just getting SC2068 and SC2145 out of the way" on Friday afternoon. This basically proves what I wrote in #1040 (comment)

You are welcome! It would be a huge amount of work to do a 100% audit of the shellcheck warnings in the current code, I agree (also depending on whether one wants simply to silence ShellCheck or rather to make meaningful improvements to the code - I strongly prefer the latter! My experience with such mass cleanups is not very positive, it can result in tons of changes that merely silence the tool, without fixing the actual bugs when they are present and sometimes introducing new bugs when the original code was fine.)

jsmeix commented at 2022-03-15 13:10:

Regarding aliases:
Next time I may even try to read the fine manual myself :-)
but usually I read manuals only when I am really desperate ;-)

pcahyna commented at 2022-03-15 13:13:

Regarding aliases: Next time I may even try to read the fine manual myself :-) but usually I read manuals only when I am really desperate ;-)

That's what I did indeed some time ago when my aliases were not working in my shell script :-)

jsmeix commented at 2022-03-15 13:36:

Regarding ShellCheck:
I fully agree with you.
This is why I am currently against an automated/enforced ShellCheck because
it puts pressure on us and our contributors to "just get that annoyance silent"
instead of actually searching the root cause and fix things properly.
What I currently do regarding ShellCheck is only to look after
what ShellCheck considers to be an "error", cf. at the end of
https://github.com/rear/rear/issues/1040#issuecomment-1062945160

E.g. the 571 of SC2168: 'local' is only valid in functions
are likely all false positives because

shellcheck -s bash -S error path/to/script.sh

cannot know that we source scripts within our Source() function.

In contrast the other SCnnnn that ShellCheck considers to be an "error"
relatively often point to real issues in our code that I like to get cleaned up.

Then we can run for all our scripts

shellcheck -s bash -S error -e SC2168 path/to/script.sh

and should no longer get anything reported.

Then we may even run this command in an automated way
but this would be strictly optional for me.

jsmeix commented at 2022-03-21 11:19:

@pcahyna
does the current code in this pull request now look OK to you?

pcahyna commented at 2022-03-21 11:34:

@jsmeix I added one more comment, otherwise it looks fine. It is not pretty that the code starts with arrays and then switches to whitespace-separated strings in the middle, it is inconsistent. But since your goal was to fix ShellCheck warnings, I don't think it is necessary to do larger cleanups of the code unrelated to the original goal.

jsmeix commented at 2022-03-21 13:58:

I would prefer to postpone a general overhaul of this code to a later time
because otherwise I could never finish it in foreseeable future.
But now the code should be at least "clearly inconsistent"

# Concatenate the old and new modules into a string

so it would be much easier to overhaul it later if needed.
Before it was rather hidden that it switched from arrays to strings.

jsmeix commented at 2022-03-22 09:52:

@pcahyna @rear/contributors
if there are no objections would merge it tomorrow afternoon.

pcahyna commented at 2022-03-22 12:24:

I would prefer to postpone a general overhaul of this code to a later time
because otherwise I could never finish it in foreseeable future.

@jsmeix I completely agree. What matters is that the code increases monotonically in quality, i.e. new changes do not make it worse. One does not have to solve all existing issues at once.

[Export of Github issue for rear/rear.]