Skip to content
This repository has been archived by the owner on Sep 23, 2021. It is now read-only.

Workaround issue #97 by copying cmd.exe into the temporary folder and running it from here #129

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zvin
Copy link

@zvin zvin commented Sep 11, 2020

Hello @jorangreef
There's been no activity on #97 or microsoft/terminal#2419 for a year so I came up with a (quite ugly) workaround.

Since the issue is with escaping the execute.bat file path, I copy cmd.exe in the temporary folder and run it from here.
It avoids dealing with the path completely.
cmd.exe is ~274KiB so it isn't taking too long to copy.

I acknowledge this is ugly but I couldn't find anything better.

@zvin zvin force-pushed the workaround-windows-amperstand-in-username branch from 1434944 to 81cab70 Compare September 11, 2020 12:20
@jorangreef
Copy link
Owner

Thanks @zvin ! It's an ugly (but beautiful) hack...

It looks good, but would you please make the copy conditional on there being an ampersand in the path? This way, the common case is not affected by the copy, and we may also avoid knock-on bugs.

@jorangreef
Copy link
Owner

@zvin, perhaps it would also be good to make the copy cmd.exe branch dependent on a few more special characters, not just ampersand, e.g. see #119

@zvin
Copy link
Author

zvin commented Sep 15, 2020

@jorangreef Added, I hope I didn't forget any

@zvin
Copy link
Author

zvin commented Nov 20, 2020

This second commit was very wrong, it avoids copying but still runs cmd from the tmp path, I'll fix that.

@zvin zvin force-pushed the workaround-windows-amperstand-in-username branch from f23153e to 687e194 Compare November 20, 2020 19:04
@zvin
Copy link
Author

zvin commented Nov 20, 2020

PR updated @jorangreef
I'd prefer merging only the first commit as it makes it less complex (not creating 2 code paths).

@zvin zvin force-pushed the workaround-windows-amperstand-in-username branch from 687e194 to 7cdede2 Compare November 23, 2020 13:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants