{"thread":{"id":"66335","subject":"[PATCH] compat/winansi: fix die_lasterr() argument formatting","startedAt":"2026-09-16T04:23:18Z","lastAt":"2026-09-23T04:41:58Z","messageCount":12,"participants":["Yongqiang Tian","Junio C Hamano","Johannes Sixt","René Scharfe"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"552777","messageId":"20260916042312.35891-1-yqtian668@gmail.com","threadId":"66335","inReplyTo":null,"subject":"[PATCH] compat/winansi: fix die_lasterr() argument formatting","fromName":"Yongqiang Tian","fromEmail":"yqtian668@gmail.com","sentAt":"2026-09-16T04:23:12Z","receivedAt":"2026-09-16T04:23:18Z","isPatch":true,"body":"During WinANSI initialization, duplicate_handle() reports the handle\nwhen DuplicateHandle() fails:\n\n die_lasterr(\"DuplicateHandle(%li) failed\", ...);\n\ndie_lasterr() collects the formatting arguments in a va_list, but\npasses that va_list to die_errno() as an ordinary variadic argument.\ndie_errno() consequently formats the representation of the va_list\ninstead of the supplied handle, producing an incorrect fatal message.\nThe other current callers pass fixed strings and are unaffected.\n\nGit does not provide a va_list-taking variant of die_errno(), so format\nthe caller's arguments separately with strbuf_vaddf(). This consumes the\noriginal va_list correctly and produces the complete diagnostic prefix,\nincluding the handle supplied by duplicate_handle().\n\nSave GetLastError() before formatting because calls made while growing\nthe strbuf may change the thread's Windows error value. Convert the\nsaved value to errno only after formatting, then pass the completed\nmessage to die_errno() through a literal \"%s\". This prevents any percent\ncharacters in the formatted message from being interpreted a second\ntime, while allowing die_errno() to append the corresponding system\nerror and terminate as before.\n\nThe updated compat/winansi.c compiles with MinGW GCC 13. A Win64 probe\nunder Wine prints a value derived from the va_list before this change\nand the supplied integer afterward.\n\nSigned-off-by: Yongqiang Tian \n---\n compat/winansi.c | 9 +++++++--\n 1 file changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex 3ce190093..5547192a2 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -7,6 +7,7 @@\n #define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"../git-compat-util.h\"\n+#include \"../strbuf.h\"\n #include \n #include \n #include \"win32.h\"\n@@ -438,11 +439,15 @@ static void winansi_exit(void)\n \n static void die_lasterr(const char *fmt, ...)\n {\n+\tDWORD err = GetLastError();\n+\tstruct strbuf message = STRBUF_INIT;\n \tva_list params;\n+\n \tva_start(params, fmt);\n-\terrno = err_win_to_posix(GetLastError());\n-\tdie_errno(fmt, params);\n+\tstrbuf_vaddf(&message, fmt, params);\n \tva_end(params);\n+\terrno = err_win_to_posix(err);\n+\tdie_errno(\"%s\", message.buf);\n }\n \n #undef dup2\n-- \n2.34.1\n\n"},{"id":"552778","messageId":"xmqqh5jpyg2p.fsf@gitster.g","threadId":"66335","inReplyTo":"20260916042312.35891-1-yqtian668@gmail.com","subject":"Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-16T04:29:18Z","receivedAt":"2026-09-16T04:29:21Z","isPatch":true,"body":"Yongqiang Tian writes:\n\n> During WinANSI initialization, duplicate_handle() reports the handle\n> when DuplicateHandle() fails:\n>\n> die_lasterr(\"DuplicateHandle(%li) failed\", ...);\n> ...\n\nI do not know about Patrick, but I do not do Windows, so please do\nnot Cc: me a patch that is primarily about Windows portability.\n\nI'll add two whose with contributions much greater than I have in\nthe area to Cc: list.\n\nThanks.\n\n> die_lasterr() collects the formatting arguments in a va_list, but\n> passes that va_list to die_errno() as an ordinary variadic argument.\n> die_errno() consequently formats the representation of the va_list\n> instead of the supplied handle, producing an incorrect fatal message.\n> The other current callers pass fixed strings and are unaffected.\n>\n> Git does not provide a va_list-taking variant of die_errno(), so format\n> the caller's arguments separately with strbuf_vaddf(). This consumes the\n> original va_list correctly and produces the complete diagnostic prefix,\n> including the handle supplied by duplicate_handle().\n>\n> Save GetLastError() before formatting because calls made while growing\n> the strbuf may change the thread's Windows error value. Convert the\n> saved value to errno only after formatting, then pass the completed\n> message to die_errno() through a literal \"%s\". This prevents any percent\n> characters in the formatted message from being interpreted a second\n> time, while allowing die_errno() to append the corresponding system\n> error and terminate as before.\n>\n> The updated compat/winansi.c compiles with MinGW GCC 13. A Win64 probe\n> under Wine prints a value derived from the va_list before this change\n> and the supplied integer afterward.\n>\n> Signed-off-by: Yongqiang Tian \n> ---\n> compat/winansi.c | 9 +++++++--\n> 1 file changed, 7 insertions(+), 2 deletions(-)\n>\n> diff --git a/compat/winansi.c b/compat/winansi.c\n> index 3ce190093..5547192a2 100644\n> --- a/compat/winansi.c\n> +++ b/compat/winansi.c\n> @@ -7,6 +7,7 @@\n> #define DISABLE_SIGN_COMPARE_WARNINGS\n> \n> #include \"../git-compat-util.h\"\n> +#include \"../strbuf.h\"\n> #include \n> #include \n> #include \"win32.h\"\n> @@ -438,11 +439,15 @@ static void winansi_exit(void)\n> \n> static void die_lasterr(const char *fmt, ...)\n> {\n> +\tDWORD err = GetLastError();\n> +\tstruct strbuf message = STRBUF_INIT;\n> \tva_list params;\n> +\n> \tva_start(params, fmt);\n> -\terrno = err_win_to_posix(GetLastError());\n> -\tdie_errno(fmt, params);\n> +\tstrbuf_vaddf(&message, fmt, params);\n> \tva_end(params);\n> +\terrno = err_win_to_posix(err);\n> +\tdie_errno(\"%s\", message.buf);\n> }\n> \n> #undef dup2\n"},{"id":"552781","messageId":"1cb6ad12-27bc-458e-b8f5-4b7eb44356fe@kdbg.org","threadId":"66335","inReplyTo":"20260916042312.35891-1-yqtian668@gmail.com","subject":"Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2026-09-16T06:13:15Z","receivedAt":"2026-09-16T06:13:32Z","isPatch":true,"body":"Am 16.09.26 um 06:23 schrieb Yongqiang Tian:\n> During WinANSI initialization, duplicate_handle() reports the handle\n> when DuplicateHandle() fails:\n> \n> die_lasterr(\"DuplicateHandle(%li) failed\", ...);\n\nThe full call is more like\n\n\tdie_lasterr(\"DuplicateHandle(%li) failed\",\n (long) (intptr_t) hnd);\n\nThis attempts to format the Windows handle value into the error message.\nThat's a pointless exercise, becaues AFAIK the value is totally opaque\nand unhelpful as a debugging aid.\n\nFor this reason, I'd suggest to go the simpler route to remove the\nformatting from the above call (the only one that passes more than just\na string) and have die_lasterr take just a single string and no variable\nargument list.\n\n-- Hannes\n\n"},{"id":"552782","messageId":"bf0351d8-05fe-4f77-958a-2ac59495029c@web.de","threadId":"66335","inReplyTo":"20260916042312.35891-1-yqtian668@gmail.com","subject":"Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-09-16T06:33:19Z","receivedAt":"2026-09-16T06:33:32Z","isPatch":true,"body":"On 9/16/26 6:23 AM, Yongqiang Tian wrote:\n> During WinANSI initialization, duplicate_handle() reports the handle\n> when DuplicateHandle() fails:\n> \n> die_lasterr(\"DuplicateHandle(%li) failed\", ...);\n> \n> die_lasterr() collects the formatting arguments in a va_list, but\n> passes that va_list to die_errno() as an ordinary variadic argument.\n> die_errno() consequently formats the representation of the va_list\n> instead of the supplied handle, producing an incorrect fatal message.\n\nGood find!\n\n> The other current callers pass fixed strings and are unaffected.\n> \n> Git does not provide a va_list-taking variant of die_errno(), so format\n> the caller's arguments separately with strbuf_vaddf(). This consumes the\n> original va_list correctly and produces the complete diagnostic prefix,\n> including the handle supplied by duplicate_handle().\n> \n> Save GetLastError() before formatting because calls made while growing\n> the strbuf may change the thread's Windows error value. Convert the\n> saved value to errno only after formatting, then pass the completed\n> message to die_errno() through a literal \"%s\". This prevents any percent\n> characters in the formatted message from being interpreted a second\n> time, while allowing die_errno() to append the corresponding system\n> error and terminate as before.\n\nThat all makes sense, but is quite complicated. die_errno() itself uses\na fixed-size buffer to avoid heap allocation, for robustness and to\navoid changing errno. How about turning die_lasterr() into a macro for\nthe same reasons?\n\n\t#define die_lasterr(...) do { \\\n\t\terrno = err_win_to_posix(GetLastError()); \\\n\t\tdie_errno(__VA_ARGS__); \\\n\t} while (0)\n\n> The updated compat/winansi.c compiles with MinGW GCC 13. A Win64 probe\n> under Wine prints a value derived from the va_list before this change\n> and the supplied integer afterward.\n> \n> Signed-off-by: Yongqiang Tian \n> ---\n> compat/winansi.c | 9 +++++++--\n> 1 file changed, 7 insertions(+), 2 deletions(-)\n> \n> diff --git a/compat/winansi.c b/compat/winansi.c\n> index 3ce190093..5547192a2 100644\n> --- a/compat/winansi.c\n> +++ b/compat/winansi.c\n> @@ -7,6 +7,7 @@\n> #define DISABLE_SIGN_COMPARE_WARNINGS\n> \n> #include \"../git-compat-util.h\"\n> +#include \"../strbuf.h\"\n> #include \n> #include \n> #include \"win32.h\"\n> @@ -438,11 +439,15 @@ static void winansi_exit(void)\n> \n> static void die_lasterr(const char *fmt, ...)\n> {\n> +\tDWORD err = GetLastError();\n> +\tstruct strbuf message = STRBUF_INIT;\n> \tva_list params;\n> +\n> \tva_start(params, fmt);\n> -\terrno = err_win_to_posix(GetLastError());\n> -\tdie_errno(fmt, params);\n> +\tstrbuf_vaddf(&message, fmt, params);\n> \tva_end(params);\n> +\terrno = err_win_to_posix(err);\n> +\tdie_errno(\"%s\", message.buf);\n> }\n> \n> #undef dup2\n\n"},{"id":"552783","messageId":"1e706ce8-bebd-4ac1-914a-a55195e7d253@kdbg.org","threadId":"66335","inReplyTo":"bf0351d8-05fe-4f77-958a-2ac59495029c@web.de","subject":"Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2026-09-16T07:09:51Z","receivedAt":"2026-09-16T07:10:04Z","isPatch":true,"body":"Am 16.09.26 um 08:33 schrieb René Scharfe:\n> That all makes sense, but is quite complicated. die_errno() itself uses\n> a fixed-size buffer to avoid heap allocation, for robustness and to\n> avoid changing errno. How about turning die_lasterr() into a macro for\n> the same reasons?\n> \n> \t#define die_lasterr(...) do { \\\n> \t\terrno = err_win_to_posix(GetLastError()); \\\n> \t\tdie_errno(__VA_ARGS__); \\\n> \t} while (0)\n\ndie_lasterr is used to diagnose errors of Windows functions. I dislike\nthat this degrades the exact error value of GetLastError() into an\nerrno. If this direction is persued, then we should remove die_errno\nfrom the picture.\n\nBut as I hinted elsewhere in the thread, this is all overengineered for\nno good reason.\n\n-- Hannes\n\n"},{"id":"552919","messageId":"CAEs0Zp4LK1ZHkL0tdi_k_=-u86HVm07FzyOJdO-=+DY9c3ZrYA@mail.gmail.com","threadId":"66335","inReplyTo":"1e706ce8-bebd-4ac1-914a-a55195e7d253@kdbg.org","subject":"Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting","fromName":"Yongqiang Tian","fromEmail":"yqtian668@gmail.com","sentAt":"2026-09-21T03:00:00Z","receivedAt":"2026-09-21T03:00:43Z","isPatch":true,"body":"Hi René and Hannes,\n\nThank you again for the feedback. I have been thinking about this over\nthe past week.\n\nI see three possible directions:\n\n1. Turn die_lasterr() into the variadic macro René suggested. This is\n the smallest change and avoids allocation, but still maps the\n original GetLastError() value to errno.\n\n2. Keep a function and format its variadic arguments into a fixed-size\n stack buffer. This would avoid allocation and could preserve the\n original Windows error code, but it adds more error-reporting\n machinery for only four call sites.\n\n3. Remove die_lasterr() and report GetLastError() directly at those\n call sites. This avoids the va_list forwarding, allocation, and\n errno conversion altogether.\n\nThe third direction now seems the simplest to me. It also follows\nexisting Windows-specific code in Git that reports GetLastError()\ndirectly, for example:\n\nhttps://github.com/git/git/blob/9a0c4701dcd5725c4184599322b52933ff5005ca/compat/win32/syslog.c#L10-L13\n\nand:\n\nhttps://github.com/git/git/blob/9a0c4701dcd5725c4184599322b52933ff5005ca/compat/fsmonitor/fsm-listen-win32.c#L106-L109\n\nThe resulting change would be approximately:\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@\n-static void die_lasterr(const char *fmt, ...)\n-{\n- va_list params;\n- va_start(params, fmt);\n- errno = err_win_to_posix(GetLastError());\n- die_errno(fmt, params);\n- va_end(params);\n-}\n-\n static HANDLE duplicate_handle(HANDLE hnd)\n {\n HANDLE hresult, hproc = GetCurrentProcess();\n if (!DuplicateHandle(hproc, hnd, hproc, &hresult, 0, TRUE,\n DUPLICATE_SAME_ACCESS))\n- die_lasterr(\"DuplicateHandle(%li) failed\",\n- (long) (intptr_t) hnd);\n+ die(\"DuplicateHandle(%p) failed: Windows error %lu\",\n+ (void *)hnd, GetLastError());\n return hresult;\n }\n@@\n if (hwrite == INVALID_HANDLE_VALUE)\n- die_lasterr(\"CreateNamedPipe failed\");\n+ die(\"CreateNamedPipe failed: Windows error %lu\",\n+ GetLastError());\n@@\n if (hread == INVALID_HANDLE_VALUE)\n- die_lasterr(\"CreateFile for named pipe failed\");\n+ die(\"CreateFile for named pipe failed: Windows error %lu\",\n+ GetLastError());\n@@\n if (!hthread)\n- die_lasterr(\"CreateThread(console_thread) failed\");\n+ die(\"CreateThread(console_thread) failed: Windows error %lu\",\n+ GetLastError());\n\nI used %p for the handle because HANDLE is pointer-sized, whereas long\nremains 32 bits on 64-bit Windows. That could also be kept separate if\nyou would prefer this revision to address only the forwarding issue.\n\nWould this be a preferable direction? If so, I would be happy to prepare\nand test a revised patch.\n\nAny suggestions would be really appreciated.\n\nThanks,\nYongqiang\n\nOn Wed, 16 Sept 2026 at 17:09, Johannes Sixt wrote:\n>\n> Am 16.09.26 um 08:33 schrieb René Scharfe:\n> > That all makes sense, but is quite complicated. die_errno() itself uses\n> > a fixed-size buffer to avoid heap allocation, for robustness and to\n> > avoid changing errno. How about turning die_lasterr() into a macro for\n> > the same reasons?\n> >\n> > #define die_lasterr(...) do { \\\n> > errno = err_win_to_posix(GetLastError()); \\\n> > die_errno(__VA_ARGS__); \\\n> > } while (0)\n>\n> die_lasterr is used to diagnose errors of Windows functions. I dislike\n> that this degrades the exact error value of GetLastError() into an\n> errno. If this direction is persued, then we should remove die_errno\n> from the picture.\n>\n> But as I hinted elsewhere in the thread, this is all overengineered for\n> no good reason.\n>\n> -- Hannes\n>\n"},{"id":"552920","messageId":"9c1cde3d-92c4-4684-834e-bae97fd33f5f@kdbg.org","threadId":"66335","inReplyTo":"CAEs0Zp4LK1ZHkL0tdi_k_=-u86HVm07FzyOJdO-=+DY9c3ZrYA@mail.gmail.com","subject":"Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2026-09-21T04:06:52Z","receivedAt":"2026-09-21T04:07:02Z","isPatch":true,"body":"Am 21.09.26 um 05:00 schrieb Yongqiang Tian:\n> 3. Remove die_lasterr() and report GetLastError() directly at those\n> call sites. This avoids the va_list forwarding, allocation, and\n> errno conversion altogether.\n> \n> The third direction now seems the simplest to me. It also follows\n> existing Windows-specific code in Git that reports GetLastError()\n> directly, for example:\n> \n> https://github.com/git/git/blob/9a0c4701dcd5725c4184599322b52933ff5005ca/compat/win32/syslog.c#L10-L13\n> \n> and:\n> \n> https://github.com/git/git/blob/9a0c4701dcd5725c4184599322b52933ff5005ca/compat/fsmonitor/fsm-listen-win32.c#L106-L109\n\nSounds reasonable to me.\n\n> - die_lasterr(\"DuplicateHandle(%li) failed\",\n> - (long) (intptr_t) hnd);\n> + die(\"DuplicateHandle(%p) failed: Windows error %lu\",\n> + (void *)hnd, GetLastError());\nBut please leave the conversion to %p for another time.\n\nConcerning the text \"Windows error\", please follow existing practice.\n\n-- Hannes\n\n"},{"id":"552921","messageId":"20260921062114.14450-1-yqtian668@gmail.com","threadId":"66335","inReplyTo":"20260916042312.35891-1-yqtian668@gmail.com","subject":"[PATCH v2] compat/winansi: fix die_lasterr() argument formatting","fromName":"Yongqiang Tian","fromEmail":"yqtian668@gmail.com","sentAt":"2026-09-21T06:20:53Z","receivedAt":"2026-09-21T06:21:32Z","isPatch":true,"body":"During WinANSI initialization, duplicate_handle() reports the handle\nwhen DuplicateHandle() fails. die_lasterr() collects the formatting\narguments in a va_list, but passes that va_list to die_errno() as an\nordinary variadic argument. die_errno() consequently formats part of\nthe va_list representation instead of the supplied handle, producing\nan incorrect fatal message.\n\nThe helper also converts GetLastError() to errno, losing the exact\nWindows error code.\n\nRemove die_lasterr() and report GetLastError() directly at its four\ncall sites, following the existing Windows diagnostic style. This\npasses the handle to the formatter correctly and preserves the Windows\nerror code. Keep the existing %li representation of the handle.\n\nWith MinGW GCC 13, compat/winansi.o builds with DEVELOPER=1 and the\ncomplete git.exe builds and links.\n\nSigned-off-by: Yongqiang Tian \n---\n\nChanges since v1:\n- replace die_lasterr() with direct die() calls;\n- preserve exact GetLastError() values instead of mapping them to errno;\n- follow the existing Windows diagnostic style and retain %li for the\n handle;\n- verify compat/winansi.o with DEVELOPER=1 and build and link the\n complete git.exe with MinGW GCC 13.\n\n compat/winansi.c | 19 +++++--------------\n 1 file changed, 5 insertions(+), 14 deletions(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex 3ce1900939..088734a1df 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -436,15 +436,6 @@ static void winansi_exit(void)\n \tCloseHandle(hthread);\n }\n \n-static void die_lasterr(const char *fmt, ...)\n-{\n-\tva_list params;\n-\tva_start(params, fmt);\n-\terrno = err_win_to_posix(GetLastError());\n-\tdie_errno(fmt, params);\n-\tva_end(params);\n-}\n-\n #undef dup2\n int winansi_dup2(int oldfd, int newfd)\n {\n@@ -462,8 +453,8 @@ static HANDLE duplicate_handle(HANDLE hnd)\n \tHANDLE hresult, hproc = GetCurrentProcess();\n \tif (!DuplicateHandle(hproc, hnd, hproc, &hresult, 0, TRUE,\n \t\t\tDUPLICATE_SAME_ACCESS))\n-\t\tdie_lasterr(\"DuplicateHandle(%li) failed\",\n-\t\t\t(long) (intptr_t) hnd);\n+\t\tdie(\"DuplicateHandle(%li) failed: %lu\",\n+\t\t (long) (intptr_t) hnd, GetLastError());\n \treturn hresult;\n }\n \n@@ -609,16 +600,16 @@ void winansi_init(void)\n \thwrite = CreateNamedPipeW(name, PIPE_ACCESS_OUTBOUND,\n \t\tPIPE_TYPE_BYTE | PIPE_WAIT, 1, BUFFER_SIZE, 0, 0, NULL);\n \tif (hwrite == INVALID_HANDLE_VALUE)\n-\t\tdie_lasterr(\"CreateNamedPipe failed\");\n+\t\tdie(\"CreateNamedPipe failed: %lu\", GetLastError());\n \n \thread = CreateFileW(name, GENERIC_READ, 0, NULL, OPEN_EXISTING, 0, NULL);\n \tif (hread == INVALID_HANDLE_VALUE)\n-\t\tdie_lasterr(\"CreateFile for named pipe failed\");\n+\t\tdie(\"CreateFile for named pipe failed: %lu\", GetLastError());\n \n \t/* start console spool thread on the pipe's read end */\n \ththread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL);\n \tif (!hthread)\n-\t\tdie_lasterr(\"CreateThread(console_thread) failed\");\n+\t\tdie(\"CreateThread(console_thread) failed: %lu\", GetLastError());\n \n \t/* schedule cleanup routine */\n \tif (atexit(winansi_exit))\n-- \n2.34.1\n"},{"id":"552941","messageId":"xmqqwlsemsjt.fsf@gitster.g","threadId":"66335","inReplyTo":"20260921062114.14450-1-yqtian668@gmail.com","subject":"Re: [PATCH v2] compat/winansi: fix die_lasterr() argument formatting","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-09-21T17:18:46Z","receivedAt":"2026-09-21T17:18:49Z","isPatch":true,"body":"Yongqiang Tian writes:\n\n> During WinANSI initialization, duplicate_handle() reports the handle\n> when DuplicateHandle() fails. die_lasterr() collects the formatting\n> arguments in a va_list, but passes that va_list to die_errno() as an\n> ordinary variadic argument. die_errno() consequently formats part of\n> the va_list representation instead of the supplied handle, producing\n> an incorrect fatal message.\n\nInteresting.\n\nIt's a shame that nobody noticed the broken calling sequence since\nthe bogosity was first introduced into the codebase at eac14f8909\n(Win32: Thread-safe windows console output, 2012-01-14).\n\n> The helper also converts GetLastError() to errno, losing the exact\n> Windows error code.\n>\n> Remove die_lasterr() and report GetLastError() directly at its four\n> call sites, following the existing Windows diagnostic style. This\n> passes the handle to the formatter correctly and preserves the Windows\n> error code. Keep the existing %li representation of the handle.\n\nOK.\n\n> With MinGW GCC 13, compat/winansi.o builds with DEVELOPER=1 and the\n> complete git.exe builds and links.\n\nI am puzzled here. What's the relevance of these two lines?\n\nAre you telling us that how you have built and tested the patch?\nUnless the set-up to test this change needs some special care, we\nusually do not write such a thing in our proposed log message.\n\n> Signed-off-by: Yongqiang Tian \n> ---\n>\n> Changes since v1:\n> - replace die_lasterr() with direct die() calls;\n> - preserve exact GetLastError() values instead of mapping them to errno;\n> - follow the existing Windows diagnostic style and retain %li for the\n> handle;\n\nGood collaboration. If I were doing this commit, judging from the\ndiscussion on v1 iteration, I would probably have added a Helped-by:\nto credit j6t, though.\n\n> - verify compat/winansi.o with DEVELOPER=1 and build and link the\n> complete git.exe with MinGW GCC 13.\n\nIs that a change, meaning v1 was sent without building, linking and\ntesting? Improving on that is a very welcome thing ;-).\n\n> compat/winansi.c | 19 +++++--------------\n> 1 file changed, 5 insertions(+), 14 deletions(-)\n\nNice.\n\n> -static void die_lasterr(const char *fmt, ...)\n> -{\n> -\tva_list params;\n> -\tva_start(params, fmt);\n> -\terrno = err_win_to_posix(GetLastError());\n> -\tdie_errno(fmt, params);\n> -\tva_end(params);\n> -}\n\nVery good to see this go.\n"},{"id":"552961","messageId":"20260921234756.77997-1-yqtian668@gmail.com","threadId":"66335","inReplyTo":"20260921062114.14450-1-yqtian668@gmail.com","subject":"[PATCH v3] compat/winansi: fix die_lasterr() argument formatting","fromName":"Yongqiang Tian","fromEmail":"yqtian668@gmail.com","sentAt":"2026-09-21T23:47:56Z","receivedAt":"2026-09-21T23:48:02Z","isPatch":true,"body":"During WinANSI initialization, duplicate_handle() reports the handle\nwhen DuplicateHandle() fails. die_lasterr() collects the formatting\narguments in a va_list, but passes that va_list to die_errno() as an\nordinary variadic argument. die_errno() consequently formats part of\nthe va_list representation instead of the supplied handle, producing\nan incorrect fatal message.\n\nThe helper also converts GetLastError() to errno, losing the exact\nWindows error code.\n\nRemove die_lasterr() and report GetLastError() directly at its four\ncall sites, following the existing Windows diagnostic style. This\npasses the handle to the formatter correctly and preserves the Windows\nerror code. Keep the existing %li representation of the handle.\n\nHelped-by: Johannes Sixt \nHelped-by: René Scharfe \nSigned-off-by: Yongqiang Tian \n---\n\nChanges since v2:\n- Add Helped-by trailers for Johannes Sixt and René Scharfe.\n- Move build validation details below the separator.\n- No code changes.\n\nValidation (performed for v2; the code is unchanged):\n- Built compat/winansi.o with DEVELOPER=1 using MinGW GCC 13.\n- Built and linked the complete git.exe.\n\n compat/winansi.c | 19 +++++--------------\n 1 file changed, 5 insertions(+), 14 deletions(-)\n\ndiff --git a/compat/winansi.c b/compat/winansi.c\nindex 3ce1900939..088734a1df 100644\n--- a/compat/winansi.c\n+++ b/compat/winansi.c\n@@ -436,15 +436,6 @@ static void winansi_exit(void)\n \tCloseHandle(hthread);\n }\n \n-static void die_lasterr(const char *fmt, ...)\n-{\n-\tva_list params;\n-\tva_start(params, fmt);\n-\terrno = err_win_to_posix(GetLastError());\n-\tdie_errno(fmt, params);\n-\tva_end(params);\n-}\n-\n #undef dup2\n int winansi_dup2(int oldfd, int newfd)\n {\n@@ -462,8 +453,8 @@ static HANDLE duplicate_handle(HANDLE hnd)\n \tHANDLE hresult, hproc = GetCurrentProcess();\n \tif (!DuplicateHandle(hproc, hnd, hproc, &hresult, 0, TRUE,\n \t\t\tDUPLICATE_SAME_ACCESS))\n-\t\tdie_lasterr(\"DuplicateHandle(%li) failed\",\n-\t\t\t(long) (intptr_t) hnd);\n+\t\tdie(\"DuplicateHandle(%li) failed: %lu\",\n+\t\t (long) (intptr_t) hnd, GetLastError());\n \treturn hresult;\n }\n \n@@ -609,16 +600,16 @@ void winansi_init(void)\n \thwrite = CreateNamedPipeW(name, PIPE_ACCESS_OUTBOUND,\n \t\tPIPE_TYPE_BYTE | PIPE_WAIT, 1, BUFFER_SIZE, 0, 0, NULL);\n \tif (hwrite == INVALID_HANDLE_VALUE)\n-\t\tdie_lasterr(\"CreateNamedPipe failed\");\n+\t\tdie(\"CreateNamedPipe failed: %lu\", GetLastError());\n \n \thread = CreateFileW(name, GENERIC_READ, 0, NULL, OPEN_EXISTING, 0, NULL);\n \tif (hread == INVALID_HANDLE_VALUE)\n-\t\tdie_lasterr(\"CreateFile for named pipe failed\");\n+\t\tdie(\"CreateFile for named pipe failed: %lu\", GetLastError());\n \n \t/* start console spool thread on the pipe's read end */\n \ththread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL);\n \tif (!hthread)\n-\t\tdie_lasterr(\"CreateThread(console_thread) failed\");\n+\t\tdie(\"CreateThread(console_thread) failed: %lu\", GetLastError());\n \n \t/* schedule cleanup routine */\n \tif (atexit(winansi_exit))\n-- \n2.34.1\n"},{"id":"552962","messageId":"CAEs0Zp7M3qtAznHj_0yyab7e0xDLWjd3+Ja1Yr3GfEPJZQr+Vw@mail.gmail.com","threadId":"66335","inReplyTo":"xmqqwlsemsjt.fsf@gitster.g","subject":"Re: [PATCH v2] compat/winansi: fix die_lasterr() argument formatting","fromName":"Yongqiang Tian","fromEmail":"yqtian668@gmail.com","sentAt":"2026-09-21T23:49:41Z","receivedAt":"2026-09-21T23:49:53Z","isPatch":true,"body":"Hi Junio,\n\nThank you very much for the suggestion.\n\n> Unless the set-up to test this change needs some special care, we\n> usually do not write such a thing in our proposed log message.\n\nOh, I see. I'll follow this convention. I've moved the build validation\ndetails below the separator in v3.\n\n> I would probably have added a Helped-by:\n> to credit j6t, though.\n\nI've added Helped-by trailers for both Johannes Sixt and René Scharfe.\nI'm grateful to both for their guidance on this fix.\n\n> Is that a change, meaning v1 was sent without building, linking and\n> testing?\n\nAh, sorry for the confusion. v1 was also compiled with MinGW and checked\nwith a Win64 probe under Wine. I should have listed the v2 validation\nseparately rather than under \"Changes since v1\".\n\nI've sent v3 separately with these message updates and no code changes.\n\nThank you very much!\n\nThanks,\nYongqiang\n\nOn Tue, 22 Sept 2026 at 03:18, Junio C Hamano wrote:\n>\n> Yongqiang Tian writes:\n>\n> > During WinANSI initialization, duplicate_handle() reports the handle\n> > when DuplicateHandle() fails. die_lasterr() collects the formatting\n> > arguments in a va_list, but passes that va_list to die_errno() as an\n> > ordinary variadic argument. die_errno() consequently formats part of\n> > the va_list representation instead of the supplied handle, producing\n> > an incorrect fatal message.\n>\n> Interesting.\n>\n> It's a shame that nobody noticed the broken calling sequence since\n> the bogosity was first introduced into the codebase at eac14f8909\n> (Win32: Thread-safe windows console output, 2012-01-14).\n>\n> > The helper also converts GetLastError() to errno, losing the exact\n> > Windows error code.\n> >\n> > Remove die_lasterr() and report GetLastError() directly at its four\n> > call sites, following the existing Windows diagnostic style. This\n> > passes the handle to the formatter correctly and preserves the Windows\n> > error code. Keep the existing %li representation of the handle.\n>\n> OK.\n>\n> > With MinGW GCC 13, compat/winansi.o builds with DEVELOPER=1 and the\n> > complete git.exe builds and links.\n>\n> I am puzzled here. What's the relevance of these two lines?\n>\n> Are you telling us that how you have built and tested the patch?\n> Unless the set-up to test this change needs some special care, we\n> usually do not write such a thing in our proposed log message.\n>\n> > Signed-off-by: Yongqiang Tian \n> > ---\n> >\n> > Changes since v1:\n> > - replace die_lasterr() with direct die() calls;\n> > - preserve exact GetLastError() values instead of mapping them to errno;\n> > - follow the existing Windows diagnostic style and retain %li for the\n> > handle;\n>\n> Good collaboration. If I were doing this commit, judging from the\n> discussion on v1 iteration, I would probably have added a Helped-by:\n> to credit j6t, though.\n>\n> > - verify compat/winansi.o with DEVELOPER=1 and build and link the\n> > complete git.exe with MinGW GCC 13.\n>\n> Is that a change, meaning v1 was sent without building, linking and\n> testing? Improving on that is a very welcome thing ;-).\n>\n> > compat/winansi.c | 19 +++++--------------\n> > 1 file changed, 5 insertions(+), 14 deletions(-)\n>\n> Nice.\n>\n> > -static void die_lasterr(const char *fmt, ...)\n> > -{\n> > - va_list params;\n> > - va_start(params, fmt);\n> > - errno = err_win_to_posix(GetLastError());\n> > - die_errno(fmt, params);\n> > - va_end(params);\n> > -}\n>\n> Very good to see this go.\n"},{"id":"553030","messageId":"3e2befed-355b-4a82-af82-25dedaee0565@kdbg.org","threadId":"66335","inReplyTo":"20260921234756.77997-1-yqtian668@gmail.com","subject":"Re: [PATCH v3] compat/winansi: fix die_lasterr() argument formatting","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2026-09-23T04:41:47Z","receivedAt":"2026-09-23T04:41:58Z","isPatch":true,"body":"Am 22.09.26 um 01:47 schrieb Yongqiang Tian:\n> During WinANSI initialization, duplicate_handle() reports the handle\n> when DuplicateHandle() fails. die_lasterr() collects the formatting\n> arguments in a va_list, but passes that va_list to die_errno() as an\n> ordinary variadic argument. die_errno() consequently formats part of\n> the va_list representation instead of the supplied handle, producing\n> an incorrect fatal message.\n> \n> The helper also converts GetLastError() to errno, losing the exact\n> Windows error code.\n> \n> Remove die_lasterr() and report GetLastError() directly at its four\n> call sites, following the existing Windows diagnostic style. This\n> passes the handle to the formatter correctly and preserves the Windows\n> error code. Keep the existing %li representation of the handle.\n> \n> Helped-by: Johannes Sixt \n> Helped-by: René Scharfe \n> Signed-off-by: Yongqiang Tian \n> ---\n> \n> Changes since v2:\n> - Add Helped-by trailers for Johannes Sixt and René Scharfe.\n> - Move build validation details below the separator.\n> - No code changes.\n\nThis round looks very good now. I tested it and it works as desired.\nThanks! FWIW:\n\nAcked-by: Johannes Sixt \n\n> \n> Validation (performed for v2; the code is unchanged):\n> - Built compat/winansi.o with DEVELOPER=1 using MinGW GCC 13.\n> - Built and linked the complete git.exe.\n> \n> compat/winansi.c | 19 +++++--------------\n> 1 file changed, 5 insertions(+), 14 deletions(-)\n> \n> diff --git a/compat/winansi.c b/compat/winansi.c\n> index 3ce1900939..088734a1df 100644\n> --- a/compat/winansi.c\n> +++ b/compat/winansi.c\n> @@ -436,15 +436,6 @@ static void winansi_exit(void)\n> \tCloseHandle(hthread);\n> }\n> \n> -static void die_lasterr(const char *fmt, ...)\n> -{\n> -\tva_list params;\n> -\tva_start(params, fmt);\n> -\terrno = err_win_to_posix(GetLastError());\n> -\tdie_errno(fmt, params);\n> -\tva_end(params);\n> -}\n> -\n> #undef dup2\n> int winansi_dup2(int oldfd, int newfd)\n> {\n> @@ -462,8 +453,8 @@ static HANDLE duplicate_handle(HANDLE hnd)\n> \tHANDLE hresult, hproc = GetCurrentProcess();\n> \tif (!DuplicateHandle(hproc, hnd, hproc, &hresult, 0, TRUE,\n> \t\t\tDUPLICATE_SAME_ACCESS))\n> -\t\tdie_lasterr(\"DuplicateHandle(%li) failed\",\n> -\t\t\t(long) (intptr_t) hnd);\n> +\t\tdie(\"DuplicateHandle(%li) failed: %lu\",\n> +\t\t (long) (intptr_t) hnd, GetLastError());\n> \treturn hresult;\n> }\n> \n> @@ -609,16 +600,16 @@ void winansi_init(void)\n> \thwrite = CreateNamedPipeW(name, PIPE_ACCESS_OUTBOUND,\n> \t\tPIPE_TYPE_BYTE | PIPE_WAIT, 1, BUFFER_SIZE, 0, 0, NULL);\n> \tif (hwrite == INVALID_HANDLE_VALUE)\n> -\t\tdie_lasterr(\"CreateNamedPipe failed\");\n> +\t\tdie(\"CreateNamedPipe failed: %lu\", GetLastError());\n> \n> \thread = CreateFileW(name, GENERIC_READ, 0, NULL, OPEN_EXISTING, 0, NULL);\n> \tif (hread == INVALID_HANDLE_VALUE)\n> -\t\tdie_lasterr(\"CreateFile for named pipe failed\");\n> +\t\tdie(\"CreateFile for named pipe failed: %lu\", GetLastError());\n> \n> \t/* start console spool thread on the pipe's read end */\n> \ththread = CreateThread(NULL, 0, console_thread, NULL, 0, NULL);\n> \tif (!hthread)\n> -\t\tdie_lasterr(\"CreateThread(console_thread) failed\");\n> +\t\tdie(\"CreateThread(console_thread) failed: %lu\", GetLastError());\n> \n> \t/* schedule cleanup routine */\n> \tif (atexit(winansi_exit))\n\n"}]}