Skip to content

Fix incorrect escape sequences during Loads under git-bash #1085

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 20, 2023
Merged

Fix incorrect escape sequences during Loads under git-bash #1085

merged 1 commit into from
May 20, 2023

Conversation

SingingTree
Copy link
Contributor

@SingingTree SingingTree commented Apr 8, 2023 •
edited
Loading

#975 had the unintended consequence of causing the RC::Load call to sometimes fail when running under git-bash (and some others Windows specific configs).

The RC::Load -> cmd.Output() calls end up interpreting characters like \b or \t as escape sequences, which is problematic for paths like 'myProject\buckets\test'.

This patch addresses this by forcing the use of / as a separator, even on Windows. This works fine in my testing in git-bash. This shouldn't impact systems where / is already in use.

This also adds a test case that covers the behaviour being adjusted. In order to run the bash tests on Windows, I had to disable a number that appear to not play nice -- but after hacking those out I was able to verify the new test case.

Fixes:

@mloskot
Copy link
Contributor

mloskot commented May 17, 2023

Any chance to get this reviewed and merged?
I've packaged direnv for winget and I noticed similar issue while testing it with Git Bash, see #1096 It would be awesome if this could be fixed, hopefully with this PR.

zimbatm
zimbatm reviewed May 19, 2023
View reviewed changes
Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Do you know if direnv edit is affected by the same issue? There is another instance of path escaping in internal/cmd/cmd_edit.go:60

@SingingTree @zimbatm
Fix incorrect escape sequences during Loads under git-bash
dd16693 
direnv#975 had the unintended consequence
of causing the `RC::Load` call to sometimes fail when running under
git-bash (and some others Windows specific configs).

The `RC::Load` -> `cmd.Output()` calls end up interpreting
characters like \b or \t as escape sequences, which is problematic if
for paths like 'myProject\buckets\test'.

This patch addresses this by forcing the use of `/` as a separator, even
on Windows. This works fine in my testing in git-bash. This shouldn't
impact systems where `/` is already in use.

This also adds a test case that covers the behaviour being adjusted. In
order to run the bash tests on Windows, I had to disabled a number that
appear to not play nice -- but after hacking those out I was able to
verify the new test case.

Fixes:
- direnv#1020
- direnv#1079
@zimbatm zimbatm force-pushed the fix-git-bash-escape-sequences-in-paths branch from 819e11c to dd16693 Compare May 19, 2023 08:19
@SingingTree
Copy link
Contributor Author

Do you know if direnv edit is affected by the same issue? There is another instance of path escaping in internal/cmd/cmd_edit.go:60

Just had a look and it doesn't seem to cause a bustage there. Example of me dumping the string during edit usage (after which I get an editor as expected):

$ ../direnv.exe edit
direnv: C:/projects/testPrograms/tempEditor/Code.exe $'C:\\projects\\direnv\\test\\.envrc'

Generally the escaping of \ seems to work on Windows. However for the case addressed in this PR I wasn't able to figure out a combination that would get that form to play nice, so switched to /.

Using / everywhere would probably play nicely on Windows, especially if it's only under git-bash (and similar). It would also be nice for consistency. However, tinkering shows the existing \ behaviour only seems to cause issues in rare cases.

@mloskot
Copy link
Contributor

mloskot commented May 19, 2023

tinkering shows the existing \ behaviour only seems to cause issues in rare cases.

Yes, I think that is correct conclusion.
e.g. I recall paths like d:\tmp or d:\azure as troublesome

@zimbatm zimbatm merged commit f34d9df into direnv:master May 20, 2023
@zimbatm
Copy link
Member

zimbatm commented May 20, 2023

thanks

@mloskot
Copy link
Contributor

mloskot commented May 20, 2023 •
edited
Loading

I've just packaged the latest release for winget, microsoft/winget-pkgs#107614, and installed it locally.
I followed the tutorial in Git Bash in /d/tmp aka D:\tmp and all works fine:

image

Thanks for the fix!

@zimbatm
Copy link
Member

zimbatm commented May 21, 2023

thanks, can you also try direnv edit?

@mloskot
Copy link
Contributor

mloskot commented May 21, 2023 •
edited
Loading

@zimbatm AFAICT, it works like a charm! I've created path such that if / is replaced with \ then the names will form the well-known escape sequences:

Git.Bash.2023-05-21.22-14-26.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimbatm zimbatm zimbatm left review comments

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@SingingTree @mloskot @zimbatm