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.18k stars 59 forks source link

Using wslpath for translating absolute paths Win<->WSL #86

Closed carlolars closed 4 years ago

carlolars commented 4 years ago

I've implemented using wslpath to translate win-paths to wsl-paths and vice versa.

The translation from windows to wsl path is as fast as the previous implementation, but the wsl to windows path translation gets a ~60ms penalty because of one extra process invoke of the output from the git command.

Benchmark results:

Before wslpath:
test bench::no_translation              ... bench:  60,846,590 ns/iter (+/- 11,856,476)
test bench::translate_absolute_argument ... bench:  71,600,430 ns/iter (+/- 2,973,202)
test bench::translate_output            ... bench:  56,521,180 ns/iter (+/- 3,329,437)
test bench::translate_relative_argument ... bench:  62,603,140 ns/iter (+/- 3,727,496)

Using wslpath:
test bench::no_translation              ... bench:  60,685,500 ns/iter (+/- 13,579,360)
test bench::translate_absolute_argument ... bench:  69,877,700 ns/iter (+/- 12,559,499)
test bench::translate_output            ... bench: 110,932,150 ns/iter (+/- 4,230,732)
test bench::translate_relative_argument ... bench:  61,110,610 ns/iter (+/- 1,364,392)
andy-5 commented 4 years ago

I have not looked at it in detail yet, but generally this looks great! Thanks for working on this.

Maybe we should document the new dependency on sed. I'm not sure if it is available in default installations of minimal distros like e.g. Alpine.

carlolars commented 4 years ago

What do you think about substitution using shell parameter expansion instead of sed? Would that be safer?
${parameter/pattern/string}

carlolars commented 4 years ago

Alright, I've removed sed, removed one annoying newline from the log-file and fixed an oversight from me; lower case drive letters are valid.

andy-5 commented 4 years ago

Great! Having no external dependency (besides bash and wslpath) is even better.

andy-5 commented 4 years ago

I relased the previous code as v0.8 (just to have a snaptshot of that state) and merged your changes. I plan to release a v0.9 with these changes soon.

Many thanks for your work.

carlolars commented 4 years ago

Great! I have one other small thing that I've worked on, I'll push a new PR later today for you to consider if you'd like to have it.

Den fre 11 okt. 2019 11:41A. R. S. notifications@github.com skrev:

I relased the previous code as v0.8 (just to have a snaptshot of that state) and merged your changes. I plan to release a v0.9 with these changes soon.

Many thanks for your work.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/andy-5/wslgit/pull/86?email_source=notifications&email_token=ADI7YAWPFDO72HXJQFOKJS3QOBC5RA5CNFSM4I6WJE6KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA7O3RQ#issuecomment-540995014, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADI7YAVCTZRPPDHLEGUTW2LQOBC5RANCNFSM4I6WJE6A .

IgnusG commented 4 years ago

I'm a bit confused about how to make use of the new root options. The new v8.0 does not work out of the box for wsl machines configured with / as root. Is there additional setup needed?

The release mentions that WSLGIT_MOUNT_ROOT was added but the changelog says this was removed and wslpath handles this now.

Using the old v7.0 wslgit-mount-at-root.exe is working as expected.

andy-5 commented 4 years ago

@IgnusG v0.8 requires setting WSLGIT_MOUNT_ROOT, this PR introduced the usage of wslpath which will be available in the upcoming v0.9.

IgnusG commented 4 years ago

Ohhhh, thanks for the clarification :)