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

Properly escape GitExe and CygPathExe for WSL #195

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

socram8888
Copy link
Contributor

Fixes #194

@kzu
Copy link
Member

kzu commented Aug 5, 2022

I can't see how removing the double quotes is fixing the issue. Could you explain how/why it would work? (or even better, could you record a short gif that showcases it working? you can use the free licecap)

@socram8888
Copy link
Contributor Author

Quotes have not been removed - instead, they've been moved to the variable definition itself (see lines 1098-1099)

The reason is twofold:

  • it makes much easier to use those variables - you no longer need to be thinking if the variable needs to be escaped or not in the rest of the code, if it is already escaped beforehand.
  • there is a considerable amount of places where the variable GitExe wasn't quoted, so it results in a smaller diff to remove the few places where the quotes existed and edit the definition, rather than add them to all the places where they were missing

@CLAassistant
Copy link

CLAassistant commented Aug 18, 2022

CLA assistant check
All committers have signed the CLA.

@kzu
Copy link
Member

kzu commented Oct 15, 2022

Looks good! As soon as you sign the CLA, I'll merge this.

Thanks for your contribution!

@kzu kzu enabled auto-merge (rebase) October 15, 2022 05:13
@kzu
Copy link
Member

kzu commented Oct 18, 2022

Would you mind rebasing on top of main to resolve conflicts?

auto-merge was automatically disabled November 7, 2022 11:04

Head branch was pushed to by a user without write access

@socram8888 socram8888 force-pushed the user-spaces branch 2 times, most recently from 9a3d964 to 83c0c09 Compare November 7, 2022 11:06
@socram8888
Copy link
Contributor Author

Rebase performed

@kzu kzu enabled auto-merge (rebase) November 16, 2022 06:36
@kzu kzu merged commit b53d1f3 into devlooped:main Nov 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Execution fails if using WSL and username contains spaces
3 participants