BUG: Fix all shellcheck error-level findings in Scripts/ (refs #397)#1974
Merged
hjmjohnson merged 6 commits intoJun 8, 2026
Merged
Conversation
sliceBySliceOperation.sh used bare $10 and $11 to read the 10th and
11th positional parameters. Bash parses $10 as ${1}0 (first arg
followed by literal '0'), not the 10th argument; the script silently
populated PARAMETERS[5] and PARAMETERS[6] with garbage whenever it
was invoked with 10+ arguments.
Use the required ${10} and ${11} braced form so the intended
positionals are read.
Refs: issue ANTsX#397.
Two patterns fixed:
1. "echo $files[1]" in antsMultivariateTemplateConstruction.sh,
antsMultivariateTemplateConstruction2.sh, buildtemplateparallel.sh,
and directlabels.sh. $files is a scalar from backtick ls, not an
array; $files[1] expands to the whole string followed by literal
'[1]'. The trailing index was dead and is removed.
2. "$TEMPLATES[$i]" inside an echo error message in both template
construction scripts. TEMPLATES is a genuine array (the surrounding
test uses ${TEMPLATES[$i]}). The unbraced form printed the first
element plus literal '[$i]', mis-identifying which template failed.
Refs: issue ANTsX#397.
Two patterns fixed in antsMultivariateTemplateConstruction{,2}.sh:
1. echo "prefix ${arr[@]}" — ${arr[@]} inside a quoted string
glues each element with adjacent text on its ends, producing a
different word list than intended. Changed to ${arr[*]} so the
array elements are space-joined into one argument as the echo
call already assumes.
2. "mv ${tmpdir}/selection/${TEMPLATES[@]} ${currentdir}/" — the
prefix glued only to the first array element; subsequent elements
were taken from the current working directory rather than the
selection subdir. Replaced with an explicit loop that moves each
template file from the selection subdirectory.
Refs: issue ANTsX#397.
Add double quotes around every flagged ${arr[@]} expansion across the
Scripts/ tree. The unquoted form re-splits each element on IFS, so any
filename, prefix, or option string containing whitespace is silently
broken into multiple shell arguments before being handed to the
downstream ANTs binary or rm/mv/echo.
Files touched:
antsAtroposN4.sh, antsBrainExtraction.sh, antsCookTemplatePriors.sh,
antsCorticalThickness.sh, antsJointLabelFusion.sh,
antsJointLabelFusion2.sh, antsLongitudinalCorticalThickness.sh,
antsLongitudinalJointLabelFusion.sh,
antsMultivariateTemplateConstruction.sh,
antsMultivariateTemplateConstruction2.sh,
antsRegistrationSpaceTime.sh, directlabels.sh,
shapeupdatetotemplate.sh, sliceBySliceOperation.sh
This is a mechanical ${arr[@]} -> "${arr[@]}" substitution applied
only to previously-unquoted occurrences; no other behaviour changes.
Refs: issue ANTsX#397.
Collaborator
Author
Contributor
|
This will help a little bit but its a proper strict mode which I think is important for not having scripts run off and continue when errors are encountered. |
Collaborator
Author
This is just one step to keep the PR not too big. |
Contributor
Fair enough. I don't have review privileges here, but I approve 👍 |
Member
|
Thanks @hjmjohnson @gdevenyi I will review ASAP |
cookpa
requested changes
Jun 2, 2026
cookpa
left a comment
cookpa
left a comment
Member
There was a problem hiding this comment.
Needs a minor change in the longitudinal thickness script, to make usage format correctly.
cookpa
approved these changes
Jun 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eliminates all 83
shellcheck -S errorfindings (4 distinct rules) inScripts/, fixing several latent bugs along the way. First step of the cleanup discussed in #397; warnings/info-level findings are not addressed here.Each shellcheck rule is its own commit so individual fixes can be reverted or cherry-picked independently.
Per-commit breakdown
BUG: SC1037 …sliceBySliceOperation.shreading$10/$11$10as${1}0; the script silently stuffed garbage into PARAMETERS[5,6] whenever it was invoked with ≥10 args. Real bug.BUG: SC1087 …antsMultivariateTemplateConstruction{,2}.sh,buildtemplateparallel.sh,directlabels.shecho $files[1]wherefilesis a scalar from backtickls(the[1]was dead text appended to the whole string) — index removed; (b)echo "$TEMPLATES[$i] was not created"where TEMPLATES is a real array — braces added so the right element is printed in the error message.BUG: SC2145 …antsMultivariateTemplateConstruction{,2}.shecho "prefix ${arr[@]}"→${arr[*]}(echo wants one space-joined string). (b)mv ${tmpdir}/selection/${TEMPLATES[@]} ${currentdir}/was a real bug — the prefix only glued to the first element, so subsequent files were picked up fromcwdinstead of${tmpdir}/selection/. Replaced with an explicitforloop.BUG: SC2068 …${arr[@]}→"${arr[@]}"everywhere it was unquoted. Done with a perl in-place regex using negative lookbehind/ahead so already-quoted expansions are left alone. Protects against filenames-with-spaces being re-split byIFS.Verification
shellcheck -S error Scripts/*.shreturns 0 lines after the four commits (was 83 lines before).bash -nsyntax check passes on every modified file.mvcross-directory move) which alter observable behavior only in the broken-case direction.What's NOT in this PR
set -euo pipefailadoption can be safely attempted.set -eadoption itself — only 1 of 46 scripts uses strict mode today. That's a separate, per-script PR with maintainer regression testing per the discussion in bash scripts don't catch errors #397.shellcheck -S error Scripts/*.shto CI to keep the 0-error invariant.Refs: #397