Format arguments - Githubissues

andy-5 / wslgit

Use Git installed in Bash on Windows/Windows Subsystem for Linux (WSL) from Windows and Visual Studio Code (VSCode)

MIT License

1.19k stars 59 forks source link

Format arguments #76

Closed carlolars closed 5 years ago

carlolars commented 5 years ago

Alternative solution for PR #75

andy-5 commented 5 years ago

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.

When you have BASH_ENV=~/.bashrc and WSLENV=BASH_ENV, .bashrc gets executed twice, similar to https://github.com/microsoft/WSL/issues/2897, but also using wsl.exe. But you can workaround this by adding unset BASH_ENV to .bashrc, so I think it's not a big problem.
The following example from #54 does not work, it seems the ` character needs to be escaped:
```
wslgit.exe config --get-regex "user.(name|email|`\$&)"
```
And can you please add the documentation on how to run the tests from #75 to this PR?

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

andy-5 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.