Bugs/dont use cdpath #1383

Merged
merged 3 commits into from Dec 15, 2012

Projects

None yet

3 participants

@richo
Member
richo commented Dec 13, 2012

This creates __rvm_cd which doesn't respect CDPATH, but more or less leaves the variable alone otherwise.

It should fix #1375

Review would be good, I haven't tested it super thoroughly, I also considered

CDPATH="." buildin cd "$@"

but I didn't have access to enough shells to check that actually does what I'd expect with builtins.

richo added some commits Dec 13, 2012
@richo richo Stub out cd with __rvm_cd
Leaves CDPATH alone except when actually switching directories

Also fix some unquoted paths
b78b823
@richo richo Rip out support for stored cdpath 88e937d
@mpapis mpapis commented on the diff Dec 13, 2012
scripts/functions/support
@@ -128,6 +128,18 @@ __rvm_sed_i()
fi
}
+# Drop in cd which _desotn't_ respect cdpath
+__rvm_cd()
+{
+ typeset old_cdpath ret
@mpapis
mpapis Dec 13, 2012 Member

try simplified code:

typeset CDPATH
CDPATH="."
chpwd_functions="" builtin cd "$@" || return $?

this works well with IFS - we have this trick in few places in code already and for now nobody complained

@richo
richo Dec 13, 2012 Member

I'm suss on what typeset CDPATH will actually do. Ill try to wire up a test
harness for major versions of bash and zsh today.
On 14 Dec 2012 03:23, "Michal Papis" notifications@github.com wrote:

In scripts/functions/support:

@@ -128,6 +128,18 @@ __rvm_sed_i()
fi
}

+# Drop in cd which desotn't respect cdpath
+__rvm_cd()
+{

  • typeset old_cdpath ret

try simplified code:

typeset CDPATH
CDPATH="."
chpwd_functions="" builtin cd "$@" || return $?

this works well with IFS - we have this trick in few places in code
already and for now nobody complained


Reply to this email directly or view it on GitHubhttps://github.com/wayneeseguin/rvm/pull/1383/files#r2409269.

@mpapis
Member
mpapis commented Dec 13, 2012

just the one comment - rest looks good - very good direction, makes me wonder why we did not have the same for grep ;)

@mpapis
Member
mpapis commented Dec 13, 2012

@richo also I finally got some working examples of PROMPT_COMMAND => https://github.com/postmodern/chruby/blob/auto-switching/share/chruby/auto.sh seams a lot simpler then I thought it would be so in the end might be worth adding support for it especially it allows reloading environment after git pull

@mpapis
Member
mpapis commented Dec 15, 2012

one more comment for:

CDPATH="." buildin cd "$@"

it would fail on some older bash versions, there are issues with properly exporting prefixing variables, it's just one version of bash but we had issues with it already

@mpapis mpapis merged commit fb66899 into master Dec 15, 2012
@mpapis
Member
mpapis commented Dec 15, 2012

@richo if you find that my proposal (the shorter one) is safe to use just push to master

@rhasti

This patch causes installer not to work on CentOS 6.3 minimal as well on Fedora 18.

See https://gist.github.com/4294194

Function __rvm_cd not defined in file binscripts/rvm-installer

Member

fixed with #1390

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment