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

Refactor: Do not ignore shellcheck 2207 #378

Merged
merged 2 commits into from
Apr 5, 2024
Merged

Refactor: Do not ignore shellcheck 2207 #378

merged 2 commits into from
Apr 5, 2024

Conversation

sandr01d
Copy link
Collaborator

Check list

  • I have performed a self-review of my code
  • I have commented my code in hard-to-understand areas
  • I have made corresponding changes to the documentation

Description

We sometimes capture multiline output in an array. We used to do so directly, while setting IFS to "\n" to control splitting. This has the downside that bashs glob expansion will be invoked.
I think the original reason we did use this technique was that it does work in both zsh and bash. Since we always execute bin/git-forgit in bash now, this is not necessary anymore. We can use read -r in such cases to add each line to the array without glob expansion. This is the idiomatic way to do so and recommended in shellcheck 2207. In my personal opinion this is also a cleaner and more straightforward approach than setting and resetting $IFS.

Type of change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation change

Test environment

  • Shell
    • bash
    • zsh
    • fish
  • OS
    • Linux
    • Mac OS X
    • Windows
    • Others:

We sometimes capture multiline output in an array. We used to do so
directly, while setting IFS to "\n" to control splitting. This has the
downside that bashs glob expansion will be invoked.
Use read -r in such cases to add each line into the array without glob
expansion.
@cjappl
Copy link
Collaborator

cjappl commented Mar 28, 2024

Do we need to tweak our github action that runs shellcheck to enforce this now? Better than backsliding later on

Copy link
Owner

@wfxr wfxr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wfxr
Copy link
Owner

wfxr commented Mar 29, 2024

Do we need to tweak our github action that runs shellcheck to enforce this now? Better than backsliding later on

I think we'd better to keep some flexibility since many checks themselves have exceptions, like SC1091, SC2145, SC2206, etc. IMO we don't have to stick to all checks as long as we have a good reason.

@sandr01d sandr01d merged commit cac700e into master Apr 5, 2024
7 checks passed
@sandr01d sandr01d deleted the shellcheck-2207 branch April 5, 2024 18:12
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.

4 participants