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

build(ci): add shellcheck linting #57

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

build(ci): add shellcheck linting #57

wants to merge 4 commits into from

Conversation

mroth
Copy link
Owner

@mroth mroth commented Dec 5, 2021

fixes #54

@mroth mroth force-pushed the shellcheck-action branch 2 times, most recently from e99b8c0 to 7cef3a8 Compare December 6, 2021 01:06
@mroth
Copy link
Owner Author

mroth commented Dec 6, 2021

I've addressed most of the shellcheck feedback, after manually setting shell=sh to try to get everything back to POSIX compatible syntax to be as portable as possible.

My concerns at this point:

  1. Quote $@ and make $file and $files local in scmpuff init -s #54 has a request to add another local (which is reflected in the current state of this PR), but local is not POSIX compatible, see https://github.com/koalaman/shellcheck/wiki/SC3043. The alternatives appear to be a.) overriding this shellcheck rule for the file (e.g. shellcheck disable=SC3043), which means the script won't be 100% POSIX compliant, but apparently most shells do support local these days, or b.) just going old school and using obscure variable names such as __i to avoid potential overrides.
  2. I've made some other changes to the scripts to get them to be POSIX compliant, and I'm worried it might have introduced some other gotchas in real-world usage. While the integration tests are passing, that doesn't compare to 7 years of messy real world usage, so I'd be much more comfortable rolling this forward once the changes have been audited by someone who knows shell scripting very well.

@mroth
Copy link
Owner Author

mroth commented Jan 30, 2022

Okay, rebased now that fish shell support has been merged. I think I just need to figure out that local thing for bash/zsh.

In addition @jdelStrother do you have any idea if something similar to shellcheck exists that can lint fish scripts? I see there is fish -n, but I believe that just checks for valid syntax. [I don't think it's necessary to have this, but just in case it exists, might as well integrate it at the same time.]

@jdelStrother
Copy link
Contributor

I don't believe so, unfortunately. (Though at least fish has a few less footguns than bash does)

fixes shellcheck SC3043:
https://github.com/koalaman/shellcheck/wiki/SC3043

strictly speaking, local is not POSIX, but is supported in many shells,
including bash, ksh, dash, and BusyBox ash.

We could disable this rule, but perhaps safer to just do the effort
to try to be fully POSIX compliant for now, even though it makes
the script more difficult to read and reason about.
@mroth
Copy link
Owner Author

mroth commented Jan 30, 2022

Hmm, trying to figure out what's going on here. Removing the function keyword to get closer to POSIX compliance seems to make it so zsh is no longer overriding an existing git alias, which makes sense, however it's not clear to me why the unalias command in the script is not successfully removing the alias first. Sigh.

It believe perhaps what happens is when this happens in an eval, the aliases are read at the beginning, but only modified after the completion of the entire statement, e.g. its the equivalent of this all "happening on one line", such that the unalias hasn't taken effect for the environment when the new function is read. Ugh.

I can duplicate this with a test file test.zsh with contents:

# remove any existing `foo` aliases or shell functions
unalias foo > /dev/null 2>&1
unset -f foo > /dev/null 2>&1

foo() {
    echo "hello world\n"
}

And running the following from a zsh shell:

$ alias foo=/bin/fooer
$ eval "$(cat ./test.zsh)"
zsh: defining function based on alias `foo'
zsh: parse error near `()'

Loading the file via source test.zsh has desired results, suggesting that eval is what causes the issue here.

@mroth mroth mentioned this pull request Feb 26, 2022
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.

Quote $@ and make $file and $files local in scmpuff init -s
2 participants