Skip to content

Conversation

mhinz
Copy link
Member

@mhinz mhinz commented Oct 18, 2016

In this line, we compare the unresolved path written to the manifest file against a resolved path from the outer for loop.

Without this patch, :CheckHealth nvim will always report an outdated manifest, because:

/Users/mhi/.vim/bundle/deoplete.nvim/rplugin/python3/deoplete
!=
/Users/mhi/.dotfiles/vim/bundle/deoplete.nvim/rplugin/python3/deoplete

@marvim marvim added the RFC label Oct 18, 2016
@@ -190,6 +190,7 @@ function! s:RegistrationCommands(host) abort
call remote#host#RegisterClone(host_id, a:host)
let pattern = s:plugin_patterns[a:host]
let paths = globpath(&rtp, 'rplugin/'.a:host.'/'.pattern, 0, 1)
let paths = map(paths, 'resolve(v:val)')
let paths = map(paths, 'tr(v:val,"\\","/")') " Normalize slashes #4795
Copy link
Member

Choose a reason for hiding this comment

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

can you add the resolve() call in here? the "let paths" is getting a bit too verbose

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

`:CheckHealth nvim` would always report an outdated manifest if symlinks were
used, because the manifest file contains unresolved paths that get compared
against resolved paths.

Now we resolve paths before they get written to the manifest file.
@mhinz mhinz force-pushed the rplugin/manifest-resolve branch from 8a76321 to b0d0b7d Compare October 18, 2016 13:32
@tweekmonster
Copy link
Contributor

Thanks for doing this. I forgot to submit a PR when I mentioned this in Gitter. My approach was going to be not resolving the paths and get an absolute path instead. I thought it'd be better to show the paths as seen in the configs since filereadable() and readfile() would still succeed even if the path involved a symlink.

@mhinz
Copy link
Member Author

mhinz commented Oct 18, 2016

I'm rather passionless about the approach. Not resolving symlinks would probably work just as well, since all the functions involved don't implicitly resolve on their own. Just mixing them is a no-go. :-P

Then again, I don't think it makes a big difference, since users won't see it anyway.

@tweekmonster
Copy link
Contributor

I don't think it's a big deal as long as they comparable paths. I mentioned it in case it sounds significant to someone. I'm sorry about your lack of passion, though. My passion about the approach is burning hot and very sexy.

@justinmk
Copy link
Member

Not to belabor this, but if there's no reason that resolve() was called in the first place, it would be less surprising and less prone to bugs if we removed it everywhere. But I thought there was a reason we added that resolve() at one time. So let's keep it I guess.

@justinmk justinmk merged commit 657ba62 into neovim:master Oct 18, 2016
@justinmk justinmk removed the RFC label Oct 18, 2016
@tweekmonster
Copy link
Contributor

Not to belabor this, but if there's no reason that resolve() was called in the first place, it would be less surprising and less prone to bugs if we removed it everywhere.

This was the gist of my reasoning.

But I thought there was a reason we added that resolve() at one time.

There actually isn't one. It was taken verbatim from nvim-python-doctor: tweekmonster/nvim-python-doctor@39ff0c4. My vague recollection of using resolve() is that I wanted $MYVIMRC to be resolved for sanity while it was in dev.

@mhinz mhinz deleted the rplugin/manifest-resolve branch October 19, 2016 00:39
justinmk added a commit to justinmk/neovim that referenced this pull request Oct 27, 2016
FEATURES:
0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu
c6ac4f8 neovim#4934 API: call any API method from vimscript
31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request
b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number()
e7e2844 has("nvim-1.2.3") checks for a specific Nvim version
522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance
719dae2 neovim#5384 events: allow event processing in getchar()
f25797f neovim#5386 API: metadata: Nvim version & API level
22dfe69 neovim#5389 API: metadata: "since", "deprecated_since"
605e743 Added QuickFixLine highlight group

CHANGES:
4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline()
6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode.
9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods.
eeec0ca neovim#5419 tui: Default to normal-mode cursor shape.

FIXES:
e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes"
10a54ad neovim#5243 signal_init: Always unblock SIGCHLD.
bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job
626065d neovim#5227 tchdir: New tab should inherit CWD.
cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid.
6127eae shada: Fix non-writeable ShaDa directory handling
ca65514 neovim#2789 system(): Respect shellxescape, shellxquote
2daf54e neovim#4874 Restore vim-like tab dragging
0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names.
3c53371 neovim#4972 from justinmk/schedule-ui_refresh
68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown.
c8b6ec2 neovim#5409 v:count broken in command-line window
6bc3bce neovim#5461 fix emoji display
51937e1 neovim#5470 fix :terminal with :argadd, :argu
79d77da neovim#5481 external UIs: opening multiple files from command-line
657ba62 neovim#5501 rplugin: resolve paths in manifest file
6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash.
1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK
2a6c5bb neovim#5450 modeline: Handle version number overflow.
0ade1bb neovim#5225 CI tests now run against Windows!
justinmk added a commit to justinmk/neovim that referenced this pull request Oct 28, 2016
FEATURES:
0b5a7e4 neovim#4432 API: external UIs can render custom popupmenu
c6ac4f8 neovim#4934 API: call any API method from vimscript
31df051 neovim#4568 API: nvim_call_atomic(): multiple calls in a single request
b268ba3 neovim#5424 API: nvim_win_get_number(), nvim_tabpage_get_number()
e7e2844 has("nvim-1.2.3") checks for a specific Nvim version
522b885 neovim#5295, neovim#5493 `:CheckHealth` checks tmux, terminfo, performance
719dae2 neovim#5384 events: allow event processing in getchar()
f25797f neovim#5386 API: metadata: Nvim version & API level
22dfe69 neovim#5389 API: metadata: "since", "deprecated_since"
605e743 Added QuickFixLine highlight group

CHANGES:
4af6ec7 neovim#5253 perf: Disable clipboard in do_cmdline()
6e9f329 neovim#5299 perf: Skip foldUpdate() in insert-mode.
9d4fcec neovim#5426 perf: Do not auto-update folds for some foldmethods.
eeec0ca neovim#5419 tui: Default to normal-mode cursor shape.

FIXES:
e838452 neovim#5436 tui: Fix "weird characters" / "bleeding termcodes"
10a54ad neovim#5243 signal_init: Always unblock SIGCHLD.
bccb49b neovim#5316 eval.c: Fix memory leak for detached pty job
626065d neovim#5227 tchdir: New tab should inherit CWD.
cd321b7 neovim#5292 getcwd(): Return empty string if CWD is invalid.
6127eae shada: Fix non-writeable ShaDa directory handling
ca65514 neovim#2789 system(): Respect shellxescape, shellxquote
2daf54e neovim#4874 Restore vim-like tab dragging
0c536b5 neovim#5319 syntax.c: Support bg/fg special color-names.
3c53371 neovim#4972 from justinmk/schedule-ui_refresh
68bcb32 neovim#4789 tui.c: Do not wait for tui loop on teardown.
c8b6ec2 neovim#5409 v:count broken in command-line window
6bc3bce neovim#5461 fix emoji display
51937e1 neovim#5470 fix :terminal with :argadd, :argu
79d77da neovim#5481 external UIs: opening multiple files from command-line
657ba62 neovim#5501 rplugin: resolve paths in manifest file
6a6f188 neovim#5502 system('foo &', 'bar'): Show error, don't crash.
1ff162c neovim#5515 os_nodetype: open fd with O_NONBLOCK
2a6c5bb neovim#5450 modeline: Handle version number overflow.
0ade1bb neovim#5225 CI tests now run against Windows!
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.

4 participants