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

[Junio C Hamano <gitster@pobox.com>]

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.