X-Git-Url: https://gerrit.fd.io/r/gitweb?p=csit.git;a=blobdiff_plain;f=docs%2Fbash_code_style.rst;h=e955f72ab406166baaf5c104e7029679da9feab6;hp=9db968f7d8c9d2c46d02dcfaab9c82eb275f9686;hb=cf2a4f97f261a8ab27578232e9f419968e8a1fb1;hpb=bf4685dcf156517f66fe68a825c768bd82a1a12b diff --git a/docs/bash_code_style.rst b/docs/bash_code_style.rst index 9db968f7d8..e955f72ab4 100644 --- a/docs/bash_code_style.rst +++ b/docs/bash_code_style.rst @@ -82,22 +82,24 @@ Safety + TODO: Consider giving examples both for good and bad usage. - + Command substitution on right hand side of assignment should be safe + + Command substitution on right hand side of assignment are safe without quotes. - + But still, quotes are RECOMMENDED, unless line length is a concern. - + Note that command substitution limits the scope for quotes, so it is NOT REQUIRED to escape the quotes in deeper levels. - + TODO: Do we prefer backtics, or "dollar round-bracket"? + + Both backtics and "dollar round-bracket" provide command substitution. + The folowing rules are RECOMMENDED: + + + For simple constructs, use "dollar round-bracket". - + Backticks are more readable, especially when there are - round brackets in the surrounding text. + + If there are round brackets in the surrounding text, use backticks, + as some editor highlighting logic can get confused. - + Backticks do not allow nested command substitution. + + Avoid nested command substitution. - + But we might want to avoid nested command substitution anyway? + + Put intermediate results into local variables, + use "|| die" on each step of command substitution. + Code SHOULD NOT be structured in a way where word splitting is intended. @@ -120,6 +122,10 @@ Safety When was the last time you checked error code of "echo" command, for example? + + Another example: "set -e" in your function has no effect + if any ancestor call is done with logical or, + for example in "func || code=$?" construct. + + As there is no reliable method of error detection, and there are two largely independent unreliable methods, the best what we can do is to apply both. So, code SHOULD explicitly @@ -162,9 +168,27 @@ Bash options + Code MUST to store "$?" and call "set -e" immediatelly afterwards. + + Code MUST NOT use this approach when calling functions. + + + That is because functions are instructed to apply "set -e" on their own + which (when triggered) will exit the whole entry script. + + + Unless overriden by ERR trap. + But code SHOULD NOT set any ERR trap. + + + If code needs exit code of a function, it is RECOMMENDED to use + pattern 'code="0"; called_function || code="${?}"'. + + + In this case, contributor MUST make sure nothing in the + called_function sub-graph relies on "set -e" behavior, + because the call being part of "or construct" disables it. + + Code MAY append "|| true" for benign commands, when it is clear non-zero exit codes make no difference. + + Also in this case, the contributor MUST make sure nothing within + the called sub-graph depends on "set -e", as it is disabled. + + Code MUST apply "-u" as unset variable is generally a typo, thus an error. + Code MAY temporarily apply "+u" if a command needs that to pass. @@ -184,8 +208,7 @@ Bash options + "Near start" means "before any nontrivial code". - + Basically only copyright and long high-level comments are - RECOMMENDED to appear before. + + Basically only copyright is RECOMMENDED to appear before. + Also code MUST put the line near start of function bodies and subshell invocations. @@ -235,7 +258,39 @@ here are some pros and cons: + This allows code blocks to support optional arguments. -TODO: Translate the "function way" into list of rules. ++ Rules: + + + Library files MUST be only "source"d. For example if "tox" calls a script, + it is an entry script. + + + Library files (upon sourcing) MUST minimize size effect. + + + The only permitted side effects MUST by directly related to: + + + Defining functions (without executing them). + + + Sourcing sub-library files. + + + If a bash script indirectly call another bash script, + it is not a "source" operation, variables are not shared, + so the called script MUST be considered an entry script, + even if it implements logic fitting into a single function. + + + Entry scripts SHOULD avoid duplicating any logic. + + + Clear duplicated blocks MUST be moved into libraries as functions. + + + Blocks with low amount of duplication MAY remain in entry scripts. + + + Usual motives for not creating functions are: + + + The extracted function would have too much logic for processing + arguments (instead of hardcoding values as in entry script). + + + The arguments needed would be too verbose. + + + And using "set +x" would take too much vertical space + (when compared to entry script implementation). Variables ~~~~~~~~~ @@ -346,6 +401,78 @@ Arguments or variables + For most Linux distros, "getopt" is RECOMMENDED. +Working directory handling +~~~~~~~~~~~~~~~~~~~~~~~~~~ + ++ Functions SHOULD act correctly without neither assuming + what the currect working directory is, nor changing it. + + + That is why global variables and arguments SHOULD contain + (normalized) full paths. + + + Motivation: Different call sites MAY rely on different working directories. + ++ A function MAY return (also with nonzero exit code) when working directory + is changed. + + + In this case the function documentation MUST clearly state where (and when) + is the working directory changed. + + + Exception: Functions with undocumented exit code. + + + Those functions MUST return nonzero code only on "set -e" or "die". + + + Note that both "set -e" and "die" by default result in exit of the whole + entry script, but the caller MAY have altered that behavior + (by registering ERR trap, or redefining die function). + + + Any callers which use "set +e" or "|| true" MUST make sure + their (and their caller ancestors') assumption on working directory + are not affected. + + + Such callers SHOULD do that by restoring the original working directory + either in their code, + + + or contributors SHOULD do such restoration in the function code, + (see below) if that is more convenient. + + + Motivation: Callers MAY rely on this side effect to simplify their logic. + ++ A function MAY assume a particular directory is already set + as the working directory (to save space). + + + In this case function documentation MUST clearly state what the assumed + working directory is. + + + Motivation: Callers MAY call several functions with common + directory of interest. + + + Example: Several dowload actions to execute in sequence, + implemented as functions assuming ${DOWNLOAD_DIR} + is the working directory. + ++ A function MAY change the working directory transiently, + before restoring it back before return. + + + Such functions SHOULD use command "pushd" to change the working directory. + + + Such functions SHOULD use "trap 'trap - RETURN; popd' RETURN" + imediately after the pushd. + + + In that case, the "trap - RETURN" part MUST be included, + to restore any trap set by ancestor. + + + Functions MAY call "trap - RETURN; popd" exlicitly. + + + Such functions MUST NOT call another pushd (before an explicit popd), + as traps do not stack within a function. + ++ If entry scripts also use traps to restore working directory (or other state), + they SHOULD use EXIT traps instead. + + + That is because "exit" command, as well as the default behavior + of "die" or "set -e" cause direct exit (without skipping function returns). + Function size ~~~~~~~~~~~~~ @@ -416,21 +543,25 @@ Library documentation + Then SHALL be the function definitions, and inside: - + "set -exuo pipefail" again. - - + Following that SHALL be the function documentation explaining API contract. + + The body SHALL sart with the function documentation explaining API contract. Similar to Robot [Documentation] or Python function-level docstring. + See below. - + Following that SHALL be varius TODOs, FIXMEs and code itself. + + Following that SHALL be various top-level TODOs and FIXMEs. + + + TODO: Document (in an appropriate place) how TODOs differ from FIXMEs. + + + "set -exuo pipefail" SHALL be the first executable line + in the function body, except functions which legitimely need + different flags. Those SHALL also start with appropriate "set" command(s). + + + Lines containing code itself SHALL follow. + "Code itself" SHALL include comment lines explaining any non-obvious logic. - + TODO: Document (in an appropriate place) how TODOs differ from FIXMEs. - - + There SHALL be two empty lines before next function definition. + + There SHALL be two empty lines between function definitions. More details on function documentation: