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

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 [off-list ref] wrote:

Yongqiang Tian [off-list ref] writes:

quoted

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

quoted

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.

quoted

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.

quoted

Signed-off-by: Yongqiang Tian <redacted>
---

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.

quoted

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

quoted

 compat/winansi.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

Nice.

quoted

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