Fix issue with PATH if bundle directory contains spaces

[JohnStarich]

I noticed that my PATH variable had become mangled when sourcing antigen. It took a while but I think I found the causes and addressed them in this pull request.

• Antigen version: develop (v2.2.1)
• zsh version: zsh 5.2 (x86_64-apple-darwin16.0)
• macOS version: 10.12.5 (16F73) "Sierra"
• link location of ~/.antigen/bundles: /Users/johnstarich/Library/Mobile Documents/com~apple~Automator/Documents/dotfiles/zsh/bundles
• expectation: PATH should add antigen bundles normally
• actual result: PATH is appended with multiple paths for a single bundle if that bundle's directory contains spaces
• steps to reproduce are included in the form of a test

If I am missing information about my scenario or need to add robustness to the tests, please let me know!

Problem

I store my antigen repositories in iCloud Drive to make use of them on both of my Macs, and I just symlink them from the home directory. Unfortunately, iCloud Drive's file path includes a space, so the PATH variable started looking like this:

PATH=/Users/johnstarich/.dotfiles/bin:/Users/johnstarich/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/Users/johnstarich/Library/Mobile:Documents/com~apple~Automator/Documents/dotfiles/zsh/bundles/robbyrussell/oh-my-zsh/lib:Documents/com~apple~Automator/Documents/dotfiles/zsh/bundles/zsh-users/zsh-syntax-highlighting:Documents/com~apple~Automator/Documents/dotfiles/zsh/bundles/zsh-users/zsh-history-substring-search:Documents/com~apple~Automator/Documents/dotfiles/zsh/bundles/zsh-users/zsh-autosuggestions

The important bit is in /Users/johnstarich/Library/Mobile:Documents/com~apple~Automator/Documents/dotfiles/zsh/bundles/robbyrussell/oh-my-zsh/lib where Mobile and Documents are separated by a space in the bundle's path name, but the space created two path segments instead of one.

Solution

This addresses the spaces issue by exporting PATH and fpath more accurately by using typeset -p VAR instead of just using variable substitution path+=($_PATH).

[JohnStarich]

It looks like older versions of zsh (<5.1.1) are failing with the same error. I'm not sure what's wrong, but I'll take another look at this later.

[desyncr]

@JohnStarich Thanks for the contribution!

I'll take a look tonight and see what's going on with zsh 5.1.1.

[JohnStarich]

I solved the problem, but I am at a loss as to why it worked. Perhaps older zsh versions cannot run subshells in the same line as local variable declarations?

[desyncr]

Glad you found out the issue!

Now, there's definitely something going on with locals on older zsh versions. I believe there is a commit with a zsh-workers thread somewhere where the issue is discussed.

I'm gonna test it a bit and merge right away! Thanks again for the contribution! 👍

[desyncr]

Please review this small issue with themes.

[desyncr]

This causes issues with themes:

Installing desyncr/void void btype... Error! Activate logging and try again.
$ 

As antigen-theme calls antigen-bundle with --btype=theme.

[JohnStarich]

If I change this back to ${=@} then it breaks the test I added because the spaces get turned into different arguments, unfortunately. Is there another way to parse args after they've been send to -antigen-parse-args?

I can also look into sending the arguments a different way if you like

[JohnStarich]

I'm not super familiar with how themes work. Do you have a test case that fails I could debug?

[desyncr]

Sorry for the slow responses.

antigen theme desyncr/void void. But I believe any theme should fail to load, the tricky part is it doesn't fail in tests.

I'm gonna try to give it a look today. Let me know if you found anything.

[JohnStarich]

I may have fixed the problem, but I'm not entirely sure. My running theory is that if you used the newish parallel antigen-bundles stuff it would not handle spaces correctly. I then told it to split each line of the heredoc on spaces. It appears that splitting on spaces here was overlooked since most bundles do not contain spaces (but themes do).

I also updated the PR's primary comment with how this solution handles the spaces splitting.

Let me know if this fixed the issue for you @desyncr!

[JohnStarich]

I found a far simpler solution. The issue turned out to be in the cache!

How does this look instead @desyncr?

[desyncr]

Awesome. That would explain why the issue doesn't come up in the tests.

Thanks for keeping up with this and sorry I couldn't be more helpful but I'm currently very busy.

I'll take a look tomorrow.

[desyncr]

Well, I'm taking a look right now and it seems it's a fundamental issue with how quoted args are passed between functions. You see when you do antigen bundle <args> these arguments are stored and passed away between multiple functions, including extensions and internal functions.

What I found is happening is that as soon as these arguments are handled by Antigen quotes are missing. One of these places is this one.

_DEFERRED_BUNDLE+=("${(j: :)${@}}")

In doing that it missing the original quoting, ie:

antigen bundle "/path/to/bundle with spaces"

Turns into:

antigen bundle /path/to/bundle with spaces

This happens in multiple places as this is saved in a structure that looks like this:

name : to/bundle
url      : /path/to/bundle with spaces
dir      : /path/to/bundle with spaces
loc      : hr-plugin
btype  : plugin
make_local_clone: true

This structure is converted to a key-value argument like this:

${(kv)bundle}

Also losing quoting, if any.

This one also seems to be causing issues.

I'll keep working on it a bit. But it seems it's a fundamental issue that needs to be addressed in a more broad perspective.

[JohnStarich]

Yes, that is along the lines of what I found. I think that is outside the scope of this PR, but I agree that could be worth fixing in the long-term. My total diff now only includes the fix to the cache.

If it helps, you can pass unquoted strings with spaces by escaping each space like so:

/this/path\ has\ spaces/

The biggest problems I saw were caused by the antigen-bundles command because it reads each line as a string. This caused issues I tried fixing in the parallel bundle installer and in the argument passing done for antigen-bundle.

Would you like to collaborate on a new issue/PR that tries to fix all of the spaces bugs?

[desyncr]

@JohnStarich Sure thing I'm taking a look at this. Changes probably will fix it long-term.

Please leave this PR open as to keep in on the radar. I'll open another one where we can work on.

[ryannielsen]

I randomly discovered this issue today while poking at some other aspects of my zsh setup, and likewise keep a subset of my dotfiles in iCloud Drive which leads to paths which include "Mobile Documents".

In trying to fix this, I spent hours playing with zsh array parameter expansion with no luck and, in the end, went with a much simpler approach: I simply append double quote delineated strings to the $_PATH and $_fpath variables, and then inject those strings into the cache output. There's no need to do anything fancier.

Only after settling on my approach did I find this PR. I'm far from a zsh expert, but I am fairly certain the resulting cached output won't have any paths.

In bin/antigen.zsh,

fpath+=(${_fpath[@]}) path+=(${_PATH[@]})

is replaced with

fpath+=(\$_fpath) path+=(\$_PATH)

and I'm not sure how this will work. It's important to remember that the code under consideration is in a heredoc, and is not actually being evaluated by the current zsh subshell. Thus, rather than including path values from the parent script, the heredoc will instead just contain

fpath+=($_fpath) path+=($_PATH)

You can confirm this by looking at the resulting $ADOTDIR/init.zsh file; the processed contents of the heredoc.

Likewise, there's no need to unset $_PATH or $_fpath, because those are never defined in the heredoc text.

I have a commit which seems to work properly in my very limited testing; happy to raise a new PR if that's appropriate: ryannielsen@3a7206b