Open
Conversation
The `mingw_strftime()` wrapper exists to work around msvcrt.dll's incomplete `strftime()` implementation by dynamically loading the version from ucrtbase.dll at runtime via `LoadLibrary()` + `GetProcAddress()`. When the binary is already linked against UCRT (i.e. when building in the UCRT64 environment), the linked-in `strftime()` is the ucrtbase.dll version, making the dynamic loading needless churn: It's calling the very same code. Simply guard both the declaration and implementation so that the unnecessary work-around is skipped in UCRT builds. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Member
|
I think this might regress #863 on UCRT. |
rimrul
reviewed
Mar 17, 2026
| /* a pointer to the original strftime in case we can't find the UCRT version */ | ||
| static size_t (*fallback)(char *, size_t, const char *, const struct tm *) = strftime; | ||
| size_t ret; | ||
| DECLARE_PROC_ADDR(ucrtbase.dll, size_t, __cdecl, strftime, char *, size_t, |
Member
There was a problem hiding this comment.
Maybe just
Suggested change
| #ifndef _UCRT | |
| DECLARE_PROC_ADDR(ucrtbase.dll, size_t, __cdecl, strftime, char *, size_t, | |
| const char *, const struct tm *); | |
| if (INIT_PROC_ADDR(strftime)) | |
| ret = strftime(s, max, format, tm); | |
| else | |
| #endif |
without any other #ifndef?
Member
Author
There was a problem hiding this comment.
You're right @rimrul ! I had forgotten about 9ee0540.
But I'd rather avoid defining the strftime local variable at all, i.e. have this instead:
#ifdef _UCRT
size_t ret = strftime(s, max, format, tm);
#else
[...]
#endif
if (!ret && errno == EINVAL)
die("invalid strftime format: '%s'", format);
return ret;Wouldn't you agree?
Member
There was a problem hiding this comment.
Right, that would be nicer. And I even thought to myself "having it called fallback on UCRT is a bit clunky".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Currently, Git for Windows is built off of the MINGW64 tool chain. But this will have to change because the MSYS2 project deprecated this tool chain in favor of UCRT64. Of course, that's only possible because they dropped support for Windows 8.1, which Git for Windows will probably have to do relatively soon. The best time to do that is probably the Git 3.0 inflection point when we already promised to drop support for older Windows versions.
To prepare for such a huge change, I investigated what needs to be changed in Git for Windows' source code. And the good news is there's actually not very much. This here patch seems to be the only change that's necessary, and not even strictly necessary: the
mingw_strftime()wrapper would still do the right thing. It would just uselessly load the same function that's already loaded, dynamically, again.strerror()override is guarded by an#ifndef _UCRT,PRIuMAXresolves to standard"llu"via<inttypes.h>(note that__MINGW64_VERSION_MAJORis defined both in MINGW64 and UCRT64, by virtue of using themingw-w64-headers),__USE_MINGW_ANSI_STDIO=0is irrelevant because_UCRTshort-circuits it, andSNPRINTF_RETURNS_BOGUShasn't been set for Git for Windows' builds since ec47a33, i.e. for a really long time.