Permalink
Browse files

Escape spaces in folder name so script won't fail

If the current directory has spaces, the script fails to change paths and fails.
  • Loading branch information...
keyvez committed Apr 9, 2013
1 parent 0198a12 commit 32a42f27b62b62bd915ab632b0b36fa939d22c0e
Showing with 2 additions and 1 deletion.
  1. +2 −1 tools/upgrade.sh
View
@@ -1,4 +1,5 @@
current_path=`pwd`
+current_path=${current_path/ /\\ }

This comment has been minimized.

Show comment Hide comment
@bobmaerten

bobmaerten Apr 17, 2013

Contributor

These substition seems not to work with basic shell (/bin/sh) from which it is call (/lib/functions.zsh).

function upgrade_oh_my_zsh() should invoke /bin/bash instead /bin/sh

@bobmaerten

bobmaerten Apr 17, 2013

Contributor

These substition seems not to work with basic shell (/bin/sh) from which it is call (/lib/functions.zsh).

function upgrade_oh_my_zsh() should invoke /bin/bash instead /bin/sh

This comment has been minimized.

Show comment Hide comment
@aschrab

aschrab Apr 20, 2013

The substitution doesn't do what was claimed by the commit message in any shell. It will only escape the first space in the variable. Would need to use a double slash after the variable name to replace all of them.

And since the only use of the variable uses it in double quotes, the backslash isn't necessary and in my testing (with several different shells) actually causes the problem that the commit claims to solve.

The substitution would also do nothing to help if the current directory name contained other types of whitespace; although, admittedly, that would be exceedingly rare.

@aschrab

aschrab Apr 20, 2013

The substitution doesn't do what was claimed by the commit message in any shell. It will only escape the first space in the variable. Would need to use a double slash after the variable name to replace all of them.

And since the only use of the variable uses it in double quotes, the backslash isn't necessary and in my testing (with several different shells) actually causes the problem that the commit claims to solve.

The substitution would also do nothing to help if the current directory name contained other types of whitespace; although, admittedly, that would be exceedingly rare.

printf '\033[0;34m%s\033[0m\n' "Upgrading Oh My Zsh"
cd $ZSH
@@ -16,4 +17,4 @@ else
printf '\033[0;31m%s\033[0m\n' 'There was an error updating. Try again later?'
fi
-cd "$current_path"
+cd "$current_path"

2 comments on commit 32a42f2

@vishaltelangre

This comment has been minimized.

Show comment Hide comment
@vishaltelangre

vishaltelangre Apr 21, 2013

Yes, this commit causing the problems while upgrading.
The error it throws is ~/.oh-my-zsh/tools/upgrade.sh: 2: Bad substitution
I had to comment the current_path=${current_path/ /\\ } line to make it work.

Yes, this commit causing the problems while upgrading.
The error it throws is ~/.oh-my-zsh/tools/upgrade.sh: 2: Bad substitution
I had to comment the current_path=${current_path/ /\\ } line to make it work.

@marcusmueller

This comment has been minimized.

Show comment Hide comment
@marcusmueller

marcusmueller May 3, 2013

Contributor

@bobmaerten: Nope, it certainly should not be run in bash; while it is probable that a system having zsh has bash to, we cannot tell.
Adding bash as requirement for oh-my-zsh is indeed a bad idea.

@aschrab: See my pull request #1775

Contributor

marcusmueller replied May 3, 2013

@bobmaerten: Nope, it certainly should not be run in bash; while it is probable that a system having zsh has bash to, we cannot tell.
Adding bash as requirement for oh-my-zsh is indeed a bad idea.

@aschrab: See my pull request #1775

Please sign in to comment.