scmbreeze / scm_breeze

Adds numbered shortcuts to the output git status, and much more
https://madebynathan.com/2011/10/19/git-shortcuts-like-youve-never-seen-before/
MIT License
2.82k stars 192 forks source link

Minimum bash and zsh version requirements #260

Closed HaleTom closed 5 years ago

HaleTom commented 5 years ago

I'm writing code which may use newer features in both shells.

What is the project's standard for the version of each shell with which we need to be compatible?

HaleTom commented 5 years ago

In particular, this:

# Quote "$@" before `eval` to prevent arbitrary code execution.
# Eg, the following will run `date`:
# evil() { eval "$@"; }; evil "echo" "foo;date"
function _safe_eval() {
  if [[ $shell = bash ]]; then
    # ${parameter@operator} where parameter is ${@} and operator is 'Q'
    # https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html
    eval "${@@Q}"
  else  # zsh
    # http://zsh.sourceforge.net/Doc/Release/Expansion.html#Parameter-Expansion-Flags
    eval "${(q-)@}"
  fi
}
ghthor commented 5 years ago

I vote we just set the minimum version to "very-modern", mark any case we know about where were using a feature with it's minimum version requirement and then let anyone that needs support in an older shell work through any of those issues.

TL:DR; Use modern features, especially if they improve security and safety.

ghthor commented 5 years ago

My shell on my work laptop is. I'm pretty sure I can override that with a newer version through homebrew if I need too.

GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17)
HaleTom commented 5 years ago

Given no answer, I'm going to declare that:

scm_breeze should work on the versions of shells in Ubuntu 14.04.5 LTS "Trusty Tahr" (end of support April 2019)

Those versions are:

bash: 4.3.11(1) zsh: 5.0.2

I guess I'll go and re-write the above quoted code :(

I'll close this issue in a week so that it has a little more visibility until then. If I forget, then please do the honours for me ;)

ghthor commented 5 years ago

Wouldn't mind leaving a nasty warning messaging in there when your shell is old enough to lack this support. You version selection decision seems solid to me.

ghthor commented 5 years ago

Also, I'm sorry this has taken longer then maybe you were expecting. I am seriously excited and very thankful to have your contributions.

HaleTom commented 5 years ago

I'm now getting 3/4 green on Travis CI.

The limiting factor is actually zsh on Ubuntu 14.04, which is supported until April 2019.

I'll edit my "declaration" above.

HaleTom commented 5 years ago

@ghthor While the versions of shells on Ubuntu 14.04 are sufficient, the aren't necessarily minimal, so I'd prefer to avoid an incorrect error message.

ghthor commented 5 years ago

Between this an the nightmare of shell I take care of at work I really like the idea of building a polyfill library for shell, ala the javascript(or older project) style. Come to think of it JS runtimes and Shell runtimes kind of share a similar type of problem. Both can have wildly different support and settings from system to system.

HaleTom commented 5 years ago

I don't know if there would be a way to ensure that you don't do:

local array=(x y z)

via a library.

It's just a thing that zsh 5.0.2 requires:

local array
array=(x y z)

Even shellcheck won't point this out as it's zsh specific. Having said that, shellcheck is highly recommended for everything else :)

(This took 24 travis builds to work out!)

ghthor commented 5 years ago

Shfmt enforcement and shellcheck linting are on my end of 2018 timeline for .scm_breeze, going to make this happen.