Clean up setup_rules.sh

[trn1ty]

I have not tested this but I will when I get my Flipper, hopefully tomorrow. This is tested working.

These are a set of patches to correct minor issues (to me) that I noticed while reading your script. SC commits refer to ShellCheck.

I submit these changes to and pass all rights to said changes onto Flipper Devices, LLC.

[zhovner]

Any updates on this? Have you test it?

[trn1ty]

I have tested it and it worked, but someone should probably double-check to make sure.

[gsurkov]

@devenblake Could you please provide the rationale for the changes? (e.g. links to standards, portability considerations, style guides, best practices etc). I'm not a shell script expert and I don't understand why these are necessary.

[trn1ty]

Line numbers are upstream|my branch.

Wrapping variables in quotes

In nearly all contexts shell variables should be quoted. Even when you wouldn't expect it leaving variables unquoted can be disastrous. A lot of my changes in this regard were unnecessary but not harmful just-in-case fixes.

Replacing echo(1) with printf(1)

See echo(1p), Application usage - "New applications are encouraged to use printf instead of echo." This is part of the POSIX standard, published by The Open Group.

Variations in echo(1) implementation are the subject of an interesting paper if you have further interest.

shell ! versus test(1) !

Ultimately this probably doesn't make a difference; dash definitely does and bash most likely provides a built-in test(1) for efficiency (to avoid the overhead of spawning a new process). Leaving the exit status inversion to the shell in my opinion makes scripts easier to read but it's down to opinion.

set -e

This stops a shell script if an unhandled unsuccessful exit code is returned from a process. It's insurance in case something unexpected happens.

&& style

Line-breaking expressions before operators is a style used by Knuth. Whether it enhances readability is opinion but I strongly support it.

Switch tee(1) for dd(1)

This is the most frivolous of my changes. tee(1) is usually used for monitoring the contents of a pipe between programs and dd(1) is usually used for input/output redirection (though originally dd(1) was meant for copying and conversion, hence dd(1)'s conv arguments) so I switched tee(1) for dd(1) which meant more sense at least colloquially.

alias warning_message versus shell function

Neither option really makes sense. This should probably just be a string constant that lives in a shell variable. Making this an alias just makes a little clearer to me that warning_message is being used specifically and exclusively for its side-effects.

Testing commands directly versus the exit status

true; if [ $# -eq 0 ] is the same as if true. true(1) in this context can be switched with any line of script. This also avoids an unnecessary use of test(1).

Slightly better RULES_TEXT formatting

I tried to clean this up but didn't remember SC1004 (escape sequences don't work within single-quoted strings - double-quoting the string instead would have interfered with the string content) so I mostly gave up and made new commits to nix the old ones. I did add a trailing newline.

SC2162

It doesn't make a difference so I just changed read to read -r to silence the shellcheck warning.

---

All in all whether or not you wish to use my PR is your choice. I believe it makes the script significantly more readable. Whether or not you use my PR please make my echo->printf change, my if [test] -> if [command] change, and quote the variables.

[gsurkov]

Thank you, this all sounds reasonable, and I have learned something.