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

Fork patch cleanup 2 #98

Closed carlolars closed 4 years ago

carlolars commented 4 years ago

Hi,

This is a PR for #97,

I've cleaned up the changes I do for the Fork patch, separating it in its own file to keep main.rs as clean as possible. I understand if you think that this is out of scope for the wslgit project, if so then maybe the other commits in this PR can be interesting on their own.

(If you decide to accept this PR then might I suggest that you create a develop-branch first and merge this PR into that branch instead of the master, since I have updated the README which would make the instructions not compatible with the latest release.)

Stanzilla commented 4 years ago

Do you have a test version for this? Your last published release always fails to read the COMMITMESSAGE file here.

fatal: could not read log file '//wsl$/Ubuntu/home/stan/projects/work/platform-documentation/.git/COMMITMESSAGE': No such file or directory

carlolars commented 4 years ago

Do you have a test version for this? Your last published release always fails to read the COMMITMESSAGE file here.

fatal: could not read log file '//wsl$/Ubuntu/home/stan/projects/work/platform-documentation/.git/COMMITMESSAGE': No such file or directory

Not related to this, I created a new issue (#99) and have pushed a fix :)

Stanzilla commented 4 years ago

Ah perfect! Do you have a test version with this AND that fix included then? :)

andy-5 commented 4 years ago

I briefly looked at this PR and I think it would be good to include it into wslgit. The refactoring is also very welcome.

A few notes:

What do you think?

andy-5 commented 4 years ago

@Stanzilla FWIW an alpha build with just the fix for #99 is available here: https://ci.appveyor.com/project/andy-5/wslgit/build/artifacts (but I guess you also want the Fork changes, which are not yet included)

Stanzilla commented 4 years ago

Yup, installed Rust now though :) Thanks!

carlolars commented 4 years ago
  • The installation step seems useful (for automatically detecting git), but I'd like to document somewhere in the readme that it is totally optional [...]

Good point, the description originated from the fork-patch which was entirely focusing on working with Fork. I'll rephrase and make it clear that it is optional and only required for when using with Fork.

  • Also, I feel like the added bash.exe and sh.exe files may conflict with other tools if they are added to the Path [...]

True. I'll change the file structure into two folders, similar to the structure of Git for Windows which has one bin and one cmd directory, where the cmd directory is what should be added to the path:

├── wslgit
│   ├── bin
│   │   ├── bash.exe (symlink) -> wsl.exe
│   │   ├── git.exe (symlink) -> ../cmd/wslgit.exe
│   │   └── sh.exe (symlink) -> wsl.exe
│   ├── cmd
│   │   ├── git.exe (symlink) -> wslgit.exe
│   │   └── wslgit.exe
│   └── install.bat
  • Maybe the installation could/should automate adding the directory to Path, i.e. with setx?

I think it should be manually added. Will avoid automatically adding multiple entries in the Path if the script is run more than once, and will avoid the responsibility of not screwing up the Path.

  • I see you link to the WSL bash.exe from your installation, but according to the WSL docs this executable is deprecated. Can it be replaced with wsl.exe?

The problem with wsl.exe is that if you pass -c command, as an application might do when using bash.exe, then wsl.exe responds with Invalid command line option: -c.
But after some testing it seems like Fork cannot use bash.exe nor wsl.exe, I suspect that it has something to do with fork beeing a 32-bit process and the others beeing 64-bit binaries, but as long as the files exists parallell to git.exe then Fork is happy. Funny that I haven't noticed this after using Fork for so long time 😄 But this is a Fork issue.

carlolars commented 4 years ago

I pushed the changes according to the feedback and my comment above.

carlolars commented 4 years ago

Hi, any progress on this? Feel free to reword if you think it is necessary.

Stanzilla commented 4 years ago

I would love to help test more but currently blocked on https://github.com/andy-5/wslgit/issues/71

andy-5 commented 4 years ago

Sorry for the delay, everything looks good to me. Thanks for your work.