Do we need double quotes for input arguments in the SHELL scripts?

[jcohenadad]

Current example scrips have double quotes for CLI arguments:

https://github.com/spinalcordtoolbox/sct_tutorial_data/blob/dde7a4bfc18f539225af78a8a465ab191b9cff18/multi_subject/process_data.sh#L117-L123

However, I don't think it is required. I find double quotes removes clarity, and might give bad ideas to users who might use them in their terminal. Previously I've used scripts without quotes, and it worked fine (example).

[joshuacwnewton]

Double quotes are added from the recommendations of shellcheck:

Line 60:
    sct_label_vertebrae -i ${file}.nii.gz -s ${file_seg}.nii.gz -c t2 -qc "${PATH_QC}" -qc-subject "${SUBJECT}"
                           ^-- SC2086 (info): Double quote to prevent globbing and word splitting.
                                             ^-- SC2086 (info): Double quote to prevent globbing and word splitting.

The relevant rule is SC2086. Without double-quotes, the following will happen:

The first argument [will be split] by IFS (Internal Field Separators, i.e. spaces, tabs and line feeds). Then, each of them [will be expanded] as if it was a glob. Lastly, all the resulting strings and filenames [will be joined] with spaces.

In other words, if e.g. ${file_t2} contains any spaces, tabs, line feeds, or globbing wildcard characters (*, ?, [, ], ^), then unexpected behavior may occur.

---

Previously I've used scripts without quotes, and it worked fine (example).

However, as you point out, if we can be 100% sure that none of the special characters are present, then double-quotes aren't necessary.

Looking at this script in particular (multi-subject/process_data.sh), since we control the data being used, we already know that the exact paths and filenames of the various subjects do not contain special characters. So, I think it would be okay to remove the double-spaces.

The only way I think this could cause issues is if:

• Later on, our users copy the example scripts into a folder that has a space (e.g. My Documents), or use subject files that have spaces in them.
• Users have installed SCT in a location with a space in the path, and thus $SCT_DIR contains a space. See: https://github.com/spinalcordtoolbox/sct_tutorial_data/blob/dde7a4bfc18f539225af78a8a465ab191b9cff18/multi_subject/process_data.sh#L156-L157

But if we are okay with these risks, then I think we can remove the double-quotes.

[jcohenadad]

Double quotes are added from the recommendations of shellcheck:

Right, I was suspecting something like this. The problem is that users copy/paste, from/to the terminal, edit the variables, replace them, add/remove some, etc. all these manipulations resulting in higher probability for "forgetting" a double quote somewhere, resulting in issues like this one:

Notice the "{PATH_QC}" folder, which was created because I forgot one double quote in the script after modifying it.

I consider myself more experienced than the average user, so I anticipate a lot of similar mistakes.

That being said, I do see the value in covering issues with spaces in file names (plenty of our users have that, unfortunately).

On the other hand, not silencing errors if there is a path with spaces is also a way to change users' behaviour in the long run.

Maybe we should discuss this during our next SCT meeting

[joshuacwnewton]

Notice the "{PATH_QC}" folder, which was created because I forgot one double quote in the script after modifying it.

One way we could perhaps get around this is by double-quoting only the environment variables:

# Before
 sct_register_multimodal -i "${SCT_DIR}/data/PAM50/template/PAM50_t2.nii.gz" \ 
                         -iseg "${SCT_DIR}/data/PAM50/template/PAM50_cord.nii.gz" \ 
# After
 sct_register_multimodal -i "${SCT_DIR}"/data/PAM50/template/PAM50_t2.nii.gz \ 
                         -iseg "${SCT_DIR}"/data/PAM50/template/PAM50_cord.nii.gz \ 

This is equally valid usage, and keeps the "${SCT_DIR}" part separate from the rest of the path. That way, if the rest of the path is edited, there is less of a chance that a closing " will be accidentally removed.

[joshuacwnewton]

This was addressed by #13 but never linked. Closing.