Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixup parameter expansions #123

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

brlin-tw
Copy link
Contributor

@brlin-tw brlin-tw commented May 19, 2018

It is possible that one's home directory path contains spaces, this patch fixes most affected expansions by wrapping them with double quotes.

This PR also includes a patch to apply quoting to all parameter expansions, to help avoid alike failures in the future.

Signed-off-by: 林博仁(Buo-ren Lin) Buo.Ren.Lin@gmail.com

It is possible that one's home directory path contains spaces, this patch fixes all REALHOME expansions by wrapping them with double quotes.

Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
@brlin-tw brlin-tw changed the title Fixup REALPATH expansions Fixup parameter expansions May 19, 2018
It is possible that one's home directory path contains spaces, this patch fixes all SNAP_USER_{DATA,COMMON} expansions by wrapping them with double quotes.

Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
@brlin-tw brlin-tw force-pushed the patch-quoting-expansion branch 2 times, most recently from 20e9ad0 to 998f96a Compare May 19, 2018 10:02
This patch quotes all non-necessary parameter expansions to help catch-up future word-splitting-prone expansions using ShellCheck.

Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
@oSoMoN
Copy link
Member

oSoMoN commented May 29, 2018

This is functionally equivalent, but wouldn't it be more readable to have:

append_dir LD_LIBRARY_PATH "$RUNTIME/lib/$ARCH"

instead of:

append_dir LD_LIBRARY_PATH "$RUNTIME"/lib/"$ARCH"

@brlin-tw
Copy link
Contributor Author

brlin-tw commented May 29, 2018

In fact, I do previously preferred the 1st style but changed my mind during generating these patches due to the search&replace friendliness and well, they are indeed functionally equivalent.

I'll switch the style soon if people felt comfortable about it.

@oSoMoN
Copy link
Member

oSoMoN commented May 29, 2018

That's not a requirement from my end, rather a mere suggestion.
I'm curious what others think about it.

@brlin-tw
Copy link
Contributor Author

brlin-tw commented May 29, 2018

Another argument is that word splitting simply doesn't happen at the external portions of the quoting, quoting them might give viewers a false impression of the quoting in bash scripting in general and abusing it unnecessarily.

But again this argument did appear during the generation of these patches, so comments are welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants