win: perform case insensitive PATH= comparison#1837
Conversation
src/win/process.c
Outdated
| static WCHAR* find_path(WCHAR *env) { | ||
| for (; env != NULL && *env != 0; env += wcslen(env) + 1) { | ||
| if (wcsncmp(env, L"PATH=", 5) == 0) | ||
| if (_wcsicmp(env, L"PATH=", 5) == 0) |
There was a problem hiding this comment.
_wcsicmp() is affected by calls to setlocale(). I'd just write it out in full to avoid any confusion:
if ((env[0] == L'P' || env[0] == L'p') &&
(env[1] == L'A' || env[1] == L'a') &&
// etc
|
Thanks for the quick turnaround on getting this change underway, @cjihrig. 👍 |
|
Good enough CI with Ben's suggestion: https://ci.nodejs.org/view/libuv/job/libuv-test-commit/831/ |
|
Should we note in the docs what happens on Windows when more than one variant of |
|
@richardlau would you be willing to open a docs PR? You seem to have a pretty good handle on what you'd like them to say. |
Refs: nodejs/node#20605 PR-URL: libuv#1837 Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
Yes, but I'd like to convince myself first that the behaviour is actually desired. Copying from nodejs/node#20605 (comment):
|
Fixes: #20605 Refs: libuv/libuv#1837 PR-URL: #32091 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Fixes: #20605 Refs: libuv/libuv#1837 PR-URL: #32091 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Fixes: #20605 Refs: libuv/libuv#1837 PR-URL: #32091 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
Fixes: #20605 Refs: libuv/libuv#1837 PR-URL: #32091 Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Bartosz Sosnowski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
5 participants
If I'm reading nodejs/node#20605 properly, then it seems that this comparison should be case insensitive. Untested locally, but that's what the CI is for.