On Wed, Dec 14, 2011 at 7:16 PM, Jeff King <peff@xxxxxxxx> wrote: > On Wed, Dec 14, 2011 at 03:07:11PM +0100, Erik Faye-Lund wrote: > >> This avoids us from accidentally dropping state, possibly leading >> to unexpected behaviour. > > I do think this is fine in a "be extra cautious" kind of way. > >> This is especially important on Windows, where the maximum size of >> the environment is 32 kB. > > But does your patch actually detect that? As Andreas pointed out, these > limits don't typically come into play at setenv time. Instead, the > environment is allocated on the heap, and then the result is passed to > exec/spawn, which will fail. > > So your patch is really detecting a failure to malloc, not an overflow > of the environment size, and Windows is just as (un)likely to run out of > heap as any other platform. > You are right; I just assumed that our setenv was implemented on top of SetEnvironmentVariable, which does impose max-limits on setting (if I read the documentation correctly): http://msdn.microsoft.com/en-us/library/windows/desktop/ms686206(v=vs.85).aspx But we don't. We have mingw_setenv, which simply modifies environ. > You can check how your platform behaves by applying this patch: > > diff --git a/git.c b/git.c > index f10e434..57f6b12 100644 > --- a/git.c > +++ b/git.c > @@ -223,6 +223,16 @@ static int handle_alias(int *argcp, const char ***argv) > alias_argv[i] = (*argv)[i]; > alias_argv[argc] = NULL; > > + /* make gigantic environment */ > + { > + int len = 256 * 1024; > + char *buf = xmalloc(len); > + memset(buf, 'z', len); > + buf[len-1] = '\0'; > + if (setenv("FOO", buf, 1)) > + die("setenv failed"); > + } > + > ret = run_command_v_opt(alias_argv, RUN_USING_SHELL); > if (ret >= 0) /* normal exit */ > exit(ret); > > and then running: > > git -c alias.foo='!echo ok' foo > > which yields: > > fatal: cannot exec 'echo ok': Argument list too long > > on Linux. > Yeah, but I'm getting: $ git -c alias.foo='!echo ok' foo ok Which is strange to me. But if I do: $ git -c alias.foo='!echo $FOO' foo it does echo a bunch of 'z's. And we get the expected amount: $ git -c alias.foo='!echo $FOO' foo | wc -c 262144 This strikes me as very strange, because we end up calling the ANSI version of CreateProcess with an environment-block beyond 32767 chars, which MSDN says should fail: "The ANSI version of this function, CreateProcessA fails if the total size of the environment block for the process exceeds 32,767 characters." http://msdn.microsoft.com/en-us/library/windows/desktop/ms682425(v=vs.85).aspx Oh well, this is strange behavior in a good way; I'm not going to complain :P -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html