Re: [Outreachy][PATCH 2/2] Port helper/test-date.c to unit-tests/t-date.c

From: Patrick Steinhardt <ps@pks.im>
Date: May 29, 2024, 05:49 UTC
Message-ID: <ZlbB_T8DkgmPeWQp@tanuki>
In-Reply-To: <xmqq7cfd7ut0.fsf@gitster.g>

On Tue, May 28, 2024 at 09:41:47AM -0700, Junio C Hamano wrote:

Show 24 quoted lines

> Patrick Steinhardt <ps@pks.im> writes:
> 
> > As I was debugging other Windows-specific issues in a VM already, Chris
> > asked me to also have a look at this issue. And indeed, most of the
> > tests fail deterministically. I also found a fix:
> > ...
> >     -	setenv("TZ", zone, 1);
> >     +	_putenv_s("TZ", zone);
> >         tzset();
> >      }
> >
> > I have no idea why that works though, and the fix is of course not
> > portable. But with this change, the timezones do get picked up by
> > `tzset()` and related date functions as expected.
> 
> The header compat/mingw.h already talks about implementing its own
> replacement by making gitsetenv() call mingw_putenv().
> 
> gitsetenv() emulates setenv() in terms of putenv(), and on Windows
> mingw_putenv() is what implements putenv(), so the difference you
> are observing is coming from the difference between mingw_putenv()
> and _putenv_s(), I would guess.  As the former is isolated within
> compat/mingw.c, it would not involve any additional portability
> issues to redo the former in terms of the latter, I would imagine.

Ah, thanks for the pointer. And indeed, `mingw_putenv()` uses `SetEnvironmentVariableW()` to provide the functionality. This is what MSDN has to say [1]:

    getenv and _putenv use the copy of the environment pointed to by the
    global variable _environ to access the environment. getenv operates
    only on the data structures accessible to the run-time library and
    not on the environment "segment" created for the process by the
    operating system. Therefore, programs that use the envp argument to
    main or wmain may retrieve invalid information.

So calling `SetEnvironmentVariableW()` will not affect calls to `getenv()`. See also issues like for example [2].

This works just fine for us because we also stub out `getenv()` to use `GetEnvironmentVariableW()`, which is its counterpart. But everything else in the C runtime that uses `getenv()` will not see the new values, including our date-related functions. We don't ever set any of those environment variables though, except for now in this new unit test.

Now the question is why we use `SetEnvironmentVariableW()` over `_putenv_s`, and whether changing it would be safe. If the answer is a strict "yes" then we could do that, but if it's a "maybe" then I'd rather not want to change it just to make these unit tests work. In that case, we might aim for a localized fix like the one I have posted.

Patrick

[1]: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/getenv-wgetenv?view=msvc-170 [2]: https://github.com/curl/curl/issues/4774