wfxr / forgit

:zzz: A utility tool powered by fzf for using git interactively.
MIT License
4.32k stars 136 forks source link

Refactor: Do not ignore shellcheck 2207 #378

Closed sandr01d closed 2 months ago

sandr01d commented 3 months ago

Check list

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

Test environment

cjappl commented 3 months ago

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

wfxr commented 3 months ago

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.