Re: [PATCH] win32: remove handling for impossible cases in win32_pthread_join

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

"AZero13 via GitGitGadget" <gitgitgadget@gmail.com> writes:

First, welcome to the Git development community (I do not think I
have seen e-mails from this address).

Common to all three patches, this name on the in-body From: line ...

> From: AZero13 <gfunni234@gmail.com>

... that overrides what is on the e-mail header (which always points at
GitGitGadget address if you are using GitGitGadget) comes from the
user.name configuration that was used when you recorded the commit.
It has to match the name you use to sign-off the patch, when you are
sending your own patch, which is ...

> WAIT_FAILED is the only real possible error here.
>
> Signed-off-by: Greg Funni <gfunni234@gmail.com>

... on this line.  Perhaps do this once

    $ git config user.name "Greg Funni"

in the repository you are doing the public work on Git, and then
check out the commit you committed under the AZero13 identity and

    $ git commit --amend --reset-author

After that, force update the pull request would be necessary to
correct them.

Other two patches I saw no issues with, but this one needs a bit
more than the above "is the only real possible" single liner
explanation.  For example, somebody who is clueless on Win32 API
(like me) would visit

  https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobjectex

and wonder why among those 5 listed, WAIT_FAILED is the only one
that can be returned.  Something like

    WAIT_TIMEOUT would not be returned as the INFINITE is given to
    the call.

would have been a reasonable explanation if this patch were removing
the case that reacts to that result (but the original does not react
to it, probably they already knew that it is impossible).

So the original code used to futz with *value_ptr when the result is
WAIT_OBJECT_0, but left *value_ptr intact when the result is
WAIT_ABANDONED.  And with these results, it called CloseHandle().

Other results, including but not limited to WAIT_FAILED, returned
error code without calling CloseHandle().  The updated code does the
"return error without calling CloseHandle()" only for WAIT_FAILED,
and for all other results, including WAIT_OBJECT_0 and
WAIT_ABANDONED, does the same futzing with *value_ptr.  Are all
these changes intended, and why are they good things?

Your proposed log message should answer these questions for future
readers of "git log", because for them, unlike me who can ask these
questions real-time to you in a patch-submitter and reviewer
relationship, it is not practical to ask you.

>  compat/win32/pthread.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 58980a529c..54c43b4146 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr UNUSED,
>  
>  int win32_pthread_join(pthread_t *thread, void **value_ptr)
>  {
> -	DWORD result = WaitForSingleObject(thread->handle, INFINITE);
> -	switch (result) {
> -	case WAIT_OBJECT_0:
> -		if (value_ptr)
> -			*value_ptr = thread->arg;
> -		CloseHandle(thread->handle);
> -		return 0;
> -	case WAIT_ABANDONED:
> -		CloseHandle(thread->handle);
> -		return EINVAL;
> -	default:
> -		/* the wait failed, so do not detach */
> +	if (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == WAIT_FAILED)
>  		return err_win_to_posix(GetLastError());
> -	}
> +
> +	if (value_ptr)
> +		*value_ptr = thread->arg;
> +
> +	CloseHandle(thread->handle);
> +	return 0;
>  }
>  
>  pthread_t pthread_self(void)
>
> base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed