patchSafe-guard the Windows code a bit more against getenv() problems

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Running up to v2.21.0, we fixed two bugs that were made prominent by the
> Windows-specific change to retain copies of only the 30 latest getenv()
> calls' returned strings, invalidating any copies of previous getenv()
> calls' return values.
> 
> While this really shines a light onto bugs of the form where we hold
> onto getenv()'s return values without copying them, it is also a real
> problem for users.
> 
> And even if Jeff King's patches merged via 773e408881 (Merge branch
> 'jk/save-getenv-result', 2019-01-29) provide further work on that front,
> we are far from done. Just one example: on Windows, we unset environment
> variables when spawning new processes, which potentially invalidates
> strings that were previously obtained via getenv(), and therefore we
> have to duplicate environment values that are somehow involved in
> spawning new processes (e.g. GIT_MAN_VIEWER in show_man_page()).
> 
> We do not have a chance to investigate, let address, all of those issues
> in time for v2.21.0, so let's at least help Windows users by increasing
> the number of getenv() calls' return values that are kept valid. The
> number 64 was determined by looking at the average number of getenv()
> calls per process in the entire test suite run on Windows (which is
> around 40) and then adding a bit for good measure. And it is a power of
> two (which would have hit yesterday's theme perfectly).
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   compat/mingw.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 4276297595..8141f77189 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -1632,7 +1632,7 @@ int mingw_kill(pid_t pid, int sig)
>    */
>   char *mingw_getenv(const char *name)
>   {
> -#define GETENV_MAX_RETAIN 30
> +#define GETENV_MAX_RETAIN 64
>   	static char *values[GETENV_MAX_RETAIN];
>   	static int value_counter;
>   	int len_key, len_value;
> 

>> diff --git a/compat/mingw.c b/compat/mingw.c
>> index 4276297595..8141f77189 100644
>> --- a/compat/mingw.c
>> +++ b/compat/mingw.c
>> @@ -1632,7 +1632,7 @@ int mingw_kill(pid_t pid, int sig)
>>    */
>>   char *mingw_getenv(const char *name)
>>   {
>> -#define GETENV_MAX_RETAIN 30
>> +#define GETENV_MAX_RETAIN 64
>>   	static char *values[GETENV_MAX_RETAIN];
>>   	static int value_counter;
>>   	int len_key, len_value;
>>
>
> Why not use a mem_pool for this?  We have that code isolated
> and re-usable now.  Have mingw_getenv() copy the string into
> the pool always return the pointer from within the pool.  The
> pool automatically handles allocating new blocks as necessary.
> And (if we care) we can bulk free the pool before existing.

> On 2/15/2019 10:17 AM, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> > 
> > diff --git a/compat/mingw.c b/compat/mingw.c
> > index 4276297595..8141f77189 100644
> > --- a/compat/mingw.c
> > +++ b/compat/mingw.c
> > @@ -1632,7 +1632,7 @@ int mingw_kill(pid_t pid, int sig)
> >   */
> >   char *mingw_getenv(const char *name)
> >   {
> > -#define GETENV_MAX_RETAIN 30
> > +#define GETENV_MAX_RETAIN 64
> >    static char *values[GETENV_MAX_RETAIN];
> >    static int value_counter;
> >    int len_key, len_value;
> > 
> 
> Why not use a mem_pool for this?  We have that code isolated
> and re-usable now.  Have mingw_getenv() copy the string into
> the pool always return the pointer from within the pool.  The
> pool automatically handles allocating new blocks as necessary.
> And (if we care) we can bulk free the pool before existing.

> > Why not use a mem_pool for this?  We have that code isolated
> > and re-usable now.  Have mingw_getenv() copy the string into
> > the pool always return the pointer from within the pool.  The
> > pool automatically handles allocating new blocks as necessary.
> > And (if we care) we can bulk free the pool before existing.
> 
> The problem with the mem_pool would be that I do *not* want to accumulate
> tons and tons of stale copies (remember that `difftool` bug? Apparently it
> calls `getenv()` in a loop that scales with the number of modified files;
> I would hate to have copies for all of those).
> 
> And all you could do to remedy this would be to blow away all of them it
> the mem_pool grew too much, *including* the latest `getenv()` (which we
> painfully keep valid, e.g. in the case of `git_committer_info()` which
> calls `getenv()` *three* times *inside* the parameter list of the
> `fmt_ident()` function call.

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> Running up to v2.21.0, we fixed two bugs that were made prominent by the
> Windows-specific change to retain copies of only the 30 latest getenv()
> calls' returned strings, invalidating any copies of previous getenv()
> calls' return values.
> 
> While this really shines a light onto bugs of the form where we hold
> onto getenv()'s return values without copying them, it is also a real
> problem for users.
> 
> And even if Jeff King's patches merged via 773e408881 (Merge branch
> 'jk/save-getenv-result', 2019-01-29) provide further work on that front,
> we are far from done. Just one example: on Windows, we unset environment
> variables when spawning new processes, which potentially invalidates
> strings that were previously obtained via getenv(), and therefore we
> have to duplicate environment values that are somehow involved in
> spawning new processes (e.g. GIT_MAN_VIEWER in show_man_page()).