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

Clean up Fork.RI and fix Windows short paths usage #126

Merged
merged 5 commits into from
May 14, 2022
Merged

Clean up Fork.RI and fix Windows short paths usage #126

merged 5 commits into from
May 14, 2022

Conversation

yannhodiesne
Copy link
Contributor

@yannhodiesne yannhodiesne commented Apr 1, 2022

While investigating around issue #124, I found out a lot of errors were thrown out of the Fork.RI script:

  • when using WSL2, uname -a does not contain the WSL build version anymore, making the current check for build 17046 or greater fail with a syntax error because of the format of the regex
  • when using Windows short paths to configure Fork's custom git instance (e.g. C:\PROGRA~1 instead of C:\Program Files to avoid spaces), the FORK_RI_EXE_PATH could contain invalid paths as the short name translates PROGRA~1 to Program/ Files, even inside a WSL environment

As such, this PR contains several changes, each one in its own commit:

  • a clean up of the Fork.RI script, to follow good practices enforced by the shellcheck tool to avoid syntax errors
  • support for WSL2 build version checks, by calling cmd.exe to retrieve the build version consistently (should work with WSL1 too)
  • a workaround to enable usage of Windows short paths inside Fork, making it possible to use Fork's interactive rebase tool with setups such as the one described by Interactive Rebase/Commit Squashing not working inside Fork #124

Several downsides can be noted:

  • using cmd.exe inside Fork.RI to get WSL build version is slower than using uname
  • the Windows short paths workaround could theoretically be avoided if Fork could run the script from a path containing spaces
  • I'm not fluent with Rust code, hence the workaround inside the shell script, I don't know if it should stay in Fork.RI or be moved inside fork.rs

As-is, this PR enables the usage of Fork's interactive rebase tool by manually editing %localappdata%/Fork/settings.json to change "GitInstancePath": "C:\\Users\\Surname Lastname\\wslgit\\bin\\git.exe", to "GitInstancePath": "C:\\Users\\SURNAM~1\\wslgit\\bin\\git.exe", (confirmed with the author of #124)

I'm planning on sending a support request to Fork's maintainers to ask them if it could support spaces in Fork.RI's path, enabling wslgit to work under all setups

resources/Fork.RI Outdated Show resolved Hide resolved
@andy-5
Copy link
Owner

andy-5 commented Apr 28, 2022

Thanks for this PR. I'm not a Fork user myself, so I haven't fully tested it. I added a small comment above, but with that resolved, I think it looks good.

@carlolars What do you think? Could you maybe test/review this PR?

@carlolars
Copy link
Contributor

Hi!

Sorry for my very late response. I've tested this change both with and without a space in the path to Fork and it works for me using WSL2 👍

Made it case insensitive to ensure it works under all locales
@yannhodiesne
Copy link
Contributor Author

Thanks for the reviews, I applied the fix for the sed call to accommodate for build/Build

I also got news from the Fork team, they told me the call to the Fork.RI script is inside of the cross-platform code base, so they are not sure how they should escape spaces properly for now, but they kept it inside their backlog

@andy-5 andy-5 changed the base branch from master to develop May 14, 2022 15:13
@andy-5 andy-5 merged commit b81bb7e into andy-5:develop May 14, 2022
@andy-5
Copy link
Owner

andy-5 commented May 14, 2022

Great, I merged your changes into the development branch, so they will be in the next release. Thanks again for your PR.

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.

3 participants