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

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