Skip to content

allow a relative symlink to vimpager to work again#193

Merged
rkitover merged 2 commits intorkitover:masterfrom
SethMilliken:relative-symlink-fx
Jun 15, 2016
Merged

allow a relative symlink to vimpager to work again#193
rkitover merged 2 commits intorkitover:masterfrom
SethMilliken:relative-symlink-fx

Conversation

@SethMilliken
Copy link
Copy Markdown
Contributor

When using a relative symlink (e.g. ln -s ../.vim/bundle/vimpager/vimpager vp)
to invoke vimpager or vimcat, the path to prologue.sh was being
calculated relative to the symlink file itself rather than to the actual
location of script. Also, we only need to perform this resolution on
symlinks (i.e. while [ -h "$link" ]; do).

(this worked prior to 1126ee)

When using a relative symlink (e.g. `ln -s ../.vim/bundle/vimpager/vimpager vp`)
to invoke `vimpager` or `vimcat`, the path to `prologue.sh` was being
calculated relative to the symlink file itself rather than to the actual
location of script. Also, we only need to perform this resolution on
symlinks (i.e. `while [ -h "$link" ]; do`).

(this worked prior to 1126ee)
vimcat Outdated
if expr "$new_link" : '/.*' > /dev/null; then
link="$new_link"
else
link="$(dirname $link)/$new_link"
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please do not use POSIX command substitution at this point (before sourcing prologue.sh). Only backticks. And please quote $link.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you think it's worth adding a comment about that POSIX command substitution restriction?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No. I think that should be mentioned somewhere more general. Somewhere in the docs. There are already comments in inc/prologue.sh about being POSIX after sourcing it.

@rkitover and I had some longer discussions and clarifications about such topics in other issues recently. Mostly #149 and #159

This was referenced Jun 9, 2016
- command subtitution: replace POSIX notation with backtick notation
- quotes: remove superfluous ones, add necessary ones
lucc added a commit that referenced this pull request Jun 10, 2016
@lucc
Copy link
Copy Markdown
Collaborator

lucc commented Jun 10, 2016

I added a test to #194 that tests the described regression. The test fails on #194 and succeds with this patch here. So I'd say lets merge it as soon as the comments above are fixed.

@SethMilliken
Copy link
Copy Markdown
Contributor Author

@lucc Comments addressed. Let me know if there's anything else I should do (e.g. squash commits). Thanks for your help!

@lucc
Copy link
Copy Markdown
Collaborator

lucc commented Jun 14, 2016

I don't care wether you squash them or not.

@rkitover From my point of view this can be merged. Also see my comment about tests above.

@rkitover rkitover merged commit 23426cf into rkitover:master Jun 15, 2016
@rkitover
Copy link
Copy Markdown
Owner

@SethMilliken thank you very much for this.

@SethMilliken
Copy link
Copy Markdown
Contributor Author

@rkitover Thank you (and @lucc) very much for continued vimpager improvements!

lucc added a commit that referenced this pull request Jun 16, 2016
lucc added a commit that referenced this pull request Jun 19, 2016
lucc added a commit that referenced this pull request Jun 20, 2016
lucc added a commit that referenced this pull request Jun 20, 2016
lucc added a commit that referenced this pull request Jun 20, 2016
lucc added a commit that referenced this pull request Jun 20, 2016
lucc added a commit that referenced this pull request Jun 20, 2016
lucc added a commit that referenced this pull request Jun 21, 2016
lucc added a commit that referenced this pull request Jun 22, 2016
lucc added a commit that referenced this pull request Jun 23, 2016
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