Skip to content
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

More robust resetting of the shell. #988

Merged
merged 2 commits into from
Aug 2, 2016
Merged

More robust resetting of the shell. #988

merged 2 commits into from
Aug 2, 2016

Conversation

arp242
Copy link
Contributor

@arp242 arp242 commented Aug 1, 2016

Don't reset it on Windows, and only reset it if /bin/sh is executable (which
should pretty much always be the case, but it's not in POSIX and IIRC POSIX
explicitly advises against relying on it).

This is basically the same method that vim-plug uses.

This improves #967 and should fix #986.

Don't reset it on Windows, and only reset it if `/bin/sh` is executable (which
should pretty much always be the case, but it's not in POSIX and IIRC POSIX
explicitly advises against relying on it).

This is basically the same method that vim-plug uses.

This improves #967 and should fix #986.
let l:output = call(s:vim_system, [a:str] + a:000)
let &shell = l:shell
return l:output
if !(has('win32') || has('win64')) && executable('/bin/sh')
Copy link
Owner

Choose a reason for hiding this comment

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

Use go#util#IsWin() here

@fatih
Copy link
Owner

fatih commented Aug 1, 2016

Maybe this is just easier (I was going to push it now, but you opened first :)):

function! go#util#System(str, ...)
  if go#util#IsWin()
    let l:output = call(s:vim_system, [a:str] + a:000)
    return l:output
  endif

  let l:shell = &shell
  let &shell = '/bin/sh'
  let l:output = call(s:vim_system, [a:str] + a:000)
  let &shell = l:shell
  return l:output
endfunction

return l:output
finally
let &shell = l:shell
endtry
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not sure why we have this try clause. Why not just :

let l:output = call(s:vim_system, [a:str] + a:000)
let &shell = l:shell
return l:output

@arp242
Copy link
Contributor Author

arp242 commented Aug 1, 2016

Well, in theory there could be systems that don't have /bin/sh... Those systems are probably very rare, but there's nothing wrong with being paranoid and doing an extra check can't hurt ;-)

The try/finally construct is better in case there are errors. Usually functions continue running commands unless the abort keyword is given, so not re-setting the shell back to l:shell should never happen ... But are you sure there are no situations where Vim doesn't stop running the function on errors? Using finally should guarantee this more...

@fatih
Copy link
Owner

fatih commented Aug 1, 2016

But are you sure there are no situations where Vim doesn't stop running the function on errors? Using finally should guarantee this more...

So you mean call() might return an error , and in that case we're not able to set the shell back?

@arp242
Copy link
Contributor Author

arp242 commented Aug 1, 2016

But are you sure there are no situations where Vim doesn't stop running the function on errors? Using finally should guarantee this more...

So you mean call() might return an error , and in that case we're not able to set the shell back?

Well, more like the system() function, but yeah.

Like I said, I'm not sure if this really makes a difference... Both versions are probably fine.

@fatih fatih merged commit 011d76f into fatih:master Aug 2, 2016
@fatih
Copy link
Owner

fatih commented Aug 2, 2016

Thanks @Carpetsmoker

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.

go#util#System is broken on Windows
2 participants