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

Format arguments #76

Merged
merged 6 commits into from
Jul 4, 2019
Merged

Format arguments #76

merged 6 commits into from
Jul 4, 2019

Conversation

carlolars
Copy link
Contributor

Alternative solution for PR #75

… can be used for both non-interactive and interactive shell.

The git_cmd that is used as argument for 'bash -c' is a string that is interpreted by bash, so quoting the arguments is straight forward.
…ces.

For example VSCode use its installation directory as cwd when doing the initial 'git --version' check, and if that directory contains spaces then the check fails to find git.
@andy-5 andy-5 mentioned this pull request Jul 3, 2019
@andy-5
Copy link
Owner

andy-5 commented Jul 3, 2019

Thanks for your work on this, and for expanding the test cases.

I like that this approach unifies some of the code and simplifies testing, and it is almost as fast as the current solution. But I noticed a few things.

Other than that, I think this PR looks good and I'm looking forward to merging it.

@andy-5
Copy link
Owner

andy-5 commented Jul 3, 2019

For escaping/quoting, maybe we should try the shell-escape crate (https://crates.io/crates/shell-escape) that was already mentioned in #54, instead of implementing our own solution. It has a shell_escape::unix::escape() function that seems to do what we need. A quick look at the source suggests that it quotes every string with special characters in single quotes '.

@carlolars
Copy link
Contributor Author

Yeah the double execution of .bashrc is annoying but I'd say it is working as expected and its not a wslgit-problem. Doing for example BASH_ENV=~/.bashrc bash -c "bash -c \"echo hello\"" on a real linux system would execute .bashrc twice, and that is essentially the same as doing wsl.exe bash -c "echo hello". The unset trick is neat.

I dismissed the example with the ` character as that command is not working inside wsl or in linux because it is not complete since it expects a matching `.

I'll add the documentation, don't know how I missed it...

@andy-5
Copy link
Owner

andy-5 commented Jul 4, 2019

Ah, yes, the ` character not working in Linux/WSL is a valid point. It is probably not a problem other than for constructed toy examples.

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