# [PATCH] compat/winansi: fix die_lasterr() argument formatting 12 messages from 2026-09-16 to 2026-09-23. Participants: Yongqiang Tian, Junio C Hamano, Johannes Sixt, René Scharfe. Thread: https://gitlist.dev/t/66335 ## Yongqiang Tian, 2026-09-16 04:23 Subject: [PATCH] compat/winansi: fix die_lasterr() argument formatting Message-ID: <20260916042312.35891-1-yqtian668@gmail.com> ``` During WinANSI initialization, duplicate_handle() reports the handle when DuplicateHandle() fails: die_lasterr("DuplicateHandle(%li) failed", ...); die_lasterr() collects the formatting arguments in a va_list, but passes that va_list to die_errno() as an ordinary variadic argument. die_errno() consequently formats the representation of the va_list instead of the supplied handle, producing an incorrect fatal message. The other current callers pass fixed strings and are unaffected. Git does not provide a va_list-taking variant of die_errno(), so format the caller's arguments separately with strbuf_vaddf(). This consumes the original va_list correctly and produces the complete diagnostic prefix, including the handle supplied by duplicate_handle(). Save GetLastError() before formatting because calls made while growing the strbuf may change the thread's Windows error value. Convert the saved value to errno only after formatting, then pass the completed message to die_errno() through a literal "%s". This prevents any percent characters in the formatted message from being interpreted a second time, while allowing die_errno() to append the corresponding system error and terminate as before. The updated compat/winansi.c compiles with MinGW GCC 13. A Win64 probe under Wine prints a value derived from the va_list before this change and the supplied integer afterward. Signed-off-by: Yongqiang Tian --- compat/winansi.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/compat/winansi.c b/compat/winansi.c index 3ce190093..5547192a2 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -7,6 +7,7 @@ #define DISABLE_SIGN_COMPARE_WARNINGS #include "../git-compat-util.h" +#include "../strbuf.h" #include #include #include "win32.h" @@ -438,11 +439,15 @@ static void winansi_exit(void) static void die_lasterr(const char *fmt, ...) { + DWORD err = GetLastError(); + struct strbuf message = STRBUF_INIT; va_list params; + va_start(params, fmt); - errno = err_win_to_posix(GetLastError()); - die_errno(fmt, params); + strbuf_vaddf(&message, fmt, params); va_end(params); + errno = err_win_to_posix(err); + die_errno("%s", message.buf); } #undef dup2 -- 2.34.1 ``` ## Junio C Hamano, 2026-09-16 04:29 Subject: Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting Message-ID: In-Reply-To: <20260916042312.35891-1-yqtian668@gmail.com> ``` Yongqiang Tian writes: > During WinANSI initialization, duplicate_handle() reports the handle > when DuplicateHandle() fails: > > die_lasterr("DuplicateHandle(%li) failed", ...); > ... I do not know about Patrick, but I do not do Windows, so please do not Cc: me a patch that is primarily about Windows portability. I'll add two whose with contributions much greater than I have in the area to Cc: list. Thanks. > die_lasterr() collects the formatting arguments in a va_list, but > passes that va_list to die_errno() as an ordinary variadic argument. > die_errno() consequently formats the representation of the va_list > instead of the supplied handle, producing an incorrect fatal message. > The other current callers pass fixed strings and are unaffected. > > Git does not provide a va_list-taking variant of die_errno(), so format > the caller's arguments separately with strbuf_vaddf(). This consumes the > original va_list correctly and produces the complete diagnostic prefix, > including the handle supplied by duplicate_handle(). > > Save GetLastError() before formatting because calls made while growing > the strbuf may change the thread's Windows error value. Convert the > saved value to errno only after formatting, then pass the completed > message to die_errno() through a literal "%s". This prevents any percent > characters in the formatted message from being interpreted a second > time, while allowing die_errno() to append the corresponding system > error and terminate as before. > > The updated compat/winansi.c compiles with MinGW GCC 13. A Win64 probe > under Wine prints a value derived from the va_list before this change > and the supplied integer afterward. > > Signed-off-by: Yongqiang Tian > --- > compat/winansi.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/compat/winansi.c b/compat/winansi.c > index 3ce190093..5547192a2 100644 > --- a/compat/winansi.c > +++ b/compat/winansi.c > @@ -7,6 +7,7 @@ > #define DISABLE_SIGN_COMPARE_WARNINGS > > #include "../git-compat-util.h" > +#include "../strbuf.h" > #include > #include > #include "win32.h" > @@ -438,11 +439,15 @@ static void winansi_exit(void) > > static void die_lasterr(const char *fmt, ...) > { > + DWORD err = GetLastError(); > + struct strbuf message = STRBUF_INIT; > va_list params; > + > va_start(params, fmt); > - errno = err_win_to_posix(GetLastError()); > - die_errno(fmt, params); > + strbuf_vaddf(&message, fmt, params); > va_end(params); > + errno = err_win_to_posix(err); > + die_errno("%s", message.buf); > } > > #undef dup2 ``` ## Johannes Sixt, 2026-09-16 06:13 Subject: Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting Message-ID: <1cb6ad12-27bc-458e-b8f5-4b7eb44356fe@kdbg.org> In-Reply-To: <20260916042312.35891-1-yqtian668@gmail.com> ``` Am 16.09.26 um 06:23 schrieb Yongqiang Tian: > During WinANSI initialization, duplicate_handle() reports the handle > when DuplicateHandle() fails: > > die_lasterr("DuplicateHandle(%li) failed", ...); The full call is more like die_lasterr("DuplicateHandle(%li) failed", (long) (intptr_t) hnd); This attempts to format the Windows handle value into the error message. That's a pointless exercise, becaues AFAIK the value is totally opaque and unhelpful as a debugging aid. For this reason, I'd suggest to go the simpler route to remove the formatting from the above call (the only one that passes more than just a string) and have die_lasterr take just a single string and no variable argument list. -- Hannes ``` ## René Scharfe, 2026-09-16 06:33 Subject: Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting Message-ID: In-Reply-To: <20260916042312.35891-1-yqtian668@gmail.com> ``` On 9/16/26 6:23 AM, Yongqiang Tian wrote: > During WinANSI initialization, duplicate_handle() reports the handle > when DuplicateHandle() fails: > > die_lasterr("DuplicateHandle(%li) failed", ...); > > die_lasterr() collects the formatting arguments in a va_list, but > passes that va_list to die_errno() as an ordinary variadic argument. > die_errno() consequently formats the representation of the va_list > instead of the supplied handle, producing an incorrect fatal message. Good find! > The other current callers pass fixed strings and are unaffected. > > Git does not provide a va_list-taking variant of die_errno(), so format > the caller's arguments separately with strbuf_vaddf(). This consumes the > original va_list correctly and produces the complete diagnostic prefix, > including the handle supplied by duplicate_handle(). > > Save GetLastError() before formatting because calls made while growing > the strbuf may change the thread's Windows error value. Convert the > saved value to errno only after formatting, then pass the completed > message to die_errno() through a literal "%s". This prevents any percent > characters in the formatted message from being interpreted a second > time, while allowing die_errno() to append the corresponding system > error and terminate as before. That all makes sense, but is quite complicated. die_errno() itself uses a fixed-size buffer to avoid heap allocation, for robustness and to avoid changing errno. How about turning die_lasterr() into a macro for the same reasons? #define die_lasterr(...) do { \ errno = err_win_to_posix(GetLastError()); \ die_errno(__VA_ARGS__); \ } while (0) > The updated compat/winansi.c compiles with MinGW GCC 13. A Win64 probe > under Wine prints a value derived from the va_list before this change > and the supplied integer afterward. > > Signed-off-by: Yongqiang Tian > --- > compat/winansi.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/compat/winansi.c b/compat/winansi.c > index 3ce190093..5547192a2 100644 > --- a/compat/winansi.c > +++ b/compat/winansi.c > @@ -7,6 +7,7 @@ > #define DISABLE_SIGN_COMPARE_WARNINGS > > #include "../git-compat-util.h" > +#include "../strbuf.h" > #include > #include > #include "win32.h" > @@ -438,11 +439,15 @@ static void winansi_exit(void) > > static void die_lasterr(const char *fmt, ...) > { > + DWORD err = GetLastError(); > + struct strbuf message = STRBUF_INIT; > va_list params; > + > va_start(params, fmt); > - errno = err_win_to_posix(GetLastError()); > - die_errno(fmt, params); > + strbuf_vaddf(&message, fmt, params); > va_end(params); > + errno = err_win_to_posix(err); > + die_errno("%s", message.buf); > } > > #undef dup2 ``` ## Johannes Sixt, 2026-09-16 07:09 Subject: Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting Message-ID: <1e706ce8-bebd-4ac1-914a-a55195e7d253@kdbg.org> In-Reply-To: ``` Am 16.09.26 um 08:33 schrieb René Scharfe: > That all makes sense, but is quite complicated. die_errno() itself uses > a fixed-size buffer to avoid heap allocation, for robustness and to > avoid changing errno. How about turning die_lasterr() into a macro for > the same reasons? > > #define die_lasterr(...) do { \ > errno = err_win_to_posix(GetLastError()); \ > die_errno(__VA_ARGS__); \ > } while (0) die_lasterr is used to diagnose errors of Windows functions. I dislike that this degrades the exact error value of GetLastError() into an errno. If this direction is persued, then we should remove die_errno from the picture. But as I hinted elsewhere in the thread, this is all overengineered for no good reason. -- Hannes ``` ## Yongqiang Tian, 2026-09-21 03:00 Subject: Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting Message-ID: In-Reply-To: <1e706ce8-bebd-4ac1-914a-a55195e7d253@kdbg.org> ``` Hi René and Hannes, Thank you again for the feedback. I have been thinking about this over the past week. I see three possible directions: 1. Turn die_lasterr() into the variadic macro René suggested. This is the smallest change and avoids allocation, but still maps the original GetLastError() value to errno. 2. Keep a function and format its variadic arguments into a fixed-size stack buffer. This would avoid allocation and could preserve the original Windows error code, but it adds more error-reporting machinery for only four call sites. 3. Remove die_lasterr() and report GetLastError() directly at those call sites. This avoids the va_list forwarding, allocation, and errno conversion altogether. The third direction now seems the simplest to me. It also follows existing Windows-specific code in Git that reports GetLastError() directly, for example: https://github.com/git/git/blob/9a0c4701dcd5725c4184599322b52933ff5005ca/compat/win32/syslog.c#L10-L13 and: https://github.com/git/git/blob/9a0c4701dcd5725c4184599322b52933ff5005ca/compat/fsmonitor/fsm-listen-win32.c#L106-L109 The resulting change would be approximately: diff --git a/compat/winansi.c b/compat/winansi.c --- a/compat/winansi.c +++ b/compat/winansi.c @@ -static void die_lasterr(const char *fmt, ...) -{ - va_list params; - va_start(params, fmt); - errno = err_win_to_posix(GetLastError()); - die_errno(fmt, params); - va_end(params); -} - static HANDLE duplicate_handle(HANDLE hnd) { HANDLE hresult, hproc = GetCurrentProcess(); if (!DuplicateHandle(hproc, hnd, hproc, &hresult, 0, TRUE, DUPLICATE_SAME_ACCESS)) - die_lasterr("DuplicateHandle(%li) failed", - (long) (intptr_t) hnd); + die("DuplicateHandle(%p) failed: Windows error %lu", + (void *)hnd, GetLastError()); return hresult; } @@ if (hwrite == INVALID_HANDLE_VALUE) - die_lasterr("CreateNamedPipe failed"); + die("CreateNamedPipe failed: Windows error %lu", + GetLastError()); @@ if (hread == INVALID_HANDLE_VALUE) - die_lasterr("CreateFile for named pipe failed"); + die("CreateFile for named pipe failed: Windows error %lu", + GetLastError()); @@ if (!hthread) - die_lasterr("CreateThread(console_thread) failed"); + die("CreateThread(console_thread) failed: Windows error %lu", + GetLastError()); I used %p for the handle because HANDLE is pointer-sized, whereas long remains 32 bits on 64-bit Windows. That could also be kept separate if you would prefer this revision to address only the forwarding issue. Would this be a preferable direction? If so, I would be happy to prepare and test a revised patch. Any suggestions would be really appreciated. Thanks, Yongqiang On Wed, 16 Sept 2026 at 17:09, Johannes Sixt wrote: > > Am 16.09.26 um 08:33 schrieb René Scharfe: > > That all makes sense, but is quite complicated. die_errno() itself uses > > a fixed-size buffer to avoid heap allocation, for robustness and to > > avoid changing errno. How about turning die_lasterr() into a macro for > > the same reasons? > > > > #define die_lasterr(...) do { \ > > errno = err_win_to_posix(GetLastError()); \ > > die_errno(__VA_ARGS__); \ > > } while (0) > > die_lasterr is used to diagnose errors of Windows functions. I dislike > that this degrades the exact error value of GetLastError() into an > errno. If this direction is persued, then we should remove die_errno > from the picture. > > But as I hinted elsewhere in the thread, this is all overengineered for > no good reason. > > -- Hannes > ``` ## Johannes Sixt, 2026-09-21 04:06 Subject: Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting Message-ID: <9c1cde3d-92c4-4684-834e-bae97fd33f5f@kdbg.org> In-Reply-To: ``` Am 21.09.26 um 05:00 schrieb Yongqiang Tian: > 3. Remove die_lasterr() and report GetLastError() directly at those > call sites. This avoids the va_list forwarding, allocation, and > errno conversion altogether. > > The third direction now seems the simplest to me. It also follows > existing Windows-specific code in Git that reports GetLastError() > directly, for example: > > https://github.com/git/git/blob/9a0c4701dcd5725c4184599322b52933ff5005ca/compat/win32/syslog.c#L10-L13 > > and: > > https://github.com/git/git/blob/9a0c4701dcd5725c4184599322b52933ff5005ca/compat/fsmonitor/fsm-listen-win32.c#L106-L109 Sounds reasonable to me. > - die_lasterr("DuplicateHandle(%li) failed", > - (long) (intptr_t) hnd); > + die("DuplicateHandle(%p) failed: Windows error %lu", > + (void *)hnd, GetLastError()); But please leave the conversion to %p for another time. Concerning the text "Windows error", please follow existing practice. -- Hannes ``` ## Yongqiang Tian, 2026-09-21 06:20 Subject: [PATCH v2] compat/winansi: fix die_lasterr() argument formatting Message-ID: <20260921062114.14450-1-yqtian668@gmail.com> In-Reply-To: <20260916042312.35891-1-yqtian668@gmail.com> ``` During WinANSI initialization, duplicate_handle() reports the handle when DuplicateHandle() fails. die_lasterr() collects the formatting arguments in a va_list, but passes that va_list to die_errno() as an ordinary variadic argument. die_errno() consequently formats part of the va_list representation instead of the supplied handle, producing an incorrect fatal message. The helper also converts GetLastError() to errno, losing the exact Windows error code. Remove die_lasterr() and report GetLastError() directly at its four call sites, following the existing Windows diagnostic style. This passes the handle to the formatter correctly and preserves the Windows error code. Keep the existing %li representation of the handle. With MinGW GCC 13, compat/winansi.o builds with DEVELOPER=1 and the complete git.exe builds and links. Signed-off-by: Yongqiang Tian --- Changes since v1: - replace die_lasterr() with direct die() calls; - preserve exact GetLastError() values instead of mapping them to errno; - follow the existing Windows diagnostic style and retain %li for the handle; - verify compat/winansi.o with DEVELOPER=1 and build and link the complete git.exe with MinGW GCC 13. compat/winansi.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/compat/winansi.c b/compat/winansi.c index 3ce1900939..088734a1df 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -436,15 +436,6 @@ static void winansi_exit(void) CloseHandle(hthread); } -static void die_lasterr(const char *fmt, ...) -{ - va_list params; - va_start(params, fmt); - errno = err_win_to_posix(GetLastError()); - die_errno(fmt, params); - va_end(params); -} - #undef dup2 int winansi_dup2(int oldfd, int newfd) { @@ -462,8 +453,8 @@ static HANDLE duplicate_handle(HANDLE hnd) HANDLE hresult, hproc = GetCurrentProcess(); if (!DuplicateHandle(hproc, hnd, hproc, &hresult, 0, TRUE, DUPLICATE_SAME_ACCESS)) - die_lasterr("DuplicateHandle(%li) failed", - (long) (intptr_t) hnd); + die("DuplicateHandle(%li) failed: %lu", + (long) (intptr_t) hnd, GetLastError()); return hresult; } @@ -609,16 +600,16 @@ void winansi_init(void) hwrite = CreateNamedPipeW(name, PIPE_ACCESS_OUTBOUND, PIPE_TYPE_BYTE | PIPE_WAIT, 1, BUFFER_SIZE, 0, 0, NULL); if (hwrite == INVALID_HANDLE_VALUE) - die_lasterr("CreateNamedPipe failed"); + die("CreateNamedPipe failed: %lu", GetLastError()); hread = CreateFileW(name, GENERIC_READ, 0, NULL, OPEN_EXISTING, 0, NULL); if (hread == INVALID_HANDLE_VALUE) - die_lasterr("CreateFile for named pipe failed"); + die("CreateFile for named pipe failed: %lu", GetLastError()); /* start console spool thread on the pipe's read end */ hthread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL); if (!hthread) - die_lasterr("CreateThread(console_thread) failed"); + die("CreateThread(console_thread) failed: %lu", GetLastError()); /* schedule cleanup routine */ if (atexit(winansi_exit)) -- 2.34.1 ``` ## Junio C Hamano, 2026-09-21 17:18 Subject: Re: [PATCH v2] compat/winansi: fix die_lasterr() argument formatting Message-ID: In-Reply-To: <20260921062114.14450-1-yqtian668@gmail.com> ``` Yongqiang Tian writes: > During WinANSI initialization, duplicate_handle() reports the handle > when DuplicateHandle() fails. die_lasterr() collects the formatting > arguments in a va_list, but passes that va_list to die_errno() as an > ordinary variadic argument. die_errno() consequently formats part of > the va_list representation instead of the supplied handle, producing > an incorrect fatal message. Interesting. It's a shame that nobody noticed the broken calling sequence since the bogosity was first introduced into the codebase at eac14f8909 (Win32: Thread-safe windows console output, 2012-01-14). > The helper also converts GetLastError() to errno, losing the exact > Windows error code. > > Remove die_lasterr() and report GetLastError() directly at its four > call sites, following the existing Windows diagnostic style. This > passes the handle to the formatter correctly and preserves the Windows > error code. Keep the existing %li representation of the handle. OK. > With MinGW GCC 13, compat/winansi.o builds with DEVELOPER=1 and the > complete git.exe builds and links. I am puzzled here. What's the relevance of these two lines? Are you telling us that how you have built and tested the patch? Unless the set-up to test this change needs some special care, we usually do not write such a thing in our proposed log message. > Signed-off-by: Yongqiang Tian > --- > > Changes since v1: > - replace die_lasterr() with direct die() calls; > - preserve exact GetLastError() values instead of mapping them to errno; > - follow the existing Windows diagnostic style and retain %li for the > handle; Good collaboration. If I were doing this commit, judging from the discussion on v1 iteration, I would probably have added a Helped-by: to credit j6t, though. > - verify compat/winansi.o with DEVELOPER=1 and build and link the > complete git.exe with MinGW GCC 13. Is that a change, meaning v1 was sent without building, linking and testing? Improving on that is a very welcome thing ;-). > compat/winansi.c | 19 +++++-------------- > 1 file changed, 5 insertions(+), 14 deletions(-) Nice. > -static void die_lasterr(const char *fmt, ...) > -{ > - va_list params; > - va_start(params, fmt); > - errno = err_win_to_posix(GetLastError()); > - die_errno(fmt, params); > - va_end(params); > -} Very good to see this go. ``` ## Yongqiang Tian, 2026-09-21 23:47 Subject: [PATCH v3] compat/winansi: fix die_lasterr() argument formatting Message-ID: <20260921234756.77997-1-yqtian668@gmail.com> In-Reply-To: <20260921062114.14450-1-yqtian668@gmail.com> ``` During WinANSI initialization, duplicate_handle() reports the handle when DuplicateHandle() fails. die_lasterr() collects the formatting arguments in a va_list, but passes that va_list to die_errno() as an ordinary variadic argument. die_errno() consequently formats part of the va_list representation instead of the supplied handle, producing an incorrect fatal message. The helper also converts GetLastError() to errno, losing the exact Windows error code. Remove die_lasterr() and report GetLastError() directly at its four call sites, following the existing Windows diagnostic style. This passes the handle to the formatter correctly and preserves the Windows error code. Keep the existing %li representation of the handle. Helped-by: Johannes Sixt Helped-by: René Scharfe Signed-off-by: Yongqiang Tian --- Changes since v2: - Add Helped-by trailers for Johannes Sixt and René Scharfe. - Move build validation details below the separator. - No code changes. Validation (performed for v2; the code is unchanged): - Built compat/winansi.o with DEVELOPER=1 using MinGW GCC 13. - Built and linked the complete git.exe. compat/winansi.c | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/compat/winansi.c b/compat/winansi.c index 3ce1900939..088734a1df 100644 --- a/compat/winansi.c +++ b/compat/winansi.c @@ -436,15 +436,6 @@ static void winansi_exit(void) CloseHandle(hthread); } -static void die_lasterr(const char *fmt, ...) -{ - va_list params; - va_start(params, fmt); - errno = err_win_to_posix(GetLastError()); - die_errno(fmt, params); - va_end(params); -} - #undef dup2 int winansi_dup2(int oldfd, int newfd) { @@ -462,8 +453,8 @@ static HANDLE duplicate_handle(HANDLE hnd) HANDLE hresult, hproc = GetCurrentProcess(); if (!DuplicateHandle(hproc, hnd, hproc, &hresult, 0, TRUE, DUPLICATE_SAME_ACCESS)) - die_lasterr("DuplicateHandle(%li) failed", - (long) (intptr_t) hnd); + die("DuplicateHandle(%li) failed: %lu", + (long) (intptr_t) hnd, GetLastError()); return hresult; } @@ -609,16 +600,16 @@ void winansi_init(void) hwrite = CreateNamedPipeW(name, PIPE_ACCESS_OUTBOUND, PIPE_TYPE_BYTE | PIPE_WAIT, 1, BUFFER_SIZE, 0, 0, NULL); if (hwrite == INVALID_HANDLE_VALUE) - die_lasterr("CreateNamedPipe failed"); + die("CreateNamedPipe failed: %lu", GetLastError()); hread = CreateFileW(name, GENERIC_READ, 0, NULL, OPEN_EXISTING, 0, NULL); if (hread == INVALID_HANDLE_VALUE) - die_lasterr("CreateFile for named pipe failed"); + die("CreateFile for named pipe failed: %lu", GetLastError()); /* start console spool thread on the pipe's read end */ hthread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL); if (!hthread) - die_lasterr("CreateThread(console_thread) failed"); + die("CreateThread(console_thread) failed: %lu", GetLastError()); /* schedule cleanup routine */ if (atexit(winansi_exit)) -- 2.34.1 ``` ## Yongqiang Tian, 2026-09-21 23:49 Subject: Re: [PATCH v2] compat/winansi: fix die_lasterr() argument formatting Message-ID: In-Reply-To: ``` Hi Junio, Thank you very much for the suggestion. > Unless the set-up to test this change needs some special care, we > usually do not write such a thing in our proposed log message. Oh, I see. I'll follow this convention. I've moved the build validation details below the separator in v3. > I would probably have added a Helped-by: > to credit j6t, though. I've added Helped-by trailers for both Johannes Sixt and René Scharfe. I'm grateful to both for their guidance on this fix. > Is that a change, meaning v1 was sent without building, linking and > testing? Ah, sorry for the confusion. v1 was also compiled with MinGW and checked with a Win64 probe under Wine. I should have listed the v2 validation separately rather than under "Changes since v1". I've sent v3 separately with these message updates and no code changes. Thank you very much! Thanks, Yongqiang On Tue, 22 Sept 2026 at 03:18, Junio C Hamano wrote: > > Yongqiang Tian writes: > > > During WinANSI initialization, duplicate_handle() reports the handle > > when DuplicateHandle() fails. die_lasterr() collects the formatting > > arguments in a va_list, but passes that va_list to die_errno() as an > > ordinary variadic argument. die_errno() consequently formats part of > > the va_list representation instead of the supplied handle, producing > > an incorrect fatal message. > > Interesting. > > It's a shame that nobody noticed the broken calling sequence since > the bogosity was first introduced into the codebase at eac14f8909 > (Win32: Thread-safe windows console output, 2012-01-14). > > > The helper also converts GetLastError() to errno, losing the exact > > Windows error code. > > > > Remove die_lasterr() and report GetLastError() directly at its four > > call sites, following the existing Windows diagnostic style. This > > passes the handle to the formatter correctly and preserves the Windows > > error code. Keep the existing %li representation of the handle. > > OK. > > > With MinGW GCC 13, compat/winansi.o builds with DEVELOPER=1 and the > > complete git.exe builds and links. > > I am puzzled here. What's the relevance of these two lines? > > Are you telling us that how you have built and tested the patch? > Unless the set-up to test this change needs some special care, we > usually do not write such a thing in our proposed log message. > > > Signed-off-by: Yongqiang Tian > > --- > > > > Changes since v1: > > - replace die_lasterr() with direct die() calls; > > - preserve exact GetLastError() values instead of mapping them to errno; > > - follow the existing Windows diagnostic style and retain %li for the > > handle; > > Good collaboration. If I were doing this commit, judging from the > discussion on v1 iteration, I would probably have added a Helped-by: > to credit j6t, though. > > > - verify compat/winansi.o with DEVELOPER=1 and build and link the > > complete git.exe with MinGW GCC 13. > > Is that a change, meaning v1 was sent without building, linking and > testing? Improving on that is a very welcome thing ;-). > > > compat/winansi.c | 19 +++++-------------- > > 1 file changed, 5 insertions(+), 14 deletions(-) > > Nice. > > > -static void die_lasterr(const char *fmt, ...) > > -{ > > - va_list params; > > - va_start(params, fmt); > > - errno = err_win_to_posix(GetLastError()); > > - die_errno(fmt, params); > > - va_end(params); > > -} > > Very good to see this go. ``` ## Johannes Sixt, 2026-09-23 04:41 Subject: Re: [PATCH v3] compat/winansi: fix die_lasterr() argument formatting Message-ID: <3e2befed-355b-4a82-af82-25dedaee0565@kdbg.org> In-Reply-To: <20260921234756.77997-1-yqtian668@gmail.com> ``` Am 22.09.26 um 01:47 schrieb Yongqiang Tian: > During WinANSI initialization, duplicate_handle() reports the handle > when DuplicateHandle() fails. die_lasterr() collects the formatting > arguments in a va_list, but passes that va_list to die_errno() as an > ordinary variadic argument. die_errno() consequently formats part of > the va_list representation instead of the supplied handle, producing > an incorrect fatal message. > > The helper also converts GetLastError() to errno, losing the exact > Windows error code. > > Remove die_lasterr() and report GetLastError() directly at its four > call sites, following the existing Windows diagnostic style. This > passes the handle to the formatter correctly and preserves the Windows > error code. Keep the existing %li representation of the handle. > > Helped-by: Johannes Sixt > Helped-by: René Scharfe > Signed-off-by: Yongqiang Tian > --- > > Changes since v2: > - Add Helped-by trailers for Johannes Sixt and René Scharfe. > - Move build validation details below the separator. > - No code changes. This round looks very good now. I tested it and it works as desired. Thanks! FWIW: Acked-by: Johannes Sixt > > Validation (performed for v2; the code is unchanged): > - Built compat/winansi.o with DEVELOPER=1 using MinGW GCC 13. > - Built and linked the complete git.exe. > > compat/winansi.c | 19 +++++-------------- > 1 file changed, 5 insertions(+), 14 deletions(-) > > diff --git a/compat/winansi.c b/compat/winansi.c > index 3ce1900939..088734a1df 100644 > --- a/compat/winansi.c > +++ b/compat/winansi.c > @@ -436,15 +436,6 @@ static void winansi_exit(void) > CloseHandle(hthread); > } > > -static void die_lasterr(const char *fmt, ...) > -{ > - va_list params; > - va_start(params, fmt); > - errno = err_win_to_posix(GetLastError()); > - die_errno(fmt, params); > - va_end(params); > -} > - > #undef dup2 > int winansi_dup2(int oldfd, int newfd) > { > @@ -462,8 +453,8 @@ static HANDLE duplicate_handle(HANDLE hnd) > HANDLE hresult, hproc = GetCurrentProcess(); > if (!DuplicateHandle(hproc, hnd, hproc, &hresult, 0, TRUE, > DUPLICATE_SAME_ACCESS)) > - die_lasterr("DuplicateHandle(%li) failed", > - (long) (intptr_t) hnd); > + die("DuplicateHandle(%li) failed: %lu", > + (long) (intptr_t) hnd, GetLastError()); > return hresult; > } > > @@ -609,16 +600,16 @@ void winansi_init(void) > hwrite = CreateNamedPipeW(name, PIPE_ACCESS_OUTBOUND, > PIPE_TYPE_BYTE | PIPE_WAIT, 1, BUFFER_SIZE, 0, 0, NULL); > if (hwrite == INVALID_HANDLE_VALUE) > - die_lasterr("CreateNamedPipe failed"); > + die("CreateNamedPipe failed: %lu", GetLastError()); > > hread = CreateFileW(name, GENERIC_READ, 0, NULL, OPEN_EXISTING, 0, NULL); > if (hread == INVALID_HANDLE_VALUE) > - die_lasterr("CreateFile for named pipe failed"); > + die("CreateFile for named pipe failed: %lu", GetLastError()); > > /* start console spool thread on the pipe's read end */ > hthread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL); > if (!hthread) > - die_lasterr("CreateThread(console_thread) failed"); > + die("CreateThread(console_thread) failed: %lu", GetLastError()); > > /* schedule cleanup routine */ > if (atexit(winansi_exit)) ```