Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Remove 'cd' Overrides #1844

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
4 participants
Contributor

jmatth commented May 23, 2013

There were multiple places in the repo where we were overriding the builtin cd command with a function. This is bad because

  1. Each time someone creates a cd function it clobbers the one before it, so if you used the plugins that did this you would only really have access to the last one.
  2. Zsh already has a hook for this.

I've included 3 different commits, each one removing a cd override from a different place. I'll use the github code commenting feature in addition to my commit messages to explain some of the changes I made.

jmatth added some commits May 23, 2013

@jmatth jmatth Replacing cd override with zsh hook in virtualenvwrapper. 580bf9b
@jmatth jmatth Moving all autoenv logic to the plugin file.
-Some of this was previously located in the directories lib.

-Also unsetting the cd override function created by autoenv and replacing
 it with a zsh hook.
e34c6f9
@jmatth jmatth Removing cd override from directories lib. 63efab7

@jmatth jmatth commented on the diff May 23, 2013

lib/directories.zsh
@@ -21,23 +21,6 @@ alias 7='cd -7'
alias 8='cd -8'
alias 9='cd -9'
-cd () {
- if [[ "x$*" == "x..." ]]; then
- cd ../..
- elif [[ "x$*" == "x...." ]]; then
- cd ../../..
- elif [[ "x$*" == "x....." ]]; then
- cd ../../../..
- elif [[ "x$*" == "x......" ]]; then
- cd ../../../../..
- elif [ -d ~/.autoenv ]; then
@jmatth

jmatth May 23, 2013

Contributor

Not sure why this (lines 33-35) was put here instead of in the autoenv plugin in the first place. It looks like it was resourcing the autoenv file every time you cd somewhere. You can see in the other diff that I moved this into the virtualenv plugin and only source it once.

@xuhdev

xuhdev May 25, 2013

Contributor

Actually it is not resourcing each time you run cd. When the activate.sh is sourced, cd is overrided by the one from autoenv. You can try which cd to observe the change.

But I do like your pull request, although I cannot decide to merge or not.

@jmatth

jmatth May 25, 2013

Contributor

Ah, I see. So then this whole function gets replaced and the part that allows you to abbreviate the dots goes away too. This is part of the reason I'm trying to get rid of all the cd overrides, with them clobbering each other it could get confusing really quick.

I'm guessing you can't decide about the pull request because you like the part of the function that lets you abbreviate the dots for going up directories? A couple of alternatives to removing it:

  1. Change the name of the function. Call it up or something else that's intuitive but not an existing UNIX command, and if you really want alias cd to it in your zshrc.
  2. Move it to a separate plugin. Same concept, you can choose whether or not you want to override cd in your zshrc.

In any case I don't think it's a good to leave this function here with the name cd, since using the autoenv plugin will source autoenv's activate.sh, which clobbers cd, and plugins are always sourced after libs. I guess that's a bit of an edge case, but I still think it would be good to remove that possible confusion.

@xuhdev

xuhdev May 25, 2013

Contributor

I said I can't decide is because I don't have the permission to merge :)

I do agree with your opinion anyway. Yes, this function is also evil for me.

@jmatth

jmatth May 25, 2013

Contributor

Yeah, I just noticed that after I made the comment :P. Oops, I just saw that and assumed you had admin lol.

Anyway, good to know I'm not the only one who thinks this was a bad idea.

@mcornella

mcornella May 27, 2014

Collaborator

This whole function definition is removed in #2422

@jmatth jmatth commented on the diff May 23, 2013

plugins/autoenv/autoenv.plugin.zsh
@@ -1,3 +1,11 @@
+# Add the functions from autoenv if it's installed.
+if [ -r ~/.autoenv/activate.sh ]; then
+ source ~/.autoenv/activate.sh
+ # Use a zsh hook instead of overriding the builtin cd.
+ unset -f cd
@jmatth

jmatth May 23, 2013

Contributor

This unsets the shell function that activate.sh overrides cd with...

@jmatth jmatth commented on the diff May 23, 2013

plugins/autoenv/autoenv.plugin.zsh
@@ -1,3 +1,11 @@
+# Add the functions from autoenv if it's installed.
+if [ -r ~/.autoenv/activate.sh ]; then
+ source ~/.autoenv/activate.sh
+ # Use a zsh hook instead of overriding the builtin cd.
+ unset -f cd
+ add-zsh-hook chpwd autoenv_init
@jmatth

jmatth May 23, 2013

Contributor

...and this adds a zsh hook to accomplish the same thing as the override.

@jmatth jmatth commented on the diff May 23, 2013

plugins/virtualenvwrapper/virtualenvwrapper.plugin.zsh
@@ -40,10 +40,8 @@ if [[ -f "$wrapsource" ]]; then
fi
}
- # New cd function that does the virtualenv magic
- function cd {
- builtin cd "$@" && workon_cwd
- }
+ # Add zsh hook for virtualenv magic
+ add-zsh-hook chpwd workon_cwd
@jmatth

jmatth May 23, 2013

Contributor

Same idea, remove function, use hook.

@mcornella

mcornella May 27, 2014

Collaborator

This is already in the repository.

@jmatth jmatth commented on the diff May 23, 2013

lib/directories.zsh
@@ -21,23 +21,6 @@ alias 7='cd -7'
alias 8='cd -8'
alias 9='cd -9'
-cd () {
@jmatth

jmatth May 23, 2013

Contributor

I removed this whole block (lines 24-39) because:

  1. There's a set of just above it that accomplish almost exactly the same thing.
  2. There's really no way I can think of to implement this exact functionality without overriding cd. If this is something we really want then I'd recommend forking it out into a separate plugin that people can choose to source. Call it directorynav or something.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment