{"thread":{"id":"49713","subject":"[PATCH 0/2] mingw: rework the environment handling","startedAt":"2018-10-30T09:22:31Z","lastAt":"2018-10-30T09:22:34Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"361973","messageId":"pull.57.git.gitgitgadget@gmail.com","threadId":"49713","inReplyTo":null,"subject":"[PATCH 0/2] mingw: rework the environment handling","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-30T09:22:27Z","receivedAt":"2018-10-30T09:22:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Once upon a time, the Git for Windows project had to decide what to do about\nUnicode support, including how to deal with the environment. Karsten Blees\nspent a ton of work on this, culminating in the final version\n[https://groups.google.com/d/msg/msysgit/wNZAyScbJG4/viWz2KXU0VYJ] which\nmade it into Git for Windows and at least partially into core Git, too.\n\nThe environment handling in particular is a bit tricky: Windows actually has \ntwo copies of the environment, one encoded in UTF-16, and the other one in\nthe local encoding. Since we want UTF-8 encoded values (which is not an\noption for the local encoding), we had to convert from/to the UTF-16\nenvironment.\n\nAt the time those patches were developed, there were so many getenv()/\nputenv() calls in Git's code base that it seemed the best solution to\nconvert the entire environment into UTF-8 in one go, at startup.\n\nThere are good reasons for us to change that paradigm now (and this patch\nseries does that):\n\n * The method we use does not work with modern MSVC runtimes (__environ can\n   no longer be overridden).\n * Our method of having a malloc()ed environment wreaks havoc if a library\n   we use calls MSVC's version of setenv() (I am looking at you, libcurl).\n * In the meantime, core Git's usage of getenv()/putenv() was reduced\n   dramatically (for unrelated reasons), so that it is actually advantageous\n   nowadays to convert on the fly, i.e. with each getenv()/putenv() call,\n   rather than doing one wholesale conversion at process startup. See also\n   the commit message of the second patch.\n\nNote: in contrast to other patches flowing from Git for Windows to Git these\ndays this patch has not been in Git for Windows for ages. Its approach has\nbeen tested in some MS Visual C++ builds (thanks, Jeff Hostetler!), though,\nso I am quite confident that it is correct, and the test suite agrees.\n\nJohannes Schindelin (2):\n  t7800: fix quoting\n  mingw: reencode environment variables on the fly (UTF-16 <-> UTF-8)\n\n compat/mingw.c      | 280 ++++++++++++++++++++++++++------------------\n compat/mingw.h      |  32 ++++-\n t/t7800-difftool.sh |   2 +-\n 3 files changed, 197 insertions(+), 117 deletions(-)\n\n\nbase-commit: c670b1f876521c9f7cd40184bf7ed05aad843433\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-57%2Fdscho%2Fmingw-utf-8-env-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-57/dscho/mingw-utf-8-env-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/57\n-- \ngitgitgadget\n"},{"id":"361974","messageId":"77b2cfb2188b9a5b65c4564faea2a292963371bf.1540891346.git.gitgitgadget@gmail.com","threadId":"49713","inReplyTo":"pull.57.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] t7800: fix quoting","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-30T09:22:29Z","receivedAt":"2018-10-30T09:22:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nWhen passing a command-line to call an external diff command to the\ndifftool, we must be prepared for paths containing special characters,\ne.g. backslashes in the temporary directory's path on Windows.\n\nThis patch is needed in preparation for the next commit, which will\nmake the MinGW version of Git *not* rewrite TMP to use forward slashes\ninstead of backslashes.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n t/t7800-difftool.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 562bd215a..22b9199d5 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -332,7 +332,7 @@ test_expect_success 'difftool --extcmd cat arg1' '\n test_expect_success 'difftool --extcmd cat arg2' '\n \techo branch >expect &&\n \tgit difftool --no-prompt \\\n-\t\t--extcmd sh\\ -c\\ \\\"cat\\ \\$2\\\" branch >actual &&\n+\t\t--extcmd sh\\ -c\\ \\\"cat\\ \\\\\\\"\\$2\\\\\\\"\\\" branch >actual &&\n \ttest_cmp expect actual\n '\n \n-- \ngitgitgadget\n\n"},{"id":"361975","messageId":"9538106fb528c747416d7193267d9ffdcbbb1de5.1540891346.git.gitgitgadget@gmail.com","threadId":"49713","inReplyTo":"pull.57.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] mingw: reencode environment variables on the fly (UTF-16 <-> UTF-8)","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-30T09:22:30Z","receivedAt":"2018-10-30T09:22:34Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin <johannes.schindelin@gmx.de>\n\nOn Windows, the authoritative environment is encoded in UTF-16. In Git\nfor Windows, we convert that to UTF-8 (because UTF-16 is *such* a\nforeign idea to Git that its source code is unprepared for it).\n\nPreviously, out of performance concerns, we converted the entire\nenvironment to UTF-8 in one fell swoop at the beginning, and upon\nputenv() and run_command() converted it back.\n\nHaving a private copy of the environment comes with its own perils: when\na library used by Git's source code tries to modify the environment, it\ndoes not really work (in Git for Windows' case, libcurl, see\nhttps://github.com/git-for-windows/git/compare/bcad1e6d58^...bcad1e6d58^2\nfor a glimpse of the issues).\n\nHence, it makes our environment handling substantially more robust if we\nswitch to on-the-fly-conversion in `getenv()`/`putenv()` calls. Based\non an initial version in the MSVC context by Jeff Hostetler, this patch\nmakes it so.\n\nSurprisingly, this has a *positive* effect on speed: at the time when\nthe current code was written, we tested the performance, and there were\n*so many* `getenv()` calls that it seemed better to convert everything\nin one go. In the meantime, though, Git has obviously been cleaned up a\nbit with regards to `getenv()` calls so that the Git processes spawned\nby the test suite use an average of only 40 `getenv()`/`putenv()` calls\nover the process lifetime.\n\nSpeaking of the entire test suite: the total time spent in the\nre-encoding in the current code takes about 32.4 seconds (out of 113\nminutes runtime), whereas the code introduced in this patch takes only\nabout 8.2 seconds in total. Not much, but it proves that we need not be\nconcerned about the performance impact introduced by this patch.\n\nHelped-by: Jeff Hostetler <jeffhost@microsoft.com>\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n compat/mingw.c | 280 +++++++++++++++++++++++++++++--------------------\n compat/mingw.h |  32 +++++-\n 2 files changed, 196 insertions(+), 116 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 44264fe3f..0cd432441 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -1071,44 +1071,121 @@ static char *path_lookup(const char *cmd, int exe_only)\n \treturn prog;\n }\n \n-static int do_putenv(char **env, const char *name, int size, int free_old);\n+static const wchar_t *wcschrnul(const wchar_t *s, wchar_t c)\n+{\n+\twhile (*s && *s != c)\n+\t\ts++;\n+\treturn s;\n+}\n+\n+/* Compare only keys */\n+static int wenvcmp(const void *a, const void *b)\n+{\n+\twchar_t *p = *(wchar_t **)a, *q = *(wchar_t **)b;\n+\tsize_t p_len, q_len;\n+\n+\t/* Find the keys */\n+\tp_len = wcschrnul(p, L'=') - p;\n+\tq_len = wcschrnul(q, L'=') - q;\n+\n+\t/* If the length differs, include the shorter key's NUL */\n+\tif (p_len < q_len)\n+\t\tp_len++;\n+\telse if (p_len > q_len)\n+\t\tp_len = q_len + 1;\n+\n+\treturn _wcsnicmp(p, q, p_len);\n+}\n \n-/* used number of elements of environ array, including terminating NULL */\n-static int environ_size = 0;\n-/* allocated size of environ array, in bytes */\n-static int environ_alloc = 0;\n+/* We need a stable sort to convert the environment between UTF-16 <-> UTF-8 */\n+#ifndef INTERNAL_QSORT\n+#include \"qsort.c\"\n+#endif\n \n /*\n- * Create environment block suitable for CreateProcess. Merges current\n- * process environment and the supplied environment changes.\n+ * Build an environment block combining the inherited environment\n+ * merged with the given list of settings.\n+ *\n+ * Values of the form \"KEY=VALUE\" in deltaenv override inherited values.\n+ * Values of the form \"KEY\" in deltaenv delete inherited values.\n+ *\n+ * Multiple entries in deltaenv for the same key are explicitly allowed.\n+ *\n+ * We return a contiguous block of UNICODE strings with a final trailing\n+ * zero word.\n  */\n static wchar_t *make_environment_block(char **deltaenv)\n {\n-\twchar_t *wenvblk = NULL;\n-\tchar **tmpenv;\n-\tint i = 0, size = environ_size, wenvsz = 0, wenvpos = 0;\n+\twchar_t *wenv = GetEnvironmentStringsW(), *wdeltaenv, *result, *p;\n+\tsize_t wlen, s, delta_size, size;\n+\n+\twchar_t **array = NULL;\n+\tsize_t alloc = 0, nr = 0, i;\n+\n+\tsize = 1; /* for extra NUL at the end */\n+\n+\t/* If there is no deltaenv to apply, simply return a copy. */\n+\tif (!deltaenv || !*deltaenv) {\n+\t\tfor (p = wenv; p && *p; ) {\n+\t\t\tsize_t s = wcslen(p) + 1;\n+\t\t\tsize += s;\n+\t\t\tp += s;\n+\t\t}\n+\n+\t\tALLOC_ARRAY(result, size);\n+\t\tmemcpy(result, wenv, size * sizeof(*wenv));\n+\t\tFreeEnvironmentStringsW(wenv);\n+\t\treturn result;\n+\t}\n+\n+\t/*\n+\t * If there is a deltaenv, let's accumulate all keys into `array`,\n+\t * sort them using the stable git_qsort() and then copy, skipping\n+\t * duplicate keys\n+\t */\n+\tfor (p = wenv; p && *p; ) {\n+\t\tALLOC_GROW(array, nr + 1, alloc);\n+\t\ts = wcslen(p) + 1;\n+\t\tarray[nr++] = p;\n+\t\tp += s;\n+\t\tsize += s;\n+\t}\n+\n+\t/* (over-)assess size needed for wchar version of deltaenv */\n+\tfor (delta_size = 0, i = 0; deltaenv[i]; i++)\n+\t\tdelta_size += strlen(deltaenv[i]) * 2 + 1;\n+\tALLOC_ARRAY(wdeltaenv, delta_size);\n+\n+\t/* convert the deltaenv, appending to array */\n+\tfor (i = 0, p = wdeltaenv; deltaenv[i]; i++) {\n+\t\tALLOC_GROW(array, nr + 1, alloc);\n+\t\twlen = xutftowcs(p, deltaenv[i], wdeltaenv + delta_size - p);\n+\t\tarray[nr++] = p;\n+\t\tp += wlen + 1;\n+\t}\n \n-\twhile (deltaenv && deltaenv[i])\n-\t\ti++;\n+\tgit_qsort(array, nr, sizeof(*array), wenvcmp);\n+\tALLOC_ARRAY(result, size + delta_size);\n \n-\t/* copy the environment, leaving space for changes */\n-\tALLOC_ARRAY(tmpenv, size + i);\n-\tmemcpy(tmpenv, environ, size * sizeof(char*));\n+\tfor (p = result, i = 0; i < nr; i++) {\n+\t\t/* Skip any duplicate keys; last one wins */\n+\t\twhile (i + 1 < nr && !wenvcmp(array + i, array + i + 1))\n+\t\t       i++;\n \n-\t/* merge supplied environment changes into the temporary environment */\n-\tfor (i = 0; deltaenv && deltaenv[i]; i++)\n-\t\tsize = do_putenv(tmpenv, deltaenv[i], size, 0);\n+\t\t/* Skip \"to delete\" entry */\n+\t\tif (!wcschr(array[i], L'='))\n+\t\t\tcontinue;\n \n-\t/* create environment block from temporary environment */\n-\tfor (i = 0; tmpenv[i]; i++) {\n-\t\tsize = 2 * strlen(tmpenv[i]) + 2; /* +2 for final \\0 */\n-\t\tALLOC_GROW(wenvblk, (wenvpos + size) * sizeof(wchar_t), wenvsz);\n-\t\twenvpos += xutftowcs(&wenvblk[wenvpos], tmpenv[i], size) + 1;\n+\t\tsize = wcslen(array[i]) + 1;\n+\t\tmemcpy(p, array[i], size * sizeof(*p));\n+\t\tp += size;\n \t}\n-\t/* add final \\0 terminator */\n-\twenvblk[wenvpos] = 0;\n-\tfree(tmpenv);\n-\treturn wenvblk;\n+\t*p = L'\\0';\n+\n+\tfree(array);\n+\tfree(wdeltaenv);\n+\tFreeEnvironmentStringsW(wenv);\n+\treturn result;\n }\n \n struct pinfo_t {\n@@ -1350,87 +1427,83 @@ int mingw_kill(pid_t pid, int sig)\n }\n \n /*\n- * Compare environment entries by key (i.e. stopping at '=' or '\\0').\n+ * UTF-8 versions of getenv(), putenv() and unsetenv().\n+ * Internally, they use the CRT's stock UNICODE routines\n+ * to avoid data loss.\n  */\n-static int compareenv(const void *v1, const void *v2)\n+char *mingw_getenv(const char *name)\n {\n-\tconst char *e1 = *(const char**)v1;\n-\tconst char *e2 = *(const char**)v2;\n+#define GETENV_MAX_RETAIN 30\n+\tstatic char *values[GETENV_MAX_RETAIN];\n+\tstatic int value_counter;\n+\tint len_key, len_value;\n+\twchar_t *w_key;\n+\tchar *value;\n+\twchar_t w_value[32768];\n \n-\tfor (;;) {\n-\t\tint c1 = *e1++;\n-\t\tint c2 = *e2++;\n-\t\tc1 = (c1 == '=') ? 0 : tolower(c1);\n-\t\tc2 = (c2 == '=') ? 0 : tolower(c2);\n-\t\tif (c1 > c2)\n-\t\t\treturn 1;\n-\t\tif (c1 < c2)\n-\t\t\treturn -1;\n-\t\tif (c1 == 0)\n-\t\t\treturn 0;\n-\t}\n-}\n+\tif (!name || !*name)\n+\t\treturn NULL;\n \n-static int bsearchenv(char **env, const char *name, size_t size)\n-{\n-\tunsigned low = 0, high = size;\n-\twhile (low < high) {\n-\t\tunsigned mid = low + ((high - low) >> 1);\n-\t\tint cmp = compareenv(&env[mid], &name);\n-\t\tif (cmp < 0)\n-\t\t\tlow = mid + 1;\n-\t\telse if (cmp > 0)\n-\t\t\thigh = mid;\n-\t\telse\n-\t\t\treturn mid;\n+\tlen_key = strlen(name) + 1;\n+\t/* We cannot use xcalloc() here because that uses getenv() itself */\n+\tw_key = calloc(len_key, sizeof(wchar_t));\n+\tif (!w_key)\n+\t\tdie(\"Out of memory, (tried to allocate %u wchar_t's)\", len_key);\n+\txutftowcs(w_key, name, len_key);\n+\tlen_value = GetEnvironmentVariableW(w_key, w_value, ARRAY_SIZE(w_value));\n+\tif (!len_value && GetLastError() == ERROR_ENVVAR_NOT_FOUND) {\n+\t\tfree(w_key);\n+\t\treturn NULL;\n \t}\n-\treturn ~low; /* not found, return 1's complement of insert position */\n+\tfree(w_key);\n+\n+\tlen_value = len_value * 3 + 1;\n+\t/* We cannot use xcalloc() here because that uses getenv() itself */\n+\tvalue = calloc(len_value, sizeof(char));\n+\tif (!value)\n+\t\tdie(\"Out of memory, (tried to allocate %u bytes)\", len_value);\n+\txwcstoutf(value, w_value, len_value);\n+\n+\t/*\n+\t * We return `value` which is an allocated value and the caller is NOT\n+\t * expecting to have to free it, so we keep a round-robin array,\n+\t * invalidating the buffer after GETENV_MAX_RETAIN getenv() calls.\n+\t */\n+\tfree(values[value_counter]);\n+\tvalues[value_counter++] = value;\n+\tif (value_counter >= ARRAY_SIZE(values))\n+\t\tvalue_counter = 0;\n+\n+\treturn value;\n }\n \n-/*\n- * If name contains '=', then sets the variable, otherwise it unsets it\n- * Size includes the terminating NULL. Env must have room for size + 1 entries\n- * (in case of insert). Returns the new size. Optionally frees removed entries.\n- */\n-static int do_putenv(char **env, const char *name, int size, int free_old)\n+int mingw_putenv(const char *namevalue)\n {\n-\tint i = bsearchenv(env, name, size - 1);\n+\tint size;\n+\twchar_t *wide, *equal;\n+\tBOOL result;\n \n-\t/* optionally free removed / replaced entry */\n-\tif (i >= 0 && free_old)\n-\t\tfree(env[i]);\n+\tif (!namevalue || !*namevalue)\n+\t\treturn 0;\n \n-\tif (strchr(name, '=')) {\n-\t\t/* if new value ('key=value') is specified, insert or replace entry */\n-\t\tif (i < 0) {\n-\t\t\ti = ~i;\n-\t\t\tmemmove(&env[i + 1], &env[i], (size - i) * sizeof(char*));\n-\t\t\tsize++;\n-\t\t}\n-\t\tenv[i] = (char*) name;\n-\t} else if (i >= 0) {\n-\t\t/* otherwise ('key') remove existing entry */\n-\t\tsize--;\n-\t\tmemmove(&env[i], &env[i + 1], (size - i) * sizeof(char*));\n+\tsize = strlen(namevalue) * 2 + 1;\n+\twide = calloc(size, sizeof(wchar_t));\n+\tif (!wide)\n+\t\tdie(\"Out of memory, (tried to allocate %u wchar_t's)\", size);\n+\txutftowcs(wide, namevalue, size);\n+\tequal = wcschr(wide, L'=');\n+\tif (!equal)\n+\t\tresult = SetEnvironmentVariableW(wide, NULL);\n+\telse {\n+\t\t*equal = L'\\0';\n+\t\tresult = SetEnvironmentVariableW(wide, equal + 1);\n \t}\n-\treturn size;\n-}\n+\tfree(wide);\n \n-char *mingw_getenv(const char *name)\n-{\n-\tchar *value;\n-\tint pos = bsearchenv(environ, name, environ_size - 1);\n-\tif (pos < 0)\n-\t\treturn NULL;\n-\tvalue = strchr(environ[pos], '=');\n-\treturn value ? &value[1] : NULL;\n-}\n+\tif (!result)\n+\t\terrno = err_win_to_posix(GetLastError());\n \n-int mingw_putenv(const char *namevalue)\n-{\n-\tALLOC_GROW(environ, (environ_size + 1) * sizeof(char*), environ_alloc);\n-\tenviron_size = do_putenv(environ, namevalue, environ_size, 1);\n-\treturn 0;\n+\treturn result ? 0 : -1;\n }\n \n /*\n@@ -2337,17 +2410,6 @@ void mingw_startup(void)\n \tmaxlen = wcslen(wargv[0]);\n \tfor (i = 1; i < argc; i++)\n \t\tmaxlen = max(maxlen, wcslen(wargv[i]));\n-\tfor (i = 0; wenv[i]; i++)\n-\t\tmaxlen = max(maxlen, wcslen(wenv[i]));\n-\n-\t/*\n-\t * nedmalloc can't free CRT memory, allocate resizable environment\n-\t * list. Note that xmalloc / xmemdupz etc. call getenv, so we cannot\n-\t * use it while initializing the environment itself.\n-\t */\n-\tenviron_size = i + 1;\n-\tenviron_alloc = alloc_nr(environ_size * sizeof(char*));\n-\tenviron = malloc_startup(environ_alloc);\n \n \t/* allocate buffer (wchar_t encodes to max 3 UTF-8 bytes) */\n \tmaxlen = 3 * maxlen + 1;\n@@ -2356,14 +2418,8 @@ void mingw_startup(void)\n \t/* convert command line arguments and environment to UTF-8 */\n \tfor (i = 0; i < argc; i++)\n \t\t__argv[i] = wcstoutfdup_startup(buffer, wargv[i], maxlen);\n-\tfor (i = 0; wenv[i]; i++)\n-\t\tenviron[i] = wcstoutfdup_startup(buffer, wenv[i], maxlen);\n-\tenviron[i] = NULL;\n \tfree(buffer);\n \n-\t/* sort environment for O(log n) getenv / putenv */\n-\tqsort(environ, i, sizeof(char*), compareenv);\n-\n \t/* fix Windows specific environment settings */\n \tsetup_windows_environment();\n \ndiff --git a/compat/mingw.h b/compat/mingw.h\nindex f31dcff2b..cb9cef275 100644\n--- a/compat/mingw.h\n+++ b/compat/mingw.h\n@@ -257,11 +257,35 @@ char *mingw_mktemp(char *template);\n char *mingw_getcwd(char *pointer, int len);\n #define getcwd mingw_getcwd\n \n+#ifdef NO_UNSETENV\n+#error \"NO_UNSETENV is incompatible with the Windows-specific startup code!\"\n+#endif\n+\n+/*\n+ * We bind *env() routines (even the mingw_ ones) to private mingw_ versions.\n+ * These talk to the CRT using UNICODE/wchar_t, but maintain the original\n+ * narrow-char API.\n+ *\n+ * Note that the MSCRT maintains both ANSI (getenv()) and UNICODE (_wgetenv())\n+ * routines and stores both versions of each environment variable in parallel\n+ * (and secretly updates both when you set one or the other), but it uses CP_ACP\n+ * to do the conversion rather than CP_UTF8.\n+ *\n+ * Since everything in the git code base is UTF8, we define the mingw_ routines\n+ * to access the CRT using the UNICODE routines and manually convert them to\n+ * UTF8.  This also avoids round-trip problems.\n+ *\n+ * This also helps with our linkage, since \"_wenviron\" is publicly exported\n+ * from the CRT.  But to access \"_environ\" we would have to statically link\n+ * to the CRT (/MT).\n+ *\n+ * We require NO_SETENV (and let gitsetenv() call our mingw_putenv).\n+ */\n+#define getenv       mingw_getenv\n+#define putenv       mingw_putenv\n+#define unsetenv     mingw_putenv\n char *mingw_getenv(const char *name);\n-#define getenv mingw_getenv\n-int mingw_putenv(const char *namevalue);\n-#define putenv mingw_putenv\n-#define unsetenv mingw_putenv\n+int   mingw_putenv(const char *name);\n \n int mingw_gethostname(char *host, int namelen);\n #define gethostname mingw_gethostname\n-- \ngitgitgadget\n"}]}