{"thread":{"id":"64503","subject":"[PATCH] win32: remove handling for impossible cases in win32_pthread_join","startedAt":"2025-11-18T01:00:08Z","lastAt":"2025-11-18T17:02:39Z","messageCount":5,"participants":["AZero13 via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"530869","messageId":"pull.2102.git.git.1763427606138.gitgitgadget@gmail.com","threadId":"64503","inReplyTo":null,"subject":"[PATCH] win32: remove handling for impossible cases in win32_pthread_join","fromName":"AZero13 via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-11-18T01:00:06Z","receivedAt":"2025-11-18T01:00:08Z","isPatch":true,"sender":{"key":"name:AZero13","avatar":null},"body":"From: AZero13 <gfunni234@gmail.com>\n\nWAIT_FAILED is the only real possible error here.\n\nSigned-off-by: Greg Funni <gfunni234@gmail.com>\n---\n    win32: remove handling for impossible cases in win32_pthread_join\n    \n    WAIT_FAILED is the only real possible error here.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2102%2FAZero13%2Fpatch-1-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2102/AZero13/patch-1-v1\nPull-Request: https://github.com/git/git/pull/2102\n\n compat/win32/pthread.c | 20 +++++++-------------\n 1 file changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nindex 58980a529c..54c43b4146 100644\n--- a/compat/win32/pthread.c\n+++ b/compat/win32/pthread.c\n@@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr UNUSED,\n \n int win32_pthread_join(pthread_t *thread, void **value_ptr)\n {\n-\tDWORD result = WaitForSingleObject(thread->handle, INFINITE);\n-\tswitch (result) {\n-\tcase WAIT_OBJECT_0:\n-\t\tif (value_ptr)\n-\t\t\t*value_ptr = thread->arg;\n-\t\tCloseHandle(thread->handle);\n-\t\treturn 0;\n-\tcase WAIT_ABANDONED:\n-\t\tCloseHandle(thread->handle);\n-\t\treturn EINVAL;\n-\tdefault:\n-\t\t/* the wait failed, so do not detach */\n+\tif (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == WAIT_FAILED)\n \t\treturn err_win_to_posix(GetLastError());\n-\t}\n+\n+\tif (value_ptr)\n+\t\t*value_ptr = thread->arg;\n+\n+\tCloseHandle(thread->handle);\n+\treturn 0;\n }\n \n pthread_t pthread_self(void)\n\nbase-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed\n-- \ngitgitgadget\n"},{"id":"530870","messageId":"87h5usf5tz.fsf@gitster.g","threadId":"64503","inReplyTo":"pull.2102.git.git.1763427606138.gitgitgadget@gmail.com","subject":"Re: [PATCH] win32: remove handling for impossible cases in win32_pthread_join","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-18T02:11:04Z","receivedAt":"2025-11-18T02:11:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"AZero13 via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\nFirst, welcome to the Git development community (I do not think I\nhave seen e-mails from this address).\n\nCommon to all three patches, this name on the in-body From: line ...\n\n> From: AZero13 <gfunni234@gmail.com>\n\n... that overrides what is on the e-mail header (which always points at\nGitGitGadget address if you are using GitGitGadget) comes from the\nuser.name configuration that was used when you recorded the commit.\nIt has to match the name you use to sign-off the patch, when you are\nsending your own patch, which is ...\n\n> WAIT_FAILED is the only real possible error here.\n>\n> Signed-off-by: Greg Funni <gfunni234@gmail.com>\n\n... on this line.  Perhaps do this once\n\n    $ git config user.name \"Greg Funni\"\n\nin the repository you are doing the public work on Git, and then\ncheck out the commit you committed under the AZero13 identity and\n\n    $ git commit --amend --reset-author\n\nAfter that, force update the pull request would be necessary to\ncorrect them.\n\nOther two patches I saw no issues with, but this one needs a bit\nmore than the above \"is the only real possible\" single liner\nexplanation.  For example, somebody who is clueless on Win32 API\n(like me) would visit\n\n  https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobjectex\n\nand wonder why among those 5 listed, WAIT_FAILED is the only one\nthat can be returned.  Something like\n\n    WAIT_TIMEOUT would not be returned as the INFINITE is given to\n    the call.\n\nwould have been a reasonable explanation if this patch were removing\nthe case that reacts to that result (but the original does not react\nto it, probably they already knew that it is impossible).\n\nSo the original code used to futz with *value_ptr when the result is\nWAIT_OBJECT_0, but left *value_ptr intact when the result is\nWAIT_ABANDONED.  And with these results, it called CloseHandle().\n\nOther results, including but not limited to WAIT_FAILED, returned\nerror code without calling CloseHandle().  The updated code does the\n\"return error without calling CloseHandle()\" only for WAIT_FAILED,\nand for all other results, including WAIT_OBJECT_0 and\nWAIT_ABANDONED, does the same futzing with *value_ptr.  Are all\nthese changes intended, and why are they good things?\n\nYour proposed log message should answer these questions for future\nreaders of \"git log\", because for them, unlike me who can ask these\nquestions real-time to you in a patch-submitter and reviewer\nrelationship, it is not practical to ask you.\n\n>  compat/win32/pthread.c | 20 +++++++-------------\n>  1 file changed, 7 insertions(+), 13 deletions(-)\n>\n> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\n> index 58980a529c..54c43b4146 100644\n> --- a/compat/win32/pthread.c\n> +++ b/compat/win32/pthread.c\n> @@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr UNUSED,\n>  \n>  int win32_pthread_join(pthread_t *thread, void **value_ptr)\n>  {\n> -\tDWORD result = WaitForSingleObject(thread->handle, INFINITE);\n> -\tswitch (result) {\n> -\tcase WAIT_OBJECT_0:\n> -\t\tif (value_ptr)\n> -\t\t\t*value_ptr = thread->arg;\n> -\t\tCloseHandle(thread->handle);\n> -\t\treturn 0;\n> -\tcase WAIT_ABANDONED:\n> -\t\tCloseHandle(thread->handle);\n> -\t\treturn EINVAL;\n> -\tdefault:\n> -\t\t/* the wait failed, so do not detach */\n> +\tif (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == WAIT_FAILED)\n>  \t\treturn err_win_to_posix(GetLastError());\n> -\t}\n> +\n> +\tif (value_ptr)\n> +\t\t*value_ptr = thread->arg;\n> +\n> +\tCloseHandle(thread->handle);\n> +\treturn 0;\n>  }\n>  \n>  pthread_t pthread_self(void)\n>\n> base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed\n"},{"id":"530906","messageId":"pull.2102.v2.git.git.1763480720264.gitgitgadget@gmail.com","threadId":"64503","inReplyTo":"pull.2102.git.git.1763427606138.gitgitgadget@gmail.com","subject":"[PATCH v2] win32: remove handling for impossible cases in win32_pthread_join","fromName":"AZero13 via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-11-18T15:45:20Z","receivedAt":"2025-11-18T15:45:22Z","isPatch":true,"sender":{"key":"name:AZero13","avatar":null},"body":"From: Greg Funni <gfunni234@gmail.com>\n\nWAIT_FAILED is the only real possible error here.\n\nWAIT_TIMEOUT would not be returned as the INFINITE\nis given to the call.\n\nWAIT_ABANDONED would be returned if the handle\npointed to a mutex object that was not released\nby the thread that owned the mutex object before\nthe owning thread terminated.\n\nWAIT_IO_COMPLETION would not be returned because\nwe pass FALSE so the wait is not alertable.\n\nSigned-off-by: Greg Funni <gfunni234@gmail.com>\n---\n    win32: remove handling for impossible cases in win32_pthread_join\n    \n    WAIT_FAILED is the only real possible error here.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2102%2FAZero13%2Fpatch-1-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2102/AZero13/patch-1-v2\nPull-Request: https://github.com/git/git/pull/2102\n\nRange-diff vs v1:\n\n 1:  73fae07514 ! 1:  e0d6b15093 win32: remove handling for impossible cases in win32_pthread_join\n     @@\n       ## Metadata ##\n     -Author: AZero13 <gfunni234@gmail.com>\n     +Author: Greg Funni <gfunni234@gmail.com>\n      \n       ## Commit message ##\n          win32: remove handling for impossible cases in win32_pthread_join\n      \n          WAIT_FAILED is the only real possible error here.\n      \n     +    WAIT_TIMEOUT would not be returned as the INFINITE\n     +    is given to the call.\n     +\n     +    WAIT_ABANDONED would be returned if the handle\n     +    pointed to a mutex object that was not released\n     +    by the thread that owned the mutex object before\n     +    the owning thread terminated.\n     +\n     +    WAIT_IO_COMPLETION would not be returned because\n     +    we pass FALSE so the wait is not alertable.\n     +\n          Signed-off-by: Greg Funni <gfunni234@gmail.com>\n      \n       ## compat/win32/pthread.c ##\n\n\n compat/win32/pthread.c | 20 +++++++-------------\n 1 file changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nindex 58980a529c..54c43b4146 100644\n--- a/compat/win32/pthread.c\n+++ b/compat/win32/pthread.c\n@@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr UNUSED,\n \n int win32_pthread_join(pthread_t *thread, void **value_ptr)\n {\n-\tDWORD result = WaitForSingleObject(thread->handle, INFINITE);\n-\tswitch (result) {\n-\tcase WAIT_OBJECT_0:\n-\t\tif (value_ptr)\n-\t\t\t*value_ptr = thread->arg;\n-\t\tCloseHandle(thread->handle);\n-\t\treturn 0;\n-\tcase WAIT_ABANDONED:\n-\t\tCloseHandle(thread->handle);\n-\t\treturn EINVAL;\n-\tdefault:\n-\t\t/* the wait failed, so do not detach */\n+\tif (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == WAIT_FAILED)\n \t\treturn err_win_to_posix(GetLastError());\n-\t}\n+\n+\tif (value_ptr)\n+\t\t*value_ptr = thread->arg;\n+\n+\tCloseHandle(thread->handle);\n+\treturn 0;\n }\n \n pthread_t pthread_self(void)\n\nbase-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed\n-- \ngitgitgadget\n"},{"id":"530907","messageId":"pull.2102.v3.git.git.1763480854213.gitgitgadget@gmail.com","threadId":"64503","inReplyTo":"pull.2102.v2.git.git.1763480720264.gitgitgadget@gmail.com","subject":"[PATCH v3] win32: remove handling for impossible cases in win32_pthread_join","fromName":"AZero13 via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-11-18T15:47:34Z","receivedAt":"2025-11-18T15:47:37Z","isPatch":true,"sender":{"key":"name:AZero13","avatar":null},"body":"From: Greg Funni <gfunni234@gmail.com>\n\nWAIT_FAILED is the only real possible error here.\n\nWAIT_TIMEOUT would not be returned as the INFINITE\nis given to the call.\n\nWAIT_ABANDONED would be returned if the handle\npointed to a mutex object that was not released\nby the thread that owned the mutex object before\nthe owning thread terminated.\n\nWAIT_IO_COMPLETION would not be returned because\nwe pass FALSE so the wait is not alertable.\n\nSigned-off-by: Greg Funni <gfunni234@gmail.com>\n---\n    win32: remove handling for impossible cases in win32_pthread_join\n    \n    WAIT_FAILED is the only real possible error here.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2102%2FAZero13%2Fpatch-1-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2102/AZero13/patch-1-v3\nPull-Request: https://github.com/git/git/pull/2102\n\nRange-diff vs v2:\n\n 1:  e0d6b15093 = 1:  20f943570f win32: remove handling for impossible cases in win32_pthread_join\n\n\n compat/win32/pthread.c | 20 +++++++-------------\n 1 file changed, 7 insertions(+), 13 deletions(-)\n\ndiff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\nindex 58980a529c..54c43b4146 100644\n--- a/compat/win32/pthread.c\n+++ b/compat/win32/pthread.c\n@@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr UNUSED,\n \n int win32_pthread_join(pthread_t *thread, void **value_ptr)\n {\n-\tDWORD result = WaitForSingleObject(thread->handle, INFINITE);\n-\tswitch (result) {\n-\tcase WAIT_OBJECT_0:\n-\t\tif (value_ptr)\n-\t\t\t*value_ptr = thread->arg;\n-\t\tCloseHandle(thread->handle);\n-\t\treturn 0;\n-\tcase WAIT_ABANDONED:\n-\t\tCloseHandle(thread->handle);\n-\t\treturn EINVAL;\n-\tdefault:\n-\t\t/* the wait failed, so do not detach */\n+\tif (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == WAIT_FAILED)\n \t\treturn err_win_to_posix(GetLastError());\n-\t}\n+\n+\tif (value_ptr)\n+\t\t*value_ptr = thread->arg;\n+\n+\tCloseHandle(thread->handle);\n+\treturn 0;\n }\n \n pthread_t pthread_self(void)\n\nbase-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed\n-- \ngitgitgadget\n"},{"id":"530911","messageId":"xmqq4iqrff4j.fsf@gitster.g","threadId":"64503","inReplyTo":"pull.2102.v2.git.git.1763480720264.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] win32: remove handling for impossible cases in win32_pthread_join","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-18T17:02:36Z","receivedAt":"2025-11-18T17:02:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"AZero13 via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Greg Funni <gfunni234@gmail.com>\n\nLooking much better, but there still are some puzzlement left.\n\n> WAIT_FAILED is the only real possible error here.\n>\n> WAIT_TIMEOUT would not be returned as the INFINITE\n> is given to the call.\n\nOK.\n\n> WAIT_ABANDONED would be returned if the handle\n> pointed to a mutex object that was not released\n> by the thread that owned the mutex object before\n> the owning thread terminated.\n\n... and we know the handle we are passing to WaitForSingleObject()\nis a Thread, not a Mutex, so this error condition is irrelevant?\n\nOK.\n\n> WAIT_IO_COMPLETION would not be returned because\n> we pass FALSE so the wait is not alertable.\n\nFALSE where?  Ah, this can be done with WaitForSingleObjectEx, but\nWaitForSingleObject() is what we call here, so WAIT_IO_COMPLETION\nwon't be returned with or without FALSE.  Is there a reason why you\nwant to change the code to call Ex variant (the manual page tells us\nto use it _if_ we want to enter an alertable wait state, and I am\nassuming that we are not interested in doing so)?\n\nIn any case, among the four possible return values from\nWaitForSingleObject(), we know WAIT_ABANDONED and WAIT_TIMEOUT will\nnot be relevant for this code path.\n\nBecause WAIT_OBJECT_0 is the cryptic synonym for \"Success!\" for this\ncall, WAIT_FAILED is indeed the only possible error here, just like\nyou said at the beginning.\n\nI think it is easier to understand for mere-mortal readers like me,\nwho are not familiar with Win32 API, if we explained this change\nmore like:\n\n  Subject: [PATCH] win32: simplify win32_pthread_join() error handling\n\n  Among the four possible result WaitForSingleObject() can return,\n  WAIT_TIMEOUT and WAIT_ABANDONED are not relevant in this code\n  path, because we do not ask for the call to time-out, and we do\n  not pass a mutex object to the call (we are passing a thread\n  object).\n\n  Simplify the code to\n\n  - return an error without closing the handle if the call failed\n    (i.e., returns WAIT_FAILED that is not zero);\n\n  - otherwise, WAIT_OBJECT_0 (which is 0) is returned to signal a\n    success.  Do exactly what the original code did in this case.\n\nWhat do you think?\n\n> Signed-off-by: Greg Funni <gfunni234@gmail.com>\n>  compat/win32/pthread.c | 20 +++++++-------------\n>  1 file changed, 7 insertions(+), 13 deletions(-)\n\n\n> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c\n> index 58980a529c..54c43b4146 100644\n> --- a/compat/win32/pthread.c\n> +++ b/compat/win32/pthread.c\n> @@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr UNUSED,\n>  \n>  int win32_pthread_join(pthread_t *thread, void **value_ptr)\n>  {\n> -\tDWORD result = WaitForSingleObject(thread->handle, INFINITE);\n> -\tswitch (result) {\n> -\tcase WAIT_OBJECT_0:\n> -\t\tif (value_ptr)\n> -\t\t\t*value_ptr = thread->arg;\n> -\t\tCloseHandle(thread->handle);\n> -\t\treturn 0;\n> -\tcase WAIT_ABANDONED:\n> -\t\tCloseHandle(thread->handle);\n> -\t\treturn EINVAL;\n> -\tdefault:\n\nAnd the above is what the patch simplifies away, which is great.\n\n> -\t\t/* the wait failed, so do not detach */\n\nI think this comment is worth keeping (I am assuming \"do not detach\"\nrefers to the fact that CloseHandle(thread->handle) is not called in\nthe error case).\n\n> +\tif (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == WAIT_FAILED)\n>  \t\treturn err_win_to_posix(GetLastError());\n\nThe change is based on out belief that WAIT_FAILED is the only\npossible error from this call.  Even if our belief turns out to be\nwrong, we would want to take the error code path, wouldn't we?  IOW,\nI think the above should be more like\n\n\tif (WaitForSingleObject(thread->handle, INFINITE))\n\t\t/* the wait failed; do not detach */\n\t\treturn err_win_to_posix(GetLastError());\n\ni.e., if we get an error, report the error to the caller, regardless\nof what kind of an error it is, even though we expect it to be a\nWAIT_FAILED.  And the case this if() condition does not catch is a\nsuccessful wait (i.e., WAIT_OBJECT_0), which is handled ...\n\n> +\tif (value_ptr)\n> +\t\t*value_ptr = thread->arg;\n> +\n> +\tCloseHandle(thread->handle);\n> +\treturn 0;\n\n... exactly as before.\n\n>  }\n>  \n>  pthread_t pthread_self(void)\n>\n> base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed\n\nLooking good.  Thanks.\n"}]}