Shell escape continued

[carlolars]

• Improved shell_escape():
  • Arguments with invalid characters and spaces should not be quoted since the space cause them to be quoted elsewhere.
  • Long arguments (--argname) and flag arguments (-f) that have invalid characters are not quoted, instead a space i appended so they also get quoted elsewhere.
  • Other arguments are enclosed in single-quotes.
• Added integration tests using assert_cli.

54

[andy-5]

I quickly skimmed your changes. Regarding the cd to the current directory - this is due to #50. The working directory is not always set correctly by wsl.exe, so I think we should keep the cd.

[carlolars]

I always forget the interactive shell, but now I've added integration tests that run both interactive and non-interactive :)
Note that at least the integration tests must be run using one test thread since they modify environment variables and will hang if run in parallel.

I updated how the arguments are formatted, splitting it in two functions:

• format_argument_for_interactive_shell():
  • Quote entire argument if:
  • Not already quoted, assumes correctly quoted then.
  • Contains invalid characters (including space).
• format_argument_for_non_interactive_shell():
  • Strip all existing quotes since they would be inside any outer quote.
  • Append space character to '--arg' and '-X' arguments if they contain invalid characters but no space.
  • Quote any other argument that contain invalid characters bu no space.

Most of changes is updated tests so the change is not that intimidating.
(Sorry if I'm breaking any essential design patterns used when programming Rust, this is the first time I program rust, usually doing low level C or asm.)

[carlolars]

Is there a reason to not just use wsl bash -c" for non-interactive? It would make all the escaping and quoting of arguments the same for both interactive and non-interactive.

[andy-5]

Yes, the only reason for the non-interactive mode is to avoid starting a Bash process at all, because this significantly slows down wslgit. The default mode uses bash -ic because it is common to start tools like ssh-agent in Bash startup scripts like .bashrc. These tools might be required for git to work, and in some configurations they are only started for interactive shells.

[carlolars]

I just had to try it out...
I measured the execution time of wslgit.exe --version and wslgit.exe log -1 --pretty=format:abc using a script that looped 100 times for both the original non-interactive (wsl git ...) and wsl bash -c "git ...", and the difference was 13-17 ms.

But the code is a lot cleaner that what I have done in this branch, and there is no "add a space to quote" oddities, it's just formatting a valid command string for bash to interpret using normal quoting. I vote for taking the 15 ms penalty in favor of cleaner code 😃

I'll push my branch with the bash -c version.

---

Side note, I use BASH_ENV to run my .bashrc even for non-interactive shell and check if $- contains the i flag, like this:

[[ $- == *i* ]] && IS_INTERACTIVE_SHELL=1

This lets me select what parts of the .bashrc should be included or excluded depending on interactive/non-interactive.

[andy-5]

Yes, you are right. Running through bash is not the slow part, but executing all of .bashrc might be slow (which is what happens in interactive mode).

Therefore, I'll go with your much simpler solution from #76 and close this.