Fix shell hook expansion on windows #1918
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Dollar Variable expansion (`$VAR`) was inadvertently disabled for windows variables, although %VARIABLES% already worked. This reduced the portability of hooks in general. Additionally, tests would fail on windows due to bad quoting of paths: the path C:/a/b/c would fail when passed to the command `cmd /q /c C:/a/b/c` because it would interpret /a /b and /c as 3 options. Using quotes makes the tests pass.
tsloughter
approved these changes
Oct 18, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Dollar Variable expansion (
$VAR) was inadvertently disabled forwindows variables, although %VARIABLES% already worked. This reduced the
portability of hooks in general.
This was disabled more than four years ago, I'm in fact not quite sure it had ever
worked as intended since
expand_sh_flag/1wrapped the env value in a way thatproplists:get_env/3always returned[]. This is re-expanding them. This mightactually be a bit risky to enable at this point.
Additionally, tests would fail on windows due to bad quoting of paths:
the path C:/a/b/c would fail when passed to the command
cmd /q /c C:/a/b/cbecause it would interpret /a /b and /c as 3options. Using quotes makes the tests pass. This one fix should at least
be safe on its own, but I'm curious as to how it ever passed without
the var expansion.