sh-i18n--envsubst; case-sensitive getenv on Windows

From: Johannes Sixt <[email protected]>

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

From: Johannes Sixt <[email protected]>

---
 sh-i18n--envsubst.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 2eb0ee4..47a306f 100644
@@ -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

From: Johannes Sixt <[email protected]>

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.

---
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(-)

index e085e8b..ee480f9 100644
@@ -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

From: Johannes Sixt <[email protected]>

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.

---
 compat/mingw.c |   23 ++++++++++++++++++++---
 1 files changed, 20 insertions(+), 3 deletions(-)

index ee480f9..6e5af32 100644
@@ -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

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