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

From

YTYongqiang Tian <yqtian668@gmail.com>

Date

Sep 21, 2026, 03:00 UTC

Message-ID

<CAEs0Zp4LK1ZHkL0tdi_k_=-u86HVm07FzyOJdO-=+DY9c3ZrYA@mail.gmail.com>

In-Reply-To

<1e706ce8-bebd-4ac1-914a-a55195e7d253@kdbg.org>

Hi René and Hannes,

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.

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
>