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

Fixes #3133: Properly handle release candidates in version comparison #3136

Merged
merged 2 commits into from
Sep 7, 2016

Conversation

mateon1
Copy link
Contributor

@mateon1 mateon1 commented Aug 27, 2016

License: MIT
Signed-off-by: Mateusz Naściszewski matin1111@wp.pl

@whyrusleeping
Copy link
Member

cc @chriscool and @lgierth for review

@whyrusleeping whyrusleeping added this to the ipfs 0.4.4 milestone Aug 31, 2016
@whyrusleeping whyrusleeping added the need/review Needs a review label Aug 31, 2016
@mateon1 mateon1 force-pushed the fix/version-comparison branch 2 times, most recently from 1d60491 to 544c577 Compare September 1, 2016 13:41
…ison

Deduplicate version checking code across scripts

License: MIT
Signed-off-by: Mateusz Naściszewski <matin1111@wp.pl>
}
# Get path to the directory containing this file
# If $0 has no slashes, uses "./"
PREFIX=$(expr $0 : "\(.*\/\)") || PREFIX='./'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be safer to use expr "$0" instead of expr $0 in case $0 contains spaces.

License: MIT
Signed-off-by: Mateusz Naściszewski <matin1111@wp.pl>
major_number() {
vers="$1"

# Hack around 'expr' exiting with code 1 when it outputs 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wat. Thats dumb. (I just ran this in my shell)

Copy link
Contributor Author

@mateon1 mateon1 Sep 4, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but that's from the original version checking code by chriscool.
The same goes for an empty string as output, hence the digit captured in both regex alternations at lines 39/52.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@whyrusleeping and @mateon1 I am not sure what you think is dump.
My opinion is that expr exiting with code 1 when it returns 0 is really dump indeed!
It looks like it comes from the fact that it can be used for arithmetics.

@whyrusleeping
Copy link
Member

@chriscool Look good to you?

@chriscool
Copy link
Contributor

@whyrusleeping yeah LGTM now!

@whyrusleeping
Copy link
Member

Thanks @mateon1!

@whyrusleeping whyrusleeping merged commit 0849375 into ipfs:master Sep 7, 2016
@mateon1 mateon1 deleted the fix/version-comparison branch September 8, 2016 00:16
@ghost ghost mentioned this pull request Dec 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/review Needs a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants