mroth / scmpuff

:1234: Numeric file shortcuts for common git commands
https://mroth.github.io/scmpuff/
MIT License
384 stars 22 forks source link

build(ci): add shellcheck linting #57

Open mroth opened 2 years ago

mroth commented 2 years ago

fixes #54

mroth commented 2 years ago

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. 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 commented 2 years ago

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 commented 2 years ago

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

mroth commented 2 years ago

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.