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

Improve awk usage for performance #1441

Merged
merged 1 commit into from
Mar 22, 2017

Conversation

PeterDaveHello
Copy link
Collaborator

No description provided.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

This code isn't well tested, so I'm concerned about changes to it.

@@ -1109,7 +1109,7 @@ nvm_compute_checksum() {
command gsha256sum "${FILE}" | command awk '{print $1}'
elif nvm_has "openssl" && ! nvm_is_alias "openssl"; then
nvm_err 'Computing checksum with openssl dgst -sha256'
command openssl dgst -sha256 "${FILE}" | rev | command awk '{print $1}' | rev
command openssl dgst -sha256 "${FILE}" | command awk '{print $NF}'
Copy link
Member

Choose a reason for hiding this comment

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

I don't think these are the same - doesn't rev reverse the entire file, including the characters on every line? In my local testing this leads to different output.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't we just get a single line by openssl dgst -sha256 in this case?

Copy link
Member

Choose a reason for hiding this comment

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

echo abc | rev | awk '{print $1}' prints out cba - echo abc | awk '{print $NF}' prints out abc. These are not equivalent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but you rev it again, the comparison should be echo abc | rev | awk '{print $1}' | rev vs echo abc | awk '{print $NF}'

Copy link
Member

Choose a reason for hiding this comment

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

lol, oops, i missed the second rev. Thanks for explaining.

@@ -1209,7 +1209,7 @@ nvm_checksum() {
elif nvm_has "gsha256sum" && ! nvm_is_alias "gsha256sum"; then
NVM_CHECKSUM="$(command gsha256sum "${1-}" | command awk '{print $1}')"
elif nvm_has "openssl" && ! nvm_is_alias "openssl"; then
NVM_CHECKSUM="$(command openssl dgst -sha256 "${1-}" | rev | command awk '{print $1}' | rev)"
NVM_CHECKSUM="$(command openssl dgst -sha256 "${1-}" | command awk '{print $NF}')"
Copy link
Member

Choose a reason for hiding this comment

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

same question here

@PeterDaveHello
Copy link
Collaborator Author

@ljharb need a rebase or something?

@ljharb ljharb added the performance This relates to anything regarding the speed of using nvm. label Mar 22, 2017
@ljharb ljharb merged commit 6cb12b0 into nvm-sh:master Mar 22, 2017
@ljharb
Copy link
Member

ljharb commented Mar 22, 2017

nope, all set :-)

@PeterDaveHello PeterDaveHello deleted the performance-improvements branch March 22, 2017 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance This relates to anything regarding the speed of using nvm.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants