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

preserve shellcmdflag and normalize for /bin/sh #2006

Merged
merged 1 commit into from
Oct 4, 2018

Conversation

mirza-s
Copy link
Contributor

@mirza-s mirza-s commented Oct 3, 2018

Fix shell related problems when shellcmdflag contains flags not understood by /bin/sh by resetting it to a safe value and restoring it at the end.

as it may contain flags not understood by /bin/sh
@mirza-s
Copy link
Contributor Author

mirza-s commented Oct 3, 2018

Related #988

@bhcleek
Copy link
Collaborator

bhcleek commented Oct 3, 2018

Can you give a concrete example of what this fixes for you?

@mirza-s
Copy link
Contributor Author

mirza-s commented Oct 3, 2018

On Ubuntu 18.04 if you set shellcmdflag=-lc it breaks all operations that use the shell such as gofmt.
So you can't even save *.go files because gofmt is called before each save.

Error detected while processing function <SNR>24_fmt_autosave[3]..go#fmt#Format[41]..go#fmt#run[5]..go#util#Exec[17]..<SNR>41_exec[7]..<SNR>41_system:
line   10:
E484: Can't open file /tmp/vcxetHD/1
Error detected while processing function <SNR>24_fmt_autosave:
line    3:
E171: Missing :endif
Press ENTER or type command to continue

This is because /bin/sh does not understand -lc (on Ubuntu sh is linked to dash)

$ sh -lc echo foo
foo: 66: foo: Syntax error: "(" unexpected

This does not happen on OSX because sh is actually bash which understands the provided shellcmdflag

So we reset shellcmdflag to -c which is understood by sh and restore it after invocation.

@bhcleek
Copy link
Collaborator

bhcleek commented Oct 3, 2018

Given that vim-go does not set shellcmdflag at all, it sounds like this is better taken care of in your vimrc than in vim-go.

@mirza-s
Copy link
Contributor Author

mirza-s commented Oct 3, 2018

But it replaces the shell that uses shellcmdflag.

@mirza-s
Copy link
Contributor Author

mirza-s commented Oct 3, 2018

Why was 881a9cc needed at all?

@bhcleek
Copy link
Collaborator

bhcleek commented Oct 3, 2018

lgtm

@bhcleek bhcleek merged commit 5971612 into fatih:master Oct 4, 2018
@bhcleek
Copy link
Collaborator

bhcleek commented Oct 4, 2018

Thank you for the contribution.

bhcleek added a commit that referenced this pull request Oct 4, 2018
@mirza-s
Copy link
Contributor Author

mirza-s commented Oct 4, 2018

Thank you.

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.

2 participants