-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fix style #5648
Fix style #5648
Conversation
Sorry, didn't realize we were style checking under |
etc/bin/swarm
Outdated
@@ -204,7 +204,7 @@ append_config () { | |||
if prompt "Write \"$LINE\" to \"$LOC\"?"; then | |||
if [[ "$POS" == top ]]; then | |||
# ... at the top of the file | |||
echo -e "${LINE}\n$(cat ${LOC})" > "$LOC" | |||
echo -e "${LINE}\n$(cat "$LOC")" > temp && mv temp "$LOC" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get the quoting, but is the temp file necessary?
cat foo > foo
wipes out foo, but the echo
here makes it safe. (I confess I'm unsure of exactly why though!)
[Update]
In cat foo > foo
, the file foo
is opened in truncation mode before the command is executed, ready to receive the output of the command, so the content is lost.
In echo -e "$(cat foo)" > foo
, the command substitution is done first (because it is used to construct the final command) before the file is opened in truncation mode, so this is safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny, shellcheck was complaining about SC2094 locally
Make sure not to read and write the same file in the same pipeline
But apparently not on GH Actions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah OK, my local shellcheck must be outdated, didn't see that. However, going by my logic above, SC2094 should not apply for this particular construct.
Follow-up to #5639