> 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

> 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

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

> 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

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

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

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

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

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