{"thread":{"id":"56508","subject":"[PATCH 0/7] Builtin FSMonitor Part 1","startedAt":"2021-09-15T20:36:26Z","lastAt":"2021-11-16T11:03:44Z","messageCount":66,"participants":["Jeff Hostetler via GitGitGadget","Eric Sunshine","Junio C Hamano","Taylor Blau","Ævar Arnfjörð Bjarmason","Jeff Hostetler","Carlo Arenas","Adam Dinwoodie","Ramsay Jones","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":7},"messages":[{"id":"436042","messageId":"pull.1040.git.1631738177.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":null,"subject":"[PATCH 0/7] Builtin FSMonitor Part 1","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-15T20:36:10Z","receivedAt":"2021-09-15T20:36:26Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"In July I sent the V3 version [3] of my Builtin FSMonitor series [1,2,3].\nThis has been in \"seen\" as branch jh/builtin-fsmonitor. At 34 commits, it\nwas already a little too big to easily review. With the feed back on [3] and\nfrom people using the experimental version that we shipped with\ngit-for-windows v2.32 and v2.33, I've prepared a new V4 version. This can be\nseen in [4].\n\nHowever, this new version currently contains 58 commits and is way too big\nto be submitted as is. So I would like to close or discard the original V3\nbranch and submit V4 in pieces to make it easier to review.\n\nHere is Part 1 of (what would be V4 of) my Builtin FSMonitor series.\n\nPart 1 contains:\n\n * A fix for a memory leak in the Trace2 code. (This was independently\n reported in last week in \"ab/tr2-leaks-and-fixes\".)\n * Various cleanups in the simple-ipc layer.\n * A new start_bg_command() function to launch a command into the background\n and wait for it to start. And a refactored consumer of it in\n test-simple-ipc. There was a large discussion on commit 14/34 in V3 [5\n thru 6] about the large ifdef'd blocks of platform specific code to spawn\n background commands, trace2 handling, and duplicated code in\n fsmonitor--daemon.c and in test-simple-ipc. That has all been addressed\n here.\n\n[1]\nhttps://lore.kernel.org/git/pull.923.git.1617291666.gitgitgadget@gmail.com/\n[2]\nhttps://lore.kernel.org/git/pull.923.v2.git.1621691828.gitgitgadget@gmail.com/\n[3]\nhttps://lore.kernel.org/git/pull.923.v3.git.1625150864.gitgitgadget@gmail.com/\n[4] https://github.com/gitgitgadget/git/pull/923 [5]\nhttps://lore.kernel.org/git/9fe902aad87f1192705fb69ea212a2d066d0286d.1625150864.git.gitgitgadget@gmail.com/\n[6] https://lore.kernel.org/git/87tukovidd.fsf@evledraar.gmail.com/\n\nJeff Hostetler (7):\n trace2: fix memory leak of thread name\n simple-ipc: preparations for supporting binary messages.\n simple-ipc: move definition of ipc_active_state outside of ifdef\n simple-ipc/ipc-win32: add trace2 debugging\n simple-ipc/ipc-win32: add Windows ACL to named pipe\n run-command: create start_bg_command\n t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command\n\n compat/simple-ipc/ipc-unix-socket.c | 14 +-\n compat/simple-ipc/ipc-win32.c | 176 +++++++++++++++++++--\n run-command.c | 123 +++++++++++++++\n run-command.h | 48 ++++++\n simple-ipc.h | 21 +--\n t/helper/test-simple-ipc.c | 227 ++++++++--------------------\n trace2/tr2_tls.c | 1 +\n 7 files changed, 416 insertions(+), 194 deletions(-)\n\n\nbase-commit: 8b7c11b8668b4e774f81a9f0b4c30144b818f1d1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1040%2Fjeffhostetler%2Fbuiltin-fsmonitor-part1-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1040/jeffhostetler/builtin-fsmonitor-part1-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1040\n-- \ngitgitgadget\n"},{"id":"436043","messageId":"30b7bb247c3c8531119f99e293d84b1836297251.1631738177.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.git.1631738177.gitgitgadget@gmail.com","subject":"[PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-15T20:36:14Z","receivedAt":"2021-09-15T20:36:27Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nCreate \"ipc-debug\" category events to log unexpected errors\nwhen creating Simple-IPC connections.\n\nSigned-off-by: Jeff Hostetler \n---\n compat/simple-ipc/ipc-win32.c | 22 ++++++++++++++++++++++\n 1 file changed, 22 insertions(+)\n\ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex 8e889d6a506..6c8a308de13 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -49,6 +49,9 @@ static enum ipc_active_state get_active_state(wchar_t *pipe_path)\n \tif (GetLastError() == ERROR_FILE_NOT_FOUND)\n \t\treturn IPC_STATE__PATH_NOT_FOUND;\n \n+\ttrace2_data_intmax(\"ipc-debug\", NULL, \"getstate/waitpipe/gle\",\n+\t\t\t (intmax_t)GetLastError());\n+\n \treturn IPC_STATE__OTHER_ERROR;\n }\n \n@@ -112,6 +115,11 @@ static enum ipc_active_state connect_to_server(\n \t\t\t\tif (GetLastError() == ERROR_SEM_TIMEOUT)\n \t\t\t\t\treturn IPC_STATE__NOT_LISTENING;\n \n+\t\t\t\tgle = GetLastError();\n+\t\t\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n+\t\t\t\t\t\t \"connect/waitpipe/gle\",\n+\t\t\t\t\t\t (intmax_t)gle);\n+\n \t\t\t\treturn IPC_STATE__OTHER_ERROR;\n \t\t\t}\n \n@@ -133,17 +141,31 @@ static enum ipc_active_state connect_to_server(\n \t\t\tbreak; /* try again */\n \n \t\tdefault:\n+\t\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n+\t\t\t\t\t \"connect/createfile/gle\",\n+\t\t\t\t\t (intmax_t)gle);\n+\n \t\t\treturn IPC_STATE__OTHER_ERROR;\n \t\t}\n \t}\n \n \tif (!SetNamedPipeHandleState(hPipe, &mode, NULL, NULL)) {\n+\t\tgle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n+\t\t\t\t \"connect/setpipestate/gle\",\n+\t\t\t\t (intmax_t)gle);\n+\n \t\tCloseHandle(hPipe);\n \t\treturn IPC_STATE__OTHER_ERROR;\n \t}\n \n \t*pfd = _open_osfhandle((intptr_t)hPipe, O_RDWR|O_BINARY);\n \tif (*pfd < 0) {\n+\t\tgle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n+\t\t\t\t \"connect/openosfhandle/gle\",\n+\t\t\t\t (intmax_t)gle);\n+\n \t\tCloseHandle(hPipe);\n \t\treturn IPC_STATE__OTHER_ERROR;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"436044","messageId":"f97038a563d889d740a7e968fcbdfaadb41e2008.1631738177.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.git.1631738177.gitgitgadget@gmail.com","subject":"[PATCH 6/7] run-command: create start_bg_command","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-15T20:36:16Z","receivedAt":"2021-09-15T20:36:28Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nCreate a variation of `run_command()` and `start_command()` to launch a command\ninto the background and optionally wait for it to become \"ready\" before returning.\n\nSigned-off-by: Jeff Hostetler \n---\n run-command.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++\n run-command.h | 48 ++++++++++++++++++++\n 2 files changed, 171 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3e4e082e94d..fe75fd08f74 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1901,3 +1901,126 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)\n \t}\n \tstrvec_pushf(env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT, new_git_dir);\n }\n+\n+enum start_bg_result start_bg_command(struct child_process *cmd,\n+\t\t\t\t start_bg_wait_cb *wait_cb,\n+\t\t\t\t void *cb_data,\n+\t\t\t\t unsigned int timeout_sec)\n+{\n+\tenum start_bg_result sbgr = SBGR_ERROR;\n+\tint ret;\n+\tint wait_status;\n+\tpid_t pid_seen;\n+\ttime_t time_limit;\n+\n+\t/*\n+\t * Silently disallow child cleanup -- even if requested.\n+\t * The child process should persist in the background\n+\t * and possibly/probably after this process exits. That\n+\t * is, don't kill the child during our atexit routine.\n+\t */\n+\tcmd->clean_on_exit = 0;\n+\n+\tret = start_command(cmd);\n+\tif (ret) {\n+\t\t/*\n+\t\t * We assume that if `start_command()` fails, we\n+\t\t * either get a complete `trace2_child_start() /\n+\t\t * trace2_child_exit()` pair or it fails before the\n+\t\t * `trace2_child_start()` is emitted, so we do not\n+\t\t * need to worry about it here.\n+\t\t *\n+\t\t * We also assume that `start_command()` does not add\n+\t\t * us to the cleanup list. And that it calls\n+\t\t * calls `child_process_clear()`.\n+\t\t */\n+\t\tsbgr = SBGR_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\ttime(&time_limit);\n+\ttime_limit += timeout_sec;\n+\n+wait:\n+\tpid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);\n+\n+\tif (pid_seen == 0) {\n+\t\t/*\n+\t\t * The child is currently running. Ask the callback\n+\t\t * if the child is ready to do work or whether we\n+\t\t * should keep waiting for it to boot up.\n+\t\t */\n+\t\tret = (*wait_cb)(cb_data, cmd);\n+\t\tif (!ret) {\n+\t\t\t/*\n+\t\t\t * The child is running and \"ready\".\n+\t\t\t *\n+\t\t\t * NEEDSWORK: As we prepare to orphan (release to\n+\t\t\t * the background) this child, it is not appropriate\n+\t\t\t * to emit a `trace2_child_exit()` event. Should we\n+\t\t\t * create a new event for this case?\n+\t\t\t */\n+\t\t\tsbgr = SBGR_READY;\n+\t\t\tgoto done;\n+\t\t} else if (ret > 0) {\n+\t\t\ttime_t now;\n+\n+\t\t\ttime(&now);\n+\t\t\tif (now < time_limit)\n+\t\t\t\tgoto wait;\n+\n+\t\t\t/*\n+\t\t\t * Our timeout has expired. We don't try to\n+\t\t\t * kill the child, but rather let it continue\n+\t\t\t * (hopefully) trying to startup.\n+\t\t\t *\n+\t\t\t * NEEDSWORK: Like the \"ready\" case, should we\n+\t\t\t * log a custom child-something Trace2 event here?\n+\t\t\t */\n+\t\t\tsbgr = SBGR_TIMEOUT;\n+\t\t\tgoto done;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * The cb gave up on this child.\n+\t\t\t *\n+\t\t\t * NEEDSWORK: Like above, should we log a custom\n+\t\t\t * Trace2 child-something event here?\n+\t\t\t */\n+\t\t\tsbgr = SBGR_CB_ERROR;\n+\t\t\tgoto done;\n+\t\t}\n+\t}\n+\n+\tif (pid_seen == cmd->pid) {\n+\t\tint child_code = -1;\n+\n+\t\t/*\n+\t\t * The child started, but exited or was terminated\n+\t\t * before becoming \"ready\".\n+\t\t *\n+\t\t * We try to match the behavior of `wait_or_whine()`\n+\t\t * and convert the child's status to a return code for\n+\t\t * tracing purposes and emit the `trace2_child_exit()`\n+\t\t * event.\n+\t\t */\n+\t\tif (WIFEXITED(wait_status))\n+\t\t\tchild_code = WEXITSTATUS(wait_status);\n+\t\telse if (WIFSIGNALED(wait_status))\n+\t\t\tchild_code = WTERMSIG(wait_status) + 128;\n+\t\ttrace2_child_exit(cmd, child_code);\n+\n+\t\tsbgr = SBGR_DIED;\n+\t\tgoto done;\n+\t}\n+\n+\tif (pid_seen < 0 && errno == EINTR)\n+\t\tgoto wait;\n+\n+\ttrace2_child_exit(cmd, -1);\n+\tsbgr = SBGR_ERROR;\n+\n+done:\n+\tchild_process_clear(cmd);\n+\tinvalidate_lstat_cache();\n+\treturn sbgr;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex af1296769f9..58065197d1b 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -496,4 +496,52 @@ int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,\n */\n void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir);\n \n+/**\n+ * Possible return values for `start_bg_command()`.\n+ */\n+enum start_bg_result {\n+\t/* child process is \"ready\" */\n+\tSBGR_READY = 0,\n+\n+\t/* child process could not be started */\n+\tSBGR_ERROR,\n+\n+\t/* callback error when testing for \"ready\" */\n+\tSBGR_CB_ERROR,\n+\n+\t/* timeout expired waiting for child to become \"ready\" */\n+\tSBGR_TIMEOUT,\n+\n+\t/* child process exited or was signalled before becomming \"ready\" */\n+\tSBGR_DIED,\n+};\n+\n+/**\n+ * Callback used by `start_bg_command()` to ask whether the\n+ * child process is ready or needs more time to become ready.\n+ *\n+ * Returns 1 is child needs more time (subject to the requested timeout).\n+ * Returns 0 if child is ready.\n+ * Returns -1 on any error and cause `start_bg_command()` to also error out.\n+ */\n+typedef int(start_bg_wait_cb)(void *cb_data,\n+\t\t\t const struct child_process *cmd);\n+\n+/**\n+ * Start a command in the background. Wait long enough for the child to\n+ * become \"ready\". Capture immediate errors (like failure to start) and\n+ * any immediate exit status (such as a shutdown/signal before the child\n+ * became \"ready\").\n+ *\n+ * This is a combination of `start_command()` and `finish_command()`, but\n+ * with a custom `wait_or_whine()` that allows the caller to define when\n+ * the child is \"ready\".\n+ *\n+ * The caller does not need to call `finish_command()`.\n+ */\n+enum start_bg_result start_bg_command(struct child_process *cmd,\n+\t\t\t\t start_bg_wait_cb *wait_cb,\n+\t\t\t\t void *cb_data,\n+\t\t\t\t unsigned int timeout_sec);\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"436045","messageId":"5eadf71929559968cafa18d03c3a623b1adff926.1631738177.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.git.1631738177.gitgitgadget@gmail.com","subject":"[PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-15T20:36:15Z","receivedAt":"2021-09-15T20:36:29Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nSet an ACL on the named pipe to allow the well-known group EVERYONE\nto read and write to the IPC server's named pipe. In the event that\nthe daemon was started with elevation, allow non-elevated clients\nto communicate with the daemon.\n\nSigned-off-by: Jeff Hostetler \n---\n compat/simple-ipc/ipc-win32.c | 140 +++++++++++++++++++++++++++++++---\n 1 file changed, 129 insertions(+), 11 deletions(-)\n\ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex 6c8a308de13..374ae2f81c7 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -3,6 +3,8 @@\n #include \"strbuf.h\"\n #include \"pkt-line.h\"\n #include \"thread-utils.h\"\n+#include \"accctrl.h\"\n+#include \"aclapi.h\"\n \n #ifndef SUPPORTS_SIMPLE_IPC\n /*\n@@ -591,11 +593,132 @@ finished:\n \treturn NULL;\n }\n \n+/*\n+ * We need to build a Windows \"SECURITY_ATTRIBUTES\" object and use it\n+ * to apply an ACL when we create the initial instance of the Named\n+ * Pipe. The construction is somewhat involved and consists of\n+ * several sequential steps and intermediate objects.\n+ *\n+ * We use this structure to hold these intermediate pointers so that\n+ * we can free them as a group. (It is unclear from the docs whether\n+ * some of these intermediate pointers can be freed before we are\n+ * finished using the \"lpSA\" member.)\n+ */\n+struct my_sa_data\n+{\n+\tPSID pEveryoneSID;\n+\tPACL pACL;\n+\tPSECURITY_DESCRIPTOR pSD;\n+\tLPSECURITY_ATTRIBUTES lpSA;\n+};\n+\n+static void init_sa(struct my_sa_data *d)\n+{\n+\tmemset(d, 0, sizeof(*d));\n+}\n+\n+static void release_sa(struct my_sa_data *d)\n+{\n+\tif (d->pEveryoneSID)\n+\t\tFreeSid(d->pEveryoneSID);\n+\tif (d->pACL)\n+\t\tLocalFree(d->pACL);\n+\tif (d->pSD)\n+\t\tLocalFree(d->pSD);\n+\tif (d->lpSA)\n+\t\tLocalFree(d->lpSA);\n+\n+\tmemset(d, 0, sizeof(*d));\n+}\n+\n+/*\n+ * Create SECURITY_ATTRIBUTES to apply to the initial named pipe. The\n+ * creator of the first server instance gets to set the ACLs on it.\n+ *\n+ * We allow the well-known group `EVERYONE` to have read+write access\n+ * to the named pipe so that clients can send queries to the daemon\n+ * and receive the response.\n+ *\n+ * Normally, this is not necessary since the daemon is usually\n+ * automatically started by a foreground command like `git status`,\n+ * but in those cases where an elevated Git command started the daemon\n+ * (such that the daemon itself runs with elevation), we need to add\n+ * the ACL so that non-elevated commands can write to it.\n+ *\n+ * The following document was helpful:\n+ * https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c--\n+ *\n+ * Returns d->lpSA set to a SA or NULL.\n+ */\n+static LPSECURITY_ATTRIBUTES get_sa(struct my_sa_data *d)\n+{\n+\tSID_IDENTIFIER_AUTHORITY sid_auth_world = SECURITY_WORLD_SID_AUTHORITY;\n+#define NR_EA (1)\n+\tEXPLICIT_ACCESS ea[NR_EA];\n+\tDWORD dwResult;\n+\n+\tif (!AllocateAndInitializeSid(&sid_auth_world, 1,\n+\t\t\t\t SECURITY_WORLD_RID, 0,0,0,0,0,0,0,\n+\t\t\t\t &d->pEveryoneSID)) {\n+\t\tDWORD gle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL, \"alloc-world-sid/gle\",\n+\t\t\t\t (intmax_t)gle);\n+\t\tgoto fail;\n+\t}\n+\n+\tmemset(ea, 0, NR_EA * sizeof(EXPLICIT_ACCESS));\n+\n+\tea[0].grfAccessPermissions = GENERIC_READ | GENERIC_WRITE;\n+\tea[0].grfAccessMode = SET_ACCESS;\n+\tea[0].grfInheritance = NO_INHERITANCE;\n+\tea[0].Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE;\n+\tea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;\n+\tea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;\n+\tea[0].Trustee.ptstrName = (LPTSTR)d->pEveryoneSID;\n+\n+\tdwResult = SetEntriesInAcl(NR_EA, ea, NULL, &d->pACL);\n+\tif (dwResult != ERROR_SUCCESS) {\n+\t\tDWORD gle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL, \"set-acl-entry/gle\",\n+\t\t\t\t (intmax_t)gle);\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL, \"set-acl-entry/dw\",\n+\t\t\t\t (intmax_t)dwResult);\n+\t\tgoto fail;\n+\t}\n+\n+\td->pSD = (PSECURITY_DESCRIPTOR)LocalAlloc(\n+\t\tLPTR, SECURITY_DESCRIPTOR_MIN_LENGTH);\n+\tif (!InitializeSecurityDescriptor(d->pSD, SECURITY_DESCRIPTOR_REVISION)) {\n+\t\tDWORD gle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL, \"init-sd/gle\", (intmax_t)gle);\n+\t\tgoto fail;\n+\t}\n+\n+\tif (!SetSecurityDescriptorDacl(d->pSD, TRUE, d->pACL, FALSE)) {\n+\t\tDWORD gle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL, \"set-sd-dacl/gle\", (intmax_t)gle);\n+\t\tgoto fail;\n+\t}\n+\n+\td->lpSA = (LPSECURITY_ATTRIBUTES)LocalAlloc(LPTR, sizeof(SECURITY_ATTRIBUTES));\n+\td->lpSA->nLength = sizeof(SECURITY_ATTRIBUTES);\n+\td->lpSA->lpSecurityDescriptor = d->pSD;\n+\td->lpSA->bInheritHandle = FALSE;\n+\n+\treturn d->lpSA;\n+\n+fail:\n+\trelease_sa(d);\n+\treturn NULL;\n+}\n+\n static HANDLE create_new_pipe(wchar_t *wpath, int is_first)\n {\n \tHANDLE hPipe;\n \tDWORD dwOpenMode, dwPipeMode;\n-\tLPSECURITY_ATTRIBUTES lpsa = NULL;\n+\tstruct my_sa_data my_sa_data;\n+\n+\tinit_sa(&my_sa_data);\n \n \tdwOpenMode = PIPE_ACCESS_INBOUND | PIPE_ACCESS_OUTBOUND |\n \t\tFILE_FLAG_OVERLAPPED;\n@@ -611,20 +734,15 @@ static HANDLE create_new_pipe(wchar_t *wpath, int is_first)\n \t\t * set the ACL / Security Attributes on the named\n \t\t * pipe; subsequent instances inherit and cannot\n \t\t * change them.\n-\t\t *\n-\t\t * TODO Should we allow the application layer to\n-\t\t * specify security attributes, such as `LocalService`\n-\t\t * or `LocalSystem`, when we create the named pipe?\n-\t\t * This question is probably not important when the\n-\t\t * daemon is started by a foreground user process and\n-\t\t * only needs to talk to the current user, but may be\n-\t\t * if the daemon is run via the Control Panel as a\n-\t\t * System Service.\n \t\t */\n+\t\tget_sa(&my_sa_data);\n \t}\n \n \thPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,\n-\t\t\t\t PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);\n+\t\t\t\t PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,\n+\t\t\t\t my_sa_data.lpSA);\n+\n+\trelease_sa(&my_sa_data);\n \n \treturn hPipe;\n }\n-- \ngitgitgadget\n\n"},{"id":"436046","messageId":"5f557caee004f22cee33e8753063f0315459d7e1.1631738177.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.git.1631738177.gitgitgadget@gmail.com","subject":"[PATCH 1/7] trace2: fix memory leak of thread name","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-15T20:36:11Z","receivedAt":"2021-09-15T20:36:30Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nDo not leak the thread name (contained within the thread context) when\na thread terminates.\n\nSigned-off-by: Jeff Hostetler \n---\n trace2/tr2_tls.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\nindex 067c23755fb..7da94aba522 100644\n--- a/trace2/tr2_tls.c\n+++ b/trace2/tr2_tls.c\n@@ -95,6 +95,7 @@ void tr2tls_unset_self(void)\n \n \tpthread_setspecific(tr2tls_key, NULL);\n \n+\tstrbuf_release(&ctx->thread_name);\n \tfree(ctx->array_us_start);\n \tfree(ctx);\n }\n-- \ngitgitgadget\n\n"},{"id":"436047","messageId":"7de207828caa9e50906f39b955e39e9de2c1768f.1631738177.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.git.1631738177.gitgitgadget@gmail.com","subject":"[PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-15T20:36:13Z","receivedAt":"2021-09-15T20:36:31Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nFrom: Carlo Marcelo Arenas Belón \n\nMove the declartion of the `enum ipc_active_state` type outside of\nthe SUPPORTS_SIMPLE_IPC ifdef.\n\nA later commit will introduce the `fsmonitor_ipc__*()` API and stub in\na \"mock\" implementation that requires this enum in some function\nsignatures.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Jeff Hostetler \n---\n simple-ipc.h | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/simple-ipc.h b/simple-ipc.h\nindex 9c7330fcda0..b396293bdfc 100644\n--- a/simple-ipc.h\n+++ b/simple-ipc.h\n@@ -5,13 +5,6 @@\n * See Documentation/technical/api-simple-ipc.txt\n */\n \n-#ifdef SUPPORTS_SIMPLE_IPC\n-#include \"pkt-line.h\"\n-\n-/*\n- * Simple IPC Client Side API.\n- */\n-\n enum ipc_active_state {\n \t/*\n \t * The pipe/socket exists and the daemon is waiting for connections.\n@@ -43,6 +36,13 @@ enum ipc_active_state {\n \tIPC_STATE__OTHER_ERROR,\n };\n \n+#ifdef SUPPORTS_SIMPLE_IPC\n+#include \"pkt-line.h\"\n+\n+/*\n+ * Simple IPC Client Side API.\n+ */\n+\n struct ipc_client_connect_options {\n \t/*\n \t * Spin under timeout if the server is running but can't\n-- \ngitgitgadget\n\n"},{"id":"436048","messageId":"7182419f6dfa49d4d8415736943c72a575ab6753.1631738177.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.git.1631738177.gitgitgadget@gmail.com","subject":"[PATCH 2/7] simple-ipc: preparations for supporting binary messages.","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-15T20:36:12Z","receivedAt":"2021-09-15T20:36:33Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nAdd `command_len` argument to the Simple IPC API.\n\nIn my original Simple IPC API, I assumed that the request\nwould always be a null-terminated string of text characters.\nThe command arg was just a `const char *`.\n\nI found a caller that would like to pass a binary command\nto the daemon, so I want to ammend the Simple IPC API to\ntake `const char *command, size_t command_len` and pass\nthat to the daemon. (Really, the first arg should just be\na `void *` or `const unsigned byte *` to make that clearer.)\n\nNote, the response side has always been a `struct strbuf`\nwhich includes the buffer and length, so we already support\nreturning a binary answer. (Yes, it feels a little weird\nreturning a binary buffer in a `strbuf`, but it works.)\n\nSigned-off-by: Jeff Hostetler \n---\n compat/simple-ipc/ipc-unix-socket.c | 14 +++++++-----\n compat/simple-ipc/ipc-win32.c | 14 +++++++-----\n simple-ipc.h | 7 ++++--\n t/helper/test-simple-ipc.c | 34 +++++++++++++++++++----------\n 4 files changed, 46 insertions(+), 23 deletions(-)\n\ndiff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c\nindex 1927e6ef4bc..4e28857a0a1 100644\n--- a/compat/simple-ipc/ipc-unix-socket.c\n+++ b/compat/simple-ipc/ipc-unix-socket.c\n@@ -168,7 +168,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection)\n \n int ipc_client_send_command_to_connection(\n \tstruct ipc_client_connection *connection,\n-\tconst char *message, struct strbuf *answer)\n+\tconst char *message, size_t message_len,\n+\tstruct strbuf *answer)\n {\n \tint ret = 0;\n \n@@ -176,7 +177,7 @@ int ipc_client_send_command_to_connection(\n \n \ttrace2_region_enter(\"ipc-client\", \"send-command\", NULL);\n \n-\tif (write_packetized_from_buf_no_flush(message, strlen(message),\n+\tif (write_packetized_from_buf_no_flush(message, message_len,\n \t\t\t\t\t connection->fd) < 0 ||\n \t packet_flush_gently(connection->fd) < 0) {\n \t\tret = error(_(\"could not send IPC command\"));\n@@ -197,7 +198,8 @@ done:\n \n int ipc_client_send_command(const char *path,\n \t\t\t const struct ipc_client_connect_options *options,\n-\t\t\t const char *message, struct strbuf *answer)\n+\t\t\t const char *message, size_t message_len,\n+\t\t\t struct strbuf *answer)\n {\n \tint ret = -1;\n \tenum ipc_active_state state;\n@@ -208,7 +210,9 @@ int ipc_client_send_command(const char *path,\n \tif (state != IPC_STATE__LISTENING)\n \t\treturn ret;\n \n-\tret = ipc_client_send_command_to_connection(connection, message, answer);\n+\tret = ipc_client_send_command_to_connection(connection,\n+\t\t\t\t\t\t message, message_len,\n+\t\t\t\t\t\t answer);\n \n \tipc_client_close_connection(connection);\n \n@@ -503,7 +507,7 @@ static int worker_thread__do_io(\n \tif (ret >= 0) {\n \t\tret = worker_thread_data->server_data->application_cb(\n \t\t\tworker_thread_data->server_data->application_data,\n-\t\t\tbuf.buf, do_io_reply_callback, &reply_data);\n+\t\t\tbuf.buf, buf.len, do_io_reply_callback, &reply_data);\n \n \t\tpacket_flush_gently(reply_data.fd);\n \t}\ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex 8dc7bda087d..8e889d6a506 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -208,7 +208,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection)\n \n int ipc_client_send_command_to_connection(\n \tstruct ipc_client_connection *connection,\n-\tconst char *message, struct strbuf *answer)\n+\tconst char *message, size_t message_len,\n+\tstruct strbuf *answer)\n {\n \tint ret = 0;\n \n@@ -216,7 +217,7 @@ int ipc_client_send_command_to_connection(\n \n \ttrace2_region_enter(\"ipc-client\", \"send-command\", NULL);\n \n-\tif (write_packetized_from_buf_no_flush(message, strlen(message),\n+\tif (write_packetized_from_buf_no_flush(message, message_len,\n \t\t\t\t\t connection->fd) < 0 ||\n \t packet_flush_gently(connection->fd) < 0) {\n \t\tret = error(_(\"could not send IPC command\"));\n@@ -239,7 +240,8 @@ done:\n \n int ipc_client_send_command(const char *path,\n \t\t\t const struct ipc_client_connect_options *options,\n-\t\t\t const char *message, struct strbuf *response)\n+\t\t\t const char *message, size_t message_len,\n+\t\t\t struct strbuf *response)\n {\n \tint ret = -1;\n \tenum ipc_active_state state;\n@@ -250,7 +252,9 @@ int ipc_client_send_command(const char *path,\n \tif (state != IPC_STATE__LISTENING)\n \t\treturn ret;\n \n-\tret = ipc_client_send_command_to_connection(connection, message, response);\n+\tret = ipc_client_send_command_to_connection(connection,\n+\t\t\t\t\t\t message, message_len,\n+\t\t\t\t\t\t response);\n \n \tipc_client_close_connection(connection);\n \n@@ -458,7 +462,7 @@ static int do_io(struct ipc_server_thread_data *server_thread_data)\n \tif (ret >= 0) {\n \t\tret = server_thread_data->server_data->application_cb(\n \t\t\tserver_thread_data->server_data->application_data,\n-\t\t\tbuf.buf, do_io_reply_callback, &reply_data);\n+\t\t\tbuf.buf, buf.len, do_io_reply_callback, &reply_data);\n \n \t\tpacket_flush_gently(reply_data.fd);\n \ndiff --git a/simple-ipc.h b/simple-ipc.h\nindex 2c48a5ee004..9c7330fcda0 100644\n--- a/simple-ipc.h\n+++ b/simple-ipc.h\n@@ -107,7 +107,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection);\n */\n int ipc_client_send_command_to_connection(\n \tstruct ipc_client_connection *connection,\n-\tconst char *message, struct strbuf *answer);\n+\tconst char *message, size_t message_len,\n+\tstruct strbuf *answer);\n \n /*\n * Used by the client to synchronously connect and send and receive a\n@@ -119,7 +120,8 @@ int ipc_client_send_command_to_connection(\n */\n int ipc_client_send_command(const char *path,\n \t\t\t const struct ipc_client_connect_options *options,\n-\t\t\t const char *message, struct strbuf *answer);\n+\t\t\t const char *message, size_t message_len,\n+\t\t\t struct strbuf *answer);\n \n /*\n * Simple IPC Server Side API.\n@@ -144,6 +146,7 @@ typedef int (ipc_server_reply_cb)(struct ipc_server_reply_data *,\n */\n typedef int (ipc_server_application_cb)(void *application_data,\n \t\t\t\t\tconst char *request,\n+\t\t\t\t\tsize_t request_len,\n \t\t\t\t\tipc_server_reply_cb *reply_cb,\n \t\t\t\t\tstruct ipc_server_reply_data *reply_data);\n \ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex 42040ef81b1..91345180750 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -112,7 +112,7 @@ static int app__slow_command(ipc_server_reply_cb *reply_cb,\n /*\n * The client sent a command followed by a (possibly very) large buffer.\n */\n-static int app__sendbytes_command(const char *received,\n+static int app__sendbytes_command(const char *received, size_t received_len,\n \t\t\t\t ipc_server_reply_cb *reply_cb,\n \t\t\t\t struct ipc_server_reply_data *reply_data)\n {\n@@ -123,6 +123,13 @@ static int app__sendbytes_command(const char *received,\n \tint errs = 0;\n \tint ret;\n \n+\t/*\n+\t * The test is setup to send:\n+\t * \"sendbytes\" SP \n+\t */\n+\tif (received_len < strlen(\"sendbytes \"))\n+\t\tBUG(\"received_len is short in app__sendbytes_command\");\n+\n \tif (skip_prefix(received, \"sendbytes \", &p))\n \t\tlen_ballast = strlen(p);\n \n@@ -160,7 +167,7 @@ static ipc_server_application_cb test_app_cb;\n * by this application.\n */\n static int test_app_cb(void *application_data,\n-\t\t const char *command,\n+\t\t const char *command, size_t command_len,\n \t\t ipc_server_reply_cb *reply_cb,\n \t\t struct ipc_server_reply_data *reply_data)\n {\n@@ -173,7 +180,7 @@ static int test_app_cb(void *application_data,\n \tif (application_data != (void*)&my_app_data)\n \t\tBUG(\"application_cb: application_data pointer wrong\");\n \n-\tif (!strcmp(command, \"quit\")) {\n+\tif (command_len == 4 && !strncmp(command, \"quit\", 4)) {\n \t\t/*\n \t\t * The client sent a \"quit\" command. This is an async\n \t\t * request for the server to shutdown.\n@@ -193,22 +200,23 @@ static int test_app_cb(void *application_data,\n \t\treturn SIMPLE_IPC_QUIT;\n \t}\n \n-\tif (!strcmp(command, \"ping\")) {\n+\tif (command_len == 4 && !strncmp(command, \"ping\", 4)) {\n \t\tconst char *answer = \"pong\";\n \t\treturn reply_cb(reply_data, answer, strlen(answer));\n \t}\n \n-\tif (!strcmp(command, \"big\"))\n+\tif (command_len == 3 && !strncmp(command, \"big\", 3))\n \t\treturn app__big_command(reply_cb, reply_data);\n \n-\tif (!strcmp(command, \"chunk\"))\n+\tif (command_len == 5 && !strncmp(command, \"chunk\", 5))\n \t\treturn app__chunk_command(reply_cb, reply_data);\n \n-\tif (!strcmp(command, \"slow\"))\n+\tif (command_len == 4 && !strncmp(command, \"slow\", 4))\n \t\treturn app__slow_command(reply_cb, reply_data);\n \n-\tif (starts_with(command, \"sendbytes \"))\n-\t\treturn app__sendbytes_command(command, reply_cb, reply_data);\n+\tif (command_len >= 10 && starts_with(command, \"sendbytes \"))\n+\t\treturn app__sendbytes_command(command, command_len,\n+\t\t\t\t\t reply_cb, reply_data);\n \n \treturn app__unhandled_command(command, reply_cb, reply_data);\n }\n@@ -488,7 +496,9 @@ static int client__send_ipc(void)\n \toptions.wait_if_busy = 1;\n \toptions.wait_if_not_found = 0;\n \n-\tif (!ipc_client_send_command(cl_args.path, &options, command, &buf)) {\n+\tif (!ipc_client_send_command(cl_args.path, &options,\n+\t\t\t\t command, strlen(command),\n+\t\t\t\t &buf)) {\n \t\tif (buf.len) {\n \t\t\tprintf(\"%s\\n\", buf.buf);\n \t\t\tfflush(stdout);\n@@ -556,7 +566,9 @@ static int do_sendbytes(int bytecount, char byte, const char *path,\n \tstrbuf_addstr(&buf_send, \"sendbytes \");\n \tstrbuf_addchars(&buf_send, byte, bytecount);\n \n-\tif (!ipc_client_send_command(path, options, buf_send.buf, &buf_resp)) {\n+\tif (!ipc_client_send_command(path, options,\n+\t\t\t\t buf_send.buf, buf_send.len,\n+\t\t\t\t &buf_resp)) {\n \t\tstrbuf_rtrim(&buf_resp);\n \t\tprintf(\"sent:%c%08d %s\\n\", byte, bytecount, buf_resp.buf);\n \t\tfflush(stdout);\n-- \ngitgitgadget\n\n"},{"id":"436049","messageId":"57f29feaadb4a732892cd193b2a1d3c838f09421.1631738177.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.git.1631738177.gitgitgadget@gmail.com","subject":"[PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-15T20:36:17Z","receivedAt":"2021-09-15T20:36:34Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nConvert test helper to use `start_bg_command()` when spawning a server\ndaemon in the background rather than blocks of platform-specific code.\n\nSigned-off-by: Jeff Hostetler \n---\n t/helper/test-simple-ipc.c | 193 ++++++++-----------------------------\n 1 file changed, 40 insertions(+), 153 deletions(-)\n\ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex 91345180750..59a950f3b00 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -9,6 +9,7 @@\n #include \"parse-options.h\"\n #include \"thread-utils.h\"\n #include \"strvec.h\"\n+#include \"run-command.h\"\n \n #ifndef SUPPORTS_SIMPLE_IPC\n int cmd__simple_ipc(int argc, const char **argv)\n@@ -274,178 +275,64 @@ static int daemon__run_server(void)\n \treturn ret;\n }\n \n-#ifndef GIT_WINDOWS_NATIVE\n-/*\n- * This is adapted from `daemonize()`. Use `fork()` to directly create and\n- * run the daemon in a child process.\n- */\n-static int spawn_server(pid_t *pid)\n-{\n-\tstruct ipc_server_opts opts = {\n-\t\t.nr_threads = cl_args.nr_threads,\n-\t};\n+static start_bg_wait_cb bg_wait_cb;\n \n-\t*pid = fork();\n-\n-\tswitch (*pid) {\n-\tcase 0:\n-\t\tif (setsid() == -1)\n-\t\t\terror_errno(_(\"setsid failed\"));\n-\t\tclose(0);\n-\t\tclose(1);\n-\t\tclose(2);\n-\t\tsanitize_stdfds();\n+static int bg_wait_cb(void *cb_data, const struct child_process *cp)\n+{\n+\tint s = ipc_get_active_state(cl_args.path);\n \n-\t\treturn ipc_server_run(cl_args.path, &opts, test_app_cb,\n-\t\t\t\t (void*)&my_app_data);\n+\tswitch (s) {\n+\tcase IPC_STATE__LISTENING:\n+\t\t/* child is \"ready\" */\n+\t\treturn 0;\n \n-\tcase -1:\n-\t\treturn error_errno(_(\"could not spawn daemon in the background\"));\n+\tcase IPC_STATE__NOT_LISTENING:\n+\tcase IPC_STATE__PATH_NOT_FOUND:\n+\t\t/* give child more time */\n+\t\treturn 1;\n \n \tdefault:\n-\t\treturn 0;\n+\tcase IPC_STATE__INVALID_PATH:\n+\tcase IPC_STATE__OTHER_ERROR:\n+\t\t/* all the time in world won't help */\n+\t\treturn -1;\n \t}\n }\n-#else\n-/*\n- * Conceptually like `daemonize()` but different because Windows does not\n- * have `fork(2)`. Spawn a normal Windows child process but without the\n- * limitations of `start_command()` and `finish_command()`.\n- */\n-static int spawn_server(pid_t *pid)\n-{\n-\tchar test_tool_exe[MAX_PATH];\n-\tstruct strvec args = STRVEC_INIT;\n-\tint in, out;\n-\n-\tGetModuleFileNameA(NULL, test_tool_exe, MAX_PATH);\n-\n-\tin = open(\"/dev/null\", O_RDONLY);\n-\tout = open(\"/dev/null\", O_WRONLY);\n-\n-\tstrvec_push(&args, test_tool_exe);\n-\tstrvec_push(&args, \"simple-ipc\");\n-\tstrvec_push(&args, \"run-daemon\");\n-\tstrvec_pushf(&args, \"--name=%s\", cl_args.path);\n-\tstrvec_pushf(&args, \"--threads=%d\", cl_args.nr_threads);\n-\n-\t*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);\n-\tclose(in);\n-\tclose(out);\n-\n-\tstrvec_clear(&args);\n \n-\tif (*pid < 0)\n-\t\treturn error(_(\"could not spawn daemon in the background\"));\n-\n-\treturn 0;\n-}\n-#endif\n-\n-/*\n- * This is adapted from `wait_or_whine()`. Watch the child process and\n- * let it get started and begin listening for requests on the socket\n- * before reporting our success.\n- */\n-static int wait_for_server_startup(pid_t pid_child)\n+static int daemon__start_server(void)\n {\n-\tint status;\n-\tpid_t pid_seen;\n-\tenum ipc_active_state s;\n-\ttime_t time_limit, now;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tenum start_bg_result sbgr;\n \n-\ttime(&time_limit);\n-\ttime_limit += cl_args.max_wait_sec;\n+\tstrvec_push(&cp.args, \"test-tool\");\n+\tstrvec_push(&cp.args, \"simple-ipc\");\n+\tstrvec_push(&cp.args, \"run-daemon\");\n+\tstrvec_pushf(&cp.args, \"--name=%s\", cl_args.path);\n+\tstrvec_pushf(&cp.args, \"--threads=%d\", cl_args.nr_threads);\n \n-\tfor (;;) {\n-\t\tpid_seen = waitpid(pid_child, &status, WNOHANG);\n+\tcp.no_stdin = 1;\n+\tcp.no_stdout = 1;\n+\tcp.no_stderr = 1;\n \n-\t\tif (pid_seen == -1)\n-\t\t\treturn error_errno(_(\"waitpid failed\"));\n+\tsbgr = start_bg_command(&cp, bg_wait_cb, NULL, cl_args.max_wait_sec);\n \n-\t\telse if (pid_seen == 0) {\n-\t\t\t/*\n-\t\t\t * The child is still running (this should be\n-\t\t\t * the normal case). Try to connect to it on\n-\t\t\t * the socket and see if it is ready for\n-\t\t\t * business.\n-\t\t\t *\n-\t\t\t * If there is another daemon already running,\n-\t\t\t * our child will fail to start (possibly\n-\t\t\t * after a timeout on the lock), but we don't\n-\t\t\t * care (who responds) if the socket is live.\n-\t\t\t */\n-\t\t\ts = ipc_get_active_state(cl_args.path);\n-\t\t\tif (s == IPC_STATE__LISTENING)\n-\t\t\t\treturn 0;\n-\n-\t\t\ttime(&now);\n-\t\t\tif (now > time_limit)\n-\t\t\t\treturn error(_(\"daemon not online yet\"));\n-\n-\t\t\tcontinue;\n-\t\t}\n+\tswitch (sbgr) {\n+\tcase SBGR_READY:\n+\t\treturn 0;\n \n-\t\telse if (pid_seen == pid_child) {\n-\t\t\t/*\n-\t\t\t * The new child daemon process shutdown while\n-\t\t\t * it was starting up, so it is not listening\n-\t\t\t * on the socket.\n-\t\t\t *\n-\t\t\t * Try to ping the socket in the odd chance\n-\t\t\t * that another daemon started (or was already\n-\t\t\t * running) while our child was starting.\n-\t\t\t *\n-\t\t\t * Again, we don't care who services the socket.\n-\t\t\t */\n-\t\t\ts = ipc_get_active_state(cl_args.path);\n-\t\t\tif (s == IPC_STATE__LISTENING)\n-\t\t\t\treturn 0;\n+\tdefault:\n+\tcase SBGR_ERROR:\n+\tcase SBGR_CB_ERROR:\n+\t\treturn error(\"daemon failed to start\");\n \n-\t\t\t/*\n-\t\t\t * We don't care about the WEXITSTATUS() nor\n-\t\t\t * any of the WIF*(status) values because\n-\t\t\t * `cmd__simple_ipc()` does the `!!result`\n-\t\t\t * trick on all function return values.\n-\t\t\t *\n-\t\t\t * So it is sufficient to just report the\n-\t\t\t * early shutdown as an error.\n-\t\t\t */\n-\t\t\treturn error(_(\"daemon failed to start\"));\n-\t\t}\n+\tcase SBGR_TIMEOUT:\n+\t\treturn error(\"daemon not online yet\");\n \n-\t\telse\n-\t\t\treturn error(_(\"waitpid is confused\"));\n+\tcase SBGR_DIED:\n+\t\treturn error(\"daemon terminated\");\n \t}\n }\n \n-/*\n- * This process will start a simple-ipc server in a background process and\n- * wait for it to become ready. This is like `daemonize()` but gives us\n- * more control and better error reporting (and makes it easier to write\n- * unit tests).\n- */\n-static int daemon__start_server(void)\n-{\n-\tpid_t pid_child;\n-\tint ret;\n-\n-\t/*\n-\t * Run the actual daemon in a background process.\n-\t */\n-\tret = spawn_server(&pid_child);\n-\tif (pid_child <= 0)\n-\t\treturn ret;\n-\n-\t/*\n-\t * Let the parent wait for the child process to get started\n-\t * and begin listening for requests on the socket.\n-\t */\n-\tret = wait_for_server_startup(pid_child);\n-\n-\treturn ret;\n-}\n-\n /*\n * This process will run a quick probe to see if a simple-ipc server\n * is active on this path.\n-- \ngitgitgadget\n"},{"id":"436050","messageId":"CAPig+cQ0qbyBrCDcJgg7=DezWGfZhyAR=YuC=vbxd38B4WA=Xw@mail.gmail.com","threadId":"56508","inReplyTo":"7182419f6dfa49d4d8415736943c72a575ab6753.1631738177.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/7] simple-ipc: preparations for supporting binary messages.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-15T20:43:33Z","receivedAt":"2021-09-15T20:43:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Sep 15, 2021 at 4:36 PM Jeff Hostetler via GitGitGadget\n wrote:\n> Add `command_len` argument to the Simple IPC API.\n>\n> In my original Simple IPC API, I assumed that the request\n> would always be a null-terminated string of text characters.\n> The command arg was just a `const char *`.\n>\n> I found a caller that would like to pass a binary command\n> to the daemon, so I want to ammend the Simple IPC API to\n\ns/ammend/amend/\n\n> take `const char *command, size_t command_len` and pass\n> that to the daemon. (Really, the first arg should just be\n> a `void *` or `const unsigned byte *` to make that clearer.)\n\nThe reader is left wondering why you didn't also change it to `const\nvoid *` (or one of the other choices) while at it.\n\n> Note, the response side has always been a `struct strbuf`\n> which includes the buffer and length, so we already support\n> returning a binary answer. (Yes, it feels a little weird\n> returning a binary buffer in a `strbuf`, but it works.)\n>\n> Signed-off-by: Jeff Hostetler \n"},{"id":"436051","messageId":"xmqqa6kdwo24.fsf@gitster.g","threadId":"56508","inReplyTo":"5f557caee004f22cee33e8753063f0315459d7e1.1631738177.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/7] trace2: fix memory leak of thread name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-15T21:01:39Z","receivedAt":"2021-09-15T21:01:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jeff Hostetler via GitGitGadget\" writes:\n\n> From: Jeff Hostetler \n>\n> Do not leak the thread name (contained within the thread context) when\n> a thread terminates.\n>\n> Signed-off-by: Jeff Hostetler \n> ---\n> trace2/tr2_tls.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\n> index 067c23755fb..7da94aba522 100644\n> --- a/trace2/tr2_tls.c\n> +++ b/trace2/tr2_tls.c\n> @@ -95,6 +95,7 @@ void tr2tls_unset_self(void)\n> \n> \tpthread_setspecific(tr2tls_key, NULL);\n> \n> +\tstrbuf_release(&ctx->thread_name);\n> \tfree(ctx->array_us_start);\n> \tfree(ctx);\n> }\n\nSo the idea is create allocates a new instance, and unset is to\nrelease the resource held by it?\n\nThis is not a problem in _this_ patch but more about the base code\nthat is being fixed here, but using strbuf as thread_name member\nsends a strong signal that it is designed to be inexpensive to\nchange thread_name after a context is created by create_self. If\nit is not the case, the member should be \"const char *\", which may\nbe computed using a temporary strbuf in create_self() and taken from\nthe strbuf with strbuf_detach(), I would think.\n\nThanks.\n"},{"id":"436052","messageId":"xmqq5yv1wnuc.fsf@gitster.g","threadId":"56508","inReplyTo":"7de207828caa9e50906f39b955e39e9de2c1768f.1631738177.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-15T21:06:19Z","receivedAt":"2021-09-15T21:06:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Jeff Hostetler via GitGitGadget\" writes:\n\n> From: Jeff Hostetler \n>\n> From: Carlo Marcelo Arenas Belón \n>\n> Move the declartion of the `enum ipc_active_state` type outside of\n> the SUPPORTS_SIMPLE_IPC ifdef.\n\nThe second one is not an in-body header since there is already a\nblank line that signals the end of in-body headers after the first\none.\n\nThis _may_ be a bug in GGG, perhaps?\n\n\n> A later commit will introduce the `fsmonitor_ipc__*()` API and stub in\n> a \"mock\" implementation that requires this enum in some function\n> signatures.\n>\n> Signed-off-by: Carlo Marcelo Arenas Belón \n> Signed-off-by: Jeff Hostetler \n> ---\n> simple-ipc.h | 14 +++++++-------\n> 1 file changed, 7 insertions(+), 7 deletions(-)\n>\n> diff --git a/simple-ipc.h b/simple-ipc.h\n> index 9c7330fcda0..b396293bdfc 100644\n> --- a/simple-ipc.h\n> +++ b/simple-ipc.h\n> @@ -5,13 +5,6 @@\n> * See Documentation/technical/api-simple-ipc.txt\n> */\n> \n> -#ifdef SUPPORTS_SIMPLE_IPC\n> -#include \"pkt-line.h\"\n> -\n> -/*\n> - * Simple IPC Client Side API.\n> - */\n> -\n> enum ipc_active_state {\n> \t/*\n> \t * The pipe/socket exists and the daemon is waiting for connections.\n> @@ -43,6 +36,13 @@ enum ipc_active_state {\n> \tIPC_STATE__OTHER_ERROR,\n> };\n> \n> +#ifdef SUPPORTS_SIMPLE_IPC\n> +#include \"pkt-line.h\"\n> +\n> +/*\n> + * Simple IPC Client Side API.\n> + */\n> +\n> struct ipc_client_connect_options {\n> \t/*\n> \t * Spin under timeout if the server is running but can't\n"},{"id":"436065","messageId":"YULF3hoaDxA9ENdO@nand.local","threadId":"56508","inReplyTo":"xmqqa6kdwo24.fsf@gitster.g","subject":"Re: [PATCH 1/7] trace2: fix memory leak of thread name","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-16T04:19:42Z","receivedAt":"2021-09-16T04:20:22Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 15, 2021 at 02:01:39PM -0700, Junio C Hamano wrote:\n> \"Jeff Hostetler via GitGitGadget\" writes:\n>\n> > From: Jeff Hostetler \n> >\n> > Do not leak the thread name (contained within the thread context) when\n> > a thread terminates.\n> >\n> > Signed-off-by: Jeff Hostetler \n> > ---\n> > trace2/tr2_tls.c | 1 +\n> > 1 file changed, 1 insertion(+)\n> >\n> > diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\n> > index 067c23755fb..7da94aba522 100644\n> > --- a/trace2/tr2_tls.c\n> > +++ b/trace2/tr2_tls.c\n> > @@ -95,6 +95,7 @@ void tr2tls_unset_self(void)\n> >\n> > \tpthread_setspecific(tr2tls_key, NULL);\n> >\n> > +\tstrbuf_release(&ctx->thread_name);\n> > \tfree(ctx->array_us_start);\n> > \tfree(ctx);\n> > }\n>\n> So the idea is create allocates a new instance, and unset is to\n> release the resource held by it?\n>\n> This is not a problem in _this_ patch but more about the base code\n> that is being fixed here, but using strbuf as thread_name member\n> sends a strong signal that it is designed to be inexpensive to\n> change thread_name after a context is created by create_self. If\n> it is not the case, the member should be \"const char *\", which may\n> be computed using a temporary strbuf in create_self() and taken from\n> the strbuf with strbuf_detach(), I would think.\n\nIt looks like we do not change the contents of the thread_name buffer\nanywhere. I assume that we store the strbuf in struct tr2tls_thread_ctx\nbecause we do some string formatting in tr2tls_create_self().\n\nBut there we could easily say ctx->thread_name = strbuf_release(&buf).\n\nMore concerning to me is that we use signed integers to keep track of nr\nand alloc of array_us_start. But both of these are separate issues and\ndon't need to be dealt with here.\n\nThanks,\nTaylor\n"},{"id":"436067","messageId":"YULNs166fGOfVUVy@nand.local","threadId":"56508","inReplyTo":"f97038a563d889d740a7e968fcbdfaadb41e2008.1631738177.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/7] run-command: create start_bg_command","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-16T04:53:07Z","receivedAt":"2021-09-16T04:53:10Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 15, 2021 at 08:36:16PM +0000, Jeff Hostetler via GitGitGadget wrote:\n> From: Jeff Hostetler \n>\n> Create a variation of `run_command()` and `start_command()` to launch a command\n> into the background and optionally wait for it to become \"ready\" before returning.\n>\n> Signed-off-by: Jeff Hostetler \n> ---\n> run-command.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> run-command.h | 48 ++++++++++++++++++++\n> 2 files changed, 171 insertions(+)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 3e4e082e94d..fe75fd08f74 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1901,3 +1901,126 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)\n> \t}\n> \tstrvec_pushf(env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT, new_git_dir);\n> }\n> +\n> +enum start_bg_result start_bg_command(struct child_process *cmd,\n> +\t\t\t\t start_bg_wait_cb *wait_cb,\n> +\t\t\t\t void *cb_data,\n> +\t\t\t\t unsigned int timeout_sec)\n> +{\n> +\tenum start_bg_result sbgr = SBGR_ERROR;\n> +\tint ret;\n> +\tint wait_status;\n> +\tpid_t pid_seen;\n> +\ttime_t time_limit;\n> +\n> +\t/*\n> +\t * Silently disallow child cleanup -- even if requested.\n> +\t * The child process should persist in the background\n> +\t * and possibly/probably after this process exits. That\n> +\t * is, don't kill the child during our atexit routine.\n> +\t */\n> +\tcmd->clean_on_exit = 0;\n> +\n> +\tret = start_command(cmd);\n> +\tif (ret) {\n> +\t\t/*\n> +\t\t * We assume that if `start_command()` fails, we\n> +\t\t * either get a complete `trace2_child_start() /\n> +\t\t * trace2_child_exit()` pair or it fails before the\n> +\t\t * `trace2_child_start()` is emitted, so we do not\n> +\t\t * need to worry about it here.\n> +\t\t *\n> +\t\t * We also assume that `start_command()` does not add\n> +\t\t * us to the cleanup list. And that it calls\n> +\t\t * calls `child_process_clear()`.\n> +\t\t */\n> +\t\tsbgr = SBGR_ERROR;\n> +\t\tgoto done;\n> +\t}\n> +\n> +\ttime(&time_limit);\n> +\ttime_limit += timeout_sec;\n\nThis jumped out to me as unsafe, since POSIX guarantees time_t to be an\nintegral value holding a number of seconds (so += timeout_sec is safe\nthere), but it isn't in the C standard.\n\nBut we have lots of other examples of adding a number of seconds\ndirectly the value filled in by time(2), so I think this is fine.\n\n> +\n> +wait:\n> +\tpid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);\n> +\n> +\tif (pid_seen == 0) {\n\nSmall nit, probably better to write this as if (!pid_seen), but not\nworth a reroll alone.\n\n> +\t\t/*\n> +\t\t * The child is currently running. Ask the callback\n> +\t\t * if the child is ready to do work or whether we\n> +\t\t * should keep waiting for it to boot up.\n> +\t\t */\n\nThis comment is simple and informative, thank you!\n\n> +\t\tret = (*wait_cb)(cb_data, cmd);\n> +\t\tif (!ret) {\n> +\t\t\t/*\n> +\t\t\t * The child is running and \"ready\".\n> +\t\t\t *\n> +\t\t\t * NEEDSWORK: As we prepare to orphan (release to\n> +\t\t\t * the background) this child, it is not appropriate\n> +\t\t\t * to emit a `trace2_child_exit()` event. Should we\n> +\t\t\t * create a new event for this case?\n\nProbably. Maybe trace2_child_orphaned() or trace2_child_background()?\n\n> +\t\t\t */\n> +\t\t\tsbgr = SBGR_READY;\n> +\t\t\tgoto done;\n> +\t\t} else if (ret > 0) {\n> +\t\t\ttime_t now;\n> +\n> +\t\t\ttime(&now);\n> +\t\t\tif (now < time_limit)\n> +\t\t\t\tgoto wait;\n> +\n> +\t\t\t/*\n> +\t\t\t * Our timeout has expired. We don't try to\n> +\t\t\t * kill the child, but rather let it continue\n> +\t\t\t * (hopefully) trying to startup.\n> +\t\t\t *\n> +\t\t\t * NEEDSWORK: Like the \"ready\" case, should we\n> +\t\t\t * log a custom child-something Trace2 event here?\n> +\t\t\t */\n> +\t\t\tsbgr = SBGR_TIMEOUT;\n> +\t\t\tgoto done;\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * The cb gave up on this child.\n> +\t\t\t *\n> +\t\t\t * NEEDSWORK: Like above, should we log a custom\n> +\t\t\t * Trace2 child-something event here?\n> +\t\t\t */\n> +\t\t\tsbgr = SBGR_CB_ERROR;\n> +\t\t\tgoto done;\n> +\t\t}\n\nOK, so assuming that the child is running, then we ask wait_cb what to\ndo. Returning zero from the callback means to background it, a positive\nvalue means to give it more time, and negative means to cause an error.\nAnd those match the documentation below, good.\n\n> +\tif (pid_seen == cmd->pid) {\n\nThis could be an \"else if\", no?\n\n> +\t\tint child_code = -1;\n> +\n> +\t\t/*\n> +\t\t * The child started, but exited or was terminated\n> +\t\t * before becoming \"ready\".\n> +\t\t *\n> +\t\t * We try to match the behavior of `wait_or_whine()`\n> +\t\t * and convert the child's status to a return code for\n> +\t\t * tracing purposes and emit the `trace2_child_exit()`\n> +\t\t * event.\n> +\t\t */\n> +\t\tif (WIFEXITED(wait_status))\n> +\t\t\tchild_code = WEXITSTATUS(wait_status);\n> +\t\telse if (WIFSIGNALED(wait_status))\n> +\t\t\tchild_code = WTERMSIG(wait_status) + 128;\n\nDo we care about emitting the same error (when it was signaled with\nsomething other than SIGINT/SIGQUIT/SIGPIPE) as is reported by\nwait_or_whine()?\n\nIf we want that error here, too, we could probably share the same code\nhere from here and in wait_or_whine(). I would probably write something\nlike:\n\n static int handle_awaited_status(int status, int *code)\n {\n if (WIFSIGNALED(status)) {\n *code = WTERMSIG(status);\n if (*code != SIGINT && *code != SIGQUIT && *code != SIGPIPE)\n error(\"%s died of signal %d\", argv0, *code);\n /*\n * This return value is chosen so that code & 0xff\n * mimics the exit code that a POSIX shell would report for\n * a program that died from this signal.\n */\n *code += 128;\n return 1;\n } else if (WIFEXITED(status)) {\n *code = WEXITSTATUS(status);\n return 1;\n }\n return 0;\n }\n\nso that we could call it in wait_or_whine() like:\n\n } else if (!handle_awaited_status(status, &code)) {\n error(\"waitpid is confused (%s)\", argv0);\n }\n\nand similarly here in this new function. Alternatively, if we don't want\nthat error, then it may help future readers to add a short comment\nexplaining why not.\n\n> +/**\n> + * Callback used by `start_bg_command()` to ask whether the\n> + * child process is ready or needs more time to become ready.\n> + *\n> + * Returns 1 is child needs more time (subject to the requested timeout).\n> + * Returns 0 if child is ready.\n> + * Returns -1 on any error and cause `start_bg_command()` to also error out.\n> + */\n> +typedef int(start_bg_wait_cb)(void *cb_data,\n> +\t\t\t const struct child_process *cmd);\n\nNitpicking, but typically I would assume that the \"extra\" void pointer\nis the last argument in a callback. It definitely does not matter,\nthough.\n\nThanks,\nTaylor\n"},{"id":"436068","messageId":"YULPBmIYJLv654kz@nand.local","threadId":"56508","inReplyTo":"YULNs166fGOfVUVy@nand.local","subject":"Re: [PATCH 6/7] run-command: create start_bg_command","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-16T04:58:46Z","receivedAt":"2021-09-16T04:58:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 16, 2021 at 12:53:07AM -0400, Taylor Blau wrote:\n> > +/**\n> > + * Callback used by `start_bg_command()` to ask whether the\n> > + * child process is ready or needs more time to become ready.\n> > + *\n> > + * Returns 1 is child needs more time (subject to the requested timeout).\n> > + * Returns 0 if child is ready.\n> > + * Returns -1 on any error and cause `start_bg_command()` to also error out.\n> > + */\n> > +typedef int(start_bg_wait_cb)(void *cb_data,\n> > +\t\t\t const struct child_process *cmd);\n>\n> Nitpicking, but typically I would assume that the \"extra\" void pointer\n> is the last argument in a callback. It definitely does not matter,\n> though.\n\nLooking at the last patch (which adds the first implementation of one of\nthese callbacks) it appears that this cb_data pointer is unused. I\nassume that it is used in later patches which aren't in this topic?\n\nIf so, then it may help future readers to indicate as much in the patch\nmessage. Perhaps \"the cb_data argument in the start_bg_wait_cb callback\nis unused in this series, but will be useful in later patches\".\n\nThanks,\nTaylor\n"},{"id":"436069","messageId":"YULQ8DJFKlrOml8Z@nand.local","threadId":"56508","inReplyTo":"57f29feaadb4a732892cd193b2a1d3c838f09421.1631738177.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-16T05:06:56Z","receivedAt":"2021-09-16T05:06:59Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote:\n> From: Jeff Hostetler \n>\n> Convert test helper to use `start_bg_command()` when spawning a server\n> daemon in the background rather than blocks of platform-specific code.\n>\n> Signed-off-by: Jeff Hostetler \n> ---\n> t/helper/test-simple-ipc.c | 193 ++++++++-----------------------------\n> 1 file changed, 40 insertions(+), 153 deletions(-)\n>\n> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\n> index 91345180750..59a950f3b00 100644\n> --- a/t/helper/test-simple-ipc.c\n> +++ b/t/helper/test-simple-ipc.c\n> @@ -9,6 +9,7 @@\n> #include \"parse-options.h\"\n> #include \"thread-utils.h\"\n> #include \"strvec.h\"\n> +#include \"run-command.h\"\n>\n> #ifndef SUPPORTS_SIMPLE_IPC\n> int cmd__simple_ipc(int argc, const char **argv)\n> @@ -274,178 +275,64 @@ static int daemon__run_server(void)\n> \treturn ret;\n> }\n>\n> -#ifndef GIT_WINDOWS_NATIVE\n> -/*\n> - * This is adapted from `daemonize()`. Use `fork()` to directly create and\n> - * run the daemon in a child process.\n> - */\n> -static int spawn_server(pid_t *pid)\n> -{\n> -\tstruct ipc_server_opts opts = {\n> -\t\t.nr_threads = cl_args.nr_threads,\n> -\t};\n> +static start_bg_wait_cb bg_wait_cb;\n\nThis whole patch is delightful to read, as the new implementation is so\nmuch cleaner as a result of the earlier work in this series.\n\nAm I correct in assuming that this is to encourage a compiler error if\nbg_wait_cb does not satisfy the type of start_bg_wait_cb? If so, then I\nthink we are already getting that by trying to pass bg_wait_cb to\nstart_bg_command().\n\nE.g., applying this (intentionally broken) diff on top:\n\n--- 8< ---\n\ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex 59a950f3b0..3aed787206 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -275,9 +275,7 @@ static int daemon__run_server(void)\n \treturn ret;\n }\n\n-static start_bg_wait_cb bg_wait_cb;\n-\n-static int bg_wait_cb(void *cb_data, const struct child_process *cp)\n+static int bg_wait_cb(const void *cb_data, const struct child_process *cp)\n {\n \tint s = ipc_get_active_state(cl_args.path);\n\n--- >8 ---\n\nand then compiling still warns of a mismatched type when calling\nstart_bg_command().\n\n> -\t*pid = fork();\n> -\n> -\tswitch (*pid) {\n> -\tcase 0:\n> -\t\tif (setsid() == -1)\n> -\t\t\terror_errno(_(\"setsid failed\"));\n> -\t\tclose(0);\n> -\t\tclose(1);\n> -\t\tclose(2);\n> -\t\tsanitize_stdfds();\n> +static int bg_wait_cb(void *cb_data, const struct child_process *cp)\n> +{\n> +\tint s = ipc_get_active_state(cl_args.path);\n>\n> -\t\treturn ipc_server_run(cl_args.path, &opts, test_app_cb,\n> -\t\t\t\t (void*)&my_app_data);\n> +\tswitch (s) {\n> +\tcase IPC_STATE__LISTENING:\n> +\t\t/* child is \"ready\" */\n> +\t\treturn 0;\n>\n> -\tcase -1:\n> -\t\treturn error_errno(_(\"could not spawn daemon in the background\"));\n> +\tcase IPC_STATE__NOT_LISTENING:\n> +\tcase IPC_STATE__PATH_NOT_FOUND:\n> +\t\t/* give child more time */\n> +\t\treturn 1;\n>\n> \tdefault:\n\nI'm always a little hesitant to have default cases when switch over enum\ntypes, since it suppresses the warning when there's a new value of that\ntype. But we already have a similar default in client__probe_server().\n\n> -\t\telse if (pid_seen == pid_child) {\n> -\t\t\t/*\n> -\t\t\t * The new child daemon process shutdown while\n> -\t\t\t * it was starting up, so it is not listening\n> -\t\t\t * on the socket.\n> -\t\t\t *\n> -\t\t\t * Try to ping the socket in the odd chance\n> -\t\t\t * that another daemon started (or was already\n> -\t\t\t * running) while our child was starting.\n> -\t\t\t *\n> -\t\t\t * Again, we don't care who services the socket.\n> -\t\t\t */\n> -\t\t\ts = ipc_get_active_state(cl_args.path);\n> -\t\t\tif (s == IPC_STATE__LISTENING)\n> -\t\t\t\treturn 0;\n> +\tdefault:\n\nDitto.\n\nThanks,\nTaylor\n"},{"id":"436078","messageId":"87fsu5m649.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"5f557caee004f22cee33e8753063f0315459d7e1.1631738177.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/7] trace2: fix memory leak of thread name","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T05:35:59Z","receivedAt":"2021-09-16T05:39:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:\n\n> From: Jeff Hostetler \n>\n> Do not leak the thread name (contained within the thread context) when\n> a thread terminates.\n>\n> Signed-off-by: Jeff Hostetler \n> ---\n> trace2/tr2_tls.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/trace2/tr2_tls.c b/trace2/tr2_tls.c\n> index 067c23755fb..7da94aba522 100644\n> --- a/trace2/tr2_tls.c\n> +++ b/trace2/tr2_tls.c\n> @@ -95,6 +95,7 @@ void tr2tls_unset_self(void)\n> \n> \tpthread_setspecific(tr2tls_key, NULL);\n> \n> +\tstrbuf_release(&ctx->thread_name);\n> \tfree(ctx->array_us_start);\n> \tfree(ctx);\n> }\n\nAs noted in your cover letter:\n\n * A fix for a memory leak in the Trace2 code. (This was independently\n reported in last week in \"ab/tr2-leaks-and-fixes\".)\n\nSo I think this patch can be dropped from this series, since it's exact\nduplicate of my 48f68715b14 (tr2: stop leaking \"thread_name\" memory,\n2021-08-27) in ab/tr2-leaks-and-fixes, currently in \"next\" and marked\nfor a merge with master.\n\nWhen submitting a series that depends on another one it's best to rebase\nit on top of it & indicate it as such in the cover letter, Junio can\nqueue such a series on top of another one.\n\nIn this case I'm still not sure why this fix is here, i.e. surely\nnothing later in the series absolutely needs this stray memory leak\nfix...\n"},{"id":"436079","messageId":"YULZbQgxuyw8iJ/R@nand.local","threadId":"56508","inReplyTo":"87fsu5m649.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/7] trace2: fix memory leak of thread name","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-16T05:43:09Z","receivedAt":"2021-09-16T05:43:14Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote:\n> So I think this patch can be dropped from this series, since it's exact\n> duplicate of my 48f68715b14 (tr2: stop leaking \"thread_name\" memory,\n> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in \"next\" and marked\n> for a merge with master.\n\nI agree it can be dropped.\n\n> When submitting a series that depends on another one it's best to rebase\n> it on top of it & indicate it as such in the cover letter, Junio can\n> queue such a series on top of another one.\n>\n> In this case I'm still not sure why this fix is here, i.e. surely\n> nothing later in the series absolutely needs this stray memory leak\n> fix...\n\nBut there's no need for Jeff to depend on your branch, since (as you\nmentioned) this cleanup isn't relevant for anything else in this series,\nwhich is a sort of grab-bag of miscellaneous clean-ups.\n\nThanks,\nTaylor\n"},{"id":"436080","messageId":"87bl4tm5s1.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"30b7bb247c3c8531119f99e293d84b1836297251.1631738177.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T05:40:58Z","receivedAt":"2021-09-16T05:46:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:\n\n> @@ -112,6 +115,11 @@ static enum ipc_active_state connect_to_server(\n> \t\t\t\tif (GetLastError() == ERROR_SEM_TIMEOUT)\n> \t\t\t\t\treturn IPC_STATE__NOT_LISTENING;\n> \n> +\t\t\t\tgle = GetLastError();\n> +\t\t\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n> +\t\t\t\t\t\t \"connect/waitpipe/gle\",\n> +\t\t\t\t\t\t (intmax_t)gle);\n> +\n> \t\t\t\treturn IPC_STATE__OTHER_ERROR;\n> \t\t\t}\n> \n\nI've never used this Win32 API (or well, any Win32 API) but I'm guessing\nthat GetLastError() isn't here to check an error in GetLastError() itself.\n\nEarlier in this function added in your 59c7b88198a (simple-ipc: add\nwin32 implementation, 2021-03-15) we assign to \"gle\", I'd really expect...:\n\n> \tif (!SetNamedPipeHandleState(hPipe, &mode, NULL, NULL)) {\n> +\t\tgle = GetLastError();\n> +\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n> +\t\t\t\t \"connect/setpipestate/gle\",\n> +\t\t\t\t (intmax_t)gle);\n> +\n> \t\tCloseHandle(hPipe);\n> \t\treturn IPC_STATE__OTHER_ERROR;\n> \t}\n> \n> \t*pfd = _open_osfhandle((intptr_t)hPipe, O_RDWR|O_BINARY);\n> \tif (*pfd < 0) {\n> +\t\tgle = GetLastError();\n> +\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n> +\t\t\t\t \"connect/openosfhandle/gle\",\n> +\t\t\t\t (intmax_t)gle);\n> +\n> \t\tCloseHandle(hPipe);\n> \t\treturn IPC_STATE__OTHER_ERROR;\n> \t}\n\n...something that looks exactly like this. I.e. as shown by the below\nhunk-at-the-end, as it is I'm either missing some subtlety that could\nreally use explaining. I.e. this reads like:\n\n int saved_errno = errno;\n if (syscall()) {\n if (errno)\n die(\"bad\");\n saved_errno = errno;\n log_it(\"...%d\", saved_errno);\n }\n\nWhen surely we want either of:\n\n int saved_errno = errno;\n if (syscall()) {\n if (errno)\n die(\"bad\");\n log_it(\"...%d\", errno);\n }\n\nOr better yet (and consistent with the rest of your code):\n\n int saved_errno = errno;\n if (syscall()) {\n saved_errno = errno;\n if (saved_errno)\n die(\"bad\");\n log_it(\"...%d\", saved_errno);\n }\n\ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex 8dc7bda087d..b0c422e4867 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -109,9 +109,12 @@ static enum ipc_active_state connect_to_server(\n \t\t\tt_start_ms = (DWORD)(getnanotime() / 1000000);\n \n \t\t\tif (!WaitNamedPipeW(wpath, timeout_ms)) {\n-\t\t\t\tif (GetLastError() == ERROR_SEM_TIMEOUT)\n+\t\t\t\tgle = GetLastError();\n+\t\t\t\tif (gle == ERROR_SEM_TIMEOUT)\n \t\t\t\t\treturn IPC_STATE__NOT_LISTENING;\n \n+\t\t\t\t/* ...rest of your patch */\n+\n \t\t\t\treturn IPC_STATE__OTHER_ERROR;\n \t\t\t}\n \n"},{"id":"436082","messageId":"877dfhm5ki.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"5eadf71929559968cafa18d03c3a623b1adff926.1631738177.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T05:47:27Z","receivedAt":"2021-09-16T05:51:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:\n\n> +struct my_sa_data\n> +{\n> +\tPSID pEveryoneSID;\n> +\tPACL pACL;\n> +\tPSECURITY_DESCRIPTOR pSD;\n> +\tLPSECURITY_ATTRIBUTES lpSA;\n> +};\n> +\n> +static void init_sa(struct my_sa_data *d)\n> +{\n> +\tmemset(d, 0, sizeof(*d));\n> +}\n> +\n> +static void release_sa(struct my_sa_data *d)\n> +{\n> +\tif (d->pEveryoneSID)\n> +\t\tFreeSid(d->pEveryoneSID);\n> +\tif (d->pACL)\n> +\t\tLocalFree(d->pACL);\n> +\tif (d->pSD)\n> +\t\tLocalFree(d->pSD);\n> +\tif (d->lpSA)\n> +\t\tLocalFree(d->lpSA);\n> +\n> +\tmemset(d, 0, sizeof(*d));\n> +}\n\n[...]\n\n> +fail:\n> +\trelease_sa(d);\n> +\treturn NULL;\n> +}\n> +\n> static HANDLE create_new_pipe(wchar_t *wpath, int is_first)\n> {\n> \tHANDLE hPipe;\n> \tDWORD dwOpenMode, dwPipeMode;\n> -\tLPSECURITY_ATTRIBUTES lpsa = NULL;\n> +\tstruct my_sa_data my_sa_data;\n> +\n> +\tinit_sa(&my_sa_data);\n> \n\n[...]\n\n> \thPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,\n> -\t\t\t\t PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);\n> +\t\t\t\t PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,\n> +\t\t\t\t my_sa_data.lpSA);\n> +\n> +\trelease_sa(&my_sa_data);\n> \n> \treturn hPipe;\n> }\n\nJust a nit on the init pattern, since we always allocate this on the\nstack (this as all the relevant \"struct my_sa_data\") I'd have thought to\nsee:\n\n #define INIT_MY_SA_DATA { 0 }\n [...]\n struct my_sa_data my_sa_data = INIT_MY_SA_DATA;\n\nWhich gets rid of the need for an init_sa() function.\n\nAlso having the release_sa() do a memset() is a bit odd, usually we have\na reset*() function do that if the intent is to re-use, but it doesn't\nappear to be in this case, and we don't return this data anywhere, do\nwe?\n\n"},{"id":"436083","messageId":"87v931gj1s.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"57f29feaadb4a732892cd193b2a1d3c838f09421.1631738177.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T05:55:32Z","receivedAt":"2021-09-16T05:56:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:\n\n[...]\n\n> +\tdefault:\n> +\tcase SBGR_ERROR:\n> +\tcase SBGR_CB_ERROR:\n> +\t\treturn error(\"daemon failed to start\");\n> \n> -\t\t\t/*\n> -\t\t\t * We don't care about the WEXITSTATUS() nor\n> -\t\t\t * any of the WIF*(status) values because\n> -\t\t\t * `cmd__simple_ipc()` does the `!!result`\n> -\t\t\t * trick on all function return values.\n> -\t\t\t *\n> -\t\t\t * So it is sufficient to just report the\n> -\t\t\t * early shutdown as an error.\n> -\t\t\t */\n> -\t\t\treturn error(_(\"daemon failed to start\"));\n> -\t\t}\n> +\tcase SBGR_TIMEOUT:\n> +\t\treturn error(\"daemon not online yet\");\n> \n> -\t\telse\n> -\t\t\treturn error(_(\"waitpid is confused\"));\n> +\tcase SBGR_DIED:\n> +\t\treturn error(\"daemon terminated\");\n> \t}\n> }\n\nIt's not mentioned in the commit message, but the while-we're-at-it\ndropping of _() makes sense here, it shouldn't have been used in a test\nhelper to begin with. I.e. translators don't need to be translating\nstuff purely internal to the test suite.\n"},{"id":"436084","messageId":"87r1dpgibi.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"f97038a563d889d740a7e968fcbdfaadb41e2008.1631738177.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 6/7] run-command: create start_bg_command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T05:56:52Z","receivedAt":"2021-09-16T06:12:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:\n\n> From: Jeff Hostetler \n>\n> Create a variation of `run_command()` and `start_command()` to launch a command\n> into the background and optionally wait for it to become \"ready\" before returning.\n>\n> Signed-off-by: Jeff Hostetler \n> ---\n> run-command.c | 123 ++++++++++++++++++++++++++++++++++++++++++++++++++\n> run-command.h | 48 ++++++++++++++++++++\n> 2 files changed, 171 insertions(+)\n>\n> diff --git a/run-command.c b/run-command.c\n> index 3e4e082e94d..fe75fd08f74 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1901,3 +1901,126 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)\n> \t}\n> \tstrvec_pushf(env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT, new_git_dir);\n> }\n> +\n> +enum start_bg_result start_bg_command(struct child_process *cmd,\n> +\t\t\t\t start_bg_wait_cb *wait_cb,\n> +\t\t\t\t void *cb_data,\n> +\t\t\t\t unsigned int timeout_sec)\n> +{\n> +\tenum start_bg_result sbgr = SBGR_ERROR;\n> +\tint ret;\n> +\tint wait_status;\n> +\tpid_t pid_seen;\n> +\ttime_t time_limit;\n> +\n> +\t/*\n> +\t * Silently disallow child cleanup -- even if requested.\n> +\t * The child process should persist in the background\n> +\t * and possibly/probably after this process exits. That\n> +\t * is, don't kill the child during our atexit routine.\n> +\t */\n> +\tcmd->clean_on_exit = 0;\n\nSince we have no existing users, can we change this to:\n\nif (cmd->clean_on_exit)\n BUG(\"start_bg_command() doesn't support non-zero clean_on_exit\");\n\nJust silently discarding what the caller asked for seems like the wrong\nthing to do, why the silence?\n\n> +\n> +\tret = start_command(cmd);\n> +\tif (ret) {\n> +\t\t/*\n> +\t\t * We assume that if `start_command()` fails, we\n> +\t\t * either get a complete `trace2_child_start() /\n> +\t\t * trace2_child_exit()` pair or it fails before the\n> +\t\t * `trace2_child_start()` is emitted, so we do not\n> +\t\t * need to worry about it here.\n> +\t\t *\n> +\t\t * We also assume that `start_command()` does not add\n> +\t\t * us to the cleanup list. And that it calls\n> +\t\t * calls `child_process_clear()`.\n> +\t\t */\n\nThese all look like sensible things to assume, but I think commentary /\nwriting on this would be much better just documenting that the\nstart_command() API does this in its comment in run-command.h, or\nperhaps the comment starting with \"The functions: child_process_init\".\n\n> +\t\tsbgr = SBGR_ERROR;\n> +\t\tgoto done;\n> +\t}\n> +\n> +\ttime(&time_limit);\n> +\ttime_limit += timeout_sec;\n> +\n> +wait:\n> +\tpid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);\n> +\n> +\tif (pid_seen == 0) {\n> +\t\t/*\n> +\t\t * The child is currently running. Ask the callback\n> +\t\t * if the child is ready to do work or whether we\n> +\t\t * should keep waiting for it to boot up.\n> +\t\t */\n> +\t\tret = (*wait_cb)(cb_data, cmd);\n> +\t\tif (!ret) {\n> +\t\t\t/*\n> +\t\t\t * The child is running and \"ready\".\n> +\t\t\t *\n> +\t\t\t * NEEDSWORK: As we prepare to orphan (release to\n> +\t\t\t * the background) this child, it is not appropriate\n> +\t\t\t * to emit a `trace2_child_exit()` event. Should we\n> +\t\t\t * create a new event for this case?\n> +\t\t\t */\n> +\t\t\tsbgr = SBGR_READY;\n> +\t\t\tgoto done;\n\nPer api-trace2.txt:\n\n `\"child_exit\"`::\n This event is generated after the current process has returned\n from the waitpid() and collected the exit information from the\n child.\n\nMy (perhaps wrong) reading of that is that yes, we should do that, after\nall if we've released the child are we otherwise going to hear from them\nagain in any way trace2 could log with a child_exit later?\n\nDitto the \"commentary better elsewhere\" whatever the result of this\ndiscussion is, let's add a note to api-trace2.txt about how detached\nchildren are handled by child_exit events.\n\n> [...]\n> +\t\t\t * NEEDSWORK: Like the \"ready\" case, should we\n> +\t\t\t * log a custom child-something Trace2 event here?\n> +\t\t\t */\n> +\t\t\tsbgr = SBGR_TIMEOUT;\n> +\t\t\tgoto done;\n\n[...]\n\n> +\t\t} else {\n> +\t\t\t/*\n> +\t\t\t * The cb gave up on this child.\n> +\t\t\t *\n> +\t\t\t * NEEDSWORK: Like above, should we log a custom\n> +\t\t\t * Trace2 child-something event here?\n> +\t\t\t */\n> +\t\t\tsbgr = SBGR_CB_ERROR;\n> +\t\t\tgoto done;\n\n*ditto*\n\n> +\t\t}\n> +\t}\n> +\n> +\tif (pid_seen == cmd->pid) {\n> +\t\tint child_code = -1;\n> +\n> +\t\t/*\n> +\t\t * The child started, but exited or was terminated\n> +\t\t * before becoming \"ready\".\n> +\t\t *\n> +\t\t * We try to match the behavior of `wait_or_whine()`\n> +\t\t * and convert the child's status to a return code for\n> +\t\t * tracing purposes and emit the `trace2_child_exit()`\n> +\t\t * event.\n> +\t\t */\n> +\t\tif (WIFEXITED(wait_status))\n> +\t\t\tchild_code = WEXITSTATUS(wait_status);\n> +\t\telse if (WIFSIGNALED(wait_status))\n> +\t\t\tchild_code = WTERMSIG(wait_status) + 128;\n> +\t\ttrace2_child_exit(cmd, child_code);\n\nAlthough with the above: Perhaps I've missed some subtleties...\n\n> [...]\n> diff --git a/run-command.h b/run-command.h\n> index af1296769f9..58065197d1b 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -496,4 +496,52 @@ int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,\n> */\n> void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir);\n> \n> +/**\n> + * Possible return values for `start_bg_command()`.\n> + */\n> +enum start_bg_result {\n> +\t/* child process is \"ready\" */\n> +\tSBGR_READY = 0,\n\nClarity nit, whenever I see an explicit enum assignment I start hunting\nfor things that depend on the specific value, as we sometimes do, but\nthere appears to be nothing...\n\n> +\t/* child process could not be started */\n> +\tSBGR_ERROR,\n> +\n> +\t/* callback error when testing for \"ready\" */\n> +\tSBGR_CB_ERROR,\n> +\n> +\t/* timeout expired waiting for child to become \"ready\" */\n> +\tSBGR_TIMEOUT,\n> +\n> +\t/* child process exited or was signalled before becomming \"ready\" */\n> +\tSBGR_DIED,\n\n...I'd think if we were tweaking the value we'd want to put all the\nerror values at < 0, but I'm fine with this pattern of them being\npositive, it encourages users to use switch/case & compiler checks, and\nsince it's all new users...\n\n> + * This is a combination of `start_command()` and `finish_command()`, but\n\nDoc nit: I think with whatever syntax standard we use for /** comments\nthat we prefer functions like_this() not quoted `like_this()`, that\nbeing for values like `123` or whatever. But I'm just going by a quick\ngrep there & what Emacs is highlighting.\n"},{"id":"436089","messageId":"87ilz1gd24.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"YULZbQgxuyw8iJ/R@nand.local","subject":"Re: [PATCH 1/7] trace2: fix memory leak of thread name","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T08:01:24Z","receivedAt":"2021-09-16T08:06:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 16 2021, Taylor Blau wrote:\n\n> On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> So I think this patch can be dropped from this series, since it's exact\n>> duplicate of my 48f68715b14 (tr2: stop leaking \"thread_name\" memory,\n>> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in \"next\" and marked\n>> for a merge with master.\n>\n> I agree it can be dropped.\n>\n>> When submitting a series that depends on another one it's best to rebase\n>> it on top of it & indicate it as such in the cover letter, Junio can\n>> queue such a series on top of another one.\n>>\n>> In this case I'm still not sure why this fix is here, i.e. surely\n>> nothing later in the series absolutely needs this stray memory leak\n>> fix...\n>\n> But there's no need for Jeff to depend on your branch, since (as you\n> mentioned) this cleanup isn't relevant for anything else in this series,\n> which is a sort of grab-bag of miscellaneous clean-ups.\n\nIndeed, to be clear it was just general advice about queue-on-top.\n\nBut to clarify what I was getting at here: If we just came up with the\nsame diff I'd have assumed Jeff just hadn't need the change in \"next\",\nbut since he clearly has I was confused by it being here.\n\nI.e. it doesn't *seem* like anything in the rest of the series depends\non it, so why have it here at all since the bug is being fixed anyway?\nOr if it does depend on it in some subtle way I've missed, perhaps it\ndoes need to be queued on top of ab/tr2-leaks-and-fixes, and the\nrelevant commit/subtle dependency needs to be called out in a commit\nmessage.\n\nOr maybe Jeff had just come up with this independently, noticed it just\nbefore submission and just updated the CL, not the patch or series\nitself :)\n"},{"id":"436126","messageId":"285a279f-85c4-06df-04cf-b998e4accbe5@jeffhostetler.com","threadId":"56508","inReplyTo":"87ilz1gd24.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/7] trace2: fix memory leak of thread name","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-09-16T15:35:51Z","receivedAt":"2021-09-16T15:35:56Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/16/21 4:01 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Sep 16 2021, Taylor Blau wrote:\n> \n>> On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>> So I think this patch can be dropped from this series, since it's exact\n>>> duplicate of my 48f68715b14 (tr2: stop leaking \"thread_name\" memory,\n>>> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in \"next\" and marked\n>>> for a merge with master.\n>>\n>> I agree it can be dropped.\n>>\n>>> When submitting a series that depends on another one it's best to rebase\n>>> it on top of it & indicate it as such in the cover letter, Junio can\n>>> queue such a series on top of another one.\n>>>\n>>> In this case I'm still not sure why this fix is here, i.e. surely\n>>> nothing later in the series absolutely needs this stray memory leak\n>>> fix...\n>>\n>> But there's no need for Jeff to depend on your branch, since (as you\n>> mentioned) this cleanup isn't relevant for anything else in this series,\n>> which is a sort of grab-bag of miscellaneous clean-ups.\n> \n> Indeed, to be clear it was just general advice about queue-on-top.\n> \n> But to clarify what I was getting at here: If we just came up with the\n> same diff I'd have assumed Jeff just hadn't need the change in \"next\",\n> but since he clearly has I was confused by it being here.\n> \n> I.e. it doesn't *seem* like anything in the rest of the series depends\n> on it, so why have it here at all since the bug is being fixed anyway?\n> Or if it does depend on it in some subtle way I've missed, perhaps it\n> does need to be queued on top of ab/tr2-leaks-and-fixes, and the\n> relevant commit/subtle dependency needs to be called out in a commit\n> message.\n> \n> Or maybe Jeff had just come up with this independently, noticed it just\n> before submission and just updated the CL, not the patch or series\n> itself :)\n> \n\nI'll drop this commit since your version is already queued up\nand headed to master. I've been carrying it in my dev branch\nfor a while and was using it to make leak reporting a little\nquieter.\n\nAnd yes, I just noticed that yours had advanced when I wrote the\ncover letter and ACKd it rather than dropping it.\n\nAnd no, nothing in the rest of the whole FSMonitor series depends\non this, so I can leave my series based upon master rather than\nyour branch.\n\nThanks\nJeff\n"},{"id":"436128","messageId":"87o88sfrnk.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"285a279f-85c4-06df-04cf-b998e4accbe5@jeffhostetler.com","subject":"Re: [PATCH 1/7] trace2: fix memory leak of thread name","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-16T15:47:57Z","receivedAt":"2021-09-16T15:48:22Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 16 2021, Jeff Hostetler wrote:\n\n> On 9/16/21 4:01 AM, Ævar Arnfjörð Bjarmason wrote:\n>> On Thu, Sep 16 2021, Taylor Blau wrote:\n>> \n>>> On Thu, Sep 16, 2021 at 07:35:59AM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>>> So I think this patch can be dropped from this series, since it's exact\n>>>> duplicate of my 48f68715b14 (tr2: stop leaking \"thread_name\" memory,\n>>>> 2021-08-27) in ab/tr2-leaks-and-fixes, currently in \"next\" and marked\n>>>> for a merge with master.\n>>>\n>>> I agree it can be dropped.\n>>>\n>>>> When submitting a series that depends on another one it's best to rebase\n>>>> it on top of it & indicate it as such in the cover letter, Junio can\n>>>> queue such a series on top of another one.\n>>>>\n>>>> In this case I'm still not sure why this fix is here, i.e. surely\n>>>> nothing later in the series absolutely needs this stray memory leak\n>>>> fix...\n>>>\n>>> But there's no need for Jeff to depend on your branch, since (as you\n>>> mentioned) this cleanup isn't relevant for anything else in this series,\n>>> which is a sort of grab-bag of miscellaneous clean-ups.\n>> Indeed, to be clear it was just general advice about queue-on-top.\n>> But to clarify what I was getting at here: If we just came up with\n>> the\n>> same diff I'd have assumed Jeff just hadn't need the change in \"next\",\n>> but since he clearly has I was confused by it being here.\n>> I.e. it doesn't *seem* like anything in the rest of the series\n>> depends\n>> on it, so why have it here at all since the bug is being fixed anyway?\n>> Or if it does depend on it in some subtle way I've missed, perhaps it\n>> does need to be queued on top of ab/tr2-leaks-and-fixes, and the\n>> relevant commit/subtle dependency needs to be called out in a commit\n>> message.\n>> Or maybe Jeff had just come up with this independently, noticed it\n>> just\n>> before submission and just updated the CL, not the patch or series\n>> itself :)\n>> \n>\n> I'll drop this commit since your version is already queued up\n> and headed to master. I've been carrying it in my dev branch\n> for a while and was using it to make leak reporting a little\n> quieter.\n>\n> And yes, I just noticed that yours had advanced when I wrote the\n> cover letter and ACKd it rather than dropping it.\n>\n> And no, nothing in the rest of the whole FSMonitor series depends\n> on this, so I can leave my series based upon master rather than\n> your branch.\n>\n> Thanks\n> Jeff\n\nSounds good, thanks for clarifying.\n\nIn any case by the time you'll re-roll this (or soon thereafter) Junio\nwill probably have merged it down anyway.\n"},{"id":"436145","messageId":"xmqqr1dotjuj.fsf@gitster.g","threadId":"56508","inReplyTo":"87ilz1gd24.fsf@evledraar.gmail.com","subject":"Re: [PATCH 1/7] trace2: fix memory leak of thread name","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-16T19:13:08Z","receivedAt":"2021-09-16T19:13:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Indeed, to be clear it was just general advice about queue-on-top.\n>\n> But to clarify what I was getting at here: If we just came up with the\n> same diff I'd have assumed Jeff just hadn't need the change in \"next\",\n> but since he clearly has I was confused by it being here.\n>\n> I.e. it doesn't *seem* like anything in the rest of the series depends\n> on it, so why have it here at all since the bug is being fixed anyway?\n\nI'd imagine that it was there just for the same reason series from\nsome people (yours included) tend to bloat, either over iterations\nor from day one, by including \"this is not necessary for the end\ngoal of this topic at all, but since I noticed it, I am including\nthis fix, which should be obvious enough\" unrelated fix.\n\nHere is a lesson to be learned.\n"},{"id":"436234","messageId":"d4c2c8f4-828a-47ca-0e8e-ca82f644f7eb@jeffhostetler.com","threadId":"56508","inReplyTo":"CAPig+cQ0qbyBrCDcJgg7=DezWGfZhyAR=YuC=vbxd38B4WA=Xw@mail.gmail.com","subject":"Re: [PATCH 2/7] simple-ipc: preparations for supporting binary messages.","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-09-17T16:52:27Z","receivedAt":"2021-09-17T16:52:31Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/15/21 4:43 PM, Eric Sunshine wrote:\n> On Wed, Sep 15, 2021 at 4:36 PM Jeff Hostetler via GitGitGadget\n> wrote:\n>> Add `command_len` argument to the Simple IPC API.\n>>\n>> In my original Simple IPC API, I assumed that the request\n>> would always be a null-terminated string of text characters.\n>> The command arg was just a `const char *`.\n>>\n>> I found a caller that would like to pass a binary command\n>> to the daemon, so I want to ammend the Simple IPC API to\n> \n> s/ammend/amend/\n> \n>> take `const char *command, size_t command_len` and pass\n>> that to the daemon. (Really, the first arg should just be\n>> a `void *` or `const unsigned byte *` to make that clearer.)\n> \n> The reader is left wondering why you didn't also change it to `const\n> void *` (or one of the other choices) while at it.\n> \n\nThe simple ipc layer just passes the buffer to the pkt-line layer\nand it takes a \"const char *\", so to avoid confusion I just left\nthe type is it was. If later we want to fix pkt-line, we can\ninvestigate passing a \"const unsigned byte *\" value down the\ncall chain, but that is more than I want to do right now.\n\n>> Note, the response side has always been a `struct strbuf`\n>> which includes the buffer and length, so we already support\n>> returning a binary answer. (Yes, it feels a little weird\n>> returning a binary buffer in a `strbuf`, but it works.)\n>>\n>> Signed-off-by: Jeff Hostetler \n"},{"id":"436236","messageId":"93ebf97b-1282-8f39-6894-90e0ea2dc851@jeffhostetler.com","threadId":"56508","inReplyTo":"xmqq5yv1wnuc.fsf@gitster.g","subject":"Re: [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-09-17T16:58:31Z","receivedAt":"2021-09-17T16:58:44Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/15/21 5:06 PM, Junio C Hamano wrote:\n> \"Jeff Hostetler via GitGitGadget\" writes:\n> \n>> From: Jeff Hostetler \n>>\n>> From: Carlo Marcelo Arenas Belón \n>>\n>> Move the declartion of the `enum ipc_active_state` type outside of\n>> the SUPPORTS_SIMPLE_IPC ifdef.\n> \n> The second one is not an in-body header since there is already a\n> blank line that signals the end of in-body headers after the first\n> one.\n> \n> This _may_ be a bug in GGG, perhaps?\n\nMaybe. I'll make a note to ask @dscho when he gets back from vacation.\n\nCredit for the commit should go to Carlo. I just added it to the series\nwith his \"From:\" line and it looks like GGG added an extra one before\nit.\n\nJeff\n"},{"id":"436239","messageId":"c99ad8cb-3a95-3df3-3df4-240980e12ef1@jeffhostetler.com","threadId":"56508","inReplyTo":"87bl4tm5s1.fsf@evledraar.gmail.com","subject":"Re: [PATCH 4/7] simple-ipc/ipc-win32: add trace2 debugging","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-09-17T17:27:14Z","receivedAt":"2021-09-17T17:27:17Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/16/21 1:40 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:\n> \n>> @@ -112,6 +115,11 @@ static enum ipc_active_state connect_to_server(\n>> \t\t\t\tif (GetLastError() == ERROR_SEM_TIMEOUT)\n>> \t\t\t\t\treturn IPC_STATE__NOT_LISTENING;\n>> \n>> +\t\t\t\tgle = GetLastError();\n>> +\t\t\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n>> +\t\t\t\t\t\t \"connect/waitpipe/gle\",\n>> +\t\t\t\t\t\t (intmax_t)gle);\n>> +\n>> \t\t\t\treturn IPC_STATE__OTHER_ERROR;\n>> \t\t\t}\n>> \n>... \n >\n> @@ -109,9 +109,12 @@ static enum ipc_active_state connect_to_server(\n> \t\t\tt_start_ms = (DWORD)(getnanotime() / 1000000);\n> \n> \t\t\tif (!WaitNamedPipeW(wpath, timeout_ms)) {\n> -\t\t\t\tif (GetLastError() == ERROR_SEM_TIMEOUT)\n> +\t\t\t\tgle = GetLastError();\n> +\t\t\t\tif (gle == ERROR_SEM_TIMEOUT)\n> \t\t\t\t\treturn IPC_STATE__NOT_LISTENING;\n> \n> +\t\t\t\t/* ...rest of your patch */\n> +\n> \t\t\t\treturn IPC_STATE__OTHER_ERROR;\n> \t\t\t}\n> \n> \n\nYeah, I was just trying to minimize the size of the diff\nand at the time was considering those debug messages to be\ntemporary, but I kind of like having them so it makes sense\nto clean up a bit as you've indicated.\n\nThanks\nJeff\n"},{"id":"436242","messageId":"9b1c8691-936f-c481-e3af-dd94efa57152@jeffhostetler.com","threadId":"56508","inReplyTo":"877dfhm5ki.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-09-17T18:10:39Z","receivedAt":"2021-09-17T18:10:43Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/16/21 1:47 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:\n> \n>> +struct my_sa_data\n>> +{\n>> +\tPSID pEveryoneSID;\n>> +\tPACL pACL;\n>> +\tPSECURITY_DESCRIPTOR pSD;\n>> +\tLPSECURITY_ATTRIBUTES lpSA;\n>> +};\n>> +\n>> +static void init_sa(struct my_sa_data *d)\n>> +{\n>> +\tmemset(d, 0, sizeof(*d));\n>> +}\n>> +\n>> +static void release_sa(struct my_sa_data *d)\n>> +{\n>> +\tif (d->pEveryoneSID)\n>> +\t\tFreeSid(d->pEveryoneSID);\n>> +\tif (d->pACL)\n>> +\t\tLocalFree(d->pACL);\n>> +\tif (d->pSD)\n>> +\t\tLocalFree(d->pSD);\n>> +\tif (d->lpSA)\n>> +\t\tLocalFree(d->lpSA);\n>> +\n>> +\tmemset(d, 0, sizeof(*d));\n>> +}\n> \n> [...]\n> \n>> +fail:\n>> +\trelease_sa(d);\n>> +\treturn NULL;\n>> +}\n>> +\n>> static HANDLE create_new_pipe(wchar_t *wpath, int is_first)\n>> {\n>> \tHANDLE hPipe;\n>> \tDWORD dwOpenMode, dwPipeMode;\n>> -\tLPSECURITY_ATTRIBUTES lpsa = NULL;\n>> +\tstruct my_sa_data my_sa_data;\n>> +\n>> +\tinit_sa(&my_sa_data);\n>> \n> \n> [...]\n> \n>> \thPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,\n>> -\t\t\t\t PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);\n>> +\t\t\t\t PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,\n>> +\t\t\t\t my_sa_data.lpSA);\n>> +\n>> +\trelease_sa(&my_sa_data);\n>> \n>> \treturn hPipe;\n>> }\n> \n> Just a nit on the init pattern, since we always allocate this on the\n> stack (this as all the relevant \"struct my_sa_data\") I'd have thought to\n> see:\n> \n> #define INIT_MY_SA_DATA { 0 }\n> [...]\n> struct my_sa_data my_sa_data = INIT_MY_SA_DATA;\n> \n> Which gets rid of the need for an init_sa() function.\n\nThe current \"my_sa_data\" is just a set of 4 pointers, so yes your\nINIT_MY_SA_DATA macro and my init_sa function are equivalent.\n(And assuming that mine and memset are inlined by the compiler, they\nare both probably exactly equivalent.) So it really doesn't matter\none way or the other.\n\n> \n> Also having the release_sa() do a memset() is a bit odd, usually we have\n> a reset*() function do that if the intent is to re-use, but it doesn't\n> appear to be in this case, and we don't return this data anywhere, do\n> we?\n> \n\nI use the release_sa() function inside my \"fail:\" label in get_sa()\nto cleanup if any of the component parts of the SA cannot be created.\nSo the return value of get_sa() is either fully constructed or contains\nNULL pointers.\n\n(The docs are jargon heavy and little obscure and circular and it\nis not at all clear if/when we might fail to create some of these\nsub-structures.)\n\nSo I allow the SA failure to be silently ignored and we create the named\npipe without an ACL. Normally, this is fine since the daemon usually\nisn't elevated.\n\nIn create_new_pipe we always call release_sa to free the pointers if\nnecessary. (So in case of a failure in get_sa, we won't double free\nthe pointers when create_new_pipe calls release_sa.)\n\nAnother way to think of it is that in release_sa, I'd like to have\nFREE_AND_NULL() variants for FreeSid() and LocalFree(), but a simple\nmemset is sufficient.\n\n\nJeff\n"},{"id":"436246","messageId":"87h7ejdn9j.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"9b1c8691-936f-c481-e3af-dd94efa57152@jeffhostetler.com","subject":"Re: [PATCH 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-17T19:14:48Z","receivedAt":"2021-09-17T19:18:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 17 2021, Jeff Hostetler wrote:\n\n> On 9/16/21 1:47 AM, Ævar Arnfjörð Bjarmason wrote:\n>> On Wed, Sep 15 2021, Jeff Hostetler via GitGitGadget wrote:\n>> \n>>> +struct my_sa_data\n>>> +{\n>>> +\tPSID pEveryoneSID;\n>>> +\tPACL pACL;\n>>> +\tPSECURITY_DESCRIPTOR pSD;\n>>> +\tLPSECURITY_ATTRIBUTES lpSA;\n>>> +};\n>>> +\n>>> +static void init_sa(struct my_sa_data *d)\n>>> +{\n>>> +\tmemset(d, 0, sizeof(*d));\n>>> +}\n>>> +\n>>> +static void release_sa(struct my_sa_data *d)\n>>> +{\n>>> +\tif (d->pEveryoneSID)\n>>> +\t\tFreeSid(d->pEveryoneSID);\n>>> +\tif (d->pACL)\n>>> +\t\tLocalFree(d->pACL);\n>>> +\tif (d->pSD)\n>>> +\t\tLocalFree(d->pSD);\n>>> +\tif (d->lpSA)\n>>> +\t\tLocalFree(d->lpSA);\n>>> +\n>>> +\tmemset(d, 0, sizeof(*d));\n>>> +}\n>> [...]\n>> \n>>> +fail:\n>>> +\trelease_sa(d);\n>>> +\treturn NULL;\n>>> +}\n>>> +\n>>> static HANDLE create_new_pipe(wchar_t *wpath, int is_first)\n>>> {\n>>> \tHANDLE hPipe;\n>>> \tDWORD dwOpenMode, dwPipeMode;\n>>> -\tLPSECURITY_ATTRIBUTES lpsa = NULL;\n>>> +\tstruct my_sa_data my_sa_data;\n>>> +\n>>> +\tinit_sa(&my_sa_data);\n>>> \n>> [...]\n>> \n>>> \thPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,\n>>> -\t\t\t\t PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);\n>>> +\t\t\t\t PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,\n>>> +\t\t\t\t my_sa_data.lpSA);\n>>> +\n>>> +\trelease_sa(&my_sa_data);\n>>> \treturn hPipe;\n>>> }\n>> Just a nit on the init pattern, since we always allocate this on the\n>> stack (this as all the relevant \"struct my_sa_data\") I'd have thought to\n>> see:\n>> #define INIT_MY_SA_DATA { 0 }\n>> [...]\n>> struct my_sa_data my_sa_data = INIT_MY_SA_DATA;\n>> Which gets rid of the need for an init_sa() function.\n>\n> The current \"my_sa_data\" is just a set of 4 pointers, so yes your\n> INIT_MY_SA_DATA macro and my init_sa function are equivalent.\n> (And assuming that mine and memset are inlined by the compiler, they\n> are both probably exactly equivalent.) So it really doesn't matter\n> one way or the other.\n\nYes it's the same to the compiler, see 5726a6b4012 (*.c *_init(): define\nin terms of corresponding *_INIT macro, 2021-07-01).\n\nBut being the same to the compiler != the same to human readers. I was\ngoing for the latter here, i.e. in git.git init with a macro tends to be\nused if the init is simple enough to be run like that, and with a\nfunction if it really does need to run some function (and then after\n5726a6b4012 the init-via-function-via-macro for things that need init\nafter malloc() or whatever).\n\nWhereas this one's just on the stack, and doesn't need anything special,\nso the nit was about just preferring the simplest construct for the job.\n\n>> Also having the release_sa() do a memset() is a bit odd, usually we\n>> have\n>> a reset*() function do that if the intent is to re-use, but it doesn't\n>> appear to be in this case, and we don't return this data anywhere, do\n>> we?\n>> \n>\n> I use the release_sa() function inside my \"fail:\" label in get_sa()\n> to cleanup if any of the component parts of the SA cannot be created.\n> So the return value of get_sa() is either fully constructed or contains\n> NULL pointers.\n>\n> (The docs are jargon heavy and little obscure and circular and it\n> is not at all clear if/when we might fail to create some of these\n> sub-structures.)\n>\n> So I allow the SA failure to be silently ignored and we create the named\n> pipe without an ACL. Normally, this is fine since the daemon usually\n> isn't elevated.\n>\n> In create_new_pipe we always call release_sa to free the pointers if\n> necessary. (So in case of a failure in get_sa, we won't double free\n> the pointers when create_new_pipe calls release_sa.)\n>\n> Another way to think of it is that in release_sa, I'd like to have\n> FREE_AND_NULL() variants for FreeSid() and LocalFree(), but a simple\n> memset is sufficient.\n\n*nod*, I meant if functions are doing that sort of intent-to-reuse we\n usually call them *_reset(), with a *_release() that's just a plain\n free() and forget.\n"},{"id":"436253","messageId":"aaad241c-4014-729c-32c9-df7039439a77@jeffhostetler.com","threadId":"56508","inReplyTo":"YULQ8DJFKlrOml8Z@nand.local","subject":"Re: [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-09-17T19:41:53Z","receivedAt":"2021-09-17T19:41:57Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/16/21 1:06 AM, Taylor Blau wrote:\n> On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote:\n>> From: Jeff Hostetler \n>>\n>> Convert test helper to use `start_bg_command()` when spawning a server\n>> daemon in the background rather than blocks of platform-specific code.\n>>\n>> Signed-off-by: Jeff Hostetler \n>> ---\n>> t/helper/test-simple-ipc.c | 193 ++++++++-----------------------------\n>> 1 file changed, 40 insertions(+), 153 deletions(-)\n>>\n>> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\n>> index 91345180750..59a950f3b00 100644\n>> --- a/t/helper/test-simple-ipc.c\n>> +++ b/t/helper/test-simple-ipc.c\n>> @@ -9,6 +9,7 @@\n>> #include \"parse-options.h\"\n>> #include \"thread-utils.h\"\n>> #include \"strvec.h\"\n>> +#include \"run-command.h\"\n>>\n>> #ifndef SUPPORTS_SIMPLE_IPC\n>> int cmd__simple_ipc(int argc, const char **argv)\n>> @@ -274,178 +275,64 @@ static int daemon__run_server(void)\n>> \treturn ret;\n>> }\n>>\n>> -#ifndef GIT_WINDOWS_NATIVE\n>> -/*\n>> - * This is adapted from `daemonize()`. Use `fork()` to directly create and\n>> - * run the daemon in a child process.\n>> - */\n>> -static int spawn_server(pid_t *pid)\n>> -{\n>> -\tstruct ipc_server_opts opts = {\n>> -\t\t.nr_threads = cl_args.nr_threads,\n>> -\t};\n>> +static start_bg_wait_cb bg_wait_cb;\n> \n> This whole patch is delightful to read, as the new implementation is so\n> much cleaner as a result of the earlier work in this series.\n> \n> Am I correct in assuming that this is to encourage a compiler error if\n> bg_wait_cb does not satisfy the type of start_bg_wait_cb? If so, then I\n> think we are already getting that by trying to pass bg_wait_cb to\n> start_bg_command().\n\nI use that trick to get the compiler to give me a compiler error at the\npoint of the function declaration.\n\nFor example, If I add an arg to the function that doesn't match what's\nin the prototype definition, I get:\n\nt/helper/test-simple-ipc.c:280:12: error: conflicting types for 'bg_wait_cb'\nstatic int bg_wait_cb(const struct child_process *cp, void *cb_data, int \nfoo)\n ^\nt/helper/test-simple-ipc.c:278:25: note: previous declaration is here\nstatic start_bg_wait_cb bg_wait_cb;\n ^\n1 error generated.\n\nYes, we may get an error when the function pointer is referenced in\nstart_bg_command() or if we're using it to initialize a vtable or\nsomething, but those errors are further away from the actual error\n(and sometimes they can be a little cryptic).\n\nAlso, it helps document that this function's signature is predefined\nfor a reason.\n\nIt's a quirky trick I know, but it has served me well over the years.\n\t\n> \n> E.g., applying this (intentionally broken) diff on top:\n> \n> --- 8< ---\n> \n> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\n> index 59a950f3b0..3aed787206 100644\n> --- a/t/helper/test-simple-ipc.c\n> +++ b/t/helper/test-simple-ipc.c\n> @@ -275,9 +275,7 @@ static int daemon__run_server(void)\n> \treturn ret;\n> }\n> \n> -static start_bg_wait_cb bg_wait_cb;\n> -\n> -static int bg_wait_cb(void *cb_data, const struct child_process *cp)\n> +static int bg_wait_cb(const void *cb_data, const struct child_process *cp)\n> {\n> \tint s = ipc_get_active_state(cl_args.path);\n> \n> --- >8 ---\n> \n> and then compiling still warns of a mismatched type when calling\n> start_bg_command().\n> \n>> -\t*pid = fork();\n>> -\n>> -\tswitch (*pid) {\n>> -\tcase 0:\n>> -\t\tif (setsid() == -1)\n>> -\t\t\terror_errno(_(\"setsid failed\"));\n>> -\t\tclose(0);\n>> -\t\tclose(1);\n>> -\t\tclose(2);\n>> -\t\tsanitize_stdfds();\n>> +static int bg_wait_cb(void *cb_data, const struct child_process *cp)\n>> +{\n>> +\tint s = ipc_get_active_state(cl_args.path);\n>>\n>> -\t\treturn ipc_server_run(cl_args.path, &opts, test_app_cb,\n>> -\t\t\t\t (void*)&my_app_data);\n>> +\tswitch (s) {\n>> +\tcase IPC_STATE__LISTENING:\n>> +\t\t/* child is \"ready\" */\n>> +\t\treturn 0;\n>>\n>> -\tcase -1:\n>> -\t\treturn error_errno(_(\"could not spawn daemon in the background\"));\n>> +\tcase IPC_STATE__NOT_LISTENING:\n>> +\tcase IPC_STATE__PATH_NOT_FOUND:\n>> +\t\t/* give child more time */\n>> +\t\treturn 1;\n>>\n>> \tdefault:\n> \n> I'm always a little hesitant to have default cases when switch over enum\n> types, since it suppresses the warning when there's a new value of that\n> type. But we already have a similar default in client__probe_server().\n\nDo all compilers now handle switching over an enum and detect unhandled\ncases? Once upon a time that wasn't the case IIRC.\n\n>...\n> \n> Ditto.\n> \n> Thanks,\n> Taylor\n> \n"},{"id":"436295","messageId":"CAPUEspioONCavBOhn6S4qkZ8tUk0ssuL3-JZG9RvUbJAHd10Pg@mail.gmail.com","threadId":"56508","inReplyTo":"93ebf97b-1282-8f39-6894-90e0ea2dc851@jeffhostetler.com","subject":"Re: [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef","fromName":"Carlo Arenas","fromEmail":"carenas@gmail.com","sentAt":"2021-09-18T07:03:18Z","receivedAt":"2021-09-18T07:03:31Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"On Fri, Sep 17, 2021 at 11:39 PM Jeff Hostetler wrote:\n>\n> Maybe. I'll make a note to ask @dscho when he gets back from vacation.\n\nI am actually more worried about GGG introducing a typo in my flawless\ncommit message.\n\nCarlo\n"},{"id":"436299","messageId":"87ilyycko3.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"aaad241c-4014-729c-32c9-df7039439a77@jeffhostetler.com","subject":"Re: [PATCH 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-18T08:59:10Z","receivedAt":"2021-09-18T09:12:02Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 17 2021, Jeff Hostetler wrote:\n\n> On 9/16/21 1:06 AM, Taylor Blau wrote:\n>> On Wed, Sep 15, 2021 at 08:36:17PM +0000, Jeff Hostetler via GitGitGadget wrote:\n>>> From: Jeff Hostetler \n>>>\n>>> Convert test helper to use `start_bg_command()` when spawning a server\n>>> daemon in the background rather than blocks of platform-specific code.\n>>>\n>>> Signed-off-by: Jeff Hostetler \n>>> ---\n>>> t/helper/test-simple-ipc.c | 193 ++++++++-----------------------------\n>>> 1 file changed, 40 insertions(+), 153 deletions(-)\n>>>\n>>> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\n>>> index 91345180750..59a950f3b00 100644\n>>> --- a/t/helper/test-simple-ipc.c\n>>> +++ b/t/helper/test-simple-ipc.c\n>>> @@ -9,6 +9,7 @@\n>>> #include \"parse-options.h\"\n>>> #include \"thread-utils.h\"\n>>> #include \"strvec.h\"\n>>> +#include \"run-command.h\"\n>>>\n>>> #ifndef SUPPORTS_SIMPLE_IPC\n>>> int cmd__simple_ipc(int argc, const char **argv)\n>>> @@ -274,178 +275,64 @@ static int daemon__run_server(void)\n>>> \treturn ret;\n>>> }\n>>>\n>>> -#ifndef GIT_WINDOWS_NATIVE\n>>> -/*\n>>> - * This is adapted from `daemonize()`. Use `fork()` to directly create and\n>>> - * run the daemon in a child process.\n>>> - */\n>>> -static int spawn_server(pid_t *pid)\n>>> -{\n>>> -\tstruct ipc_server_opts opts = {\n>>> -\t\t.nr_threads = cl_args.nr_threads,\n>>> -\t};\n>>> +static start_bg_wait_cb bg_wait_cb;\n>> This whole patch is delightful to read, as the new implementation is\n>> so\n>> much cleaner as a result of the earlier work in this series.\n>> Am I correct in assuming that this is to encourage a compiler error\n>> if\n>> bg_wait_cb does not satisfy the type of start_bg_wait_cb? If so, then I\n>> think we are already getting that by trying to pass bg_wait_cb to\n>> start_bg_command().\n>\n> I use that trick to get the compiler to give me a compiler error at the\n> point of the function declaration.\n>\n> For example, If I add an arg to the function that doesn't match what's\n> in the prototype definition, I get:\n>\n> t/helper/test-simple-ipc.c:280:12: error: conflicting types for 'bg_wait_cb'\n> static int bg_wait_cb(const struct child_process *cp, void *cb_data,\n> int foo)\n> ^\n> t/helper/test-simple-ipc.c:278:25: note: previous declaration is here\n> static start_bg_wait_cb bg_wait_cb;\n> ^\n> 1 error generated.\n>\n> Yes, we may get an error when the function pointer is referenced in\n> start_bg_command() or if we're using it to initialize a vtable or\n> something, but those errors are further away from the actual error\n> (and sometimes they can be a little cryptic).\n>\n> Also, it helps document that this function's signature is predefined\n> for a reason.\n>\n> It's a quirky trick I know, but it has served me well over the years.\n\nI haven't seen this idiom before. I think it's best to avoid patterns\ndesigned to massage messages out of any specific compilers/versions.\n\nIt seems inevitable that it'll either be counter-productive or\nredundant. Here with clang v11 doing this makes the warning\nworse. I.e. without the forward declaration:\n\n t/helper/test-simple-ipc.c:315:31: error: incompatible function\n pointer types passing 'int (void *, const struct child_process *,\n int)' to parameter of type ' start_bg_wait_cb *' (aka 'int (*)(void\n *, const struct child_process *)')\n [-Werror,-Wincompatible-function-pointer-types]\n sbgr = start_bg_command(&cp, bg_wait_cb, NULL, cl_args.max_wait_sec);\n ^~~~~~~~~~\n ./run-command.h:564:29: note: passing argument to parameter 'wait_cb' here\n start_bg_wait_cb *wait_cb,\n ^\n 1 error generated.\n\nI.e. we get the specific warning category for this type of error\n(-Werror,-Wincompatible-function-pointer-types), and we're pointed at\nthe caller in question (which to be fair, it seems you don't prefer),\nbut also a reference to the run-command.h definition.\n\nMost importantly, we get quoted what the type is/should be, which is\nmissing with the forward declaration. It's the equivalent of saying \"you\ndid bad!\" instead of \"you did bad X, do Y instead!\".\n\n>> E.g., applying this (intentionally broken) diff on top:\n>> --- 8< ---\n>> diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\n>> index 59a950f3b0..3aed787206 100644\n>> --- a/t/helper/test-simple-ipc.c\n>> +++ b/t/helper/test-simple-ipc.c\n>> @@ -275,9 +275,7 @@ static int daemon__run_server(void)\n>> \treturn ret;\n>> }\n>> -static start_bg_wait_cb bg_wait_cb;\n>> -\n>> -static int bg_wait_cb(void *cb_data, const struct child_process *cp)\n>> +static int bg_wait_cb(const void *cb_data, const struct child_process *cp)\n>> {\n>> \tint s = ipc_get_active_state(cl_args.path);\n>> --- >8 ---\n>> and then compiling still warns of a mismatched type when calling\n>> start_bg_command().\n>> \n>>> -\t*pid = fork();\n>>> -\n>>> -\tswitch (*pid) {\n>>> -\tcase 0:\n>>> -\t\tif (setsid() == -1)\n>>> -\t\t\terror_errno(_(\"setsid failed\"));\n>>> -\t\tclose(0);\n>>> -\t\tclose(1);\n>>> -\t\tclose(2);\n>>> -\t\tsanitize_stdfds();\n>>> +static int bg_wait_cb(void *cb_data, const struct child_process *cp)\n>>> +{\n>>> +\tint s = ipc_get_active_state(cl_args.path);\n>>>\n>>> -\t\treturn ipc_server_run(cl_args.path, &opts, test_app_cb,\n>>> -\t\t\t\t (void*)&my_app_data);\n>>> +\tswitch (s) {\n>>> +\tcase IPC_STATE__LISTENING:\n>>> +\t\t/* child is \"ready\" */\n>>> +\t\treturn 0;\n>>>\n>>> -\tcase -1:\n>>> -\t\treturn error_errno(_(\"could not spawn daemon in the background\"));\n>>> +\tcase IPC_STATE__NOT_LISTENING:\n>>> +\tcase IPC_STATE__PATH_NOT_FOUND:\n>>> +\t\t/* give child more time */\n>>> +\t\treturn 1;\n>>>\n>>> \tdefault:\n>> I'm always a little hesitant to have default cases when switch over\n>> enum\n>> types, since it suppresses the warning when there's a new value of that\n>> type. But we already have a similar default in client__probe_server().\n>\n> Do all compilers now handle switching over an enum and detect unhandled\n> cases? Once upon a time that wasn't the case IIRC.\n\nI don't think so, but the ones we widely use do, i.e. clang and gcc at\nleast.\n\nFor this sort of thing it really doesn't matter if *all* compilers\nsupport it, since we'll only need to catch such \"missing enum arm\"\nissues with one of them.\n\nE.g. in my 338abb0f045 (builtins + test helpers: use return instead of\nexit() in cmd_*, 2021-06-08) I fixed something that I've only gotten\nOracle SunCC to emit (gcc and clang don't detect it), but as long as\nthat one compiler does & someone checks it regularly...\n\nBy having a \"default\" case you're hiding that detection from the\ncompilers capable of detecting a logic error in this code, whereas if\nthe compiler can't do that it'll just ignore it.\n"},{"id":"436424","messageId":"pull.1040.v2.git.1632152178.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.git.1631738177.gitgitgadget@gmail.com","subject":"[PATCH v2 0/7] Builtin FSMonitor Part 1","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T15:36:11Z","receivedAt":"2021-09-20T15:36:24Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"Here is V2 of Part 1 of my Builtin FSMonitor series.\n\nChanges since V1 include:\n\n * Drop the Trace2 memory leak.\n * Added a new \"child_ready\" event to Trace2 as an alternative to the\n \"child_exit\" event for background processes.\n * Convert the Trace2-related NEEDSWORK items in \"start_bg_command()\" to use\n the new \"child_ready\" event.\n * Various minor code and documentation cleanups.\n\nJeff Hostetler (7):\n trace2: add trace2_child_ready() to report on background children\n simple-ipc: preparations for supporting binary messages.\n simple-ipc: move definition of ipc_active_state outside of ifdef\n simple-ipc/ipc-win32: add trace2 debugging\n simple-ipc/ipc-win32: add Windows ACL to named pipe\n run-command: create start_bg_command\n t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command\n\n Documentation/technical/api-trace2.txt | 40 +++++\n compat/simple-ipc/ipc-unix-socket.c | 14 +-\n compat/simple-ipc/ipc-win32.c | 179 +++++++++++++++++--\n run-command.c | 129 ++++++++++++++\n run-command.h | 57 ++++++\n simple-ipc.h | 21 ++-\n t/helper/test-simple-ipc.c | 233 +++++++------------------\n trace2.c | 31 ++++\n trace2.h | 25 +++\n trace2/tr2_tgt.h | 5 +\n trace2/tr2_tgt_event.c | 22 +++\n trace2/tr2_tgt_normal.c | 14 ++\n trace2/tr2_tgt_perf.c | 15 ++\n 13 files changed, 587 insertions(+), 198 deletions(-)\n\n\nbase-commit: 8b7c11b8668b4e774f81a9f0b4c30144b818f1d1\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1040%2Fjeffhostetler%2Fbuiltin-fsmonitor-part1-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1040/jeffhostetler/builtin-fsmonitor-part1-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1040\n\nRange-diff vs v1:\n\n 1: 5f557caee00 < -: ----------- trace2: fix memory leak of thread name\n -: ----------- > 1: f88e9feff26 trace2: add trace2_child_ready() to report on background children\n 2: 7182419f6df ! 2: 258baa0df8c simple-ipc: preparations for supporting binary messages.\n @@ Commit message\n \n Add `command_len` argument to the Simple IPC API.\n \n - In my original Simple IPC API, I assumed that the request\n - would always be a null-terminated string of text characters.\n - The command arg was just a `const char *`.\n + In my original Simple IPC API, I assumed that the request would always\n + be a null-terminated string of text characters. The `command`\n + argument was just a `const char *`.\n \n - I found a caller that would like to pass a binary command\n - to the daemon, so I want to ammend the Simple IPC API to\n - take `const char *command, size_t command_len` and pass\n - that to the daemon. (Really, the first arg should just be\n - a `void *` or `const unsigned byte *` to make that clearer.)\n + I found a caller that would like to pass a binary command to the\n + daemon, so I am amending the Simple IPC API to receive `const char\n + *command, size_t command_len` arguments.\n \n - Note, the response side has always been a `struct strbuf`\n - which includes the buffer and length, so we already support\n - returning a binary answer. (Yes, it feels a little weird\n - returning a binary buffer in a `strbuf`, but it works.)\n + I considered changing the `command` argument to be a `void *`, but the\n + IPC layer simply passes it to the pkt-line layer which takes a `const\n + char *`, so to avoid confusion I left it as is.\n +\n + Note, the response side has always been a `struct strbuf` which\n + includes the buffer and length, so we already support returning a\n + binary answer. (Yes, it feels a little weird returning a binary\n + buffer in a `strbuf`, but it works.)\n \n Signed-off-by: Jeff Hostetler \n \n 3: 7de207828ca = 3: c94b4cbcbf2 simple-ipc: move definition of ipc_active_state outside of ifdef\n 4: 30b7bb247c3 ! 4: 82b6ce0dd6a simple-ipc/ipc-win32: add trace2 debugging\n @@ compat/simple-ipc/ipc-win32.c: static enum ipc_active_state get_active_state(wch\n }\n \n @@ compat/simple-ipc/ipc-win32.c: static enum ipc_active_state connect_to_server(\n - \t\t\t\tif (GetLastError() == ERROR_SEM_TIMEOUT)\n + \t\t\tt_start_ms = (DWORD)(getnanotime() / 1000000);\n + \n + \t\t\tif (!WaitNamedPipeW(wpath, timeout_ms)) {\n +-\t\t\t\tif (GetLastError() == ERROR_SEM_TIMEOUT)\n ++\t\t\t\tDWORD gleWait = GetLastError();\n ++\n ++\t\t\t\tif (gleWait == ERROR_SEM_TIMEOUT)\n \t\t\t\t\treturn IPC_STATE__NOT_LISTENING;\n \n -+\t\t\t\tgle = GetLastError();\n +\t\t\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n +\t\t\t\t\t\t \"connect/waitpipe/gle\",\n -+\t\t\t\t\t\t (intmax_t)gle);\n ++\t\t\t\t\t\t (intmax_t)gleWait);\n +\n \t\t\t\treturn IPC_STATE__OTHER_ERROR;\n \t\t\t}\n 5: 5eadf719295 = 5: faf6034848e simple-ipc/ipc-win32: add Windows ACL to named pipe\n 6: f97038a563d ! 6: 0822118c4b5 run-command: create start_bg_command\n @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char\n +\ttime_t time_limit;\n +\n +\t/*\n -+\t * Silently disallow child cleanup -- even if requested.\n -+\t * The child process should persist in the background\n -+\t * and possibly/probably after this process exits. That\n -+\t * is, don't kill the child during our atexit routine.\n ++\t * We do not allow clean-on-exit because the child process\n ++\t * should persist in the background and possibly/probably\n ++\t * after this process exits. So we don't want to kill the\n ++\t * child during our atexit routine.\n +\t */\n -+\tcmd->clean_on_exit = 0;\n ++\tif (cmd->clean_on_exit)\n ++\t\tBUG(\"start_bg_command() does not allow non-zero clean_on_exit\");\n ++\n ++\tif (!cmd->trace2_child_class)\n ++\t\tcmd->trace2_child_class = \"background\";\n +\n +\tret = start_command(cmd);\n +\tif (ret) {\n @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char\n +wait:\n +\tpid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);\n +\n -+\tif (pid_seen == 0) {\n ++\tif (!pid_seen) {\n +\t\t/*\n +\t\t * The child is currently running. Ask the callback\n +\t\t * if the child is ready to do work or whether we\n +\t\t * should keep waiting for it to boot up.\n +\t\t */\n -+\t\tret = (*wait_cb)(cb_data, cmd);\n ++\t\tret = (*wait_cb)(cmd, cb_data);\n +\t\tif (!ret) {\n +\t\t\t/*\n +\t\t\t * The child is running and \"ready\".\n -+\t\t\t *\n -+\t\t\t * NEEDSWORK: As we prepare to orphan (release to\n -+\t\t\t * the background) this child, it is not appropriate\n -+\t\t\t * to emit a `trace2_child_exit()` event. Should we\n -+\t\t\t * create a new event for this case?\n +\t\t\t */\n ++\t\t\ttrace2_child_ready(cmd, \"ready\");\n +\t\t\tsbgr = SBGR_READY;\n +\t\t\tgoto done;\n +\t\t} else if (ret > 0) {\n ++\t\t\t/*\n ++\t\t\t * The callback said to give it more time to boot up\n ++\t\t\t * (subject to our timeout limit).\n ++\t\t\t */\n +\t\t\ttime_t now;\n +\n +\t\t\ttime(&now);\n @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char\n +\t\t\t * Our timeout has expired. We don't try to\n +\t\t\t * kill the child, but rather let it continue\n +\t\t\t * (hopefully) trying to startup.\n -+\t\t\t *\n -+\t\t\t * NEEDSWORK: Like the \"ready\" case, should we\n -+\t\t\t * log a custom child-something Trace2 event here?\n +\t\t\t */\n ++\t\t\ttrace2_child_ready(cmd, \"timeout\");\n +\t\t\tsbgr = SBGR_TIMEOUT;\n +\t\t\tgoto done;\n +\t\t} else {\n +\t\t\t/*\n -+\t\t\t * The cb gave up on this child.\n -+\t\t\t *\n -+\t\t\t * NEEDSWORK: Like above, should we log a custom\n -+\t\t\t * Trace2 child-something event here?\n ++\t\t\t * The cb gave up on this child. It is still running,\n ++\t\t\t * but our cb got an error trying to probe it.\n +\t\t\t */\n ++\t\t\ttrace2_child_ready(cmd, \"error\");\n +\t\t\tsbgr = SBGR_CB_ERROR;\n +\t\t\tgoto done;\n +\t\t}\n +\t}\n +\n -+\tif (pid_seen == cmd->pid) {\n ++\telse if (pid_seen == cmd->pid) {\n +\t\tint child_code = -1;\n +\n +\t\t/*\n @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char\n +\t\t * before becoming \"ready\".\n +\t\t *\n +\t\t * We try to match the behavior of `wait_or_whine()`\n ++\t\t * WRT the handling of WIFSIGNALED() and WIFEXITED()\n +\t\t * and convert the child's status to a return code for\n +\t\t * tracing purposes and emit the `trace2_child_exit()`\n +\t\t * event.\n ++\t\t *\n ++\t\t * We do not want the wait_or_whine() error message\n ++\t\t * because we will be called by client-side library\n ++\t\t * routines.\n +\t\t */\n +\t\tif (WIFEXITED(wait_status))\n +\t\t\tchild_code = WEXITSTATUS(wait_status);\n @@ run-command.c: void prepare_other_repo_env(struct strvec *env_array, const char\n +\t\tgoto done;\n +\t}\n +\n -+\tif (pid_seen < 0 && errno == EINTR)\n ++\telse if (pid_seen < 0 && errno == EINTR)\n +\t\tgoto wait;\n +\n +\ttrace2_child_exit(cmd, -1);\n @@ run-command.h: int run_processes_parallel_tr2(int n, get_next_task_fn, start_fai\n void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir);\n \n +/**\n -+ * Possible return values for `start_bg_command()`.\n ++ * Possible return values for start_bg_command().\n + */\n +enum start_bg_result {\n +\t/* child process is \"ready\" */\n @@ run-command.h: int run_processes_parallel_tr2(int n, get_next_task_fn, start_fai\n +};\n +\n +/**\n -+ * Callback used by `start_bg_command()` to ask whether the\n -+ * child process is ready or needs more time to become ready.\n ++ * Callback used by start_bg_command() to ask whether the\n ++ * child process is ready or needs more time to become \"ready\".\n ++ *\n ++ * The callback will receive the cmd and cb_data arguments given to\n ++ * start_bg_command().\n + *\n + * Returns 1 is child needs more time (subject to the requested timeout).\n -+ * Returns 0 if child is ready.\n -+ * Returns -1 on any error and cause `start_bg_command()` to also error out.\n ++ * Returns 0 if child is \"ready\".\n ++ * Returns -1 on any error and cause start_bg_command() to also error out.\n + */\n -+typedef int(start_bg_wait_cb)(void *cb_data,\n -+\t\t\t const struct child_process *cmd);\n ++typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);\n +\n +/**\n -+ * Start a command in the background. Wait long enough for the child to\n -+ * become \"ready\". Capture immediate errors (like failure to start) and\n -+ * any immediate exit status (such as a shutdown/signal before the child\n -+ * became \"ready\").\n ++ * Start a command in the background. Wait long enough for the child\n ++ * to become \"ready\" (as defined by the provided callback). Capture\n ++ * immediate errors (like failure to start) and any immediate exit\n ++ * status (such as a shutdown/signal before the child became \"ready\")\n ++ * and return this like start_command().\n ++ *\n ++ * We run a custom wait loop using the provided callback to wait for\n ++ * the child to start and become \"ready\". This is limited by the given\n ++ * timeout value.\n ++ *\n ++ * If the child does successfully start and become \"ready\", we orphan\n ++ * it into the background.\n + *\n -+ * This is a combination of `start_command()` and `finish_command()`, but\n -+ * with a custom `wait_or_whine()` that allows the caller to define when\n -+ * the child is \"ready\".\n ++ * The caller must not call finish_command().\n + *\n -+ * The caller does not need to call `finish_command()`.\n ++ * The opaque cb_data argument will be forwarded to the callback for\n ++ * any instance data that it might require. This may be NULL.\n + */\n +enum start_bg_result start_bg_command(struct child_process *cmd,\n +\t\t\t\t start_bg_wait_cb *wait_cb,\n 7: 57f29feaadb ! 7: 6b7a058284b t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command\n @@ Commit message\n Convert test helper to use `start_bg_command()` when spawning a server\n daemon in the background rather than blocks of platform-specific code.\n \n + Also, while here, remove _() translation around error messages since\n + this is a test helper and not Git code.\n +\n Signed-off-by: Jeff Hostetler \n \n ## t/helper/test-simple-ipc.c ##\n @@ t/helper/test-simple-ipc.c\n #ifndef SUPPORTS_SIMPLE_IPC\n int cmd__simple_ipc(int argc, const char **argv)\n @@ t/helper/test-simple-ipc.c: static int daemon__run_server(void)\n + \t */\n + \tret = ipc_server_run(cl_args.path, &opts, test_app_cb, (void*)&my_app_data);\n + \tif (ret == -2)\n +-\t\terror(_(\"socket/pipe already in use: '%s'\"), cl_args.path);\n ++\t\terror(\"socket/pipe already in use: '%s'\", cl_args.path);\n + \telse if (ret == -1)\n +-\t\terror_errno(_(\"could not start server on: '%s'\"), cl_args.path);\n ++\t\terror_errno(\"could not start server on: '%s'\", cl_args.path);\n + \n \treturn ret;\n }\n \n @@ t/helper/test-simple-ipc.c: static int daemon__run_server(void)\n -\t\tclose(1);\n -\t\tclose(2);\n -\t\tsanitize_stdfds();\n -+static int bg_wait_cb(void *cb_data, const struct child_process *cp)\n ++static int bg_wait_cb(const struct child_process *cp, void *cb_data)\n +{\n +\tint s = ipc_get_active_state(cl_args.path);\n \n @@ t/helper/test-simple-ipc.c: static int daemon__run_server(void)\n /*\n * This process will run a quick probe to see if a simple-ipc server\n * is active on this path.\n +@@ t/helper/test-simple-ipc.c: static int client__stop_server(void)\n + \n + \t\ttime(&now);\n + \t\tif (now > time_limit)\n +-\t\t\treturn error(_(\"daemon has not shutdown yet\"));\n ++\t\t\treturn error(\"daemon has not shutdown yet\");\n + \t}\n + }\n + \n\n-- \ngitgitgadget\n"},{"id":"436425","messageId":"f88e9feff26a44b07db91d0bb8159529dc082bb1.1632152178.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.v2.git.1632152178.gitgitgadget@gmail.com","subject":"[PATCH v2 1/7] trace2: add trace2_child_ready() to report on background children","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T15:36:12Z","receivedAt":"2021-09-20T15:36:26Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nCreate \"child_ready\" event to capture the state of a child process\ncreated in the background.\n\nWhen a child command is started a \"child_start\" event is generated in\nthe Trace2 log. For normal synchronous children, a \"child_exit\" event\nis later generated when the child exits or is terminated. The two events\ninclude information, such as the \"child_id\" and \"pid\", to allow post\nanalysis to match-up the command line and exit status.\n\nWhen a child is started in the background (and may outlive the parent\nprocess), it is not possible for the parent to emit a \"child_exit\"\nevent. Create a new \"child_ready\" event to indicate whether the\nchild was successfully started. Also include the \"child_id\" and \"pid\"\nto allow similar post processing.\n\nThis will be used in a later commit with the new \"start_bg_command()\".\n\nSigned-off-by: Jeff Hostetler \n---\n Documentation/technical/api-trace2.txt | 40 ++++++++++++++++++++++++++\n trace2.c | 31 ++++++++++++++++++++\n trace2.h | 25 ++++++++++++++++\n trace2/tr2_tgt.h | 5 ++++\n trace2/tr2_tgt_event.c | 22 ++++++++++++++\n trace2/tr2_tgt_normal.c | 14 +++++++++\n trace2/tr2_tgt_perf.c | 15 ++++++++++\n 7 files changed, 152 insertions(+)\n\ndiff --git a/Documentation/technical/api-trace2.txt b/Documentation/technical/api-trace2.txt\nindex b9f3198fbe7..ef7fe02a8f7 100644\n--- a/Documentation/technical/api-trace2.txt\n+++ b/Documentation/technical/api-trace2.txt\n@@ -613,6 +613,46 @@ stopping after the waitpid() and includes OS process creation overhead).\n So this time will be slightly larger than the atexit time reported by\n the child process itself.\n \n+`\"child_ready\"`::\n+\tThis event is generated after the current process has started\n+\ta background process and released all handles to it.\n++\n+------------\n+{\n+\t\"event\":\"child_ready\",\n+\t...\n+\t\"child_id\":2,\n+\t\"pid\":14708,\t # child PID\n+\t\"ready\":\"ready\", # child ready state\n+\t\"t_rel\":0.110605 # observed run-time of child process\n+}\n+------------\n++\n+Note that the session-id of the child process is not available to\n+the current/spawning process, so the child's PID is reported here as\n+a hint for post-processing. (But it is only a hint because the child\n+process may be a shell script which doesn't have a session-id.)\n++\n+This event is generated after the child is started in the background\n+and given a little time to boot up and start working. If the child\n+startups normally and while the parent is still waiting, the \"ready\"\n+field will have the value \"ready\".\n+If the child is too slow to start and the parent times out, the field\n+will have the value \"timeout\".\n+If the child starts but the parent is unable to probe it, the field\n+will have the value \"error\".\n++\n+After the parent process emits this event, it will release all of its\n+handles to the child process and treat the child as a background\n+daemon. So even if the child does eventually finish booting up,\n+the parent will not emit an updated event.\n++\n+Note that the `t_rel` field contains the observed run time in seconds\n+when the parent released the child process into the background.\n+The child is assumed to be a long-running daemon process and may\n+outlive the parent process. So the parent's child event times should\n+not be compared to the child's atexit times.\n+\n `\"exec\"`::\n \tThis event is generated before git attempts to `exec()`\n \tanother command rather than starting a child process.\ndiff --git a/trace2.c b/trace2.c\nindex b9b154ac440..b2d471526fd 100644\n--- a/trace2.c\n+++ b/trace2.c\n@@ -394,6 +394,37 @@ void trace2_child_exit_fl(const char *file, int line, struct child_process *cmd,\n \t\t\t\t\t\t us_elapsed_child);\n }\n \n+void trace2_child_ready_fl(const char *file, int line,\n+\t\t\t struct child_process *cmd,\n+\t\t\t const char *ready)\n+{\n+\tstruct tr2_tgt *tgt_j;\n+\tint j;\n+\tuint64_t us_now;\n+\tuint64_t us_elapsed_absolute;\n+\tuint64_t us_elapsed_child;\n+\n+\tif (!trace2_enabled)\n+\t\treturn;\n+\n+\tus_now = getnanotime() / 1000;\n+\tus_elapsed_absolute = tr2tls_absolute_elapsed(us_now);\n+\n+\tif (cmd->trace2_child_us_start)\n+\t\tus_elapsed_child = us_now - cmd->trace2_child_us_start;\n+\telse\n+\t\tus_elapsed_child = 0;\n+\n+\tfor_each_wanted_builtin (j, tgt_j)\n+\t\tif (tgt_j->pfn_child_ready_fl)\n+\t\t\ttgt_j->pfn_child_ready_fl(file, line,\n+\t\t\t\t\t\t us_elapsed_absolute,\n+\t\t\t\t\t\t cmd->trace2_child_id,\n+\t\t\t\t\t\t cmd->pid,\n+\t\t\t\t\t\t ready,\n+\t\t\t\t\t\t us_elapsed_child);\n+}\n+\n int trace2_exec_fl(const char *file, int line, const char *exe,\n \t\t const char **argv)\n {\ndiff --git a/trace2.h b/trace2.h\nindex 9b7286c572f..6fe9802598b 100644\n--- a/trace2.h\n+++ b/trace2.h\n@@ -253,6 +253,31 @@ void trace2_child_exit_fl(const char *file, int line, struct child_process *cmd,\n #define trace2_child_exit(cmd, code) \\\n \ttrace2_child_exit_fl(__FILE__, __LINE__, (cmd), (code))\n \n+/**\n+ * Emits a \"child_ready\" message containing the \"child-id\" and a flag\n+ * indicating whether the child was considered \"ready\" when we\n+ * released it.\n+ *\n+ * This function should be called after starting a daemon process in\n+ * the background (and after giving it sufficient time to boot\n+ * up) to indicate that we no longer control or own it.\n+ *\n+ * The \"ready\" argument should contain one of { \"ready\", \"timeout\",\n+ * \"error\" } to indicate the state of the running daemon when we\n+ * released it.\n+ *\n+ * If the daemon process fails to start or it exits or is terminated\n+ * while we are still waiting for it, the caller should emit a\n+ * regular \"child_exit\" to report the normal process exit information.\n+ *\n+ */\n+void trace2_child_ready_fl(const char *file, int line,\n+\t\t\t struct child_process *cmd,\n+\t\t\t const char *ready);\n+\n+#define trace2_child_ready(cmd, ready) \\\n+\ttrace2_child_ready_fl(__FILE__, __LINE__, (cmd), (ready))\n+\n /**\n * Emit an 'exec' event prior to calling one of exec(), execv(),\n * execvp(), and etc. On Unix-derived systems, this will be the\ndiff --git a/trace2/tr2_tgt.h b/trace2/tr2_tgt.h\nindex 1f66fd65730..65f94e15748 100644\n--- a/trace2/tr2_tgt.h\n+++ b/trace2/tr2_tgt.h\n@@ -45,6 +45,10 @@ typedef void(tr2_tgt_evt_child_exit_fl_t)(const char *file, int line,\n \t\t\t\t\t uint64_t us_elapsed_absolute, int cid,\n \t\t\t\t\t int pid, int code,\n \t\t\t\t\t uint64_t us_elapsed_child);\n+typedef void(tr2_tgt_evt_child_ready_fl_t)(const char *file, int line,\n+\t\t\t\t\t uint64_t us_elapsed_absolute,\n+\t\t\t\t\t int cid, int pid, const char *ready,\n+\t\t\t\t\t uint64_t us_elapsed_child);\n \n typedef void(tr2_tgt_evt_thread_start_fl_t)(const char *file, int line,\n \t\t\t\t\t uint64_t us_elapsed_absolute);\n@@ -116,6 +120,7 @@ struct tr2_tgt {\n \ttr2_tgt_evt_alias_fl_t *pfn_alias_fl;\n \ttr2_tgt_evt_child_start_fl_t *pfn_child_start_fl;\n \ttr2_tgt_evt_child_exit_fl_t *pfn_child_exit_fl;\n+\ttr2_tgt_evt_child_ready_fl_t *pfn_child_ready_fl;\n \ttr2_tgt_evt_thread_start_fl_t *pfn_thread_start_fl;\n \ttr2_tgt_evt_thread_exit_fl_t *pfn_thread_exit_fl;\n \ttr2_tgt_evt_exec_fl_t *pfn_exec_fl;\ndiff --git a/trace2/tr2_tgt_event.c b/trace2/tr2_tgt_event.c\nindex 578a9a5287a..70cfc2f77cc 100644\n--- a/trace2/tr2_tgt_event.c\n+++ b/trace2/tr2_tgt_event.c\n@@ -383,6 +383,27 @@ static void fn_child_exit_fl(const char *file, int line,\n \tjw_release(&jw);\n }\n \n+static void fn_child_ready_fl(const char *file, int line,\n+\t\t\t uint64_t us_elapsed_absolute, int cid, int pid,\n+\t\t\t const char *ready, uint64_t us_elapsed_child)\n+{\n+\tconst char *event_name = \"child_ready\";\n+\tstruct json_writer jw = JSON_WRITER_INIT;\n+\tdouble t_rel = (double)us_elapsed_child / 1000000.0;\n+\n+\tjw_object_begin(&jw, 0);\n+\tevent_fmt_prepare(event_name, file, line, NULL, &jw);\n+\tjw_object_intmax(&jw, \"child_id\", cid);\n+\tjw_object_intmax(&jw, \"pid\", pid);\n+\tjw_object_string(&jw, \"ready\", ready);\n+\tjw_object_double(&jw, \"t_rel\", 6, t_rel);\n+\tjw_end(&jw);\n+\n+\ttr2_dst_write_line(&tr2dst_event, &jw.json);\n+\n+\tjw_release(&jw);\n+}\n+\n static void fn_thread_start_fl(const char *file, int line,\n \t\t\t uint64_t us_elapsed_absolute)\n {\n@@ -610,6 +631,7 @@ struct tr2_tgt tr2_tgt_event = {\n \tfn_alias_fl,\n \tfn_child_start_fl,\n \tfn_child_exit_fl,\n+\tfn_child_ready_fl,\n \tfn_thread_start_fl,\n \tfn_thread_exit_fl,\n \tfn_exec_fl,\ndiff --git a/trace2/tr2_tgt_normal.c b/trace2/tr2_tgt_normal.c\nindex a5751c88644..58d9e430f05 100644\n--- a/trace2/tr2_tgt_normal.c\n+++ b/trace2/tr2_tgt_normal.c\n@@ -251,6 +251,19 @@ static void fn_child_exit_fl(const char *file, int line,\n \tstrbuf_release(&buf_payload);\n }\n \n+static void fn_child_ready_fl(const char *file, int line,\n+\t\t\t uint64_t us_elapsed_absolute, int cid, int pid,\n+\t\t\t const char *ready, uint64_t us_elapsed_child)\n+{\n+\tstruct strbuf buf_payload = STRBUF_INIT;\n+\tdouble elapsed = (double)us_elapsed_child / 1000000.0;\n+\n+\tstrbuf_addf(&buf_payload, \"child_ready[%d] pid:%d ready:%s elapsed:%.6f\",\n+\t\t cid, pid, ready, elapsed);\n+\tnormal_io_write_fl(file, line, &buf_payload);\n+\tstrbuf_release(&buf_payload);\n+}\n+\n static void fn_exec_fl(const char *file, int line, uint64_t us_elapsed_absolute,\n \t\t int exec_id, const char *exe, const char **argv)\n {\n@@ -330,6 +343,7 @@ struct tr2_tgt tr2_tgt_normal = {\n \tfn_alias_fl,\n \tfn_child_start_fl,\n \tfn_child_exit_fl,\n+\tfn_child_ready_fl,\n \tNULL, /* thread_start */\n \tNULL, /* thread_exit */\n \tfn_exec_fl,\ndiff --git a/trace2/tr2_tgt_perf.c b/trace2/tr2_tgt_perf.c\nindex af4d65a0a5f..e4acca13d64 100644\n--- a/trace2/tr2_tgt_perf.c\n+++ b/trace2/tr2_tgt_perf.c\n@@ -360,6 +360,20 @@ static void fn_child_exit_fl(const char *file, int line,\n \tstrbuf_release(&buf_payload);\n }\n \n+static void fn_child_ready_fl(const char *file, int line,\n+\t\t\t uint64_t us_elapsed_absolute, int cid, int pid,\n+\t\t\t const char *ready, uint64_t us_elapsed_child)\n+{\n+\tconst char *event_name = \"child_ready\";\n+\tstruct strbuf buf_payload = STRBUF_INIT;\n+\n+\tstrbuf_addf(&buf_payload, \"[ch%d] pid:%d ready:%s\", cid, pid, ready);\n+\n+\tperf_io_write_fl(file, line, event_name, NULL, &us_elapsed_absolute,\n+\t\t\t &us_elapsed_child, NULL, &buf_payload);\n+\tstrbuf_release(&buf_payload);\n+}\n+\n static void fn_thread_start_fl(const char *file, int line,\n \t\t\t uint64_t us_elapsed_absolute)\n {\n@@ -553,6 +567,7 @@ struct tr2_tgt tr2_tgt_perf = {\n \tfn_alias_fl,\n \tfn_child_start_fl,\n \tfn_child_exit_fl,\n+\tfn_child_ready_fl,\n \tfn_thread_start_fl,\n \tfn_thread_exit_fl,\n \tfn_exec_fl,\n-- \ngitgitgadget\n\n"},{"id":"436426","messageId":"258baa0df8ca8c068fd953bd52cbe246d2e7fa09.1632152178.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.v2.git.1632152178.gitgitgadget@gmail.com","subject":"[PATCH v2 2/7] simple-ipc: preparations for supporting binary messages.","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T15:36:13Z","receivedAt":"2021-09-20T15:36:28Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nAdd `command_len` argument to the Simple IPC API.\n\nIn my original Simple IPC API, I assumed that the request would always\nbe a null-terminated string of text characters. The `command`\nargument was just a `const char *`.\n\nI found a caller that would like to pass a binary command to the\ndaemon, so I am amending the Simple IPC API to receive `const char\n*command, size_t command_len` arguments.\n\nI considered changing the `command` argument to be a `void *`, but the\nIPC layer simply passes it to the pkt-line layer which takes a `const\nchar *`, so to avoid confusion I left it as is.\n\nNote, the response side has always been a `struct strbuf` which\nincludes the buffer and length, so we already support returning a\nbinary answer. (Yes, it feels a little weird returning a binary\nbuffer in a `strbuf`, but it works.)\n\nSigned-off-by: Jeff Hostetler \n---\n compat/simple-ipc/ipc-unix-socket.c | 14 +++++++-----\n compat/simple-ipc/ipc-win32.c | 14 +++++++-----\n simple-ipc.h | 7 ++++--\n t/helper/test-simple-ipc.c | 34 +++++++++++++++++++----------\n 4 files changed, 46 insertions(+), 23 deletions(-)\n\ndiff --git a/compat/simple-ipc/ipc-unix-socket.c b/compat/simple-ipc/ipc-unix-socket.c\nindex 1927e6ef4bc..4e28857a0a1 100644\n--- a/compat/simple-ipc/ipc-unix-socket.c\n+++ b/compat/simple-ipc/ipc-unix-socket.c\n@@ -168,7 +168,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection)\n \n int ipc_client_send_command_to_connection(\n \tstruct ipc_client_connection *connection,\n-\tconst char *message, struct strbuf *answer)\n+\tconst char *message, size_t message_len,\n+\tstruct strbuf *answer)\n {\n \tint ret = 0;\n \n@@ -176,7 +177,7 @@ int ipc_client_send_command_to_connection(\n \n \ttrace2_region_enter(\"ipc-client\", \"send-command\", NULL);\n \n-\tif (write_packetized_from_buf_no_flush(message, strlen(message),\n+\tif (write_packetized_from_buf_no_flush(message, message_len,\n \t\t\t\t\t connection->fd) < 0 ||\n \t packet_flush_gently(connection->fd) < 0) {\n \t\tret = error(_(\"could not send IPC command\"));\n@@ -197,7 +198,8 @@ done:\n \n int ipc_client_send_command(const char *path,\n \t\t\t const struct ipc_client_connect_options *options,\n-\t\t\t const char *message, struct strbuf *answer)\n+\t\t\t const char *message, size_t message_len,\n+\t\t\t struct strbuf *answer)\n {\n \tint ret = -1;\n \tenum ipc_active_state state;\n@@ -208,7 +210,9 @@ int ipc_client_send_command(const char *path,\n \tif (state != IPC_STATE__LISTENING)\n \t\treturn ret;\n \n-\tret = ipc_client_send_command_to_connection(connection, message, answer);\n+\tret = ipc_client_send_command_to_connection(connection,\n+\t\t\t\t\t\t message, message_len,\n+\t\t\t\t\t\t answer);\n \n \tipc_client_close_connection(connection);\n \n@@ -503,7 +507,7 @@ static int worker_thread__do_io(\n \tif (ret >= 0) {\n \t\tret = worker_thread_data->server_data->application_cb(\n \t\t\tworker_thread_data->server_data->application_data,\n-\t\t\tbuf.buf, do_io_reply_callback, &reply_data);\n+\t\t\tbuf.buf, buf.len, do_io_reply_callback, &reply_data);\n \n \t\tpacket_flush_gently(reply_data.fd);\n \t}\ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex 8dc7bda087d..8e889d6a506 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -208,7 +208,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection)\n \n int ipc_client_send_command_to_connection(\n \tstruct ipc_client_connection *connection,\n-\tconst char *message, struct strbuf *answer)\n+\tconst char *message, size_t message_len,\n+\tstruct strbuf *answer)\n {\n \tint ret = 0;\n \n@@ -216,7 +217,7 @@ int ipc_client_send_command_to_connection(\n \n \ttrace2_region_enter(\"ipc-client\", \"send-command\", NULL);\n \n-\tif (write_packetized_from_buf_no_flush(message, strlen(message),\n+\tif (write_packetized_from_buf_no_flush(message, message_len,\n \t\t\t\t\t connection->fd) < 0 ||\n \t packet_flush_gently(connection->fd) < 0) {\n \t\tret = error(_(\"could not send IPC command\"));\n@@ -239,7 +240,8 @@ done:\n \n int ipc_client_send_command(const char *path,\n \t\t\t const struct ipc_client_connect_options *options,\n-\t\t\t const char *message, struct strbuf *response)\n+\t\t\t const char *message, size_t message_len,\n+\t\t\t struct strbuf *response)\n {\n \tint ret = -1;\n \tenum ipc_active_state state;\n@@ -250,7 +252,9 @@ int ipc_client_send_command(const char *path,\n \tif (state != IPC_STATE__LISTENING)\n \t\treturn ret;\n \n-\tret = ipc_client_send_command_to_connection(connection, message, response);\n+\tret = ipc_client_send_command_to_connection(connection,\n+\t\t\t\t\t\t message, message_len,\n+\t\t\t\t\t\t response);\n \n \tipc_client_close_connection(connection);\n \n@@ -458,7 +462,7 @@ static int do_io(struct ipc_server_thread_data *server_thread_data)\n \tif (ret >= 0) {\n \t\tret = server_thread_data->server_data->application_cb(\n \t\t\tserver_thread_data->server_data->application_data,\n-\t\t\tbuf.buf, do_io_reply_callback, &reply_data);\n+\t\t\tbuf.buf, buf.len, do_io_reply_callback, &reply_data);\n \n \t\tpacket_flush_gently(reply_data.fd);\n \ndiff --git a/simple-ipc.h b/simple-ipc.h\nindex 2c48a5ee004..9c7330fcda0 100644\n--- a/simple-ipc.h\n+++ b/simple-ipc.h\n@@ -107,7 +107,8 @@ void ipc_client_close_connection(struct ipc_client_connection *connection);\n */\n int ipc_client_send_command_to_connection(\n \tstruct ipc_client_connection *connection,\n-\tconst char *message, struct strbuf *answer);\n+\tconst char *message, size_t message_len,\n+\tstruct strbuf *answer);\n \n /*\n * Used by the client to synchronously connect and send and receive a\n@@ -119,7 +120,8 @@ int ipc_client_send_command_to_connection(\n */\n int ipc_client_send_command(const char *path,\n \t\t\t const struct ipc_client_connect_options *options,\n-\t\t\t const char *message, struct strbuf *answer);\n+\t\t\t const char *message, size_t message_len,\n+\t\t\t struct strbuf *answer);\n \n /*\n * Simple IPC Server Side API.\n@@ -144,6 +146,7 @@ typedef int (ipc_server_reply_cb)(struct ipc_server_reply_data *,\n */\n typedef int (ipc_server_application_cb)(void *application_data,\n \t\t\t\t\tconst char *request,\n+\t\t\t\t\tsize_t request_len,\n \t\t\t\t\tipc_server_reply_cb *reply_cb,\n \t\t\t\t\tstruct ipc_server_reply_data *reply_data);\n \ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex 42040ef81b1..91345180750 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -112,7 +112,7 @@ static int app__slow_command(ipc_server_reply_cb *reply_cb,\n /*\n * The client sent a command followed by a (possibly very) large buffer.\n */\n-static int app__sendbytes_command(const char *received,\n+static int app__sendbytes_command(const char *received, size_t received_len,\n \t\t\t\t ipc_server_reply_cb *reply_cb,\n \t\t\t\t struct ipc_server_reply_data *reply_data)\n {\n@@ -123,6 +123,13 @@ static int app__sendbytes_command(const char *received,\n \tint errs = 0;\n \tint ret;\n \n+\t/*\n+\t * The test is setup to send:\n+\t * \"sendbytes\" SP \n+\t */\n+\tif (received_len < strlen(\"sendbytes \"))\n+\t\tBUG(\"received_len is short in app__sendbytes_command\");\n+\n \tif (skip_prefix(received, \"sendbytes \", &p))\n \t\tlen_ballast = strlen(p);\n \n@@ -160,7 +167,7 @@ static ipc_server_application_cb test_app_cb;\n * by this application.\n */\n static int test_app_cb(void *application_data,\n-\t\t const char *command,\n+\t\t const char *command, size_t command_len,\n \t\t ipc_server_reply_cb *reply_cb,\n \t\t struct ipc_server_reply_data *reply_data)\n {\n@@ -173,7 +180,7 @@ static int test_app_cb(void *application_data,\n \tif (application_data != (void*)&my_app_data)\n \t\tBUG(\"application_cb: application_data pointer wrong\");\n \n-\tif (!strcmp(command, \"quit\")) {\n+\tif (command_len == 4 && !strncmp(command, \"quit\", 4)) {\n \t\t/*\n \t\t * The client sent a \"quit\" command. This is an async\n \t\t * request for the server to shutdown.\n@@ -193,22 +200,23 @@ static int test_app_cb(void *application_data,\n \t\treturn SIMPLE_IPC_QUIT;\n \t}\n \n-\tif (!strcmp(command, \"ping\")) {\n+\tif (command_len == 4 && !strncmp(command, \"ping\", 4)) {\n \t\tconst char *answer = \"pong\";\n \t\treturn reply_cb(reply_data, answer, strlen(answer));\n \t}\n \n-\tif (!strcmp(command, \"big\"))\n+\tif (command_len == 3 && !strncmp(command, \"big\", 3))\n \t\treturn app__big_command(reply_cb, reply_data);\n \n-\tif (!strcmp(command, \"chunk\"))\n+\tif (command_len == 5 && !strncmp(command, \"chunk\", 5))\n \t\treturn app__chunk_command(reply_cb, reply_data);\n \n-\tif (!strcmp(command, \"slow\"))\n+\tif (command_len == 4 && !strncmp(command, \"slow\", 4))\n \t\treturn app__slow_command(reply_cb, reply_data);\n \n-\tif (starts_with(command, \"sendbytes \"))\n-\t\treturn app__sendbytes_command(command, reply_cb, reply_data);\n+\tif (command_len >= 10 && starts_with(command, \"sendbytes \"))\n+\t\treturn app__sendbytes_command(command, command_len,\n+\t\t\t\t\t reply_cb, reply_data);\n \n \treturn app__unhandled_command(command, reply_cb, reply_data);\n }\n@@ -488,7 +496,9 @@ static int client__send_ipc(void)\n \toptions.wait_if_busy = 1;\n \toptions.wait_if_not_found = 0;\n \n-\tif (!ipc_client_send_command(cl_args.path, &options, command, &buf)) {\n+\tif (!ipc_client_send_command(cl_args.path, &options,\n+\t\t\t\t command, strlen(command),\n+\t\t\t\t &buf)) {\n \t\tif (buf.len) {\n \t\t\tprintf(\"%s\\n\", buf.buf);\n \t\t\tfflush(stdout);\n@@ -556,7 +566,9 @@ static int do_sendbytes(int bytecount, char byte, const char *path,\n \tstrbuf_addstr(&buf_send, \"sendbytes \");\n \tstrbuf_addchars(&buf_send, byte, bytecount);\n \n-\tif (!ipc_client_send_command(path, options, buf_send.buf, &buf_resp)) {\n+\tif (!ipc_client_send_command(path, options,\n+\t\t\t\t buf_send.buf, buf_send.len,\n+\t\t\t\t &buf_resp)) {\n \t\tstrbuf_rtrim(&buf_resp);\n \t\tprintf(\"sent:%c%08d %s\\n\", byte, bytecount, buf_resp.buf);\n \t\tfflush(stdout);\n-- \ngitgitgadget\n\n"},{"id":"436427","messageId":"c94b4cbcbf2900ccf1c54873f926099b99e4d663.1632152178.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.v2.git.1632152178.gitgitgadget@gmail.com","subject":"[PATCH v2 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T15:36:14Z","receivedAt":"2021-09-20T15:36:29Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nFrom: Carlo Marcelo Arenas Belón \n\nMove the declartion of the `enum ipc_active_state` type outside of\nthe SUPPORTS_SIMPLE_IPC ifdef.\n\nA later commit will introduce the `fsmonitor_ipc__*()` API and stub in\na \"mock\" implementation that requires this enum in some function\nsignatures.\n\nSigned-off-by: Carlo Marcelo Arenas Belón \nSigned-off-by: Jeff Hostetler \n---\n simple-ipc.h | 14 +++++++-------\n 1 file changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/simple-ipc.h b/simple-ipc.h\nindex 9c7330fcda0..b396293bdfc 100644\n--- a/simple-ipc.h\n+++ b/simple-ipc.h\n@@ -5,13 +5,6 @@\n * See Documentation/technical/api-simple-ipc.txt\n */\n \n-#ifdef SUPPORTS_SIMPLE_IPC\n-#include \"pkt-line.h\"\n-\n-/*\n- * Simple IPC Client Side API.\n- */\n-\n enum ipc_active_state {\n \t/*\n \t * The pipe/socket exists and the daemon is waiting for connections.\n@@ -43,6 +36,13 @@ enum ipc_active_state {\n \tIPC_STATE__OTHER_ERROR,\n };\n \n+#ifdef SUPPORTS_SIMPLE_IPC\n+#include \"pkt-line.h\"\n+\n+/*\n+ * Simple IPC Client Side API.\n+ */\n+\n struct ipc_client_connect_options {\n \t/*\n \t * Spin under timeout if the server is running but can't\n-- \ngitgitgadget\n\n"},{"id":"436428","messageId":"82b6ce0dd6a8934c202dfd38f89daf81e0ed45c7.1632152178.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.v2.git.1632152178.gitgitgadget@gmail.com","subject":"[PATCH v2 4/7] simple-ipc/ipc-win32: add trace2 debugging","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T15:36:15Z","receivedAt":"2021-09-20T15:36:32Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nCreate \"ipc-debug\" category events to log unexpected errors\nwhen creating Simple-IPC connections.\n\nSigned-off-by: Jeff Hostetler \n---\n compat/simple-ipc/ipc-win32.c | 25 ++++++++++++++++++++++++-\n 1 file changed, 24 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex 8e889d6a506..a8dd46bd922 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -49,6 +49,9 @@ static enum ipc_active_state get_active_state(wchar_t *pipe_path)\n \tif (GetLastError() == ERROR_FILE_NOT_FOUND)\n \t\treturn IPC_STATE__PATH_NOT_FOUND;\n \n+\ttrace2_data_intmax(\"ipc-debug\", NULL, \"getstate/waitpipe/gle\",\n+\t\t\t (intmax_t)GetLastError());\n+\n \treturn IPC_STATE__OTHER_ERROR;\n }\n \n@@ -109,9 +112,15 @@ static enum ipc_active_state connect_to_server(\n \t\t\tt_start_ms = (DWORD)(getnanotime() / 1000000);\n \n \t\t\tif (!WaitNamedPipeW(wpath, timeout_ms)) {\n-\t\t\t\tif (GetLastError() == ERROR_SEM_TIMEOUT)\n+\t\t\t\tDWORD gleWait = GetLastError();\n+\n+\t\t\t\tif (gleWait == ERROR_SEM_TIMEOUT)\n \t\t\t\t\treturn IPC_STATE__NOT_LISTENING;\n \n+\t\t\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n+\t\t\t\t\t\t \"connect/waitpipe/gle\",\n+\t\t\t\t\t\t (intmax_t)gleWait);\n+\n \t\t\t\treturn IPC_STATE__OTHER_ERROR;\n \t\t\t}\n \n@@ -133,17 +142,31 @@ static enum ipc_active_state connect_to_server(\n \t\t\tbreak; /* try again */\n \n \t\tdefault:\n+\t\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n+\t\t\t\t\t \"connect/createfile/gle\",\n+\t\t\t\t\t (intmax_t)gle);\n+\n \t\t\treturn IPC_STATE__OTHER_ERROR;\n \t\t}\n \t}\n \n \tif (!SetNamedPipeHandleState(hPipe, &mode, NULL, NULL)) {\n+\t\tgle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n+\t\t\t\t \"connect/setpipestate/gle\",\n+\t\t\t\t (intmax_t)gle);\n+\n \t\tCloseHandle(hPipe);\n \t\treturn IPC_STATE__OTHER_ERROR;\n \t}\n \n \t*pfd = _open_osfhandle((intptr_t)hPipe, O_RDWR|O_BINARY);\n \tif (*pfd < 0) {\n+\t\tgle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL,\n+\t\t\t\t \"connect/openosfhandle/gle\",\n+\t\t\t\t (intmax_t)gle);\n+\n \t\tCloseHandle(hPipe);\n \t\treturn IPC_STATE__OTHER_ERROR;\n \t}\n-- \ngitgitgadget\n\n"},{"id":"436429","messageId":"0822118c4b50ddd3668ded97a89cab2c47895078.1632152178.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.v2.git.1632152178.gitgitgadget@gmail.com","subject":"[PATCH v2 6/7] run-command: create start_bg_command","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T15:36:17Z","receivedAt":"2021-09-20T15:36:33Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nCreate a variation of `run_command()` and `start_command()` to launch a command\ninto the background and optionally wait for it to become \"ready\" before returning.\n\nSigned-off-by: Jeff Hostetler \n---\n run-command.c | 129 ++++++++++++++++++++++++++++++++++++++++++++++++++\n run-command.h | 57 ++++++++++++++++++++++\n 2 files changed, 186 insertions(+)\n\ndiff --git a/run-command.c b/run-command.c\nindex 3e4e082e94d..76bbef9d96d 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1901,3 +1901,132 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)\n \t}\n \tstrvec_pushf(env_array, \"%s=%s\", GIT_DIR_ENVIRONMENT, new_git_dir);\n }\n+\n+enum start_bg_result start_bg_command(struct child_process *cmd,\n+\t\t\t\t start_bg_wait_cb *wait_cb,\n+\t\t\t\t void *cb_data,\n+\t\t\t\t unsigned int timeout_sec)\n+{\n+\tenum start_bg_result sbgr = SBGR_ERROR;\n+\tint ret;\n+\tint wait_status;\n+\tpid_t pid_seen;\n+\ttime_t time_limit;\n+\n+\t/*\n+\t * We do not allow clean-on-exit because the child process\n+\t * should persist in the background and possibly/probably\n+\t * after this process exits. So we don't want to kill the\n+\t * child during our atexit routine.\n+\t */\n+\tif (cmd->clean_on_exit)\n+\t\tBUG(\"start_bg_command() does not allow non-zero clean_on_exit\");\n+\n+\tif (!cmd->trace2_child_class)\n+\t\tcmd->trace2_child_class = \"background\";\n+\n+\tret = start_command(cmd);\n+\tif (ret) {\n+\t\t/*\n+\t\t * We assume that if `start_command()` fails, we\n+\t\t * either get a complete `trace2_child_start() /\n+\t\t * trace2_child_exit()` pair or it fails before the\n+\t\t * `trace2_child_start()` is emitted, so we do not\n+\t\t * need to worry about it here.\n+\t\t *\n+\t\t * We also assume that `start_command()` does not add\n+\t\t * us to the cleanup list. And that it calls\n+\t\t * calls `child_process_clear()`.\n+\t\t */\n+\t\tsbgr = SBGR_ERROR;\n+\t\tgoto done;\n+\t}\n+\n+\ttime(&time_limit);\n+\ttime_limit += timeout_sec;\n+\n+wait:\n+\tpid_seen = waitpid(cmd->pid, &wait_status, WNOHANG);\n+\n+\tif (!pid_seen) {\n+\t\t/*\n+\t\t * The child is currently running. Ask the callback\n+\t\t * if the child is ready to do work or whether we\n+\t\t * should keep waiting for it to boot up.\n+\t\t */\n+\t\tret = (*wait_cb)(cmd, cb_data);\n+\t\tif (!ret) {\n+\t\t\t/*\n+\t\t\t * The child is running and \"ready\".\n+\t\t\t */\n+\t\t\ttrace2_child_ready(cmd, \"ready\");\n+\t\t\tsbgr = SBGR_READY;\n+\t\t\tgoto done;\n+\t\t} else if (ret > 0) {\n+\t\t\t/*\n+\t\t\t * The callback said to give it more time to boot up\n+\t\t\t * (subject to our timeout limit).\n+\t\t\t */\n+\t\t\ttime_t now;\n+\n+\t\t\ttime(&now);\n+\t\t\tif (now < time_limit)\n+\t\t\t\tgoto wait;\n+\n+\t\t\t/*\n+\t\t\t * Our timeout has expired. We don't try to\n+\t\t\t * kill the child, but rather let it continue\n+\t\t\t * (hopefully) trying to startup.\n+\t\t\t */\n+\t\t\ttrace2_child_ready(cmd, \"timeout\");\n+\t\t\tsbgr = SBGR_TIMEOUT;\n+\t\t\tgoto done;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * The cb gave up on this child. It is still running,\n+\t\t\t * but our cb got an error trying to probe it.\n+\t\t\t */\n+\t\t\ttrace2_child_ready(cmd, \"error\");\n+\t\t\tsbgr = SBGR_CB_ERROR;\n+\t\t\tgoto done;\n+\t\t}\n+\t}\n+\n+\telse if (pid_seen == cmd->pid) {\n+\t\tint child_code = -1;\n+\n+\t\t/*\n+\t\t * The child started, but exited or was terminated\n+\t\t * before becoming \"ready\".\n+\t\t *\n+\t\t * We try to match the behavior of `wait_or_whine()`\n+\t\t * WRT the handling of WIFSIGNALED() and WIFEXITED()\n+\t\t * and convert the child's status to a return code for\n+\t\t * tracing purposes and emit the `trace2_child_exit()`\n+\t\t * event.\n+\t\t *\n+\t\t * We do not want the wait_or_whine() error message\n+\t\t * because we will be called by client-side library\n+\t\t * routines.\n+\t\t */\n+\t\tif (WIFEXITED(wait_status))\n+\t\t\tchild_code = WEXITSTATUS(wait_status);\n+\t\telse if (WIFSIGNALED(wait_status))\n+\t\t\tchild_code = WTERMSIG(wait_status) + 128;\n+\t\ttrace2_child_exit(cmd, child_code);\n+\n+\t\tsbgr = SBGR_DIED;\n+\t\tgoto done;\n+\t}\n+\n+\telse if (pid_seen < 0 && errno == EINTR)\n+\t\tgoto wait;\n+\n+\ttrace2_child_exit(cmd, -1);\n+\tsbgr = SBGR_ERROR;\n+\n+done:\n+\tchild_process_clear(cmd);\n+\tinvalidate_lstat_cache();\n+\treturn sbgr;\n+}\ndiff --git a/run-command.h b/run-command.h\nindex af1296769f9..17b1b80c3d7 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -496,4 +496,61 @@ int run_processes_parallel_tr2(int n, get_next_task_fn, start_failure_fn,\n */\n void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir);\n \n+/**\n+ * Possible return values for start_bg_command().\n+ */\n+enum start_bg_result {\n+\t/* child process is \"ready\" */\n+\tSBGR_READY = 0,\n+\n+\t/* child process could not be started */\n+\tSBGR_ERROR,\n+\n+\t/* callback error when testing for \"ready\" */\n+\tSBGR_CB_ERROR,\n+\n+\t/* timeout expired waiting for child to become \"ready\" */\n+\tSBGR_TIMEOUT,\n+\n+\t/* child process exited or was signalled before becomming \"ready\" */\n+\tSBGR_DIED,\n+};\n+\n+/**\n+ * Callback used by start_bg_command() to ask whether the\n+ * child process is ready or needs more time to become \"ready\".\n+ *\n+ * The callback will receive the cmd and cb_data arguments given to\n+ * start_bg_command().\n+ *\n+ * Returns 1 is child needs more time (subject to the requested timeout).\n+ * Returns 0 if child is \"ready\".\n+ * Returns -1 on any error and cause start_bg_command() to also error out.\n+ */\n+typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);\n+\n+/**\n+ * Start a command in the background. Wait long enough for the child\n+ * to become \"ready\" (as defined by the provided callback). Capture\n+ * immediate errors (like failure to start) and any immediate exit\n+ * status (such as a shutdown/signal before the child became \"ready\")\n+ * and return this like start_command().\n+ *\n+ * We run a custom wait loop using the provided callback to wait for\n+ * the child to start and become \"ready\". This is limited by the given\n+ * timeout value.\n+ *\n+ * If the child does successfully start and become \"ready\", we orphan\n+ * it into the background.\n+ *\n+ * The caller must not call finish_command().\n+ *\n+ * The opaque cb_data argument will be forwarded to the callback for\n+ * any instance data that it might require. This may be NULL.\n+ */\n+enum start_bg_result start_bg_command(struct child_process *cmd,\n+\t\t\t\t start_bg_wait_cb *wait_cb,\n+\t\t\t\t void *cb_data,\n+\t\t\t\t unsigned int timeout_sec);\n+\n #endif\n-- \ngitgitgadget\n\n"},{"id":"436430","messageId":"faf6034848e78205630c64624aaeb509b505af82.1632152178.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.v2.git.1632152178.gitgitgadget@gmail.com","subject":"[PATCH v2 5/7] simple-ipc/ipc-win32: add Windows ACL to named pipe","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T15:36:16Z","receivedAt":"2021-09-20T15:36:35Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nSet an ACL on the named pipe to allow the well-known group EVERYONE\nto read and write to the IPC server's named pipe. In the event that\nthe daemon was started with elevation, allow non-elevated clients\nto communicate with the daemon.\n\nSigned-off-by: Jeff Hostetler \n---\n compat/simple-ipc/ipc-win32.c | 140 +++++++++++++++++++++++++++++++---\n 1 file changed, 129 insertions(+), 11 deletions(-)\n\ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex a8dd46bd922..20ea7b65e0b 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -3,6 +3,8 @@\n #include \"strbuf.h\"\n #include \"pkt-line.h\"\n #include \"thread-utils.h\"\n+#include \"accctrl.h\"\n+#include \"aclapi.h\"\n \n #ifndef SUPPORTS_SIMPLE_IPC\n /*\n@@ -592,11 +594,132 @@ finished:\n \treturn NULL;\n }\n \n+/*\n+ * We need to build a Windows \"SECURITY_ATTRIBUTES\" object and use it\n+ * to apply an ACL when we create the initial instance of the Named\n+ * Pipe. The construction is somewhat involved and consists of\n+ * several sequential steps and intermediate objects.\n+ *\n+ * We use this structure to hold these intermediate pointers so that\n+ * we can free them as a group. (It is unclear from the docs whether\n+ * some of these intermediate pointers can be freed before we are\n+ * finished using the \"lpSA\" member.)\n+ */\n+struct my_sa_data\n+{\n+\tPSID pEveryoneSID;\n+\tPACL pACL;\n+\tPSECURITY_DESCRIPTOR pSD;\n+\tLPSECURITY_ATTRIBUTES lpSA;\n+};\n+\n+static void init_sa(struct my_sa_data *d)\n+{\n+\tmemset(d, 0, sizeof(*d));\n+}\n+\n+static void release_sa(struct my_sa_data *d)\n+{\n+\tif (d->pEveryoneSID)\n+\t\tFreeSid(d->pEveryoneSID);\n+\tif (d->pACL)\n+\t\tLocalFree(d->pACL);\n+\tif (d->pSD)\n+\t\tLocalFree(d->pSD);\n+\tif (d->lpSA)\n+\t\tLocalFree(d->lpSA);\n+\n+\tmemset(d, 0, sizeof(*d));\n+}\n+\n+/*\n+ * Create SECURITY_ATTRIBUTES to apply to the initial named pipe. The\n+ * creator of the first server instance gets to set the ACLs on it.\n+ *\n+ * We allow the well-known group `EVERYONE` to have read+write access\n+ * to the named pipe so that clients can send queries to the daemon\n+ * and receive the response.\n+ *\n+ * Normally, this is not necessary since the daemon is usually\n+ * automatically started by a foreground command like `git status`,\n+ * but in those cases where an elevated Git command started the daemon\n+ * (such that the daemon itself runs with elevation), we need to add\n+ * the ACL so that non-elevated commands can write to it.\n+ *\n+ * The following document was helpful:\n+ * https://docs.microsoft.com/en-us/windows/win32/secauthz/creating-a-security-descriptor-for-a-new-object-in-c--\n+ *\n+ * Returns d->lpSA set to a SA or NULL.\n+ */\n+static LPSECURITY_ATTRIBUTES get_sa(struct my_sa_data *d)\n+{\n+\tSID_IDENTIFIER_AUTHORITY sid_auth_world = SECURITY_WORLD_SID_AUTHORITY;\n+#define NR_EA (1)\n+\tEXPLICIT_ACCESS ea[NR_EA];\n+\tDWORD dwResult;\n+\n+\tif (!AllocateAndInitializeSid(&sid_auth_world, 1,\n+\t\t\t\t SECURITY_WORLD_RID, 0,0,0,0,0,0,0,\n+\t\t\t\t &d->pEveryoneSID)) {\n+\t\tDWORD gle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL, \"alloc-world-sid/gle\",\n+\t\t\t\t (intmax_t)gle);\n+\t\tgoto fail;\n+\t}\n+\n+\tmemset(ea, 0, NR_EA * sizeof(EXPLICIT_ACCESS));\n+\n+\tea[0].grfAccessPermissions = GENERIC_READ | GENERIC_WRITE;\n+\tea[0].grfAccessMode = SET_ACCESS;\n+\tea[0].grfInheritance = NO_INHERITANCE;\n+\tea[0].Trustee.MultipleTrusteeOperation = NO_MULTIPLE_TRUSTEE;\n+\tea[0].Trustee.TrusteeForm = TRUSTEE_IS_SID;\n+\tea[0].Trustee.TrusteeType = TRUSTEE_IS_WELL_KNOWN_GROUP;\n+\tea[0].Trustee.ptstrName = (LPTSTR)d->pEveryoneSID;\n+\n+\tdwResult = SetEntriesInAcl(NR_EA, ea, NULL, &d->pACL);\n+\tif (dwResult != ERROR_SUCCESS) {\n+\t\tDWORD gle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL, \"set-acl-entry/gle\",\n+\t\t\t\t (intmax_t)gle);\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL, \"set-acl-entry/dw\",\n+\t\t\t\t (intmax_t)dwResult);\n+\t\tgoto fail;\n+\t}\n+\n+\td->pSD = (PSECURITY_DESCRIPTOR)LocalAlloc(\n+\t\tLPTR, SECURITY_DESCRIPTOR_MIN_LENGTH);\n+\tif (!InitializeSecurityDescriptor(d->pSD, SECURITY_DESCRIPTOR_REVISION)) {\n+\t\tDWORD gle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL, \"init-sd/gle\", (intmax_t)gle);\n+\t\tgoto fail;\n+\t}\n+\n+\tif (!SetSecurityDescriptorDacl(d->pSD, TRUE, d->pACL, FALSE)) {\n+\t\tDWORD gle = GetLastError();\n+\t\ttrace2_data_intmax(\"ipc-debug\", NULL, \"set-sd-dacl/gle\", (intmax_t)gle);\n+\t\tgoto fail;\n+\t}\n+\n+\td->lpSA = (LPSECURITY_ATTRIBUTES)LocalAlloc(LPTR, sizeof(SECURITY_ATTRIBUTES));\n+\td->lpSA->nLength = sizeof(SECURITY_ATTRIBUTES);\n+\td->lpSA->lpSecurityDescriptor = d->pSD;\n+\td->lpSA->bInheritHandle = FALSE;\n+\n+\treturn d->lpSA;\n+\n+fail:\n+\trelease_sa(d);\n+\treturn NULL;\n+}\n+\n static HANDLE create_new_pipe(wchar_t *wpath, int is_first)\n {\n \tHANDLE hPipe;\n \tDWORD dwOpenMode, dwPipeMode;\n-\tLPSECURITY_ATTRIBUTES lpsa = NULL;\n+\tstruct my_sa_data my_sa_data;\n+\n+\tinit_sa(&my_sa_data);\n \n \tdwOpenMode = PIPE_ACCESS_INBOUND | PIPE_ACCESS_OUTBOUND |\n \t\tFILE_FLAG_OVERLAPPED;\n@@ -612,20 +735,15 @@ static HANDLE create_new_pipe(wchar_t *wpath, int is_first)\n \t\t * set the ACL / Security Attributes on the named\n \t\t * pipe; subsequent instances inherit and cannot\n \t\t * change them.\n-\t\t *\n-\t\t * TODO Should we allow the application layer to\n-\t\t * specify security attributes, such as `LocalService`\n-\t\t * or `LocalSystem`, when we create the named pipe?\n-\t\t * This question is probably not important when the\n-\t\t * daemon is started by a foreground user process and\n-\t\t * only needs to talk to the current user, but may be\n-\t\t * if the daemon is run via the Control Panel as a\n-\t\t * System Service.\n \t\t */\n+\t\tget_sa(&my_sa_data);\n \t}\n \n \thPipe = CreateNamedPipeW(wpath, dwOpenMode, dwPipeMode,\n-\t\t\t\t PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0, lpsa);\n+\t\t\t\t PIPE_UNLIMITED_INSTANCES, 1024, 1024, 0,\n+\t\t\t\t my_sa_data.lpSA);\n+\n+\trelease_sa(&my_sa_data);\n \n \treturn hPipe;\n }\n-- \ngitgitgadget\n\n"},{"id":"436431","messageId":"6b7a058284b93fab52458b12a6aede5e8aed6652.1632152179.git.gitgitgadget@gmail.com","threadId":"56508","inReplyTo":"pull.1040.v2.git.1632152178.gitgitgadget@gmail.com","subject":"[PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Jeff Hostetler via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-09-20T15:36:18Z","receivedAt":"2021-09-20T15:36:37Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"From: Jeff Hostetler \n\nConvert test helper to use `start_bg_command()` when spawning a server\ndaemon in the background rather than blocks of platform-specific code.\n\nAlso, while here, remove _() translation around error messages since\nthis is a test helper and not Git code.\n\nSigned-off-by: Jeff Hostetler \n---\n t/helper/test-simple-ipc.c | 199 ++++++++-----------------------------\n 1 file changed, 43 insertions(+), 156 deletions(-)\n\ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex 91345180750..28365ff85b6 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -9,6 +9,7 @@\n #include \"parse-options.h\"\n #include \"thread-utils.h\"\n #include \"strvec.h\"\n+#include \"run-command.h\"\n \n #ifndef SUPPORTS_SIMPLE_IPC\n int cmd__simple_ipc(int argc, const char **argv)\n@@ -267,185 +268,71 @@ static int daemon__run_server(void)\n \t */\n \tret = ipc_server_run(cl_args.path, &opts, test_app_cb, (void*)&my_app_data);\n \tif (ret == -2)\n-\t\terror(_(\"socket/pipe already in use: '%s'\"), cl_args.path);\n+\t\terror(\"socket/pipe already in use: '%s'\", cl_args.path);\n \telse if (ret == -1)\n-\t\terror_errno(_(\"could not start server on: '%s'\"), cl_args.path);\n+\t\terror_errno(\"could not start server on: '%s'\", cl_args.path);\n \n \treturn ret;\n }\n \n-#ifndef GIT_WINDOWS_NATIVE\n-/*\n- * This is adapted from `daemonize()`. Use `fork()` to directly create and\n- * run the daemon in a child process.\n- */\n-static int spawn_server(pid_t *pid)\n-{\n-\tstruct ipc_server_opts opts = {\n-\t\t.nr_threads = cl_args.nr_threads,\n-\t};\n+static start_bg_wait_cb bg_wait_cb;\n \n-\t*pid = fork();\n-\n-\tswitch (*pid) {\n-\tcase 0:\n-\t\tif (setsid() == -1)\n-\t\t\terror_errno(_(\"setsid failed\"));\n-\t\tclose(0);\n-\t\tclose(1);\n-\t\tclose(2);\n-\t\tsanitize_stdfds();\n+static int bg_wait_cb(const struct child_process *cp, void *cb_data)\n+{\n+\tint s = ipc_get_active_state(cl_args.path);\n \n-\t\treturn ipc_server_run(cl_args.path, &opts, test_app_cb,\n-\t\t\t\t (void*)&my_app_data);\n+\tswitch (s) {\n+\tcase IPC_STATE__LISTENING:\n+\t\t/* child is \"ready\" */\n+\t\treturn 0;\n \n-\tcase -1:\n-\t\treturn error_errno(_(\"could not spawn daemon in the background\"));\n+\tcase IPC_STATE__NOT_LISTENING:\n+\tcase IPC_STATE__PATH_NOT_FOUND:\n+\t\t/* give child more time */\n+\t\treturn 1;\n \n \tdefault:\n-\t\treturn 0;\n+\tcase IPC_STATE__INVALID_PATH:\n+\tcase IPC_STATE__OTHER_ERROR:\n+\t\t/* all the time in world won't help */\n+\t\treturn -1;\n \t}\n }\n-#else\n-/*\n- * Conceptually like `daemonize()` but different because Windows does not\n- * have `fork(2)`. Spawn a normal Windows child process but without the\n- * limitations of `start_command()` and `finish_command()`.\n- */\n-static int spawn_server(pid_t *pid)\n-{\n-\tchar test_tool_exe[MAX_PATH];\n-\tstruct strvec args = STRVEC_INIT;\n-\tint in, out;\n-\n-\tGetModuleFileNameA(NULL, test_tool_exe, MAX_PATH);\n-\n-\tin = open(\"/dev/null\", O_RDONLY);\n-\tout = open(\"/dev/null\", O_WRONLY);\n-\n-\tstrvec_push(&args, test_tool_exe);\n-\tstrvec_push(&args, \"simple-ipc\");\n-\tstrvec_push(&args, \"run-daemon\");\n-\tstrvec_pushf(&args, \"--name=%s\", cl_args.path);\n-\tstrvec_pushf(&args, \"--threads=%d\", cl_args.nr_threads);\n-\n-\t*pid = mingw_spawnvpe(args.v[0], args.v, NULL, NULL, in, out, out);\n-\tclose(in);\n-\tclose(out);\n-\n-\tstrvec_clear(&args);\n \n-\tif (*pid < 0)\n-\t\treturn error(_(\"could not spawn daemon in the background\"));\n-\n-\treturn 0;\n-}\n-#endif\n-\n-/*\n- * This is adapted from `wait_or_whine()`. Watch the child process and\n- * let it get started and begin listening for requests on the socket\n- * before reporting our success.\n- */\n-static int wait_for_server_startup(pid_t pid_child)\n+static int daemon__start_server(void)\n {\n-\tint status;\n-\tpid_t pid_seen;\n-\tenum ipc_active_state s;\n-\ttime_t time_limit, now;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\tenum start_bg_result sbgr;\n \n-\ttime(&time_limit);\n-\ttime_limit += cl_args.max_wait_sec;\n+\tstrvec_push(&cp.args, \"test-tool\");\n+\tstrvec_push(&cp.args, \"simple-ipc\");\n+\tstrvec_push(&cp.args, \"run-daemon\");\n+\tstrvec_pushf(&cp.args, \"--name=%s\", cl_args.path);\n+\tstrvec_pushf(&cp.args, \"--threads=%d\", cl_args.nr_threads);\n \n-\tfor (;;) {\n-\t\tpid_seen = waitpid(pid_child, &status, WNOHANG);\n+\tcp.no_stdin = 1;\n+\tcp.no_stdout = 1;\n+\tcp.no_stderr = 1;\n \n-\t\tif (pid_seen == -1)\n-\t\t\treturn error_errno(_(\"waitpid failed\"));\n+\tsbgr = start_bg_command(&cp, bg_wait_cb, NULL, cl_args.max_wait_sec);\n \n-\t\telse if (pid_seen == 0) {\n-\t\t\t/*\n-\t\t\t * The child is still running (this should be\n-\t\t\t * the normal case). Try to connect to it on\n-\t\t\t * the socket and see if it is ready for\n-\t\t\t * business.\n-\t\t\t *\n-\t\t\t * If there is another daemon already running,\n-\t\t\t * our child will fail to start (possibly\n-\t\t\t * after a timeout on the lock), but we don't\n-\t\t\t * care (who responds) if the socket is live.\n-\t\t\t */\n-\t\t\ts = ipc_get_active_state(cl_args.path);\n-\t\t\tif (s == IPC_STATE__LISTENING)\n-\t\t\t\treturn 0;\n-\n-\t\t\ttime(&now);\n-\t\t\tif (now > time_limit)\n-\t\t\t\treturn error(_(\"daemon not online yet\"));\n-\n-\t\t\tcontinue;\n-\t\t}\n+\tswitch (sbgr) {\n+\tcase SBGR_READY:\n+\t\treturn 0;\n \n-\t\telse if (pid_seen == pid_child) {\n-\t\t\t/*\n-\t\t\t * The new child daemon process shutdown while\n-\t\t\t * it was starting up, so it is not listening\n-\t\t\t * on the socket.\n-\t\t\t *\n-\t\t\t * Try to ping the socket in the odd chance\n-\t\t\t * that another daemon started (or was already\n-\t\t\t * running) while our child was starting.\n-\t\t\t *\n-\t\t\t * Again, we don't care who services the socket.\n-\t\t\t */\n-\t\t\ts = ipc_get_active_state(cl_args.path);\n-\t\t\tif (s == IPC_STATE__LISTENING)\n-\t\t\t\treturn 0;\n+\tdefault:\n+\tcase SBGR_ERROR:\n+\tcase SBGR_CB_ERROR:\n+\t\treturn error(\"daemon failed to start\");\n \n-\t\t\t/*\n-\t\t\t * We don't care about the WEXITSTATUS() nor\n-\t\t\t * any of the WIF*(status) values because\n-\t\t\t * `cmd__simple_ipc()` does the `!!result`\n-\t\t\t * trick on all function return values.\n-\t\t\t *\n-\t\t\t * So it is sufficient to just report the\n-\t\t\t * early shutdown as an error.\n-\t\t\t */\n-\t\t\treturn error(_(\"daemon failed to start\"));\n-\t\t}\n+\tcase SBGR_TIMEOUT:\n+\t\treturn error(\"daemon not online yet\");\n \n-\t\telse\n-\t\t\treturn error(_(\"waitpid is confused\"));\n+\tcase SBGR_DIED:\n+\t\treturn error(\"daemon terminated\");\n \t}\n }\n \n-/*\n- * This process will start a simple-ipc server in a background process and\n- * wait for it to become ready. This is like `daemonize()` but gives us\n- * more control and better error reporting (and makes it easier to write\n- * unit tests).\n- */\n-static int daemon__start_server(void)\n-{\n-\tpid_t pid_child;\n-\tint ret;\n-\n-\t/*\n-\t * Run the actual daemon in a background process.\n-\t */\n-\tret = spawn_server(&pid_child);\n-\tif (pid_child <= 0)\n-\t\treturn ret;\n-\n-\t/*\n-\t * Let the parent wait for the child process to get started\n-\t * and begin listening for requests on the socket.\n-\t */\n-\tret = wait_for_server_startup(pid_child);\n-\n-\treturn ret;\n-}\n-\n /*\n * This process will run a quick probe to see if a simple-ipc server\n * is active on this path.\n@@ -548,7 +435,7 @@ static int client__stop_server(void)\n \n \t\ttime(&now);\n \t\tif (now > time_limit)\n-\t\t\treturn error(_(\"daemon has not shutdown yet\"));\n+\t\t\treturn error(\"daemon has not shutdown yet\");\n \t}\n }\n \n-- \ngitgitgadget\n"},{"id":"436433","messageId":"xmqqzgs7ckjc.fsf@gitster.g","threadId":"56508","inReplyTo":"93ebf97b-1282-8f39-6894-90e0ea2dc851@jeffhostetler.com","subject":"Re: [PATCH 3/7] simple-ipc: move definition of ipc_active_state outside of ifdef","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-20T15:51:35Z","receivedAt":"2021-09-20T15:51:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> On 9/15/21 5:06 PM, Junio C Hamano wrote:\n>> \"Jeff Hostetler via GitGitGadget\" writes:\n>> \n>>> From: Jeff Hostetler \n>>>\n>>> From: Carlo Marcelo Arenas Belón \n>>>\n>>> Move the declartion of the `enum ipc_active_state` type outside of\n>>> the SUPPORTS_SIMPLE_IPC ifdef.\n>> The second one is not an in-body header since there is already a\n>> blank line that signals the end of in-body headers after the first\n>> one.\n>> This _may_ be a bug in GGG, perhaps?\n>\n> Maybe. I'll make a note to ask @dscho when he gets back from vacation.\n>\n> Credit for the commit should go to Carlo. I just added it to the series\n> with his \"From:\" line and it looks like GGG added an extra one before\n> it.\n\nThanks for a clarification. I'll tweak the authorship when queuing.\n"},{"id":"436859","messageId":"87v92r49mt.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"pull.1040.v2.git.1632152178.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/7] Builtin FSMonitor Part 1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-23T14:33:20Z","receivedAt":"2021-09-23T15:04:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:\n\n> Here is V2 of Part 1 of my Builtin FSMonitor series.\n>\n> Changes since V1 include:\n>\n> * Drop the Trace2 memory leak.\n> * Added a new \"child_ready\" event to Trace2 as an alternative to the\n> \"child_exit\" event for background processes.\n> * Convert the Trace2-related NEEDSWORK items in \"start_bg_command()\" to use\n> the new \"child_ready\" event.\n> * Various minor code and documentation cleanups.\n\nI see 7/7 still has a pattern you included only to make a compiler error\nbetter. I noted in\nhttps://lore.kernel.org/git/87ilyycko3.fsf@evledraar.gmail.com/ that it\nmake the error worse, on at least clang. You didn't note which compiler\nyou were massaging, presumably MSVC.\n\nI think that's a relatively small matter, but it *is* one I know about,\nand there's no reply there, mention of it being unaddressed here or in\nthe commit message.\n\nI haven't gone back & re-read v1 and seen if there's more unaddresed\nfeedback from others, instead I wanted to encourage you to provide such\na summary.\n\nIt really helps when a series is re-rolled to aid review both for\nnewcomers and returning reviewers. I really don't care about \"getting my\nway\" on such a minor thing.\n\nBut it is frustrating to have the state of a re-roll be observably\nindistinguishable from one's E-Mail not having been received, when\nthat's the case you've got to go back and re-read the thread, scour the\nrange-diff, and generalyl do a lot of work that the person doing the\nre-roll has done, but either didn't keep notes, or didn't share them.\n\nPersonally I'm in the habit of \"flagging\" (starring in GMail terms)\nE-Mails with outstanding unaddresed comments I get on my own topics,\nthen when I re-roll them I look at the thread, and \"unwind the stack\" as\nit were by removing flags on E-Mails that I've either addressed via\nupdated commit messages, or added a note to a WIP cover letter.\n\nE.g. here (just an example that includes Taylor, since he reviewed v1\nhere) is a case where Taylor suggested something that I didn't go for,\nbut i'd like to think noting it helped him catch up:\nhttps://lore.kernel.org/git/cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com/\n\nAll the best, just trying to make the reviewer & re-rolling process\nbetter for everyone.\n"},{"id":"436860","messageId":"87r1df497s.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"6b7a058284b93fab52458b12a6aede5e8aed6652.1632152179.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-23T15:03:56Z","receivedAt":"2021-09-23T15:12:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:\n\n> +\tswitch (sbgr) {\n> +\tcase SBGR_READY:\n> +\t\treturn 0;\n> \n> -\t\telse if (pid_seen == pid_child) {\n> -\t\t\t/*\n> -\t\t\t * The new child daemon process shutdown while\n> -\t\t\t * it was starting up, so it is not listening\n> -\t\t\t * on the socket.\n> -\t\t\t *\n> -\t\t\t * Try to ping the socket in the odd chance\n> -\t\t\t * that another daemon started (or was already\n> -\t\t\t * running) while our child was starting.\n> -\t\t\t *\n> -\t\t\t * Again, we don't care who services the socket.\n> -\t\t\t */\n> -\t\t\ts = ipc_get_active_state(cl_args.path);\n> -\t\t\tif (s == IPC_STATE__LISTENING)\n> -\t\t\t\treturn 0;\n> +\tdefault:\n> +\tcase SBGR_ERROR:\n> +\tcase SBGR_CB_ERROR:\n> +\t\treturn error(\"daemon failed to start\");\n\nThere was a discussion on v1 about the \"default\" being redundant here\nand hiding future compiler checks, this is another \"not sure what you\nthought of that\" case (per [1]).\n\nInterestingly in this case if I drop the \"default\" my local gcc\nuncharacteristically complains about a missing \"return\" in this\nfunction, but clang doesn't. I needed to add a BUG() to shut up the\nformer. Maybe I'm wrong, but perhaps it's a sign of some deeper\ntrouble. This is with gcc/clang 10.2.1-6/11.0.1-2.\n\n1. https://lore.kernel.org/git/87v92r49mt.fsf@evledraar.gmail.com/\n\nI played with the diff below on top of this, I can't remember if it was\nnoted already, but the way you declare function ptrs and use them isn't\nthe usual style:\n\n-- >8 --\ndiff --git a/run-command.c b/run-command.c\nindex 76bbef9d96d..5c831545201 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1903,7 +1903,7 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)\n }\n \n enum start_bg_result start_bg_command(struct child_process *cmd,\n-\t\t\t\t start_bg_wait_cb *wait_cb,\n+\t\t\t\t start_bg_wait_cb wait_cb,\n \t\t\t\t void *cb_data,\n \t\t\t\t unsigned int timeout_sec)\n {\ndiff --git a/run-command.h b/run-command.h\nindex 17b1b80c3d7..e8a637d1967 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -527,7 +527,7 @@ enum start_bg_result {\n * Returns 0 if child is \"ready\".\n * Returns -1 on any error and cause start_bg_command() to also error out.\n */\n-typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);\n+typedef int (*start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);\n \n /**\n * Start a command in the background. Wait long enough for the child\n@@ -549,7 +549,7 @@ typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);\n * any instance data that it might require. This may be NULL.\n */\n enum start_bg_result start_bg_command(struct child_process *cmd,\n-\t\t\t\t start_bg_wait_cb *wait_cb,\n+\t\t\t\t start_bg_wait_cb wait_cb,\n \t\t\t\t void *cb_data,\n \t\t\t\t unsigned int timeout_sec);\n \ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex 28365ff85b6..82dc2906a2a 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -275,9 +275,7 @@ static int daemon__run_server(void)\n \treturn ret;\n }\n \n-static start_bg_wait_cb bg_wait_cb;\n-\n-static int bg_wait_cb(const struct child_process *cp, void *cb_data)\n+static int bg_wait_cb(const struct child_process *cp, void *cb_data, int foo)\n {\n \tint s = ipc_get_active_state(cl_args.path);\n \n@@ -319,9 +317,8 @@ static int daemon__start_server(void)\n \tswitch (sbgr) {\n \tcase SBGR_READY:\n \t\treturn 0;\n-\n-\tdefault:\n \tcase SBGR_ERROR:\n+\t\treturn 0;\n \tcase SBGR_CB_ERROR:\n \t\treturn error(\"daemon failed to start\");\n \n@@ -331,6 +328,7 @@ static int daemon__start_server(void)\n \tcase SBGR_DIED:\n \t\treturn error(\"daemon terminated\");\n \t}\n+\tBUG(\"unreachable\");\n }\n \n /*\n\n"},{"id":"436871","messageId":"0ec69aff-40a9-aac1-5fca-08033c967d88@jeffhostetler.com","threadId":"56508","inReplyTo":"87v92r49mt.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/7] Builtin FSMonitor Part 1","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-09-23T17:12:49Z","receivedAt":"2021-09-23T17:13:00Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/23/21 10:33 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:\n> \n>> Here is V2 of Part 1 of my Builtin FSMonitor series.\n>>\n>> Changes since V1 include:\n>>\n>> * Drop the Trace2 memory leak.\n>> * Added a new \"child_ready\" event to Trace2 as an alternative to the\n>> \"child_exit\" event for background processes.\n>> * Convert the Trace2-related NEEDSWORK items in \"start_bg_command()\" to use\n>> the new \"child_ready\" event.\n>> * Various minor code and documentation cleanups.\n> \n> I see 7/7 still has a pattern you included only to make a compiler error\n> better. I noted in\n> https://lore.kernel.org/git/87ilyycko3.fsf@evledraar.gmail.com/ that it\n> make the error worse, on at least clang. You didn't note which compiler\n> you were massaging, presumably MSVC.\n> \n\nI've been holding my tongue for days on this issue and hoping a third\nparty would step in an render an opinion one way or another.\n\nToo me, a forward declaration seemed like no big deal and it does\nhave value as I tried to explain. And frankly, it felt a little bit\nlike bike-shedding and was trying to avoid that again.\n\n\nThe error message I quoted was from Clang v11.0.3. My forward\ndeclaration of the function prior to the actual definition of\nthe function causes the compiler to stop at the function definition\nand complain with a short message saying that the function itself\nis incorrectly defined and doesn't match the typedef that it is\nassociated with.\n\nWhen I use MSVC I get a similar error at the function definition.\n\nWhen I use GCC I get error messages at both the function definition\nand the usage in the call.\n\n\nAdditionally, the forward declaration states that the function is\nassociated with that typedef (something that is otherwise implicit\nand may be hard to discover (more on that in a minute)).\n\nAnd it doesn't require a reference to the function pointer (either\non the right side of an assignment, a vtable initialization, or passing\nit in a function call) to flag the error. We always get the error\nat the point of the definition.\n\n\nThe error message in your example is, I feel, worse than mine.\nIt splats 2 different function signatures -- only one of which has\nthe typedef name -- in a large, poorly wrapped brick of text.\n\nYes, your error message does print corresponding arg in the function\nprototype of \"start_bg_command()\" that doesn't agree with the symbol\nused at the call-site, but that is much later than where the actual\nerror occurred. And if the forward declaration were present, you'd\nalready know that back up at the definition, right.\n\n\nLet's look at this from another point of view.\n\nSuppose for example we have two function prototypes with the same\nsignature. Perhaps they describe groups of functions with different\nsemantics -- the fact that they have the same argument list and return\ntype is just a coincidence.\n\n typedef int(t_fn_1)(int);\n typedef int(t_fn_2)(int);\n\nAnd then declare one or more instances of functions in those groups:\n\n int foo_a(int x) { ... }\n int foo_b(int x) { ... }\n int foo_c(int x) { ... }\n int foo_d(int x) { ... }\n int foo_e(int x) { ... }\n int foo_f(int x) { ... }\n int foo_g(int x) { ... }\n\nWhich of those functions should be associated with \"t_fn_1\" and which\nwith \"t_fn_2\"? Again, they all have the same signature, but different\nsemantics. The author knows when they wrote the code, but it may be\nhard to automatically determine later.\n\nIf I then have a function like start_bg_command() that receives a\nfunction pointer:\n\n int test(..., t_fn_1 *fn, ...) { ... }\n\nIn C -- even with the use of forward function declarations -- the\ncompiler won't complain if you pass test() a pointer of type t_fn_2\n-- as long a t_fn_1 and t_fn_2 have the same signature.\n\nBut it does give the human a chance to catch the error. Of if we\nlater change the function signature in the t_fn_1 typedef, we will\nautomatically get a list of which of those foo_x functions do and\ndo not need to be updated.\n\n\nAnyway, I've soapboxed on this enough. I think it is a worthy\nfeature for the price.\n\n\nJeff\n"},{"id":"436881","messageId":"YUy+sd3X+Hc+pFnz@nand.local","threadId":"56508","inReplyTo":"87v92r49mt.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/7] Builtin FSMonitor Part 1","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-23T17:51:45Z","receivedAt":"2021-09-23T17:51:50Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 23, 2021 at 04:33:20PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> E.g. here (just an example that includes Taylor, since he reviewed v1\n> here) is a case where Taylor suggested something that I didn't go for,\n> but i'd like to think noting it helped him catch up:\n> https://lore.kernel.org/git/cover-v4-0.5-00000000000-20210921T131003Z-avarab@gmail.com/\n\nIt did, but to be honest I would have been totally fine with you\nmentioning the changes you did incorporate into the rerolled version,\nand omitting mention of any insignificant suggestions you decided to\nignore.\n\nIn other words, when I got to the same spot in the rerolled version, I\nwould have either thought \"looks like Ævar didn't take my suggestion,\nOK\" or not have remembered it in the first place. Either way, the point\nwas trivial enough that I didn't bother to pursue it further in that\nthread.\n\nAnd I think that's what is happening here, too. Yes, I find the forward\ndeclaration useless on GCC, though it appears to be helpful on MSVC and\nhurtful on clang. Even if it does produce a strictly worse error message\non clang, do we really care? It may cause some mild inconvenience for a\ndeveloper later on, but I find it highly unlikely that it would allow us\nto ship a bug that wouldn't have been caught one way or another during\ndevelopment.\n\nSo I was a little disappointed to see such a back-and-forth about this\nquite trivial point. I realize that I'm piling on here by adding my\ntwo-cents, but I think it's worth it to ask ourselves more often what\npoints we're willing to concede and which are worth advocating more\nstrongly for.\n\nThanks,\nTaylor\n"},{"id":"436882","messageId":"2df757dc-bb19-555f-767e-6a4de9852bdb@jeffhostetler.com","threadId":"56508","inReplyTo":"87r1df497s.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-09-23T17:58:48Z","receivedAt":"2021-09-23T17:58:55Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/23/21 11:03 AM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:\n> \n>> +\tswitch (sbgr) {\n>> +\tcase SBGR_READY:\n>> +\t\treturn 0;\n>> \n>> -\t\telse if (pid_seen == pid_child) {\n>> -\t\t\t/*\n>> -\t\t\t * The new child daemon process shutdown while\n>> -\t\t\t * it was starting up, so it is not listening\n>> -\t\t\t * on the socket.\n>> -\t\t\t *\n>> -\t\t\t * Try to ping the socket in the odd chance\n>> -\t\t\t * that another daemon started (or was already\n>> -\t\t\t * running) while our child was starting.\n>> -\t\t\t *\n>> -\t\t\t * Again, we don't care who services the socket.\n>> -\t\t\t */\n>> -\t\t\ts = ipc_get_active_state(cl_args.path);\n>> -\t\t\tif (s == IPC_STATE__LISTENING)\n>> -\t\t\t\treturn 0;\n>> +\tdefault:\n>> +\tcase SBGR_ERROR:\n>> +\tcase SBGR_CB_ERROR:\n>> +\t\treturn error(\"daemon failed to start\");\n> \n> There was a discussion on v1 about the \"default\" being redundant here\n> and hiding future compiler checks, this is another \"not sure what you\n> thought of that\" case (per [1]).\n> \n> Interestingly in this case if I drop the \"default\" my local gcc\n> uncharacteristically complains about a missing \"return\" in this\n> function, but clang doesn't. I needed to add a BUG() to shut up the\n> former. Maybe I'm wrong, but perhaps it's a sign of some deeper\n> trouble. This is with gcc/clang 10.2.1-6/11.0.1-2.\n\nThe issue of whether C needs a \"default\" case in switch statements\non an enum is one I didn't have bandwidth to think about (and is\ncompletely independent of my series).\n\nI was thinking that as a later task, someone could investigate which\ncompilers do and do not generate errors for missing enum values in\nthe switch. Perhaps that leads to a macro in config.mak.uname on\na system-by-system basis that \"does the right thing\".\n\nThen one could have something like:\n\n switch (e) {\n DEFAULT_HANDLER;\n case e1: ...\n case e2: ...\n }\n\n\n> \n> 1. https://lore.kernel.org/git/87v92r49mt.fsf@evledraar.gmail.com/\n> \n> I played with the diff below on top of this, I can't remember if it was\n> noted already, but the way you declare function ptrs and use them isn't\n> the usual style:\n> \n> -- >8 --\n> diff --git a/run-command.c b/run-command.c\n> index 76bbef9d96d..5c831545201 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1903,7 +1903,7 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)\n> }\n> \n> enum start_bg_result start_bg_command(struct child_process *cmd,\n> -\t\t\t\t start_bg_wait_cb *wait_cb,\n> +\t\t\t\t start_bg_wait_cb wait_cb,\n> \t\t\t\t void *cb_data,\n> \t\t\t\t unsigned int timeout_sec)\n> {\n> diff --git a/run-command.h b/run-command.h\n> index 17b1b80c3d7..e8a637d1967 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -527,7 +527,7 @@ enum start_bg_result {\n> * Returns 0 if child is \"ready\".\n> * Returns -1 on any error and cause start_bg_command() to also error out.\n> */\n> -typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);\n> +typedef int (*start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);\n\nBy defining the typedef WITHOUT the star, we get a function type.\n\nWe can then use it for forward function declarations.\n\nBut additionally, when declare a function that takes a function\npointer or when we define a vtable of function pointers, they look\nlike pointers.\n\nstart_bg_wait_cb *pfn = my_cb;\n\nint foo(struct child_process *cmd, start_bg_wait_cb *cb);\n\nOr if we look a the target vtable in Trace2. This looks like\na structure of (function) pointers.\n\nstruct tr2_tgt {\n\ttr2_tgt_init_t *pfn_init;\n\ttr2_tgt_term_t *pfn_term;\n\t...\n};\n\nSo I prefer to leave the star out of function typedef and then\nwe can use the typedef in both contexts.\n\nJeff\n"},{"id":"436889","messageId":"xmqqczozyw8i.fsf@gitster.g","threadId":"56508","inReplyTo":"87r1df497s.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-23T18:37:01Z","receivedAt":"2021-09-23T18:37:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> I played with the diff below on top of this, I can't remember if it was\n> noted already, but the way you declare function ptrs and use them isn't\n> the usual style:\n>\n> -- >8 --\n> diff --git a/run-command.c b/run-command.c\n> index 76bbef9d96d..5c831545201 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -1903,7 +1903,7 @@ void prepare_other_repo_env(struct strvec *env_array, const char *new_git_dir)\n> }\n> \n> enum start_bg_result start_bg_command(struct child_process *cmd,\n> -\t\t\t\t start_bg_wait_cb *wait_cb,\n> +\t\t\t\t start_bg_wait_cb wait_cb,\n> -typedef int(start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);\n> +typedef int (*start_bg_wait_cb)(const struct child_process *cmd, void *cb_data);\n\nI have no comment on the \"default\" thing, but I agree that the\npreimage does look _unusual_ in our codebase. You cannot even\ndeclare a \"variable\" of that type with the typedef, i.e.\n\n\tstart_bg_wait_cb an_instance_of_that;\n\nIf there is a good reason behind choosing the unusual \"the type of\nthe function is...\", that is OK, but otherwise...\n"},{"id":"436895","messageId":"87ee9f3t0m.fsf@evledraar.gmail.com","threadId":"56508","inReplyTo":"0ec69aff-40a9-aac1-5fca-08033c967d88@jeffhostetler.com","subject":"Re: [PATCH v2 0/7] Builtin FSMonitor Part 1","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-23T20:47:13Z","receivedAt":"2021-09-23T21:02:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Sep 23 2021, Jeff Hostetler wrote:\n\n> On 9/23/21 10:33 AM, Ævar Arnfjörð Bjarmason wrote:\n>> On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:\n>> \n>>> Here is V2 of Part 1 of my Builtin FSMonitor series.\n>>>\n>>> Changes since V1 include:\n>>>\n>>> * Drop the Trace2 memory leak.\n>>> * Added a new \"child_ready\" event to Trace2 as an alternative to the\n>>> \"child_exit\" event for background processes.\n>>> * Convert the Trace2-related NEEDSWORK items in \"start_bg_command()\" to use\n>>> the new \"child_ready\" event.\n>>> * Various minor code and documentation cleanups.\n>> I see 7/7 still has a pattern you included only to make a compiler\n>> error\n>> better. I noted in\n>> https://lore.kernel.org/git/87ilyycko3.fsf@evledraar.gmail.com/ that it\n>> make the error worse, on at least clang. You didn't note which compiler\n>> you were massaging, presumably MSVC.\n>> \n>\n> I've been holding my tongue for days on this issue and hoping a third\n> party would step in an render an opinion one way or another.\n>\n> Too me, a forward declaration seemed like no big deal and it does\n> have value as I tried to explain. And frankly, it felt a little bit\n> like bike-shedding and was trying to avoid that again.\n\nI agree with you that it's no big deal in the end,\n\nI thought I made it clear in <87v92r49mt.fsf@evledraar.gmail.com> but\nthe main thing I'm commenting on is not that I or anyone else suggested\nY over X, and you said nah and went for X in the end.\n\nThat's fine, I mean, depending on the comment/issue etc. it's something\nother reviewers & Junio can draw their own conclusions about.\n\nWhat I am saying that it's much better for review of iterations of\npatches in general, and especially of a complex multi-part series if\nreviewers don't have to read the cover letter of vX and wonder what's\nomitted/unaddressed in the V(X-1) comments, and then go and re-read the\ndiscussion themselves. It's not the \"nah\", but that the \"nah\" is\nimplicit and only apparent when sending an E-Mail like this.\n\nOf course that's never perfect, you can't summarize every point\netc. Personally I try to do this, but I've sometimes noticed after the\nfact that I've gotten it wrong etc.\n\nIn the end I and I think anyone else offering their time to review\nthings is trying to move the relevant topic forward in one way or\nanother. I'd much rather spend my time on a vX discussing new things &\ngetting the thing closer to merge-able state, than re-reading all of\nv(X-1) & effectively coming up with my own cover letter summary in my\nhead or in my own notes as I read along.\n\nAnyway, sorry about the bikeshedding getting out of hand, and what seems\nto have been at least partially a misunderstanding in the last couple of\nE-Mails between us, but the above is all I was going for.\n\n> The error message I quoted was from Clang v11.0.3. My forward\n> declaration of the function prior to the actual definition of\n> the function causes the compiler to stop at the function definition\n> and complain with a short message saying that the function itself\n> is incorrectly defined and doesn't match the typedef that it is\n> associated with.\n>\n> When I use MSVC I get a similar error at the function definition.\n>\n> When I use GCC I get error messages at both the function definition\n> and the usage in the call.\n>\n>\n> Additionally, the forward declaration states that the function is\n> associated with that typedef (something that is otherwise implicit\n> and may be hard to discover (more on that in a minute)).\n>\n> And it doesn't require a reference to the function pointer (either\n> on the right side of an assignment, a vtable initialization, or passing\n> it in a function call) to flag the error. We always get the error\n> at the point of the definition.\n>\n>\n> The error message in your example is, I feel, worse than mine.\n> It splats 2 different function signatures -- only one of which has\n> the typedef name -- in a large, poorly wrapped brick of text.\n\nFor what it's worth any poor wrapping is my fault, I have a relatively\nwide terminal and re-wrapped this when composing the E-Mail. I think\nboth GCC & Clang (and most other mature compilers) would give the person\ngetting the error sane wrapping based on their $COLUMNS.\n\n> Yes, your error message does print corresponding arg in the function\n> prototype of \"start_bg_command()\" that doesn't agree with the symbol\n> used at the call-site, but that is much later than where the actual\n> error occurred. And if the forward declaration were present, you'd\n> already know that back up at the definition, right.\n>\n>\n> Let's look at this from another point of view.\n>\n> Suppose for example we have two function prototypes with the same\n> signature. Perhaps they describe groups of functions with different\n> semantics -- the fact that they have the same argument list and return\n> type is just a coincidence.\n>\n> typedef int(t_fn_1)(int);\n> typedef int(t_fn_2)(int);\n>\n> And then declare one or more instances of functions in those groups:\n>\n> int foo_a(int x) { ... }\n> int foo_b(int x) { ... }\n> int foo_c(int x) { ... }\n> int foo_d(int x) { ... }\n> int foo_e(int x) { ... }\n> int foo_f(int x) { ... }\n> int foo_g(int x) { ... }\n>\n> Which of those functions should be associated with \"t_fn_1\" and which\n> with \"t_fn_2\"? Again, they all have the same signature, but different\n> semantics. The author knows when they wrote the code, but it may be\n> hard to automatically determine later.\n>\n> If I then have a function like start_bg_command() that receives a\n> function pointer:\n>\n> int test(..., t_fn_1 *fn, ...) { ... }\n>\n> In C -- even with the use of forward function declarations -- the\n> compiler won't complain if you pass test() a pointer of type t_fn_2\n> -- as long a t_fn_1 and t_fn_2 have the same signature.\n>\n> But it does give the human a chance to catch the error. Of if we\n> later change the function signature in the t_fn_1 typedef, we will\n> automatically get a list of which of those foo_x functions do and\n> do not need to be updated.\n>\n>\n> Anyway, I've soapboxed on this enough. I think it is a worthy\n> feature for the price.\n\nCode in git.git generally just declares say an \"int foo(int)\" and leaves\nit at passing the \"foo\", we're not concerned about that \"foo\" just so\nhappening to be passed to some other interface that takes the same\nsignature, certainly not something within a <1k line t/helper/* file.\n\nWe all have habits we've picked up from other codebases prior to working\non git.git. I'm not arguing that what you're describing is worse in some\nabtract sense, but that there's a larger value in following conventions\nwithin the codebase as Junio noted in his reply.\n"},{"id":"437147","messageId":"8de10d97-f5a0-b08a-ca36-a86b4280db94@jeffhostetler.com","threadId":"56508","inReplyTo":"87ee9f3t0m.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 0/7] Builtin FSMonitor Part 1","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-09-27T13:37:22Z","receivedAt":"2021-09-27T13:37:25Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/23/21 4:47 PM, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Thu, Sep 23 2021, Jeff Hostetler wrote:\n> \n>> On 9/23/21 10:33 AM, Ævar Arnfjörð Bjarmason wrote:\n>>> On Mon, Sep 20 2021, Jeff Hostetler via GitGitGadget wrote:\n>>>\n>>>> Here is V2 of Part 1 of my Builtin FSMonitor series.\n>>>>\n>>>> Changes since V1 include:\n>>>>\n>>>> * Drop the Trace2 memory leak.\n>>>> * Added a new \"child_ready\" event to Trace2 as an alternative to the\n>>>> \"child_exit\" event for background processes.\n>>>> * Convert the Trace2-related NEEDSWORK items in \"start_bg_command()\" to use\n>>>> the new \"child_ready\" event.\n>>>> * Various minor code and documentation cleanups.\n>>> I see 7/7 still has a pattern you included only to make a compiler\n>>> error\n>>> better. I noted in\n>>> https://lore.kernel.org/git/87ilyycko3.fsf@evledraar.gmail.com/ that it\n>>> make the error worse, on at least clang. You didn't note which compiler\n>>> you were massaging, presumably MSVC.\n>>>\n>>\n>> I've been holding my tongue for days on this issue and hoping a third\n>> party would step in an render an opinion one way or another.\n>>\n>> Too me, a forward declaration seemed like no big deal and it does\n>> have value as I tried to explain. And frankly, it felt a little bit\n>> like bike-shedding and was trying to avoid that again.\n> \n> I agree with you that it's no big deal in the end,\n> \n> I thought I made it clear in <87v92r49mt.fsf@evledraar.gmail.com> but\n> the main thing I'm commenting on is not that I or anyone else suggested\n> Y over X, and you said nah and went for X in the end.\n> \n> That's fine, I mean, depending on the comment/issue etc. it's something\n> other reviewers & Junio can draw their own conclusions about.\n> \n> What I am saying that it's much better for review of iterations of\n> patches in general, and especially of a complex multi-part series if\n> reviewers don't have to read the cover letter of vX and wonder what's\n> omitted/unaddressed in the V(X-1) comments, and then go and re-read the\n> discussion themselves. It's not the \"nah\", but that the \"nah\" is\n> implicit and only apparent when sending an E-Mail like this.\n> \n> Of course that's never perfect, you can't summarize every point\n> etc. Personally I try to do this, but I've sometimes noticed after the\n> fact that I've gotten it wrong etc.\n> \n> In the end I and I think anyone else offering their time to review\n> things is trying to move the relevant topic forward in one way or\n> another. I'd much rather spend my time on a vX discussing new things &\n> getting the thing closer to merge-able state, than re-reading all of\n> v(X-1) & effectively coming up with my own cover letter summary in my\n> head or in my own notes as I read along.\n> \n> Anyway, sorry about the bikeshedding getting out of hand, and what seems\n> to have been at least partially a misunderstanding in the last couple of\n> E-Mails between us, but the above is all I was going for.\n\nThanks. Yeah, email is a terrible communication medium and prone\nto misunderstandings. It's easy to forget that at times, since we\nspend so much time in it.\n\nDrafting a v(X+1) cover letter is a bit of an art. It is easy to\nerr on the less-is-better side when trying to decide how much to\ninclude to explain the new version vs not wanting to including\nevery little typo or nit.\n\nI tend to drop / cross-off issues that I decide to ignore or not act\nupon rather than report them. However, you're right, I should have\nincluded a brief statement about not changing the stuff mentioned in\n7/7, since there was a larger conversation around it. Sorry.\n\n\nJeff\n"},{"id":"440477","messageId":"20211104194619.GA12886@dinwoodie.org","threadId":"56508","inReplyTo":"6b7a058284b93fab52458b12a6aede5e8aed6652.1632152179.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-11-04T19:46:19Z","receivedAt":"2021-11-04T19:46:35Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Monday 20 September 2021 at 03:36 pm +0000, Jeff Hostetler via GitGitGadget wrote:\n> From: Jeff Hostetler \n> \n> Convert test helper to use `start_bg_command()` when spawning a server\n> daemon in the background rather than blocks of platform-specific code.\n> \n> Also, while here, remove _() translation around error messages since\n> this is a test helper and not Git code.\n\nAs part of testing the v2.34.0-rc0 release candidate on Cygwin, I've\nfound this commit -- 05881a6fc9 (t/helper/simple-ipc: convert\ntest-simple-ipc to use start_bg_command, 2021-09-20), according to my\nbisecting -- is causing t0052.1 to fail on 32-bit Cygwin. Somehow this\nis only affecting the 32-bit Cygwin build; the 64-bit build is working\nas expected.\n\nSpecifically, the failure I'm seeing is as below:\n\n```\n$ GIT_TRACE=1 sh t0052-simple-ipc.sh -vix\ntrace: built-in: git init '/cygdrive/d/git-32/t/trash directory.t0052-simple-ipc'\nInitialized empty Git repository in /cygdrive/d/git-32/t/trash directory.t0052-simple-ipc/.git/\nexpecting success of 0052.1 'start simple command server': \n\ttest_atexit stop_simple_IPC_server &&\n\ttest-tool simple-ipc start-daemon --threads=8 &&\n\ttest-tool simple-ipc is-active\n\n++ test_atexit stop_simple_IPC_server\n++ test 0 = 0\n++ test_atexit_cleanup='{ stop_simple_IPC_server\n\t\t} && (exit \"$eval_ret\"); eval_ret=$?; :'\n++ test-tool simple-ipc start-daemon --threads=8\ntrace: run_command: test-tool simple-ipc run-daemon --name=ipc-test --threads=8\nerror: daemon failed to start\nerror: last command exited with $?=1\nnot ok 1 - start simple command server\n#\t\n#\t\ttest_atexit stop_simple_IPC_server &&\n#\t\ttest-tool simple-ipc start-daemon --threads=8 &&\n#\t\ttest-tool simple-ipc is-active\n#\t\n++ stop_simple_IPC_server\n++ test-tool simple-ipc stop-daemon\n++ exit 1\n++ eval_ret=1\n++ :\n```\n\nI've had a look at the code changes, and cannot work out what might be\nbeing handled differently in 32-bit and 64-bit Cygwin environments.\nGiven the Cygwin project is considering dropping support for 32-bit\nCygwin anyway, it might not be worth doing anything about this. But I\nthought it worth reporting in case there's something obvious to folk\nmore familiar with this code.\n"},{"id":"440482","messageId":"079bdbee-e1fe-83e0-f986-c5795ca20848@ramsayjones.plus.com","threadId":"56508","inReplyTo":"20211104194619.GA12886@dinwoodie.org","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2021-11-04T20:14:07Z","receivedAt":"2021-11-04T20:14:14Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Adam,\n\nOn 04/11/2021 19:46, Adam Dinwoodie wrote:\n> On Monday 20 September 2021 at 03:36 pm +0000, Jeff Hostetler via GitGitGadget wrote:\n>> From: Jeff Hostetler \n>>\n>> Convert test helper to use `start_bg_command()` when spawning a server\n>> daemon in the background rather than blocks of platform-specific code.\n>>\n>> Also, while here, remove _() translation around error messages since\n>> this is a test helper and not Git code.\n> \n> As part of testing the v2.34.0-rc0 release candidate on Cygwin, I've\n> found this commit -- 05881a6fc9 (t/helper/simple-ipc: convert\n> test-simple-ipc to use start_bg_command, 2021-09-20), according to my\n> bisecting -- is causing t0052.1 to fail on 32-bit Cygwin. Somehow this\n> is only affecting the 32-bit Cygwin build; the 64-bit build is working\n> as expected.\n\nHmmm, I am seeing exactly the same, but on 64-bit cygwin!\n\nI haven't found time to look at this in detail yet (except for\nwhat you have already done). Unfortunately, about an hour ago,\nI did a 'make test' for the '-rc1' build, so I won't be able to\ntake a look for hours yet, ... :(\n\nATB,\nRamsay Jones\n"},{"id":"440665","messageId":"2af70f2a-e74c-15a8-6340-e18bf265598c@jeffhostetler.com","threadId":"56508","inReplyTo":"20211104194619.GA12886@dinwoodie.org","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2021-11-08T14:58:00Z","receivedAt":"2021-11-08T14:58:03Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 11/4/21 3:46 PM, Adam Dinwoodie wrote:\n> On Monday 20 September 2021 at 03:36 pm +0000, Jeff Hostetler via GitGitGadget wrote:\n>> From: Jeff Hostetler \n>>\n>> Convert test helper to use `start_bg_command()` when spawning a server\n>> daemon in the background rather than blocks of platform-specific code.\n>>\n>> Also, while here, remove _() translation around error messages since\n>> this is a test helper and not Git code.\n> \n> As part of testing the v2.34.0-rc0 release candidate on Cygwin, I've\n> found this commit -- 05881a6fc9 (t/helper/simple-ipc: convert\n> test-simple-ipc to use start_bg_command, 2021-09-20), according to my\n> bisecting -- is causing t0052.1 to fail on 32-bit Cygwin. Somehow this\n> is only affecting the 32-bit Cygwin build; the 64-bit build is working\n> as expected.\n> \n> Specifically, the failure I'm seeing is as below:\n> \n> ```\n> $ GIT_TRACE=1 sh t0052-simple-ipc.sh -vix\n> trace: built-in: git init '/cygdrive/d/git-32/t/trash directory.t0052-simple-ipc'\n> Initialized empty Git repository in /cygdrive/d/git-32/t/trash directory.t0052-simple-ipc/.git/\n> expecting success of 0052.1 'start simple command server':\n> \ttest_atexit stop_simple_IPC_server &&\n> \ttest-tool simple-ipc start-daemon --threads=8 &&\n> \ttest-tool simple-ipc is-active\n> \n> ++ test_atexit stop_simple_IPC_server\n> ++ test 0 = 0\n> ++ test_atexit_cleanup='{ stop_simple_IPC_server\n> \t\t} && (exit \"$eval_ret\"); eval_ret=$?; :'\n> ++ test-tool simple-ipc start-daemon --threads=8\n> trace: run_command: test-tool simple-ipc run-daemon --name=ipc-test --threads=8\n> error: daemon failed to start\n> error: last command exited with $?=1\n> not ok 1 - start simple command server\n> #\t\n> #\t\ttest_atexit stop_simple_IPC_server &&\n> #\t\ttest-tool simple-ipc start-daemon --threads=8 &&\n> #\t\ttest-tool simple-ipc is-active\n> #\t\n> ++ stop_simple_IPC_server\n> ++ test-tool simple-ipc stop-daemon\n> ++ exit 1\n> ++ eval_ret=1\n> ++ :\n> ```\n> \n> I've had a look at the code changes, and cannot work out what might be\n> being handled differently in 32-bit and 64-bit Cygwin environments.\n> Given the Cygwin project is considering dropping support for 32-bit\n> Cygwin anyway, it might not be worth doing anything about this. But I\n> thought it worth reporting in case there's something obvious to folk\n> more familiar with this code.\n> \n\nHow odd! Thanks for the report. I'll investigate.\nJeff\n"},{"id":"440706","messageId":"nycvar.QRO.7.76.6.2111090051530.54@tvgsbejvaqbjf.bet","threadId":"56508","inReplyTo":"20211104194619.GA12886@dinwoodie.org","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-08T23:59:34Z","receivedAt":"2021-11-08T23:59:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Adam,\n\nOn Thu, 4 Nov 2021, Adam Dinwoodie wrote:\n\n> On Monday 20 September 2021 at 03:36 pm +0000, Jeff Hostetler via GitGitGadget wrote:\n> > From: Jeff Hostetler \n> >\n> > Convert test helper to use `start_bg_command()` when spawning a server\n> > daemon in the background rather than blocks of platform-specific code.\n> >\n> > Also, while here, remove _() translation around error messages since\n> > this is a test helper and not Git code.\n>\n> As part of testing the v2.34.0-rc0 release candidate on Cygwin, I've\n> found this commit -- 05881a6fc9 (t/helper/simple-ipc: convert\n> test-simple-ipc to use start_bg_command, 2021-09-20), according to my\n> bisecting -- is causing t0052.1 to fail on 32-bit Cygwin. Somehow this\n> is only affecting the 32-bit Cygwin build; the 64-bit build is working\n> as expected.\n>\n> Specifically, the failure I'm seeing is as below:\n>\n> ```\n> $ GIT_TRACE=1 sh t0052-simple-ipc.sh -vix\n> trace: built-in: git init '/cygdrive/d/git-32/t/trash directory.t0052-simple-ipc'\n> Initialized empty Git repository in /cygdrive/d/git-32/t/trash directory.t0052-simple-ipc/.git/\n> expecting success of 0052.1 'start simple command server':\n> \ttest_atexit stop_simple_IPC_server &&\n> \ttest-tool simple-ipc start-daemon --threads=8 &&\n> \ttest-tool simple-ipc is-active\n>\n> ++ test_atexit stop_simple_IPC_server\n> ++ test 0 = 0\n> ++ test_atexit_cleanup='{ stop_simple_IPC_server\n> \t\t} && (exit \"$eval_ret\"); eval_ret=$?; :'\n> ++ test-tool simple-ipc start-daemon --threads=8\n> trace: run_command: test-tool simple-ipc run-daemon --name=ipc-test --threads=8\n> error: daemon failed to start\n> error: last command exited with $?=1\n> not ok 1 - start simple command server\n> #\n> #\t\ttest_atexit stop_simple_IPC_server &&\n> #\t\ttest-tool simple-ipc start-daemon --threads=8 &&\n> #\t\ttest-tool simple-ipc is-active\n> #\n> ++ stop_simple_IPC_server\n> ++ test-tool simple-ipc stop-daemon\n> ++ exit 1\n> ++ eval_ret=1\n> ++ :\n> ```\n>\n> I've had a look at the code changes, and cannot work out what might be\n> being handled differently in 32-bit and 64-bit Cygwin environments.\n> Given the Cygwin project is considering dropping support for 32-bit\n> Cygwin anyway, it might not be worth doing anything about this. But I\n> thought it worth reporting in case there's something obvious to folk\n> more familiar with this code.\n\nI had a look at this and could reproduce... partially. I only managed to\nmake it fail every once in a while.\n\nDigging deeper, it turns out that the `lstat()` call in\n`ipc_get_active_state()` does not receive an `st_mode` indicating a\nsocket, but rather a file (in my tests, it was usually 0100644, but\nsometimes even 0100755).\n\nThe reason is, of course, that Cygwin _emulates_ Unix sockets. What is in\nthe file system is just a special file, it is marked with the `system` bit\n(which only exists on Windows), and its contents start with the tell-tale\n`!`.\n\nAnd as you might have guessed, there is a race going on between Cygwin\nwriting that file _and_ flipping that `system` bit, and Git trying to\naccess the Unix socket and encountering an unexpected file.\n\nNow, why this only happens in your 32-bit setup, I have no idea.\n\nIn my tests, the following patch works around the issue. Could I ask you\nto test it in your environment?\n\n-- snip --\ndiff --git a/compat/simple-ipc/ipc-unix-socket.c\nb/compat/simple-ipc/ipc-unix-socket.c\nindex 4e28857a0a..1c591b2adf 100644\n--- a/compat/simple-ipc/ipc-unix-socket.c\n+++ b/compat/simple-ipc/ipc-unix-socket.c\n@@ -36,6 +36,23 @@ enum ipc_active_state ipc_get_active_state(const char\n*path)\n \t}\n\n \t/* also complain if a plain file is in the way */\n+#ifdef __CYGWIN__\n+\t{\n+\t\tstatic const int delay[] = { 1, 10, 20, 40, -1 };\n+\t\tint i;\n+\n+\t\tfor (i = 0; S_ISREG(st.st_mode) && delay[i] > 0; i++) {\n+\t\t\t/*\n+\t\t\t * Cygwin might still be in the process of marking the\n+\t\t\t * underlying file as a system file.\n+\t\t\t */\n+\t\t\tsleep_millisec(delay[i]);\n+\t\t\tif (lstat(path, &st) == -1)\n+\t\t\t\treturn IPC_STATE__INVALID_PATH;\n+\t\t}\n+\t}\n+#endif\n+\n \tif ((st.st_mode & S_IFMT) != S_IFSOCK)\n \t\treturn IPC_STATE__INVALID_PATH;\n\n-- snap --\n\nFWIW it looks as if the loop might be a bit of an overkill, as I could not\nget the code to need more than a single one-millisecond sleep, but it's\nprobably safer to just keep the delay loop in place as-is.\n\nCiao,\nDscho\n"},{"id":"440760","messageId":"dca03da9-5684-10c7-2315-2d10affd4f0a@ramsayjones.plus.com","threadId":"56508","inReplyTo":"nycvar.QRO.7.76.6.2111090051530.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2021-11-09T18:53:39Z","receivedAt":"2021-11-09T18:53:55Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 08/11/2021 23:59, Johannes Schindelin wrote:\n[snip]\n> I had a look at this and could reproduce... partially. I only managed to\n> make it fail every once in a while.\n> \n> Digging deeper, it turns out that the `lstat()` call in\n> `ipc_get_active_state()` does not receive an `st_mode` indicating a\n> socket, but rather a file (in my tests, it was usually 0100644, but\n> sometimes even 0100755).\n> \n> The reason is, of course, that Cygwin _emulates_ Unix sockets. What is in\n> the file system is just a special file, it is marked with the `system` bit\n> (which only exists on Windows), and its contents start with the tell-tale\n> `!`.\n> \n> And as you might have guessed, there is a race going on between Cygwin\n> writing that file _and_ flipping that `system` bit, and Git trying to\n> access the Unix socket and encountering an unexpected file.\n> \n> Now, why this only happens in your 32-bit setup, I have no idea.\n> \n> In my tests, the following patch works around the issue. Could I ask you\n> to test it in your environment?\n\nJust FYI, I just tried the patch below (on 64-bit cygwin) and this test\nnow works fine for me. (well, run 5 times by hand - not with --stress).\n\nThis is on windows 10 21H1 and cygwin:\n\n $ uname -a\n CYGWIN_NT-10.0 satellite 3.3.2(0.341/5/3) 2021-11-08 16:55 x86_64 Cygwin\n $ \n\n[Yes, I updated last night!]\n\nATB,\nRamsay Jones\n\n> -- snip --\n> diff --git a/compat/simple-ipc/ipc-unix-socket.c\n> b/compat/simple-ipc/ipc-unix-socket.c\n> index 4e28857a0a..1c591b2adf 100644\n> --- a/compat/simple-ipc/ipc-unix-socket.c\n> +++ b/compat/simple-ipc/ipc-unix-socket.c\n> @@ -36,6 +36,23 @@ enum ipc_active_state ipc_get_active_state(const char\n> *path)\n> \t}\n> \n> \t/* also complain if a plain file is in the way */\n> +#ifdef __CYGWIN__\n> +\t{\n> +\t\tstatic const int delay[] = { 1, 10, 20, 40, -1 };\n> +\t\tint i;\n> +\n> +\t\tfor (i = 0; S_ISREG(st.st_mode) && delay[i] > 0; i++) {\n> +\t\t\t/*\n> +\t\t\t * Cygwin might still be in the process of marking the\n> +\t\t\t * underlying file as a system file.\n> +\t\t\t */\n> +\t\t\tsleep_millisec(delay[i]);\n> +\t\t\tif (lstat(path, &st) == -1)\n> +\t\t\t\treturn IPC_STATE__INVALID_PATH;\n> +\t\t}\n> +\t}\n> +#endif\n> +\n> \tif ((st.st_mode & S_IFMT) != S_IFSOCK)\n> \t\treturn IPC_STATE__INVALID_PATH;\n> \n> -- snap --\n> \n> FWIW it looks as if the loop might be a bit of an overkill, as I could not\n> get the code to need more than a single one-millisecond sleep, but it's\n> probably safer to just keep the delay loop in place as-is.\n> \n> Ciao,\n> Dscho\n> \n"},{"id":"440774","messageId":"nycvar.QRO.7.76.6.2111092358240.54@tvgsbejvaqbjf.bet","threadId":"56508","inReplyTo":"dca03da9-5684-10c7-2315-2d10affd4f0a@ramsayjones.plus.com","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-09T23:01:24Z","receivedAt":"2021-11-09T23:01:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Ramsay,\n\nOn Tue, 9 Nov 2021, Ramsay Jones wrote:\n\n> On 08/11/2021 23:59, Johannes Schindelin wrote:\n> [snip]\n> > I had a look at this and could reproduce... partially. I only managed to\n> > make it fail every once in a while.\n> >\n> > Digging deeper, it turns out that the `lstat()` call in\n> > `ipc_get_active_state()` does not receive an `st_mode` indicating a\n> > socket, but rather a file (in my tests, it was usually 0100644, but\n> > sometimes even 0100755).\n> >\n> > The reason is, of course, that Cygwin _emulates_ Unix sockets. What is in\n> > the file system is just a special file, it is marked with the `system` bit\n> > (which only exists on Windows), and its contents start with the tell-tale\n> > `!`.\n> >\n> > And as you might have guessed, there is a race going on between Cygwin\n> > writing that file _and_ flipping that `system` bit, and Git trying to\n> > access the Unix socket and encountering an unexpected file.\n> >\n> > Now, why this only happens in your 32-bit setup, I have no idea.\n> >\n> > In my tests, the following patch works around the issue. Could I ask you\n> > to test it in your environment?\n>\n> Just FYI, I just tried the patch below (on 64-bit cygwin) and this test\n> now works fine for me. (well, run 5 times by hand - not with --stress).\n\nVery good!\n\nI fear that it is a bit late in the -rc cycle to try to get this into the\nofficial v2.34.0. Adam, since you are the maintainer of the Cygwin git\npackage, would you mind incorporating this patch into Cygwin's version of\nGit?\n\n> This is on windows 10 21H1 and cygwin:\n>\n> $ uname -a\n> CYGWIN_NT-10.0 satellite 3.3.2(0.341/5/3) 2021-11-08 16:55 x86_64 Cygwin\n> $\n>\n> [Yes, I updated last night!]\n\nGood thing, too: v3.3.2 fixes a critical bug in the pipe code. One symptom\nwas that you could not use Git Credential Manager Core as credential\nhelper (because Git thought that the helper had hung up, well before the\nhelper sent any information).\n\nCiao,\nDscho\n\n>\n> ATB,\n> Ramsay Jones\n>\n> > -- snip --\n> > diff --git a/compat/simple-ipc/ipc-unix-socket.c\n> > b/compat/simple-ipc/ipc-unix-socket.c\n> > index 4e28857a0a..1c591b2adf 100644\n> > --- a/compat/simple-ipc/ipc-unix-socket.c\n> > +++ b/compat/simple-ipc/ipc-unix-socket.c\n> > @@ -36,6 +36,23 @@ enum ipc_active_state ipc_get_active_state(const char\n> > *path)\n> > \t}\n> >\n> > \t/* also complain if a plain file is in the way */\n> > +#ifdef __CYGWIN__\n> > +\t{\n> > +\t\tstatic const int delay[] = { 1, 10, 20, 40, -1 };\n> > +\t\tint i;\n> > +\n> > +\t\tfor (i = 0; S_ISREG(st.st_mode) && delay[i] > 0; i++) {\n> > +\t\t\t/*\n> > +\t\t\t * Cygwin might still be in the process of marking the\n> > +\t\t\t * underlying file as a system file.\n> > +\t\t\t */\n> > +\t\t\tsleep_millisec(delay[i]);\n> > +\t\t\tif (lstat(path, &st) == -1)\n> > +\t\t\t\treturn IPC_STATE__INVALID_PATH;\n> > +\t\t}\n> > +\t}\n> > +#endif\n> > +\n> > \tif ((st.st_mode & S_IFMT) != S_IFSOCK)\n> > \t\treturn IPC_STATE__INVALID_PATH;\n> >\n> > -- snap --\n> >\n> > FWIW it looks as if the loop might be a bit of an overkill, as I could not\n> > get the code to need more than a single one-millisecond sleep, but it's\n> > probably safer to just keep the delay loop in place as-is.\n> >\n> > Ciao,\n> > Dscho\n> >\n>\n>\n"},{"id":"440786","messageId":"xmqqee7ozyx4.fsf@gitster.g","threadId":"56508","inReplyTo":"nycvar.QRO.7.76.6.2111092358240.54@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-09T23:34:15Z","receivedAt":"2021-11-09T23:34:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> I fear that it is a bit late in the -rc cycle to try to get this into the\n> official v2.34.0. Adam, since you are the maintainer of the Cygwin git\n> package, would you mind incorporating this patch into Cygwin's version of\n> Git?\n\nI do not mind taking a Cygwin-only #ifdef block in compat/ like we\nsee below from folks who have stake in Cygwin, and who are clearly\nleading Cygwin users on the list, like Ramsay and Adam are, even\nafter I tag -rc2.\n\nI cannot give the change any better test than they can, and it is\ntheir platform to improve, or break by accident while trying to do\nso.\n\nThanks.\n"},{"id":"440834","messageId":"nycvar.QRO.7.76.6.2111101324020.21127@tvgsbejvaqbjf.bet","threadId":"56508","inReplyTo":"xmqqee7ozyx4.fsf@gitster.g","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-10T12:27:55Z","receivedAt":"2021-11-10T12:28:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Tue, 9 Nov 2021, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n>\n> > I fear that it is a bit late in the -rc cycle to try to get this into the\n> > official v2.34.0. Adam, since you are the maintainer of the Cygwin git\n> > package, would you mind incorporating this patch into Cygwin's version of\n> > Git?\n>\n> I do not mind taking a Cygwin-only #ifdef block in compat/ like we\n> see below from folks who have stake in Cygwin, and who are clearly\n> leading Cygwin users on the list, like Ramsay and Adam are, even\n> after I tag -rc2.\n\nThank you for your encouragement, I contributed it here:\nhttps://lore.kernel.org/git/pull.1074.git.1636542550889.gitgitgadget@gmail.com\n\n> I cannot give the change any better test than they can, and it is\n> their platform to improve, or break by accident while trying to do\n> so.\n\nRight. I tested this as well as I could, via the `--stress` option, and am\nfairly confident that it is correct. Since the patch touches only\n`simply-ipc` code, the only test that could possibly affected is t0052,\nand it passes with `--stress` over here (when it failed without the\npatch).\n\nCiao,\nDscho\n\nP.S.: in case you wondered, no, I did not run the entire test suite. With\nthe performance characteristics of the POSIX emulation provided by the\nCygwin runtime, this would simply take too long. It's not the first time I\nwish our test suite was more efficient, across _all_ supported platforms.\n"},{"id":"440973","messageId":"CA+kUOanh1m=dkE-gDikg53zOPXx_7v65ggqxPspuZWDbdOrR=g@mail.gmail.com","threadId":"56508","inReplyTo":"nycvar.QRO.7.76.6.2111101324020.21127@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-11-12T08:56:42Z","receivedAt":"2021-11-12T08:57:22Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Wed, 10 Nov 2021 at 12:28, Johannes Schindelin\n wrote:\n>\n> Hi Junio,\n>\n> On Tue, 9 Nov 2021, Junio C Hamano wrote:\n>\n> > Johannes Schindelin writes:\n> >\n> > > I fear that it is a bit late in the -rc cycle to try to get this into the\n> > > official v2.34.0. Adam, since you are the maintainer of the Cygwin git\n> > > package, would you mind incorporating this patch into Cygwin's version of\n> > > Git?\n> >\n> > I do not mind taking a Cygwin-only #ifdef block in compat/ like we\n> > see below from folks who have stake in Cygwin, and who are clearly\n> > leading Cygwin users on the list, like Ramsay and Adam are, even\n> > after I tag -rc2.\n>\n> Thank you for your encouragement, I contributed it here:\n> https://lore.kernel.org/git/pull.1074.git.1636542550889.gitgitgadget@gmail.com\n>\n> > I cannot give the change any better test than they can, and it is\n> > their platform to improve, or break by accident while trying to do\n> > so.\n>\n> Right. I tested this as well as I could, via the `--stress` option, and am\n> fairly confident that it is correct. Since the patch touches only\n> `simply-ipc` code, the only test that could possibly affected is t0052,\n> and it passes with `--stress` over here (when it failed without the\n> patch).\n>\n> Ciao,\n> Dscho\n>\n> P.S.: in case you wondered, no, I did not run the entire test suite. With\n> the performance characteristics of the POSIX emulation provided by the\n> Cygwin runtime, this would simply take too long. It's not the first time I\n> wish our test suite was more efficient, across _all_ supported platforms.\n\nI have just run the complete test suite on rc2, both with and without\nthis patch, and I can confirm it resolves this problem and doesn't\ncause any other new test failures.\n\nBut yes, the (lack of) speed of running the Git test suite on Cygwin\nis one of the reasons I run the tests on high-spec Azure VMs rather\nthan my own systems. Unfortunately the Cygwin compatibility layer plus\nthe overheads of NTFS mean things are unlikely to get significantly\nquicker any time soon, and between WSL and Git for Windows, I expect\ninterest in improving Cygwin's performance is going to continue to\nwane.\n"},{"id":"440986","messageId":"xmqqtughtlb9.fsf@gitster.g","threadId":"56508","inReplyTo":"CA+kUOanh1m=dkE-gDikg53zOPXx_7v65ggqxPspuZWDbdOrR=g@mail.gmail.com","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-12T16:01:30Z","receivedAt":"2021-11-12T16:01:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie writes:\n\n> I have just run the complete test suite on rc2, both with and without\n> this patch, and I can confirm it resolves this problem and doesn't\n> cause any other new test failures.\n\nThanks for a good news.\n\n> But yes, the (lack of) speed of running the Git test suite on Cygwin\n> is one of the reasons I run the tests on high-spec Azure VMs rather\n> than my own systems. Unfortunately the Cygwin compatibility layer plus\n> the overheads of NTFS mean things are unlikely to get significantly\n> quicker any time soon, and between WSL and Git for Windows, I expect\n> interest in improving Cygwin's performance is going to continue to\n> wane.\n\nOut of curiosity, are the use cases and user base of Cygwin waning,\nor are there still viable cases where Cygwin is a more preferred\nsolution over WSL (the question is not limited to use of Git)?\n\n\n"},{"id":"441002","messageId":"CA+kUOa=FBpzy4zNMXY1UZybc+13mwLnXrZRVN5HP+NF3h4PYow@mail.gmail.com","threadId":"56508","inReplyTo":"xmqqtughtlb9.fsf@gitster.g","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-11-12T21:33:53Z","receivedAt":"2021-11-12T21:34:26Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Fri, 12 Nov 2021 at 16:01, Junio C Hamano wrote:\n>\n> Adam Dinwoodie writes:\n> > But yes, the (lack of) speed of running the Git test suite on Cygwin\n> > is one of the reasons I run the tests on high-spec Azure VMs rather\n> > than my own systems. Unfortunately the Cygwin compatibility layer plus\n> > the overheads of NTFS mean things are unlikely to get significantly\n> > quicker any time soon, and between WSL and Git for Windows, I expect\n> > interest in improving Cygwin's performance is going to continue to\n> > wane.\n>\n> Out of curiosity, are the use cases and user base of Cygwin waning,\n> or are there still viable cases where Cygwin is a more preferred\n> solution over WSL (the question is not limited to use of Git)?\n\nNo formal research here, just impressions as someone who has used\nCygwin for a long time and who hangs out on the Cygwin mailing list:\nfor a lot of use cases, WSL is at least as good, if not better, than\nCygwin. There are a few areas where Cygwin is still a better solution,\nthough:\n\n- WSL requires essentially installing an entire operating system. Disk\nspace is relatively cheap, so that's not nearly the obstacle it used\nto be, but it is an obstacle. This is more relevant for people who\nwant to distribute packaged installers to Windows users: most\nnon-technical users won't want to get WSL working, but if you've\nwritten code for *nix and don't want to port it manually to Windows,\nit's relatively straightforward to compile it using Cygwin and bundle\nthe cygwin1.dll file with the installer. That'll mostly get your code\nworking with a user experience that doesn't differ too much from a\nfully native Windows application. (This is essentially what Git for\nWindows is doing, albeit with an increasingly distant Cygwin fork.)\n\n- There are some functions that Cygwin offers that WSL doesn't. The\nkey one for me is the ability to access Windows network file shares,\nwhich WSL doesn't support (or at least didn't last time I checked). I\nexpect some of these gaps will disappear as WSL gets more features,\nbut I expect some of them are fairly fundamental restrictions: Cygwin\napplications can have code specifically to handle the fact that\nthere's a Windows OS there, so they can -- with care -- interact with\nthe Windows OS directly to (say) use Windows file access APIs or the\nWindows clipboard. WSL applications generally don't have that ability;\nif I install something from apt on my Debian WSL installation, it'll\npull exactly the same binary as if I'd installed it on a normal Debian\nsystem. I guess in theory people could write code to detect that\nthey're running in WSL and handle that specially, in the same way that\nit's normally possible to detect and handle when you're running in a\nVM versus running on bare metal. I expect that'll be much less common,\nthough, just as Git has code for handling Cygwin specially but doesn't\nhave code for handling Linux-within-WSL specially, even though both\ncould be used to access a Git repository stored in the same Windows\nNTFS directory.\n\nI expect some folk who historically have used Cygwin will shift over\nto WSL, some will stick with Cygwin, and a small number (as I do) will\nuse both in parallel for slightly different jobs.\n\ntl;dr IMO both is true: WSL is better than Cygwin for some use cases\nso the user base is waning, but Cygwin is still a very viable\npreference over WSL for other use cases.\n"},{"id":"441070","messageId":"02b6cfa8-bd0a-275d-fce0-a3a9f316ded3@ramsayjones.plus.com","threadId":"56508","inReplyTo":"nycvar.QRO.7.76.6.2111101324020.21127@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2021-11-13T19:11:27Z","receivedAt":"2021-11-13T19:12:53Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 10/11/2021 12:27, Johannes Schindelin wrote:\n[snip]\n>> I cannot give the change any better test than they can, and it is\n>> their platform to improve, or break by accident while trying to do\n>> so.\n> \n> Right. I tested this as well as I could, via the `--stress` option, and am\n> fairly confident that it is correct. Since the patch touches only\n> `simply-ipc` code, the only test that could possibly affected is t0052,\n> and it passes with `--stress` over here (when it failed without the\n> patch).\n> \n> Ciao,\n> Dscho\n> \n> P.S.: in case you wondered, no, I did not run the entire test suite. With\n> the performance characteristics of the POSIX emulation provided by the\n> Cygwin runtime, this would simply take too long. It's not the first time I\n> wish our test suite was more efficient, across _all_ supported platforms.\n\n\n[I seem to have lost Adam's reply about this now being Hunky-Dory, but ...]\n\nI ran the test-suite on -rc2 on thursday night (note _not_ rc2 + this patch)\nand it deadlocked on me; I didn't notice for 4 hours, so (in the early hours)\nI simply Ctl+C-ed it and went to bed. I haven't had the test-suite deadlock\nfor many many years - I've been spoilt! ;-)\n\nI tried -rc2 again last night; this time it finished, but I gained another\ntest failure: t0301-credential-cache.sh. I have _never_ had this test fail\nbefore, so that was unexpected. :(\n\n[Yes, t0052-simple-ipc.sh failed as expected, since this patch was not\napplied].\n\nAlso, I was half expecting a small speed-up due to the new pipe code in\nv3.3.2 of the cygwin dll, but it actually took an hour longer than normal. :(\n\nThe only change to my setup, between -rc1 and -rc2, was the cygwin update\nto v3.3.2, so this may point to some more fallout from the new pipe code\n(maybe?).\n\nAnyway, I haven't even looked at the new failure (see below), which we will\nprobably not have time to fix before release, so I am just now building\ncurrent master (v2.34.0-rc2-16-g5a73c6bdc7) to give that a try. (So, I won't\nhave anything to report until tomorrow).\n\nJust FYI:\n\n $ ./t0301-credential-cache.sh\n ...\n not ok 13 - socket defaults to ~/.cache/git/credential/socket\n #\n # test_when_finished \"\n # git credential-cache exit &&\n # rmdir -p .cache/git/credential/\n # \" &&\n # test_path_is_missing \"$HOME/.git-credential-cache\" &&\n # test_path_is_socket \"$HOME/.cache/git/credential/socket\"\n #\n ...\n not ok 26 - use custom XDG_CACHE_HOME if set and default sockets are not created\n #\n # test_when_finished \"git credential-cache exit\" &&\n # test_path_is_socket \"$XDG_CACHE_HOME/git/credential/socket\" &&\n # test_path_is_missing \"$HOME/.git-credential-cache/socket\" &&\n # test_path_is_missing \"$HOME/.cache/git/credential/socket\"\n #\n not ok 27 - credential-cache --socket option overrides default location\n #\n # test_when_finished \"\n # git credential-cache exit --socket \\\"\\$HOME/dir/socket\\\" &&\n # rmdir \\\"\\$HOME/dir\\\"\n # \" &&\n # check approve \"cache --socket \\\"\\$HOME/dir/socket\\\"\" <<-\\EOF &&\n # protocol=https\n # host=example.com\n # username=store-user\n # password=store-pass\n # EOF\n # test_path_is_socket \"$HOME/dir/socket\"\n #\n not ok 28 - use custom XDG_CACHE_HOME even if xdg socket exists\n #\n # test_when_finished \"\n # git credential-cache exit &&\n # sane_unset XDG_CACHE_HOME\n # \" &&\n # check approve cache <<-\\EOF &&\n # protocol=https\n # host=example.com\n # username=store-user\n # password=store-pass\n # EOF\n # test_path_is_socket \"$HOME/.cache/git/credential/socket\" &&\n # XDG_CACHE_HOME=\"$HOME/xdg\" &&\n # export XDG_CACHE_HOME &&\n # check approve cache <<-\\EOF &&\n # protocol=https\n # host=example.com\n # username=store-user\n # password=store-pass\n # EOF\n # test_path_is_socket \"$XDG_CACHE_HOME/git/credential/socket\"\n #\n not ok 29 - use user socket if user directory exists\n #\n # test_when_finished \"\n # git credential-cache exit &&\n # rmdir \\\"\\$HOME/.git-credential-cache/\\\"\n # \" &&\n # mkdir -p \"$HOME/.git-credential-cache/\" &&\n # chmod 700 \"$HOME/.git-credential-cache/\" &&\n # check approve cache <<-\\EOF &&\n # protocol=https\n # host=example.com\n # username=store-user\n # password=store-pass\n # EOF\n # test_path_is_socket \"$HOME/.git-credential-cache/socket\"\n #\n not ok 30 - use user socket if user directory is a symlink to a directory\n #\n # test_when_finished \"\n # git credential-cache exit &&\n # rmdir \\\"\\$HOME/dir/\\\" &&\n # rm \\\"\\$HOME/.git-credential-cache\\\"\n # \" &&\n # mkdir -p -m 700 \"$HOME/dir/\" &&\n # ln -s \"$HOME/dir\" \"$HOME/.git-credential-cache\" &&\n # check approve cache <<-\\EOF &&\n # protocol=https\n # host=example.com\n # username=store-user\n # password=store-pass\n # EOF\n # test_path_is_socket \"$HOME/.git-credential-cache/socket\"\n #\n ok 31 - helper (cache --timeout=1) times out\n # failed 6 among 31 test(s)\n 1..31\n $\n \nATB,\nRamsay Jones\n"},{"id":"441096","messageId":"c3fb4778-9537-f909-f792-5a298dbb3907@ramsayjones.plus.com","threadId":"56508","inReplyTo":"02b6cfa8-bd0a-275d-fce0-a3a9f316ded3@ramsayjones.plus.com","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2021-11-14T19:34:14Z","receivedAt":"2021-11-14T19:34:35Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 13/11/2021 19:11, Ramsay Jones wrote:\n[snip]\n> The only change to my setup, between -rc1 and -rc2, was the cygwin update\n> to v3.3.2, so this may point to some more fallout from the new pipe code\n> (maybe?).\n> \n> Anyway, I haven't even looked at the new failure (see below), which we will\n> probably not have time to fix before release, so I am just now building\n> current master (v2.34.0-rc2-16-g5a73c6bdc7) to give that a try. (So, I won't\n> have anything to report until tomorrow).\n\nSo, current 'master' fixes t0052-simple-ipc.sh, which is good, but the\nt0301-credential-cache.sh test is still failing. Also, I can confirm\nthat cygwin v3.3.2 adds an additional hour to a test-suite run. :(\n\n[ie it now takes 6 hours rather than 5 hours to run - I remember a time\nwhen it used to only take 2 hours; those were the days!]\n\nATB,\nRamsay Jones\n\n\n"},{"id":"441097","messageId":"CA+kUOanwFBdv=kd+txAmfkb6jdC=gdSyaBEi89T0neE1w5iO0w@mail.gmail.com","threadId":"56508","inReplyTo":"02b6cfa8-bd0a-275d-fce0-a3a9f316ded3@ramsayjones.plus.com","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2021-11-14T20:10:21Z","receivedAt":"2021-11-14T20:10:59Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Sat, 13 Nov 2021 at 19:11, Ramsay Jones wrote:\n> On 10/11/2021 12:27, Johannes Schindelin wrote:\n> [snip]\n> >> I cannot give the change any better test than they can, and it is\n> >> their platform to improve, or break by accident while trying to do\n> >> so.\n> >\n> > Right. I tested this as well as I could, via the `--stress` option, and am\n> > fairly confident that it is correct. Since the patch touches only\n> > `simply-ipc` code, the only test that could possibly affected is t0052,\n> > and it passes with `--stress` over here (when it failed without the\n> > patch).\n> >\n> > Ciao,\n> > Dscho\n> >\n> > P.S.: in case you wondered, no, I did not run the entire test suite. With\n> > the performance characteristics of the POSIX emulation provided by the\n> > Cygwin runtime, this would simply take too long. It's not the first time I\n> > wish our test suite was more efficient, across _all_ supported platforms.\n>\n>\n> [I seem to have lost Adam's reply about this now being Hunky-Dory, but ...]\n>\n> I ran the test-suite on -rc2 on thursday night (note _not_ rc2 + this patch)\n> and it deadlocked on me; I didn't notice for 4 hours, so (in the early hours)\n> I simply Ctl+C-ed it and went to bed. I haven't had the test-suite deadlock\n> for many many years - I've been spoilt! ;-)\n>\n> I tried -rc2 again last night; this time it finished, but I gained another\n> test failure: t0301-credential-cache.sh. I have _never_ had this test fail\n> before, so that was unexpected. :(\n>\n> [Yes, t0052-simple-ipc.sh failed as expected, since this patch was not\n> applied].\n>\n> Also, I was half expecting a small speed-up due to the new pipe code in\n> v3.3.2 of the cygwin dll, but it actually took an hour longer than normal. :(\n>\n> The only change to my setup, between -rc1 and -rc2, was the cygwin update\n> to v3.3.2, so this may point to some more fallout from the new pipe code\n> (maybe?).\n>\n> Anyway, I haven't even looked at the new failure (see below), which we will\n> probably not have time to fix before release, so I am just now building\n> current master (v2.34.0-rc2-16-g5a73c6bdc7) to give that a try. (So, I won't\n> have anything to report until tomorrow).\n\nI'm seeing the same failure. It isn't caused by a change in Git --\nI've rebuilt and re-run the test on old versions where that test was\npassing, and it's now failing -- so this is clearly something in the\nCygwin environment. I've not investigated further, but it's clearly\ncaused by a Cygwin change rather than a Git change, so I don't think\nthere's any reason to hold up the Git release.\n\n(I should probably report it on the Cygwin mailing list, but I haven't\ngot around to that yet...)\n"},{"id":"441292","messageId":"nycvar.QRO.7.76.6.2111161152310.21127@tvgsbejvaqbjf.bet","threadId":"56508","inReplyTo":"CA+kUOa=FBpzy4zNMXY1UZybc+13mwLnXrZRVN5HP+NF3h4PYow@mail.gmail.com","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-16T10:56:47Z","receivedAt":"2021-11-16T10:57:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 12 Nov 2021, Adam Dinwoodie wrote:\n\n> On Fri, 12 Nov 2021 at 16:01, Junio C Hamano wrote:\n> >\n> > Out of curiosity, are the use cases and user base of Cygwin waning,\n> > or are there still viable cases where Cygwin is a more preferred\n> > solution over WSL (the question is not limited to use of Git)?\n>\n> No formal research here, just impressions as someone who has used\n> Cygwin for a long time and who hangs out on the Cygwin mailing list:\n> for a lot of use cases, WSL is at least as good, if not better, than\n> Cygwin. There are a few areas where Cygwin is still a better solution,\n> though:\n>\n> - WSL requires essentially installing an entire operating system. Disk\n> space is relatively cheap, so that's not nearly the obstacle it used\n> to be, but it is an obstacle. This is more relevant for people who\n> want to distribute packaged installers to Windows users: most\n> non-technical users won't want to get WSL working, but if you've\n> written code for *nix and don't want to port it manually to Windows,\n> it's relatively straightforward to compile it using Cygwin and bundle\n> the cygwin1.dll file with the installer. That'll mostly get your code\n> working with a user experience that doesn't differ too much from a\n> fully native Windows application. (This is essentially what Git for\n> Windows is doing, albeit with an increasingly distant Cygwin fork.)\n>\n> - There are some functions that Cygwin offers that WSL doesn't. The\n> key one for me is the ability to access Windows network file shares,\n> which WSL doesn't support (or at least didn't last time I checked). I\n> expect some of these gaps will disappear as WSL gets more features,\n> but I expect some of them are fairly fundamental restrictions: Cygwin\n> applications can have code specifically to handle the fact that\n> there's a Windows OS there, so they can -- with care -- interact with\n> the Windows OS directly to (say) use Windows file access APIs or the\n> Windows clipboard. WSL applications generally don't have that ability;\n> if I install something from apt on my Debian WSL installation, it'll\n> pull exactly the same binary as if I'd installed it on a normal Debian\n> system. I guess in theory people could write code to detect that\n> they're running in WSL and handle that specially, in the same way that\n> it's normally possible to detect and handle when you're running in a\n> VM versus running on bare metal. I expect that'll be much less common,\n> though, just as Git has code for handling Cygwin specially but doesn't\n> have code for handling Linux-within-WSL specially, even though both\n> could be used to access a Git repository stored in the same Windows\n> NTFS directory.\n\nI would like to add two additional scenarios where Cygwin needs to be used\ninstead of WSL:\n\n- In order to install WSL, you need to switch your machine to Developer\n Mode, which requires administrative privileges (which not evey developer\n enjoys, and given recent and not so recent security news, maybe that's a\n good thing, too). Cygwin does not require administrative privileges.\n\n- There is (finally!) a way to run graphical Linux applications in WSL,\n but it requires Windows 11. That excludes many existing Windows users.\n\nSo yeah, I think Cygwin is here to stay. Besides, as long as I don't find\nany better way to have a POSIX-compliant shell (Git will continue to\ndepend on one for a long, long time, I expect), Git for Windows will\nindirectly _have_ to depend on Cygwin (Git for Windows bundles a Bash\nusing the MSYS2 runtime, which is a very close fork of the Cygwin\nruntime).\n\nCiao,\nDscho\n"},{"id":"441294","messageId":"nycvar.QRO.7.76.6.2111161158230.21127@tvgsbejvaqbjf.bet","threadId":"56508","inReplyTo":"CA+kUOanh1m=dkE-gDikg53zOPXx_7v65ggqxPspuZWDbdOrR=g@mail.gmail.com","subject":"Re: [PATCH v2 7/7] t/helper/simple-ipc: convert test-simple-ipc to use start_bg_command","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2021-11-16T11:02:18Z","receivedAt":"2021-11-16T11:03:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Adam,\n\nOn Fri, 12 Nov 2021, Adam Dinwoodie wrote:\n\n> [...] the (lack of) speed of running the Git test suite on Cygwin\n> is one of the reasons I run the tests on high-spec Azure VMs rather\n> than my own systems. Unfortunately the Cygwin compatibility layer plus\n> the overheads of NTFS mean things are unlikely to get significantly\n> quicker any time soon, and between WSL and Git for Windows, I expect\n> interest in improving Cygwin's performance is going to continue to\n> wane.\n\nWell, at least from the Git point of view, there is still _some_ hope. At\nthe Git Contributor Summit, we talked (very, very briefly) about moving\nparts of Git's test infrastructure from shell code to C.\n\nThis would definitely help not only save electricity (and we all _do_ have\nto get used to the idea that we cannot continue spending as much energy as\nwe do right now) when running the CI/PR builds, but also accelerate\nrunning the test suite on Cygwin (or for that matter, I suspect HP NonStop\nto be helped tremendously, too).\n\nCiao,\nDscho\n"}]}