threads / patch / 27552
Subject: [PATCH 0/3] sh-i18n--envsubst; case-sensitive getenv on Windows
Johannes SixtFrom: Johannes Sixt <j6t@kdbg.org>
Here are the fixes needed on Windows for the ab/i18n-scripts topic.
Johannes Sixt (3): sh-i18n--envsubst: do not crash when no arguments are given mingw.c: move definition of mingw_getenv down Windows: teach getenv to do a case-sensitive search
compat/mingw.c | 47 ++++++++++++++++++++++++++++++++--------------- sh-i18n--envsubst.c | 1 + 2 files changed, 33 insertions(+), 15 deletions(-)
-- 1.7.6.rc0.1186.gfb4fd
Johannes Sixt[PATCH 1/3] sh-i18n--envsubst: do not crash when no arguments are given
From: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- sh-i18n--envsubst.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/sh-i18n--envsubst.c b/sh-i18n--envsubst.c index 2eb0ee4..47a306f 100644 --- a/sh-i18n--envsubst.c +++ b/sh-i18n--envsubst.c @@ -74,6 +74,7 @@ main (int argc, char *argv[]) { case 1: error ("we won't substitute all variables on stdin for you"); + break; /* all_variables = 1; subst_from_stdin ();
-- 1.7.6.rc0.1186.gfb4fd
Johannes Sixt[PATCH 2/3] mingw.c: move definition of mingw_getenv down
From: Johannes Sixt <j6t@kdbg.org>
We want to use static lookup_env() in a subsequent change.
At first sight, this change looks innocent. But it is not due to the #undef getenv. There is one caller of getenv between the old location and the new location whose behavior could change. But as can be seen from the defintion of mingw_getenv, the behavior for this caller does not change substantially.
To ensure consistent behavior in the future, change all getenv callers in mingw.c to use mingw_getenv.
With this patch, this is not a big deal, yet, but with the subsequent change, where we teach getenv to do a case-sensitive lookup, the behavior of all call sites is changed.
Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- Erik, Dscho,
the 4msysgit tree has additional calls of getenv in mingw.c. You might want to update them to call mingw_getenv() instead.
But then not doing so would not have a dramatic effect. It is similar to GIT_ASK_YESNO: People can still set Git_Ask_YesNo, and it will work in the same way as usual, *unless* they also have GIT_ASK_YESNO set as well (and with a different value).
compat/mingw.c | 30 +++++++++++++++--------------- 1 files changed, 15 insertions(+), 15 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c index e085e8b..ee480f9 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -178,7 +178,7 @@ static int ask_yes_no_if_possible(const char *format, ...) vsnprintf(question, sizeof(question), format, args); va_end(args); - if ((retry_hook[0] = getenv("GIT_ASK_YESNO"))) { + if ((retry_hook[0] = mingw_getenv("GIT_ASK_YESNO"))) { retry_hook[1] = question; return !run_command_v_opt(retry_hook, 0); } @@ -599,19 +599,6 @@ char *mingw_getcwd(char *pointer, int len) return ret; } -#undef getenv -char *mingw_getenv(const char *name) -{ - char *result = getenv(name); - if (!result && !strcmp(name, "TMPDIR")) { - /* on Windows it is TMP and TEMP */ - result = getenv("TMP"); - if (!result) - result = getenv("TEMP"); - } - return result; -} - /* * See http://msdn2.microsoft.com/en-us/library/17w5ykft(vs.71).aspx * (Parsing C++ Command-Line Arguments) @@ -711,7 +698,7 @@ static const char *parse_interpreter(const char *cmd) */ static char **get_path_split(void) { - char *p, **path, *envpath = getenv("PATH"); + char *p, **path, *envpath = mingw_getenv("PATH"); int i, n = 0; if (!envpath || !*envpath) @@ -1128,6 +1115,19 @@ char **make_augmented_environ(const char *const *vars) return env; } +#undef getenv +char *mingw_getenv(const char *name) +{ + char *result = getenv(name); + if (!result && !strcmp(name, "TMPDIR")) { + /* on Windows it is TMP and TEMP */ + result = getenv("TMP"); + if (!result) + result = getenv("TEMP"); + } + return result; +} + /* * Note, this isn't a complete replacement for getaddrinfo. It assumes * that service contains a numerical port, or that it is null. It
-- 1.7.6.rc0.1186.gfb4fd
Johannes Sixt[PATCH 3/3] Windows: teach getenv to do a case-sensitive search
From: Johannes Sixt <j6t@kdbg.org>
getenv() on Windows looks up environment variables in a case-insensitive manner. Even though all documentations claim that the environment is case-insensitive, it is possible for applications to pass an environment to child processes that has variables that differ only in case. Bash on Windows does this, for example, and sh-i18n--envsubst depends on this behavior.
With this patch environment variables are first looked up in a case-sensitive manner; only if this finds nothing, the system's getenv() is used as a fallback.
Signed-off-by: Johannes Sixt <j6t@kdbg.org> --- compat/mingw.c | 23 ++++++++++++++++++++--- 1 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c index ee480f9..6e5af32 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1116,14 +1116,31 @@ char **make_augmented_environ(const char *const *vars) } #undef getenv + +/* + * The system's getenv looks up the name in a case-insensitive manner. + * This version tries a case-sensitive lookup and falls back to + * case-insensitive if nothing was found. This is necessary because, + * as a prominent example, CMD sets 'Path', but not 'PATH'. + * Warning: not thread-safe. + */ +static char *getenv_cs(const char *name) +{ + size_t len = strlen(name); + int i = lookup_env(environ, name, len); + if (i >= 0) + return environ[i] + len + 1; /* skip past name and '=' */ + return getenv(name); +} + char *mingw_getenv(const char *name) { - char *result = getenv(name); + char *result = getenv_cs(name); if (!result && !strcmp(name, "TMPDIR")) { /* on Windows it is TMP and TEMP */ - result = getenv("TMP"); + result = getenv_cs("TMP"); if (!result) - result = getenv("TEMP"); + result = getenv_cs("TEMP"); } return result; }
-- 1.7.6.rc0.1186.gfb4fd
Karsten BleesRe: [PATCH 3/3] Windows: teach getenv to do a case-sensitive search
Hmm...this looks like a pretty fragile solution to me. Wouldn't it be simpler and safer to just fix the conflicting variables, instead of inventing entirely new environment semantics for Windows?
I looked at the eval_gettext occurences, and only found '$path' in git-submodule.sh to obviously conflict with existing environment variables. So the straightforward solution IMO would be to fix the variable in that script.
Small solution (only affects gettext): in git-submodule.sh, replace all 'eval_gettext...$path' with 'modulepath=$path eval_gettext...$modulepath'
Big solution (enables git-submodule-foreach scripts on Windows, but is a breaking change for existing foreach scripts on Unix): in git-submodule.sh, t/t7407-submodule-foreach.sh, Documentation/git-submodule.txt, replace all '$path' with '$modulepath' (also a few 'path=...' and 'while read...path')
Just a few failure scenarios that come to mind with the current solution:
- Given environment variables "Path" and "path", the 'case-sensitive first'
approach works fine for 'getenv("path")', but 'getenv("PATH")' still has a 50%
chance of failure.
- The other environment functions have not been changed to reflect the
'case-sensitive first' logic: setenv("path"...) and setenv("PATH"...) both have
a chance of overwriting the wrong entry, same for putenv.
- Windows applications generally don't support case-sensitive environment
variables, e.g. all MSYS and Cygwin programs convert environment variable names
to upper case on startup, eliminating duplicates in the process. With git.exe
beeing the only case-sensitive tool, any change to git-sh-i18n.sh (e.g.
replacing git-envsubst with a real gettext-envsubst) is likely to break again.
- As you already mentioned, Windows doesn't support case-sensitive environment
variable names, MSDN is pretty clear on that. Expressly violating this
documentation may cease to work with any new Windows version or patch.Cheers, Karsten