[PATCH] compat/winansi: fix die_lasterr() argument formatting

[PATCH] compat/winansi: fix die_lasterr() argument formatting

[Yongqiang Tian]

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 <yqtian668@gmail.com>
---
 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 <wingdi.h>
 #include <winreg.h>
 #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

Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting

[Junio C Hamano]

Yongqiang Tian <yqtian668@gmail.com> 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 <yqtian668@gmail.com>
> ---
>  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 <wingdi.h>
>  #include <winreg.h>
>  #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

Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting

[Johannes Sixt]

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

Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting

[René Scharfe]

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 <yqtian668@gmail.com>
> ---
>  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 <wingdi.h>
>  #include <winreg.h>
>  #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

Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting

[Johannes Sixt]

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

Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting

[Yongqiang Tian]

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 <j6t@kdbg.org> 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
>

Re: [PATCH] compat/winansi: fix die_lasterr() argument formatting

[Johannes Sixt]

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

[PATCH v2] compat/winansi: fix die_lasterr() argument formatting

[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.

With MinGW GCC 13, compat/winansi.o builds with DEVELOPER=1 and the
complete git.exe builds and links.

Signed-off-by: Yongqiang Tian <yqtian668@gmail.com>
---

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

Re: [PATCH v2] compat/winansi: fix die_lasterr() argument formatting

[Junio C Hamano]

Yongqiang Tian <yqtian668@gmail.com> 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 <yqtian668@gmail.com>
> ---
>
> 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.

[PATCH v3] compat/winansi: fix die_lasterr() argument formatting

[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 <j6t@kdbg.org>
Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Yongqiang Tian <yqtian668@gmail.com>
---

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

Re: [PATCH v2] compat/winansi: fix die_lasterr() argument formatting

[Yongqiang Tian]

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 <gitster@pobox.com> wrote:
>
> Yongqiang Tian <yqtian668@gmail.com> 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 <yqtian668@gmail.com>
> > ---
> >
> > 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.

Re: [PATCH v3] compat/winansi: fix die_lasterr() argument formatting

[Johannes Sixt]

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 <j6t@kdbg.org>
> Helped-by: René Scharfe <l.s.r@web.de>
> Signed-off-by: Yongqiang Tian <yqtian668@gmail.com>
> ---
> 
> 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 <j6t@kdbg.org>

> 
> 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))