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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 18, 2025, 02:11 UTC
Message-ID: <87h5usf5tz.fsf@gitster.g>
In-Reply-To: <pull.2102.git.git.1763427606138.gitgitgadget@gmail.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.

Show 37 quoted lines

>  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