New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

man.vim: Fix filename argument in mandoc #6693

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@m-wynn
Contributor

m-wynn commented May 5, 2017

This uses the -l flag to open a man file. While this does not work with
SunOS, it does not appear that man.vim worked with SunOS in the first place.

Fixes #6683

@jamessan

This comment has been minimized.

Member

jamessan commented May 5, 2017

Cc @nhooyr

@@ -135,7 +135,7 @@ function! s:get_page(path) abort
let manwidth = empty($MANWIDTH) ? winwidth(0) : $MANWIDTH
" Force MANPAGER=cat to ensure Vim is not recursively invoked (by man-db).
" http://comments.gmane.org/gmane.editors.vim.devel/29085
return s:system(['env', 'MANPAGER=cat', 'MANWIDTH='.manwidth, 'man', a:path])
return s:system(['env', 'MANPAGER=cat', 'MANWIDTH='.manwidth, 'man', '-l', a:path])

This comment has been minimized.

@nhooyr

nhooyr May 6, 2017

Contributor

man -l does not work on macOS. Since both man-db and mandoc support this flag fine, we just need to add a special case to not send the flag on macOS.

Aside from that, I'm not aware of any other manpage distributions that this flag should be tested on. So we should be good.

This comment has been minimized.

@m-wynn

m-wynn May 6, 2017

Contributor

I do not have a mac to test this on, but this should work

This comment has been minimized.

@nhooyr

nhooyr May 6, 2017

Contributor

I'm on macOS right now.

screen shot 2017-05-06 at 2 52 56 am

This comment has been minimized.

@m-wynn

m-wynn May 6, 2017

Contributor

sorry, I meant the diff I just pushed

This comment has been minimized.

@nhooyr

nhooyr May 6, 2017

Contributor

Ah, ok.

@@ -135,7 +135,15 @@ function! s:get_page(path) abort
let manwidth = empty($MANWIDTH) ? winwidth(0) : $MANWIDTH
" Force MANPAGER=cat to ensure Vim is not recursively invoked (by man-db).
" http://comments.gmane.org/gmane.editors.vim.devel/29085
return s:system(['env', 'MANPAGER=cat', 'MANWIDTH='.manwidth, 'man', a:path])
try
if !has('win32') && system('uname -s') =~ 'Darwin'

This comment has been minimized.

@justinmk

justinmk May 6, 2017

Member

if has('mac') should be enough.

@@ -135,7 +135,15 @@ function! s:get_page(path) abort
let manwidth = empty($MANWIDTH) ? winwidth(0) : $MANWIDTH
" Force MANPAGER=cat to ensure Vim is not recursively invoked (by man-db).
" http://comments.gmane.org/gmane.editors.vim.devel/29085
return s:system(['env', 'MANPAGER=cat', 'MANWIDTH='.manwidth, 'man', a:path])
try
if !has('win32') && system('uname -s') =~ 'Darwin'

This comment has been minimized.

@nhooyr

nhooyr May 6, 2017

Contributor

There is no need for the win32 check or the try statement, you can remove them. Also, please change the =~ to #=~.

edit: just do what @justinmk said and change the system('uname -s') =~ 'Darwin' to has('mac')

return s:system(['env', 'MANPAGER=cat', 'MANWIDTH='.manwidth, 'man', '-l', a:path])
endif
catch /E145:/
" Restricted mode

This comment has been minimized.

@justinmk

justinmk May 6, 2017

Member

What's the effect of this? Showing an error is better than silently doing nothing.

This comment has been minimized.

@m-wynn

m-wynn May 6, 2017

Contributor

I copied that from line 12.

@nhooyr

nhooyr approved these changes May 6, 2017

@m-wynn

This comment has been minimized.

Contributor

m-wynn commented May 6, 2017

Addressed those comments

@@ -135,7 +135,11 @@ function! s:get_page(path) abort
let manwidth = empty($MANWIDTH) ? winwidth(0) : $MANWIDTH
" Force MANPAGER=cat to ensure Vim is not recursively invoked (by man-db).
" http://comments.gmane.org/gmane.editors.vim.devel/29085
return s:system(['env', 'MANPAGER=cat', 'MANWIDTH='.manwidth, 'man', a:path])
if has('mac')
return s:system(['env', 'MANPAGER=cat', 'MANWIDTH='.manwidth, 'man', a:path])

This comment has been minimized.

@nhooyr

nhooyr May 6, 2017

Contributor

actually, one more thing. can you change the above to:

  " Force MANPAGER=cat to ensure Vim is not recursively invoked (by man-db).
  " http://comments.gmane.org/gmane.editors.vim.devel/29085
  let cmd = ['env', 'MANPAGER=cat', 'MANWIDTH='.manwidth, 'man']
  if has('mac')
    return s:system(cmd + [a:path])
  else
    return s:system(cmd + ['-l', a:path])
  endif

This will more clearly present the code that the comment is related to.

man.vim: Fix filename argument in mandoc
This uses the -l flag to open a man file.  While this does not work with
SunOS, it does not appear that man.vim worked with SunOS in the first place.

Fixes #6683
@nhooyr

nhooyr approved these changes May 6, 2017

@justinmk

This comment has been minimized.

Member

justinmk commented May 8, 2017

Merged, thanks everyone

@justinmk justinmk closed this May 8, 2017

justinmk added a commit that referenced this pull request May 8, 2017

man.vim: Fix filename argument in mandoc #6693
Use the -l flag to open a man file.
TODO: Does not work on SunOS.

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