Skip to content

Conversation

xudyang1
Copy link
Contributor

Some Windows users installed Git for Windows by Windows installer and did not add path_to_Git/bin/ to PATH. We can by default use C:/Program Files/Git/bin/sh.exe when SHELL is not set by the user.

Related: #1220 #1341

@L3MON4D3
Copy link
Owner

LGTM, Thank you for another fix! :)

@L3MON4D3 L3MON4D3 merged commit 0d61a3e into L3MON4D3:master May 12, 2025
4 checks passed
xudyang1 added a commit to xudyang1/LuaSnip that referenced this pull request May 15, 2025
- Problem:

PR L3MON4D3#1343 did not use the correct git shell path as the space is not
escaped.

- Solution:

1. Adding a backslash to escape the space does not fix the issue as `SHELL`
still expands to `sh.exe` even after assignment. Instead of directly
copying `$(WIN_GIT_SHELL)`, use `$(wildcard $(WIN_GIT_SHELL))` returned
from the Makefile function to overwrite `SHELL`.

2. Similar to fixes on `$(shell %COMMAND%)`, semicolons are added after
each command to make sure `make` can call `CreateProcess` correctly:

- Without semicolon: calls `CreateProcess(NULL,%COMMAND%,...)`
- With semicolon: calls `CreateProcess(NULL,$(SHELL) $(.SHELLFLAGS)
  "%COMMAND%;",...)`
xudyang1 added a commit to xudyang1/LuaSnip that referenced this pull request May 29, 2025
- Problem:

PR L3MON4D3#1343 did not use the correct git shell path as the space is not
escaped.

- Solution:

1. Adding a backslash to escape the space does not fix the issue as `SHELL`
still expands to `sh.exe` even after assignment. Instead of directly
copying `$(WIN_GIT_SHELL)`, use `$(wildcard $(WIN_GIT_SHELL))` returned
from the Makefile function to overwrite `SHELL`.

2. Similar to fixes on `$(shell %COMMAND%)`, semicolons are added after
each command to make sure `make` can call `CreateProcess` correctly:

- Without semicolon: calls `CreateProcess(NULL,%COMMAND%,...)`
- With semicolon: calls `CreateProcess(NULL,$(SHELL) $(.SHELLFLAGS)
  "%COMMAND%;",...)`
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.

2 participants