search

tpope / rbenv-aliases

Create aliases for rbenv Ruby versions
MIT License
127 stars 18 forks source link

Fix bugs with --auto #14

Closed jasonkarns closed 8 years ago

jasonkarns commented 8 years ago

Found a few bugs with how --auto works.

When the semver support was added, it broke the existing tests, but was never caught.

  1. rbenv alias <version> --auto never even accepted semver style or jruby-1.2.3 style versions. The argument typically passed here is the intended symlink, so the case statement only accepted 1.2.3 (which would be the argument to match 1.2.3-pXXX). So I relaxed the case statement to accept X.Y (which would be the minimal argument to auto alias a semver version: 1.2 -> 1.2.3).

  2. rbenv alias <version> --auto wasn't sorting the patch version correctly. (This has been broken since 2e752b0df250104b55cc9cf54414de77626cf8bd, to my knowledge)

    • it would sort p99 above p145
    • it would sort 1.2.5 above 1.2.40
    • it would sort jruby-1.7.2 above jruby-1.7.11

    My approach for sorting the patch versions correctly is to derive/guess the character index at which the patch version begins. (For, -p style that's the length of the point_release string +3: 2 for -p and 1 for 0-based-indexing. For 1.2.3/rbx-1.2.3 style, that's the length of the point_release string +2: 1 for the . and 1 for 0-based-indexing.) Knowing the character index of the patch version, we can use the -k argument to sort.

  3. rbenv alias --auto apparently didn't work correctly either, because the point_releases function (intended to return all non-symlink versions without their patch fields) had a mistake in the sed expression. It was stripping off a trailing {dot}{digit}{literal +}. The + meant as a quantity operator for the digit class was being escaped so it was searching for a literal trailing +. Which meant that semver and rbx-1.2.3 versions never got truncated. (So, for example, it would attempt to create a symlink for rbx-1.2.3 as rbx-1.2.3, instead of rbx-1.2.) I'm assuming the escape was mistakenly added to the + because + is not a valid quantifier with sed Basic Regular Expressions. Ideally, we would use extended regexes, but the flag for e-regex differs between gnu/linux (-r) and bsd (-E). So instead, I just changed the expression to end with [0-9]*. Technically, this will match and truncate a single trailing ., but I suspect that is rare enough to not warrant the kludgy alternatives to using extended regexes. If you have a better suggestion, there's room to improve here.

Since the existing test suite isn't already running on travis, you can see the travis build of my fork here: https://travis-ci.org/jasonkarns/rbenv-aliases/builds/95101958

tpope commented 8 years ago

As evidenced by previous oversights I don't really have the bandwidth to thoroughly audit this sort of stuff, so I am adding you as a collaborator. If you accept, feel free to take responsibility for as much or as little as you like; just leave me in charge of tagging/release cutting, and keep me in the loop on anything majorly backwards incompatible

blackjid commented 8 years ago

Awesome fix!!!, thanks!!, looking forward for the tag!

jasonkarns commented 8 years ago

Should be soon. I'd like to knock out #8 first On Dec 6, 2015 5:01 PM, "Juan Ignacio Donoso" notifications@github.com wrote:

Awesome fix!!!, thanks!!, looking forward for the tag!

— Reply to this email directly or view it on GitHub https://github.com/tpope/rbenv-aliases/pull/14#issuecomment-162352736.