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

Use built-in Change Directory(cd) command #178

Merged
merged 3 commits into from
Oct 17, 2017

Conversation

carloseduardosx
Copy link
Contributor

@carloseduardosx carloseduardosx commented Oct 12, 2017

The problem

When we use command to execute any built-in command, first it'll look inside the PATH variable for any command that the argument passed may match, if it finds any command that match, then it'll execute that command, if it doesn't find any command in the PATH, then it'll look for built-in commands of your command shell and then execute it.

OSX has a cd command located at /usr/bin/cd and in general it's inside the PATH variable. But if you use ZSH or any other shell, you should be using their built-in commands.

For example, for who use ZSH, the built-in commands may be located at /usr/local/Cellar/zsh/<zsh_version>/share/zsh/functions. And in this case the built-in cd command it's inside this folder, more precisely at /usr/local/Cellar/zsh/5.4.2_1/share/zsh/functions/_cd.

As you can see on zsh repository, the cd command has the -q option.

The solution

So, the solution is to use the builtin command, which looks directly inside the built-in commands, skipping the PATH part.

Fixes #144

@carolynvs
Copy link
Collaborator

Sweet! Someone who knows what's going on with shells! 🎉 ❤️ Thanks for the explanation of command vs builtin.

Can you help me understand what is executed if we didn't use either builtin or command, and just called cd directly? Do you know if all the shells support builtin?

I would absolutely love to get this fixed ASAP. I just want to make sure that if we make this change that it will work for all shells (not just zsh).

@carloseduardosx
Copy link
Contributor Author

carloseduardosx commented Oct 13, 2017

The main reason to use command or builtin is because they look directly inside built-in commands, skipping functions and alias. In case of the user have an alias or function with name cd, when we execute the cd command, the shell will call one of them, in this case our script probably won't work.

@carloseduardosx
Copy link
Contributor Author

carloseduardosx commented Oct 13, 2017

About the builtin command, I can't have 100% sure that all command shells have this command as built-in, but the two most used command shell have it, which is: bash and zsh

@carloseduardosx
Copy link
Contributor Author

@carolynvs For better security, I added a check for the builtin command before executing it. Now, in case of the shell doesn't have the builtin command, we use the cd command alone, this should solve the problem.

@rgbkrk
Copy link
Member

rgbkrk commented Oct 14, 2017

👀

@rgbkrk
Copy link
Member

rgbkrk commented Oct 14, 2017

Yeah this looks like some good tradeoffs.

if command -v builtin >/dev/null 2>&1; then
export DVM_HELPER="$(builtin cd $DVM_CD_FLAGS "$(dirname "${DVM_SCRIPT_SOURCE:-$0}")" > /dev/null && command pwd)/dvm-helper/dvm-helper"
else
export DVM_HELPER="$(cd $DVM_CD_FLAGS "$(dirname "${DVM_SCRIPT_SOURCE:-$0}")" > /dev/null && command pwd)/dvm-helper/dvm-helper"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why pwd (and rm later down in the file) aren't also getting the same treatment as cd?

If we need it in both plances, perhaps we can have a variable $CMD_PREFIX or something like that which is set to either builtin or ""?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why the pwd and rm programs aren't being invoked by the builtin is because the OSX doesn't have a specific built-in command for them, like it have for cd.

@carolynvs carolynvs merged commit 9d9a4c1 into howtowhale:master Oct 17, 2017
@carolynvs
Copy link
Collaborator

Thank you for the fix and all the explanations! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants